fix(exception): raise try-nesting limit to 1024 to avoid process abort#5071
Conversation
📝 WalkthroughWalkthroughThis PR raises the maximum try-block nesting depth from 128 to 1024 and adds a regression test. The constant is updated with expanded documentation explaining the new TLS sizing expectations, and a new test validates that deeply nested try frames no longer panic the process. ChangesTry Block Nesting Depth Increase
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-runtime/src/exception.rs (1)
417-438: 💤 Low valueRegression test correctly validates deep try nesting.
The test properly exercises the fix by pushing >128 try frames and verifying no panic occurs. The relative depth calculation (
base = current_try_depth()) makes it robust under shared TLS when tests run sequentially.Minor note: The assertion
pushes > 128(lines 425-428) assumesbaseis small enough to leave room for >128 additional frames. In practice this holds because the other test in this module cleans up properly, but the assertion could theoretically fail ifbase >= 895. Consider adding a comment explaining this assumption, or usingassert!(pushes > 128 || base > 0, "...")to make the constraint explicit.🤖 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-runtime/src/exception.rs` around lines 417 - 438, The assertion in test try_push_pop_beyond_old_limit_does_not_panic assumes base (from current_try_depth()) leaves room for >128 additional frames and could theoretically fail if base is large; update the test to make the assumption explicit by either adding a clarifying comment near the pushes > 128 check referencing current_try_depth()/MAX_TRY_DEPTH, or change the assertion to something like assert!(pushes > 128 || base > 0, "expected room for >128 frames beyond the old limit or nonzero base"), so the test documents/handles the edge case while still validating js_try_push and current_try_depth behavior.
🤖 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-runtime/src/exception.rs`:
- Around line 417-438: The assertion in test
try_push_pop_beyond_old_limit_does_not_panic assumes base (from
current_try_depth()) leaves room for >128 additional frames and could
theoretically fail if base is large; update the test to make the assumption
explicit by either adding a clarifying comment near the pushes > 128 check
referencing current_try_depth()/MAX_TRY_DEPTH, or change the assertion to
something like assert!(pushes > 128 || base > 0, "expected room for >128 frames
beyond the old limit or nonzero base"), so the test documents/handles the edge
case while still validating js_try_push and current_try_depth behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a18667b4-68ef-4bac-b19a-bfab01b48d38
📒 Files selected for processing (1)
crates/perry-runtime/src/exception.rs
What
Raises
MAX_TRY_DEPTHfrom 128 to 1024 incrates/perry-runtime/src/exception.rs.Why
js_try_pushstores per-threadtrystate in fixed-size arrays of length 128 andpanic!s (aborting the whole process) on the 129th simultaneously-activetryframe. Legal TypeScript — e.g. a recursive function whose body is wrapped intry/catch(recursive-descent parser, tree walker) — could hit this and crash, where Node.js keeps running. Raising the cap to 1024 removes the realistic failure cases; genuinely unbounded recursion hits a native stack overflow well before 1024 nestedsetjmpframes anyway.Notes
const fninitializer preserved) — lowest-risk change.JmpBufis 256 B, sojump_buffersgrows to ~256 KB per OS thread;ShadowSavepointis 16 B (verified). Converting the overflow into a catchableRangeErrorwas considered but deliberately deferred as higher-risk.Test
New unit test
try_push_pop_beyond_old_limit_does_not_panicpushes >128 frames and unwinds without panic.cargo test -p perry-runtime exception→ green.Closes #5065
Summary by CodeRabbit
Bug Fixes
Tests