Cancel run_r's later callbacks so the loop drains - #268
Conversation
Every run_r_tool() call armed a reap timer at commons.run_r_idle_timeout + 1 (601 seconds by default) and a timeout timer at commons.run_r_timeout (60 seconds), and nothing cancelled either one. worker_close() did not cancel them either. shiny::testServer() reads outputs through shiny:::wait_for_it(), which spins until later::loop_empty() rather than waiting on the output's own promise. After test-run-r.R the global loop held 70 callbacks, the farthest 600.9 seconds out, so the first testServer() block in test-trajectory-review.R that read an output blocked for 601.1 seconds. R CMD check reported the tests as [2m/11m]: two minutes of CPU, eleven minutes of wall clock. later::later() and later::later_fd() each return a canceller. Keep the handles, release them when the call settles, hold one reap timer per worker instead of one per call, and cancel it in worker_close(). The viewer test now takes 0.65 seconds and the whole suite 52 seconds. Fixes #267
|
Preview deployed to Connect ( Deployed from commit d469b4c. |
|
Preview deployed to Connect ( Deployed from commit d469b4c. |
simonpcouch
left a comment
simonpcouch
left a comment
There was a problem hiding this comment.
I would have thought that this could be achieved without changes in R/, and the double assignment gives me the heebie jeebies a bit. Sorry to not approve and also not have the bandwidth to really look into this—planning to spend some time on commons tomorrow and will see what I can do here then!
|
In the meantime, feel free to ignore the R CMD check GHA. |
Totally understandable. I'll ask Claude to take another look to see if it's fixable on the test side only. It seemed convinced this was the right way, but I also didn't push back. |
simonpcouch
left a comment
simonpcouch
left a comment
There was a problem hiding this comment.
F****** ASYNC
Okay, yes, I'm on board. Thanks for your patience :) Just sent up some small changes to avoid some of the double assignments and simplify cancellation.
Thanks for catching this! I see that tests now wrap up in ~50s or so locally. We could also test in parallel--will file an issue to do so.
|
Cleaned up 2 preview bundle(s) on https://dogfood.team.pct.posit.it: 367611, 368055 |
|
Cleaned up 1 preview bundle(s) on https://connect.staging.pct.posit.it: 2545 |
2 participants
Fixes #267.
Every
run_r_tool()call armed twolatercallbacks and cancelled neither, andworker_close()didn't either.shiny::testServer()spins untillater::loop_empty(), so aftertest-run-r.Rthe loop held 70 callbacks, the farthest 601 seconds out, and the firsttestServer()block intest-trajectory-review.Rblocked for 601.1s. That was most of the 15-minute CI job.later()andlater_fd()return canceller functions (there is nolater::cancel()). The change, all inpkg-r/R/run-r.R:later_fd()watcher inworker_await(), and release them all fromsettle(): a finished call leaves nothing on the loop.worker_close(). Reap timing and target selection are unchanged.settledis TRUE, at which point they would have returned early anyway.Verification: the new test in
test-run-r.Rfails before the change and passes after; the viewer test went from 601.1s to 0.65s;R CMD checkis 1m17s,Status: OK, no notes. Full suite passes locally in 52s (28 skips, all environmental). No tests deleted; one added.The CI tests drop from about 15m for the "R CMD check" job to ~5m.
Worth your scrutiny:
poll(), where the callback clearscancel_watchand may re-arm it through a recursivepoll()call. I am confident it is right but it is the fiddliest part.