Skip to content

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

Merged
dduugg merged 4 commits into
mainfrom
prism-follow-ups
Oct 13, 2026
Merged

dduugg merged 4 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, and so does the thread the command runs on, since rayon parses a lone file, as check <file> and check-contents have, on the thread that asks.
  • 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 most constructs after 10,000 levels and reports "nesting too deep". In a release build 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.
  • What 16 MiB doesn't hold. Prism doesn't cap everything. Method chains and elsif chains are flat in Prism's grammar but deep in its tree, and they overflowed 2 MiB only past about 16,000 levels; with 16 MiB, a chain of 100,000 links parses and one of 200,000 aborts. Nested case/in array patterns abort between 30,000 and 40,000 levels, and hash patterns between 20,000 and 30,000. A real fix for those needs walks that don't recurse.
  • The command's thread. On the main thread, which has 8 MiB on macOS and Linux and 1 MiB on Windows, a single file nested 9,990 deep aborted check and check-contents even with 16 MiB on the pool. cli::run keeps the global pool and spawns a thread for the command, rather than running the command in a pool with install, so that the command doesn't hold one of the pool's threads: with RAYON_NUM_THREADS=1, jwalk would wait a second for the busy pool and then walk nothing.
  • 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 tests' depths. A debug build takes more than ten times the stack per level. It overflows 2 MiB at about 200 levels of nested arrays, 8 MiB at about 700 and 16 MiB at about 1,350, so the list-references test nests 500 and the check test 1,000.

Test plan

  • New test: list-references on 500 nested arrays, under both parsers. It aborts without the pool's stack.
  • New test: check on one file of 1,000 nested arrays, under both parsers. It aborts on the main thread without the command's thread.
  • The non-UTF-8 test now requires stderr to be exactly the warning. It fails without the hook.
  • New test: a panic after a recovered one still prints. It fails if the hook swallows every panic, never sets or never clears its flag, or isn't installed.
  • With a release build, check <file>, check-contents and list-references on a file nested 9,990 deep finish under both parsers.
  • 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.
@github-project-automation github-project-automation Bot moved this to Triage in Modularity Oct 13, 2026
@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
rayon runs a `par_iter` that won't split on the thread that calls it, so
a command with one file to parse, as `check <file>` and `check-contents`
have, parsed it on the main thread. That thread has 8 MiB on macOS and
Linux and 1 MiB on Windows, so in a release build a file nested about
9,990 deep still aborted pks. `cli::run` now runs the command on a
thread with the same 16 MiB as the pool's.

It keeps the global pool and spawns a thread, rather than running the
command inside a pool with `install`, so that the command doesn't hold
one of the pool's threads. With `RAYON_NUM_THREADS=1`, jwalk would wait
a second for the busy pool and then walk nothing.

The comment on the stack size now says it holds 10,000 levels in a
release build, where Prism stops nesting most constructs. A debug build
overflows 16 MiB at about 1,350 levels, and nested `case`/`in` patterns
and long method chains go past Prism's cap.

The new test runs `check` on one file nested 1,000 deep, which overflows
an 8 MiB thread in a debug build.
A hook that swallowed every panic, or a flag left set after reading a
message, passed the tests, since none of them checked that a panic still
prints. The new test reads a message that panics and then panics itself,
in a child of the test binary since the hook is the process's, and
checks that only the second panic printed.
@dduugg
dduugg merged commit 45ecc84 into main Oct 13, 2026
21 checks passed
@dduugg
dduugg deleted the prism-follow-ups branch October 13, 2026 18:42
@github-project-automation github-project-automation Bot moved this from Triage to Done in Modularity Oct 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant