Fix async resume through control-flow statements#2469
Merged
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c17b6d6b18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
nfcampos
force-pushed
the
nc/async-resume-control-flow
branch
from
May 11, 2026 11:25
c17b6d6 to
4f777d3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f777d3766
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
nfcampos
force-pushed
the
nc/async-resume-control-flow
branch
from
May 11, 2026 11:50
4f777d3 to
278d35f
Compare
There was a problem hiding this comment.
💡 Codex Review
Avoid reading uninitialized for-loop bindings on suspend
When suspension happens while evaluating a let/const initializer in the for header, e.g. for (let i = await Promise.resolve(0); i < 1; i++), the loop binding has been created but not initialized yet. This new save path walks every bound name and calls GetBindingValue, which throws the TDZ ReferenceError while trying to save suspend state, so the async function rejects instead of resuming the initializer.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
nfcampos
force-pushed
the
nc/async-resume-control-flow
branch
from
May 11, 2026 12:11
278d35f to
f9e081c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9e081c258
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
nfcampos
force-pushed
the
nc/async-resume-control-flow
branch
from
May 11, 2026 13:01
f9e081c to
23adf97
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23adf97a81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
nfcampos
force-pushed
the
nc/async-resume-control-flow
branch
from
May 11, 2026 13:41
23adf97 to
a53c4ae
Compare
## Summary Fix async resume through control-flow statements. ## Details - Resume async execution inside catch/finally without rerunning the try block or leaking catch bindings. - Resume selected if/while/do-while/for/switch paths without rerunning already-completed guards, bodies, discriminants, or case tests. - Preserve accumulated loop/switch completion values and binary-expression left operands across await suspension. - Add focused AsyncTests coverage for catch/finally, branch, loop, switch, and binary-expression resume behavior. ## Linked issue Refs # ## Test plan - [x] Added or updated unit tests in Jint.Tests - [ ] Ran dotnet test --configuration Release locally - [ ] For ECMAScript spec changes: ran Jint.Tests.Test262 and confirmed no regressions - [ ] For interop changes: covered in Jint.Tests/Runtime/Interop - [ ] For perf changes: included before/after numbers from Jint.Benchmark Additional local validation: - dotnet test Jint.Tests/Jint.Tests.csproj --filter "FullyQualifiedName~AsyncTests" - dotnet build Jint/Jint.csproj --configuration Release ## Breaking change? No.
GetSuspensionNode and IsNodeInsideRange in JintStatement are consumed by the JintStatement subclasses and by JintSwitchBlock (a non-derived internal class). Neither caller is outside the Jint assembly, so 'protected internal' over-promises -- 'internal static' is the narrowest that compiles. Follows the internal-first guideline in CLAUDE.md.
…sions
The same "left side re-evaluated on resume" bug fixed for binary
expressions in the previous commit also affected `a && b`, `a || b`,
and `a ? b : c` when only the right side (or the consequent / alternate)
contained the await. Example:
let d = 0;
const ok = (++d > 0) && (await Promise.resolve(true));
// d should be 1, was 2 — `++d` ran on resume too
Generalize the previous fix:
- Rename BinaryExpressionSuspendData to LeftOperandSuspendData (a
single-field container holding the already-evaluated left side).
- Reuse it in JintLogicalAndExpression, JintLogicalOrExpression and
JintConditionalExpression. On right-side / branch suspension, save
the left/test value keyed by `this`; on resume restore it and skip
re-evaluation. On short-circuit, clear suspend data so a later use
of the same expression starts fresh.
Add 6 AsyncTests covering binary, &&, ||, ?: (consequent and alternate),
and the short-circuit-clears-data invariant.
A compound assignment like `obj[++i] += await y` would re-evaluate the
LHS on resume after the right side suspended — running the `++i` side
effect twice and aiming the assignment at a different slot. Generalize
the resume model used by binary/logical/conditional expressions:
- Add AssignmentSuspendData storing the resolved Reference and the
pre-mutation original value.
- In JintAssignmentExpression, detect resume state up front and reuse
the saved Reference + original value instead of re-running
`_left.Evaluate(context)`. Only the slow path (anything other than
plain identifier) is preserved — the fast path has no observable
LHS side effect, so re-resolving on resume is fine and we keep
pool churn down.
- Replace each operator branch's `_referencePool.Return(lref)` on
suspension with a single HandleSuspendedRight helper that either
saves the data (slow path) or returns the pooled reference (fast
path).
- Clear suspend data on every successful exit, including
short-circuit returns from `??=`, `&&=`, `||=`, to avoid stale
state when the same expression is re-entered (e.g. in a loop).
Add 4 AsyncTests covering computed indexer +=, property accessor with
observable side effect, nullish-coalescing `??=`, and the simple
identifier (fast-path) baseline.
A call expression like `foo(++i, ++i, await Promise.resolve(++i))` was
re-running the entire argument list on resume after the await
suspended, re-incrementing earlier counters even though the await
result was already cached. Same pattern for `new X(...)`.
- Add CallArgumentsSuspendData carrying the rented buffer and the
next index to resume at.
- ExpressionCache.ArgumentListEvaluation now takes a `key` (the
owning call/new expression). On resume it reuses the saved buffer
and starts BuildArguments at the saved index; on suspension it
stows the buffer + index in suspend data and tells the caller
rented=false (so the caller does not return the buffer to the
pool). On normal completion it clears the suspend data and
transfers buffer ownership to the caller with rented=true.
- Update the three JintCallExpression call sites (regular call,
eval, super) and JintNewExpression to pass `this` as the key.
- JintNewExpression also gains an IsSuspended check around the
arg-list evaluation that it was previously missing — without it,
`new X(await y)` would proceed into Construct() with a partially
filled buffer.
- Spread-argument path (BuildArgumentsWithSpreads) is left
untouched; same suspend-and-replay bug exists there but iterator
state makes it a more involved fix. Worth a follow-up.
Add 4 AsyncTests covering plain call, call with multiple awaits,
new-expression, and the suspend-data-cleared-between-calls invariant.
Sync generators (yield), async generators (yield in async function*), and async functions (await) all share the same ISuspendable.Data suspend infrastructure plus the GetSuspensionNode / IsNodeInsideRange helpers introduced by the control-flow resume fix. The fixes in the previous commits are therefore generic, but the AsyncTests in the PR only exercise the async-function path. Add 11 GeneratorTests that mirror the key bug patterns: - yield inside catch — try block not re-run - yield inside if — test not re-run - yield inside for body — test/update not re-run - yield inside switch case — discriminant not re-run - binary, logical, conditional left-operand preservation - compound assignment LHS preservation - call-argument preservation - switch lexical binding preserved across yield - switch suspend-data cleared after resumed break in nested loop - async-generator equivalent of catch resume These are regression guards against drift between the three suspend paths now that the resume machinery is shared.
lahma
force-pushed
the
nc/async-resume-control-flow
branch
from
May 14, 2026 09:42
a53c4ae to
455e5e0
Compare
Collaborator
|
Rebased onto current Stack on top of
Why these specifically: the same "left half re-evaluated on resume" bug the original fix repairs for binary expressions also exists in Commit 6 adds 11 new tests in Verification on the rebased branch:
One follow-up explicitly left out of this PR: |
lahma
enabled auto-merge (squash)
May 14, 2026 09:53
5 tasks
lahma
added a commit
that referenced
this pull request
May 14, 2026
…2469 (#2475) * Async resume: rename CallArgumentsSuspendData to ExpressionBufferSuspendData The type is about to back both call/new argument lists and array literal element lists, which share the same shape (JsValue[] Buffer + int NextIndex). Mechanical rename only; no behavior change. * Async resume: preserve evaluated array literal elements across await `[++i, ++i, await Promise.resolve(++i)]` re-ran the first two `++i` expressions on resume because JintArrayExpression.EvaluateInternal called BuildArguments without a suspend key. Apply the same pattern PR #2469 introduced for call/new arguments: on the non-spread path, save the partial buffer + next index in ExpressionBufferSuspendData (keyed by `this`); on resume, restart BuildArguments at the saved index reusing the same buffer; clear on completion. Spread path (`[a, ...iter, await x]`) is not addressed here — it has the same suspend-and-replay bug but lives in ExpressionCache.BuildArgumentsWithSpreads, handled in the next commit. Add 3 AsyncTests (single await, multiple awaits, suspend-data cleared between successive evaluations) and a sync-generator parity test. * Async resume: preserve spread argument target list across await `foo(...g, await x)` and `[...g, await x]` previously re-iterated a custom (one-shot) iterator on resume, producing a wrong result — sync iterators are drained after the first pass and yield empty on the second. The same code path also unnecessarily re-evaluated non-spread side-effectful elements that had already been added. Changes: - `BuildArgumentsWithSpreads` now returns the next expression index and accepts a `startIndex`. Suspension at a non-spread element now bails BEFORE `target.Add(...)`, so the resume re-evaluates and appends exactly once. - New `ArgumentListEvaluationWithSpreadsResumable(context, key)` helper that wraps `BuildArgumentsWithSpreads` with suspend-data save/restore, keyed by the owning call/new/array expression. - `SpreadArgumentsSuspendData(List<JsValue> Target, int NextExpressionIndex)` holds the partial target list across suspension. - `ExpressionCache.ArgumentListEvaluation`'s `HasSpreads` branch and `JintArrayExpression.EvaluateInternal`'s spread branch both delegate to the new helper, passing `this` as the suspend key. Spread iteration itself (the `JsArray { HasOriginalIterator: true }` fast path and the `ArraySpreadProtocol.Execute()` generic path) is synchronous and runs to completion once started — no mid-iterator state needs preserving. Add 4 AsyncTests (array-literal spread with one-shot iterator, call spread with one-shot iterator, target survives multiple suspensions, no spurious post-suspend side-effect) and 1 GeneratorTests parity. * Async resume: preserve left operand of ?? across right-side await `getX() ?? (await y)` re-ran `getX()` on resume. Sibling of the `&&` / `||` / `?:` fix in PR #2469 commit 3 that was overlooked — reuse the same `LeftOperandSuspendData` pattern. Add an AsyncTests case and a sync-generator parity test. * Async resume: preserve member expression base across property-side await `getObj()[await x]` re-ran `getObj()` on resume because the slow path of JintMemberExpression.EvaluateInternal resolves the object side (possibly via _objectExpression.Evaluate) before _propertyExpression may suspend. On resume the entire EvaluateInternal re-runs, doubling any side effects in getObj(). Add MemberExpressionSuspendData carrying the resolved (BaseValue, BaseReferenceName, ActualThis) triple keyed by `this`. On resume, skip object-side resolution and the optional-chaining null check (which already passed on the original run). Save on property suspension; clear on completion. GetValue's fast path is intentionally NOT changed: it has a property that's already a JsString constant (no _propertyExpression), so suspension there can only happen INSIDE _objectExpression itself — and that expression has its own resume infrastructure. Add 2 AsyncTests (computed property and chained member) plus a sync-generator parity test. * Async resume: preserve object literal properties across await `{ a: ++i, b: await x, c: ++j }` re-evaluated `++i` on resume because both BuildObjectFast and BuildObjectNormal restart their property loop from index 0. Add ObjectExpressionSuspendData carrying: - Target — the partially-built ObjectInstance (fast path uses JsObject, normal path uses Realm.Intrinsics.Object.Construct's result; both are ObjectInstance subtypes). - FastProperties — the in-flight PropertyDictionary used only by the fast-path builder (installed on Target via SetProperties at the end). - NextIndex — where to resume in _properties. Both BuildObjectFast and BuildObjectNormal: - On resume, restore Target (+ FastProperties for the fast path) and start at NextIndex. - On suspension at any of the three points in the normal path (spread value, computed property key, init value), save Target + current index in a new helper SaveObjectExpressionSuspendState. - Clear suspend data on successful completion. Add 3 AsyncTests (init properties, multiple awaits, computed key) and a sync-generator parity test. * Async resume: preserve template literal accumulator across await Both untagged template literals (\`\${expr}\`) and tagged templates (tag\`\${expr}\`) had two related bugs: 1. No `IsSuspended` check inside the interpolation loop — when one interpolation suspended, later interpolations continued to run during the suspended pass, doubling their side effects. 2. On resume, the entire interpolation loop restarted from index 0, re-evaluating leading interpolations (whose awaits are cached, but surrounding side effects in their expressions like `++i` are not). For JintTemplateLiteralExpression: - Add `TemplateLiteralSuspendData(StringBuilder Accumulator, int NextExpressionIndex)`. System.Text.StringBuilder is used (rather than ValueStringBuilder which is a ref struct and can't live in suspend data) — a one-time string copy happens on suspension, which is the rare path. - On suspension after appending quasi[i]: save the accumulator + the expression index i. On resume: restore the accumulator (which already includes quasi[i]) and skip the quasi append for iteration i, going straight to the interpolation evaluation. For JintTaggedTemplateExpression: - Add `TaggedTemplateSuspendData(ICallable Tagger, JsValue ThisObject, JsValue[] Args, int NextExpressionIndex)`. - Resolve the tagger + thisObject before the interpolation loop. Save them along with the partial args buffer. - On suspension during interpolation evaluation, save state and bail. - On resume, skip tag resolution and continue from saved index. - Add the missing `IsSuspended` break after `_tagIdentifier.Evaluate`. Add 3 AsyncTests (template interpolation preservation, no post-suspend side effects, tagged template) plus a sync-generator parity test. * Async resume: fix double-evaluation of RHS in operator-overloaded compound assignment When OperatorOverloadingAllowed is enabled, JintAssignmentExpression runs EvaluateOperatorOverloading before the operator switch. Both call _right.GetValue(context). If RHS suspended in the first call, the switch would call it again on the same execution pass: - First _right.GetValue: suspends, registers promise handlers H1, inner side effects run. - TryOperatorOverloading called with the suspended sentinel; returns false (no CLR match). - Switch case runs: _right.GetValue called AGAIN. - JintAwaitExpression doesn't yet have IsResuming=true (we're still on the first pass) and _completedAwaits is empty, so it falls through to _awaitExpression.GetValue — re-running the inner side effect (e.g. `Promise.resolve(new Vector2(++counter, 10))` evaluates `++counter` twice). - Then registers a second pair of handlers H2 on a new promise. When H1 fires, the function completes; when H2 fires, AsyncFunctionResume sees state=Completed and bails. So output is correct but side effects in the awaited expression have doubled. Fix: - In EvaluateOperatorOverloading, return immediately after _right.GetValue if IsSuspended. - In EvaluateInternal, after calling EvaluateOperatorOverloading, check IsSuspended and call the existing HandleSuspendedRight helper to save LHS state and bail. Add a test in OperatorOverloadingTests that uses the existing Vector2 overload and a counter callable: verifies the RHS side effect runs exactly once across the full suspend/resume cycle.
This was referenced May 18, 2026
This was referenced Jun 4, 2026
This was referenced Jun 29, 2026
This was referenced Jul 7, 2026
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.
Summary
Fix async resume through control-flow statements.
Details
Linked issue
Refs #
Test plan
Additional local validation:
Breaking change?
No.