Adopt resolve-with-thenable on the event loop (fix async-ordering flake)#983
Merged
Merged
Conversation
…erminism) Resolving a promise with another promise (resolve(thenable) / await of a promise resolved with a promise) adopted the inner promise via `innerTask.ContinueWith(..., TaskScheduler.Default)` — a thread-pool hop that settled the outer promise off the event-loop thread with nothing queued. When the inner was already settled (e.g. a pending promise resolved with an already-resolved one), the event loop could observe "no active handles AND empty callback queue" and exit before the adoption settled, so the outer promise's .then reaction never ran. Whether the loop won that race depended on machine load, so Test262 Promise/resolve-thenable-deferred flipped Pass/Fail run-to-run. Route the adoption through the event loop instead (new Interpreter.AdoptInnerPromise): the settle is delivered as an event-loop callback via ExecuteSynchronously, so an already-settled inner enqueues it inline (the loop never starts idle) and a never-settling inner enqueues nothing (the loop exits normally, matching Node — a pending adoption does not keep the program alive). Used by both the new-Promise executor resolve (SharpTSPromiseClass) and await-thenable adoption (AdoptThenable). Interpreter-only — compiled mode emits its own event loop and has the same gap, tracked separately. Test262 interpreted baseline: resolve-thenable-deferred Fail->Pass, 0 regressions. Full xUnit suite 14412/0; +2 regression tests.
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
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.
Background
Follow-up to the interpreted-Test262 flake investigation. After PR #982 removed the orphan-thread amplifier, I profiled the residual flake: the suite is fast (11,303/11,385 tests <50ms, 0 genuine timeouts in isolation), and worker oversubscription alone never reproduces a flip — but under genuine full-machine CPU saturation (a 12-core burner), one test reproducibly flipped:
Promise/resolve-thenable-deferred.js(Fail→Pass). A Fail→Pass flip hard-fails the baseline gate ("new pass"), so this async-ordering non-determinism — not timeouts — is the residual run-to-run flake.Root cause
Resolving a promise with another promise —
resolve(thenable), orawaitof a promise that was resolved with a promise — adopted the inner promise viainnerTask.ContinueWith(..., TaskScheduler.Default)(SharpTSPromiseClass.RunExecutorandInterpreter.AdoptThenable). That's a thread-pool hop: it settles the outer promise off the event-loop thread with nothing on the callback queue. When the inner promise is already settled (the "deferred" case: a pending promise resolved with an already-resolved one), the event loop can observe!HasActiveHandles && _callbackQueue.Count == 0and exit before the adoption settles, so the outer promise's.thenreaction never runs. Whether the loop wins that race depends on scheduling, so the test flips Pass/Fail under load.(The CLI happened to win the race consistently; the Test262 runner — which runs each test on a dedicated thread — lost it in isolation, which is why the test was "pinned Fail.")
Fix (interpreter-only)
Route the adoption through the event loop via a new
Interpreter.AdoptInnerPromise: deliver the settle as an event-loop callback usingTaskContinuationOptions.ExecuteSynchronously. So:resolve, before the loop starts) → the loop never starts idle → deterministic;.thencontinuations back onto the loop.Used by both the
new Promiseexecutor resolve and theawait-thenable path.SettleFromTask(subclass capability adoption) already usedExecuteSynchronouslyand is unchanged.Validation
Promise/resolve-thenable-deferred.js: Fail → Pass, deterministic with and under the 12-core burner (the condition that originally flipped it). Test262 interpreted baseline: +1, 0 regressions.PromiseMethodTests.ResolveWithThenable_*).Notes
main.