Run each run_r test on its own later loop - #279
Closed
jat255 wants to merge 1 commit into
Closed
Conversation
The R test suite spends about ten minutes idle. Every run_r call arms a reap timer at commons.run_r_idle_timeout + 1 (601 seconds) and a timeout timer at commons.run_r_timeout (60 seconds), and nothing cancels either. Only test-run-r.R creates a worker, so it hands the global loop about 70 pending callbacks. shiny::testServer() then reads an output through shiny:::wait_for_it(), which spins until later::loop_empty() rather than waiting on the output's own promise, so the first such read in test-trajectory-review.R blocks for the full 601 seconds. later::later() and later::later_fd() schedule onto later::current_loop(), so giving each test its own loop confines the leftovers to a loop that is destroyed when the test ends. later ships with_loop() but no local_ form, so local_temp_loop() sets the current loop for the caller's scope and restores it on exit. The whole suite now runs in 50 seconds, and test-trajectory-review.R in 1.4 rather than 594. This is containment in the test suite only. The leak itself is untouched, and the production fix that cancels the timers is on jat255/267-cancel-later-timers. Refs #267
jat255
marked this pull request as ready for review
September 4, 2026 15:45
|
Preview deployed to Connect ( Deployed from commit b760bfb. |
|
Preview deployed to Connect ( Deployed from commit b760bfb. |
jat255
added
r
simonpcouch
reviewed
Sep 4, 2026
| local_temp_loop <- function(env = parent.frame()) { | ||
| old <- later::current_loop() | ||
| loop <- later::create_loop(parent = NULL) | ||
| later:::setCurrentRegistryId(loop$id) |
Collaborator
There was a problem hiding this comment.
Not big on ::: here🙃 This will likely introduce an R CMD check NOTE.
If you're game to wait a day, I can have a fix for this in!
Collaborator
Author
|
Closed in favor of #268 |
|
Cleaned up 1 preview bundle(s) on https://dogfood.team.pct.posit.it: 367850 |
|
Cleaned up 1 preview bundle(s) on https://connect.staging.pct.posit.it: 2559 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The R test suite spends about ten minutes doing nothing, and this makes it stop, without touching package code. Each
run_rtest now runs on its ownlaterloop, so the timers a call leaves behind are discarded with the test instead of stalling later test files.Summary
run_rleaks a 601 second reap timer and a 60 second timeout timer per call, andshiny::testServer()reads an output by spinning untillater::loop_empty(), so the first such read aftertest-run-r.Rwaits for the farthest one. #267 has the mechanism and the measurements.later::later()andlater::later_fd()schedule ontolater::current_loop(), so a test on its own loop cannot leak past itself.local_temp_loop()switches the current loop for the caller's scope and restores it on exit; every test in the file calls it first.Review Notes
This is containment in the test suite, not a fix. The leak is untouched, #268 cancels the timers, and this branch should be reverted when that one lands.
local_temp_loop()calls the unexportedlater:::setCurrentRegistryId, for the reason given in the comment above it. Wrapping each body in the exportedlater::with_temp_loop()also works, but the 80 column limit then reindents all 22 test bodies. Atest_that_local_loop()wrapper avoids that and was rejected because it breaks testthat's failure locations.The new global-loop test skips in a full-suite run:
test-chat.Ralready leaves callbacks on the global loop, so the test skips when the loop was busy at file load rather than fail for something it did not cause. Its companion, which asserts leftovers land on the loop the call ran on, always runs.Testing
The whole suite passes in 50 seconds, against about eleven minutes before, and
test-trajectory-review.Rin the same session goes from 594 seconds to 1.4.R CMD checkwas not run locally, since no package code changed.Refs #267
R changes
Nothing under
pkg-r/R/changed, so the package has no behaviour delta: no function was touched and no call site moved. The diff is one test file.The helper is a withr-style local: record the current loop, create and enter a fresh one, defer restoring the old one and destroying the new one. Ordering is the part to check.
withr::deferis LIFO and each test enters its loop before creating a worker, so the worker's deferredworker_close()runs while its loop is still alive; swapping those two lines in a test would tear the loop down underneath a closing worker.Confinement rests on timers being scheduled through
later::current_loop(), which is what the leak already depends on. I checked it rather than assuming: with the helper on only the tests usinglocal_worker()the stall persisted, because several tests build a worker throughtest_agent(), and it went away once all 22 tests had it.Worth your scrutiny: the
later:::call is the one thing alaterupgrade can break, and it would fail loudly rather than quietly resume leaking. No hand-written tests were deleted.