Repository navigation
Replace caller-depth guard with O(1) re-entrancy detection - #28
Merged
Merged
Conversation
Requiring errgonomic/rails directly, as the tests do, previously skipped lib/errgonomic.rb, so the Errgonomic error classes were undefined. The root require is cycle-safe: its own conditional rails require is guarded on Rails::Railtie, which is not yet defined.
The generated optional readers guarded recursion with caller.length > 1024, which materializes the whole backtrace on every attribute read: ~4us per read on a shallow stack, ~40us at 100 frames, versus ~0.4us for the wrapping itself. It also approximated the hazard: a deep-but-legitimate stack tripped it, while actual re-entry under 1024 frames did not. Track in-flight reads in a thread-local keyed by [object_id, attr] instead, raising RecursiveOptionalReadError at the first repeated frame. This keeps the diagnostic that motivated the guard (a clear early error instead of a SystemStackError thousands of frames later) at ~0.1us constant overhead per read, independent of stack depth. Fixes #16.
nz
enabled auto-merge
August 7, 2026 17:58
lutzcc1
reviewed
Aug 7, 2026
Comment on lines
+21
to
+33
| reads = Thread.current[:errgonomic_optional_reads] ||= {} | ||
| key = [object_id, :#{name}] | ||
| if reads[key] | ||
| raise Errgonomic::RecursiveOptionalReadError, | ||
| "\#{self.class}##{name} re-entered itself; something beneath this reader reads it again" | ||
| end | ||
|
|
||
| reads[key] = true | ||
| begin | ||
| val = super | ||
| ensure | ||
| reads.delete(key) | ||
| end |
lutzcc1
approved these changes
Aug 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
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.
Fixes #16.
Every generated optional reader began with
raise "stack too deep" if caller.length > 1024, which materializes the entire backtrace per attribute read. Measured on the gem's readers (100k reads, warm, Ruby 3.4):callerguard (as shipped)This confirms the issue's numbers: the wrapping is cheap, the guard is the entire overhead, and it grows with stack depth.
Why not just delete it (issue's option 1)
The guard existed for a reason: it once caught accidental infinite recursion through the generated readers that was otherwise hard to diagnose. That bug was fixed at some point, but nothing pins it down, so plain deletion would silently drop the diagnostic. This PR keeps the diagnostic and drops the cost — the issue's option 2:
[object_id, attribute].Errgonomic::RecursiveOptionalReadError(selectively rescuable, unlike the old bareRuntimeError) at the first repeated frame, naming the class and attribute — instead of ~1024 frames in, or aSystemStackErrorthousands of frames later.Tests
RecursiveOptionalReadErrornaming the attribute — this is the regression net for the original recursion bug: any recurrence in any form of same-reader re-entry now fails fast and legibly.Also fixes
errgonomic/railsnot requiring the gem root, which left theErrgonomicerror classes undefined when required directly (as the tests do).