Skip to content

Give the parsing threads more stack, and quiet a recovered Prism panic - #78

Open
dduugg wants to merge 2 commits into
mainfrom
prism-follow-ups
Open

dduugg wants to merge 2 commits into
mainfrom
prism-follow-ups

Conversation

@dduugg

@dduugg dduugg commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Two fixes from a review of #73 after it merged. Neither problem has been released, since #73 didn't bump the version.

  • Deeply nested files no longer abort pks. Prism parses recursively, as pks's walk of its tree does, so each level of nesting takes stack. On a rayon thread's default 2 MiB, about 1,400 to 2,500 levels of nested calls, hashes, arrays or blocks overflowed it, and the whole run aborted with fatal runtime error: stack overflow. lib-ruby-parser held 16,000 levels or more. The global thread pool's threads now get 16 MiB.
  • No panic text before the non-UTF-8 warning. pks recovers from the panic that Diagnostic::message raises on a message that isn't UTF-8, but Rust's default panic hook printed it first, with a path into the cargo registry, so the run looked as if it had crashed. Now the warning is all that prints.

Design notes

  • Why 16 MiB. Prism stops nesting after 10,000 levels and reports "nesting too deep". With 16 MiB, each of 14 constructs nested 12,000 deep parses with that warning. 8 MiB still overflows on arrays, parentheses, !, splats and indexes. Method chains and elsif chains are flat in Prism's grammar but deep in its tree, so the cap doesn't stop them, but they overflowed 2 MiB only past about 16,000 levels.
  • Cost. A thread's stack is reserved, not committed, so peak memory on two large Rails apps is unchanged. The machine was busy while I measured, so I don't claim a timing result, though hyperfine showed no difference beyond its noise.
  • The panic hook. ruby-prism has no way to read a message that can't panic, so pks still catches the panic. The hook it installs leaves out only a panic raised while a message is being read, on that thread, and passes every other panic to the hook it replaced. The proper fix belongs in ruby-prism, where Diagnostic::message could replace invalid bytes instead of panicking.
  • The test's depth. A debug build takes more than ten times the stack per level. It overflows 2 MiB at about 200 levels of nested arrays and 16 MiB at about 1,300, so the test nests 500.

Test plan

  • New test: list-references on 500 nested arrays, under both parsers. It aborts without the stack change.
  • The non-UTF-8 test now requires stderr to be exactly the warning. It fails without the hook.
  • cargo fmt --check, cargo clippy --all-targets --all-features -- -Dwarnings and the full suite pass.

Prism parses recursively, as the walk of its tree does, so each level of
nesting in a file takes stack. A rayon thread has 2 MiB by default, which
with Prism holds only about 1,400 to 2,500 levels of nested calls,
hashes, arrays or blocks, where lib-ruby-parser held 16,000 or more.
Past that, the whole run aborts with a stack overflow.

Prism stops nesting after 10,000 levels and reports an error in the
file. 16 MiB holds that many levels of each construct tried, where
8 MiB doesn't for arrays, parentheses, `!`, splats or indexes. A
thread's stack is reserved, not committed, so peak memory on two
large Rails apps is unchanged.

A debug build takes more than ten times the stack per level, so the
test's 500 levels overflow 2 MiB there, but not 16 MiB.
pks catches the panic that `Diagnostic::message` raises on a message
that isn't UTF-8, but Rust's default panic hook had already printed it,
with a path into the cargo registry that ruby-prism was built from, so
the run looked as if it had crashed. A hook now leaves out a panic
raised while a message is read, on that thread only, and passes any
other to the hook it replaced.
@dduugg
dduugg marked this pull request as ready for review October 13, 2026 01:11
@dduugg
dduugg requested a review from a team as a code owner October 13, 2026 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Triage

Development

Successfully merging this pull request may close these issues.

1 participant