fix(transform): try/catch inside an async generator never caught anything#6331
Open
proggeramlug wants to merge 2 commits into
Open
fix(transform): try/catch inside an async generator never caught anything#6331proggeramlug wants to merge 2 commits into
proggeramlug wants to merge 2 commits into
Conversation
…hing
In an `async function*`, a `try { … } catch (e) { … }` whose `try` block contains a
`yield` never ran its catch. The exception escaped the generator body entirely and
surfaced to the consumer as an unhandled rejection:
async function* g() {
try { yield 1; throw new Error("boom"); }
catch (e) { yield "caught:" + e.message; }
}
for await (const v of g()) console.log(v);
// node: 1, caught:boom
// perry: 1, then Uncaught (in promise) Error: boom
It applied to every way the `try` could fail — a plain `throw`, an exception from a
callee, and an `await` of a rejecting promise — and to nested try/catch (the inner
catch was skipped too). Sync generators and plain `async` functions were correct;
only `async function*` was affected.
Cause: when a `try` contains a `yield`, the linearizer must destroy the `Stmt::Try`
and re-emit the catch body as its own states (`generator/linearize.rs`), recording a
`CatchRoute` whose `catch_entry_state` is reachable *only* by the dispatch loop
setting `__gen_state = catch_entry_state`. That routing lives in `wrap_dispatch_loop`
— and it was gated off for async generators by a `!is_async_generator` conjunct
(PerryTS#4438 enabled it for sync generators only). So for an async generator the catch
states were emitted but nothing could ever jump to them: the throw unwound past the
state loop to the resume-body handler, which force-completes the generator and
rejects. That is exactly the observed symptom.
The trigger is the *presence* of a `yield` in the try, not its execution — a `try`
with no `yield` keeps its native `Stmt::Try` and worked, which is why this hid for so
long.
Three gates lift:
- `wrap_dispatch` (lower.rs) no longer excludes async generators, so a throw
executing inside a protected region during `.next()` dispatch routes to the
matching catch's states.
- the completion-resume check on a yielding `finally` no longer excludes async
generators. It was dead code while nothing routed into those finallys; now that
throws do, a pending throw parked for a yielding `finally` has to be re-raised
after the finally body or it would be silently swallowed (the hazard called out in
`lower/abrupt.rs`).
- `throw_continuation` is now `Some(..)` for async generators as well. The legacy
async path inlined the catch body into the `.throw()` closure, which is empty once
the linearizer has moved that body into states — so `gen.throw(e)` resolved to
`{value: undefined, done: false}` instead of the value the catch yields.
Validation: a node-vs-perry differential over 14 generator shapes is byte-identical
where 4 of them previously diverged — catch after yield; catch of an awaited
rejection; yielding `finally` with the throw escaping; catch-rethrow through a
yielding `finally`; `return` through a yielding `finally`; throw inside a loop body's
try; nested catch + rethrow to an outer catch; optional catch binding; consumer-driven
`gen.throw()` routed into the catch; early `break` (the `return()` path); plus sync
generator try/catch/finally and return-through-finally as regression anchors.
perry-transform / perry-hir / perry-runtime suites pass (the one failing test,
`every_dispatch_entry_has_manifest_counterpart` for `ws::readyState`, fails identically
on a clean tree — pre-existing manifest drift, unrelated).
Found on a large esbuild-bundled CLI app whose request loop is an async generator:
its `catch` classified API errors, so with the catch dead every API error escaped the
loop and the app reported a generic execution failure instead of the specific,
actionable message Node prints.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAsync generator lowering now applies completion resume checks and dispatch-loop exception routing to async generators, and changes ChangesAsync generator exception routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AsyncGenerator
participant StateDispatchLoop
participant CatchOrFinallyState
AsyncGenerator->>StateDispatchLoop: throw completion
StateDispatchLoop->>CatchOrFinallyState: route exception
CatchOrFinallyState->>StateDispatchLoop: continue after catch or finally
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-transform/src/generator/lower.rs (1)
211-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate comments that now describe the async path incorrectly.
Lines 150-151 still say async generators never set pending completion state, and Lines 369-374 say only sync
.throw()uses the continuation loop. Both are invalid after these changes.Proposed documentation update
- // check. (Sync generators only — async never sets these, so the appended - // completion checks are inert on the async path.) + // check. Both sync and async generators use these when abrupt completion + // is routed through a yielding finally. - // instead of returning {value: undefined, done: false} and deferring to - // the next .next(). Only the sync-generator .throw() path uses this. + // instead of returning {value: undefined, done: false} and deferring to + // the next .next(). Async generators use this too when catch bodies were + // linearized into separate states.Also applies to: 875-885
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-transform/src/generator/lower.rs` around lines 211 - 215, Update the comments around the async generator handling in the lowering logic, including the descriptions near the pending completion state, continuation loop, and the additional locations around the async throw path. Remove claims that async generators never set pending completion state or that only sync .throw() uses the continuation loop, and document the shared behavior accurately without changing implementation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/perry-transform/src/generator/lower.rs`:
- Around line 211-215: Update the comments around the async generator handling
in the lowering logic, including the descriptions near the pending completion
state, continuation loop, and the additional locations around the async throw
path. Remove claims that async generators never set pending completion state or
that only sync .throw() uses the continuation loop, and document the shared
behavior accurately without changing implementation logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d80999d-d9f8-428b-8609-d3b14cd19038
📒 Files selected for processing (1)
crates/perry-transform/src/generator/lower.rs
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
1 participant
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.
In an
async function*, atry { … } catch (e) { … }whosetryblock contains ayieldnever ran its catch. The exception escaped the generator body entirely and surfaced to the consumer as an unhandled rejection:It applied to every way the
trycould fail — a plainthrow, an exception from a callee, and anawaitof a rejecting promise — and to nested try/catch (the inner catch was skipped too). Sync generators and plainasyncfunctions were correct; onlyasync function*was affected.Cause
When a
trycontains ayield, the linearizer must destroy theStmt::Tryand re-emit the catch body as its own states (generator/linearize.rs), recording aCatchRoutewhosecatch_entry_stateis reachable only by the dispatch loop setting__gen_state = catch_entry_state. That routing lives inwrap_dispatch_loop— and it was gated off for async generators by a!is_async_generatorconjunct (#4438 enabled it for sync generators only).So for an async generator the catch states were emitted but nothing could ever jump to them: the throw unwound past the state loop to the resume-body handler, which force-completes the generator and rejects.
The trigger is the presence of a
yieldin the try, not its execution — atrywith noyieldkeeps its nativeStmt::Tryand worked, which is why this hid for so long.The fix
Three gates lift:
wrap_dispatchno longer excludes async generators, so a throw executing inside a protected region during.next()dispatch routes to the matching catch's states.finallyno longer excludes async generators. It was dead code while nothing routed into those finallys; now that throws do, a pending throw parked for a yieldingfinallyhas to be re-raised after the finally body or it would be silently swallowed (the hazard called out inlower/abrupt.rs).throw_continuationis nowSome(..)for async generators as well. The legacy async path inlined the catch body into the.throw()closure, which is empty once the linearizer has moved that body into states — sogen.throw(e)resolved to{value: undefined, done: false}instead of the value the catch yields.Validation
A node-vs-perry differential over 14 generator shapes is byte-identical where 4 previously diverged: catch after yield; catch of an awaited rejection; yielding
finallywith the throw escaping; catch-rethrow through a yieldingfinally;returnthrough a yieldingfinally; throw inside a loop body's try; nested catch + rethrow to an outer catch; optional catch binding; consumer-drivengen.throw()routed into the catch; earlybreak(thereturn()path); plus sync-generator try/catch/finally and return-through-finally as regression anchors.perry-transform48 passed / 0 failed;perry-runtime1233 passed / 0 failed.Found on a large esbuild-bundled CLI app whose request loop is an async generator: its
catchclassified API errors, so with the catch dead every API error escaped the loop and the app reported a generic execution failure instead of the specific, actionable message Node prints.Summary by CodeRabbit
try/catch(andtry+yield) is split across states.yieldinsidetry/catch/finally..throw()handling so execution resumes through the normal continuation path after a caught exception.