Repository navigation
bun test: rank "no timeout" as the latest deadline for spawnSync in a concurrent group #46213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
robobun
wants to merge
2
commits into
main
Choose a base branch
from
robobun/31c3e4c6/spawnsync-concurrent-no-timeout
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 A timed concurrent test that calls spawnSync after an await now keeps a hung child forever when any running sibling has timeout 0, hanging the whole
bun testrun. At src/runtime/test_runner/jest.rs:179 a runningEPOCHentry wins as latest, so the wait loop in js_bun_spawn_bindings.rs:1841 gets no deadline at all. On the base branch the timed test's own deadline still ended the child. Fix: when the caller is unknown, keep a deadline for the timed entries (for example the latest non-EPOCH deadline, or a far deadline for 0 asInfinityalready uses) so a test's timeout still ends its child; the PR lists the(pass)overrun as a downside, but the never-exiting-child case is a hang, not an overrun. [also at: src/runtime/test_runner/jest.rs:184 - A timed concurrent test whose spawnSync child never exits now hangs the wholebun testrun forever when a sibling with timeout 0 is also running; on the base it is killed at its own deadline and fails.]Why this was flagged
Trigger: a concurrent group with test A
timeout: 100that doesawait x; Bun.spawnSync(hangingCmd)and test Btimeout: 0that is executing at the same time. The wait loop at src/runtime/api/bun/js_bun_spawn_bindings.rs:1841 calls get_active_timeout;on_stack_entryis None after the await, so the loop at src/runtime/test_runner/jest.rs:170-182 runs. B's entry has timespecEPOCH(Execution.rs:627) andexecutingtrue, soorder_ignore_epoch(EPOCH, A_deadline)is Greater (src/bun_core/util.rs:4946) andlatestbecomes EPOCH, returned at jest.rs:184. The wait loop then hashas_bun_test_timeout == falseand waits for the child indefinitely. On the base branchts.order(&l)ranked EPOCH lowest, so A's 100 ms deadline was returned and the child was signalled at that deadline. After merging, a child that never exits blocks the JS thread forever; A's timeout never fires because the thread is blocked, and the run only ends when CI kills it. The PR description acknowledges the timed test reading(pass)beside a 0-timeout sibling but not the hang.Verification:
order_ignore_epoch(src/bun_core/util.rs:4942-4953) returnsGreaterwhents == EPOCH, so jest.rs:183-185 returnsEPOCHdirectly. In the wait loop (src/runtime/api/bun/js_bun_spawn_bindings.rs:1838-1858)has_bun_test_timeoutis then false andtick_with_timeout(None)blocks until the child exits. On the base commit the child was signalled at A's deadline (js_bun_spawn_bindings.rs:1895-1911).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the first Downside of the description, in its worst case. The description now says it in those words: a child that never exits blocks the run.
The two rules proposed here do not remove it:
EPOCHis what main computes today. It is the defect of this PR: the child of the test with timeout0is signalled at the deadline of its sibling. The 6 new cases fail with it.0is whatInfinityhas on main (ScopeFunctions.rs:708, 49 days). The fold then returns that deadline, and the wait lasts 49 days.After an
awaitthe runner does not know which running test called spawnSync. It can signal at a deadline that can belong to another test, or it can wait while one running test has no timeout. #33832 chose the second ("so a sibling never terminates early"), and the comment atjs_bun_spawn_bindings.rs:1895says the same.Main keeps the own deadline of a timed caller only when every other running test has timeout
0. On 1.4.3-canary, with a 100 ms caller after anawaitand asleep 3child:0SIGTERMafter 101 msInfinity5000On this branch all three rows are exit 0 after about 3000 ms. So
0now behaves asInfinitydoes, anddocs/test/runtime-behavior.mdxgives the two as one setting. 1.3.14 did not signal a sync child in a concurrent group at all.A rule that ends the child of the timed caller, and never the child of the test with no timeout, needs the identity of the caller. #44833 works on that for the callback that is on the stack. I leave this thread open for a maintainer to decide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New measurements for this thread, on Linux x64. The caller has a 300 ms timeout and calls
spawnSynconsleep 2after anawait. The other test of the group has timeout0and is still running.SIGTERMat 301 msPromise.allarmSIGTERMat 302 msexpect(() => execSync("sleep 2")).not.toThrow()The two right columns are debug builds.
With #39440 the verdict is the verdict of 1.3.13 in each row. The child is still not ended, so a child that never exits still blocks the run. This PR should land after #39440.
To end the child at the caller's own timeout, the runner has to know the caller. I built that from the await chain of the async function that is running. The code is on the branch
robobun/3524c085/spawnsync-caller-from-await-chain. I do not propose it, for these reasons:await, an awaited helper and aPromise.racearm. It does not name aPromise.allarm, a.then()job, a timer callback, a.finally()hop or a call insideexpect(() => ...). Those callers stay on the rule of this PR.child.bind(null)(), the test fails at 301 ms. WithBUN_JSC_useJIT=0it passes at 1203 ms.The other mechanism is an identity that the runner gives to each callback and that its continuations carry, as #38880 does with the async context. It has a cost for every test callback. I leave that choice to a maintainer.