Repository navigation
Guard _set_state! against a missing env info object - #229
Open
hikmetba-bit wants to merge 1 commit into
Open
hikmetba-bit wants to merge 1 commit into
hikmetba-bit wants to merge 1 commit into
Conversation
_set_state! unconditionally called env[ENV_INFO_KEY].state = state. The heartbeat (run every second while a request is active) and the timeout callback both run asynchronously via the scheduler thread, and can fire after the request's info object is no longer present in env, raising: NoMethodError: undefined method 'state=' for nil:NilClass which terminates the scheduler thread outright (reported against real Puma/Rack usage, with a second, same-root-cause crash in StateChangeLoggingObserver#log_state_change once the notification callback fires with a nil-info env). Return early when there's no info object to update instead of crashing - there's nothing meaningful to set the state on or notify observers about at that point, and StateChangeLoggingObserver reads env[ENV_INFO_KEY] itself, so skipping the observer notification here also prevents its crash. Fixes zombocom#225.
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.
Summary
Fixes #225.
_set_state!unconditionally didenv[ENV_INFO_KEY].state = state. The heartbeat (which re-fires every second while a request is:active) and the timeout callback both run asynchronously via the scheduler thread and can execute after the request'sRequestDetailsinfo object is no longer present inenv, raising:which terminates the scheduler thread outright — reported against real Puma/Rack production usage in the issue, along with a second crash with the same root cause in
StateChangeLoggingObserver#log_state_change(undefined method 'state' for nil:NilClass) once the state-change notification fires with a nil-infoenv.Fix
Return early from
_set_state!when there's no info object to update — there's nothing meaningful to set state on or notify observers about at that point. SinceStateChangeLoggingObserver#log_state_changere-readsenv[ENV_INFO_KEY]itself rather than receiving the info object directly, skipping the observer notification here also prevents its crash, so one guard covers both reported traces.This doesn't attempt to fix why the info object goes missing in the reporter's setup (that's environment/timing-specific and outside what's cleanly diagnosable from the issue alone) — it converts an unhandled crash that kills the scheduler thread into a safe no-op, which is what the reporter themselves suggested: "should rack-timeout check that env[ENV_INFO_KEY] is either a valid object... before trying to call state on it?"
Test
Added
test/set_state_test.rbwith 3 cases:_set_state!no longer raises whenenvhas no info object (the regression case), it still updates state normally when the info object is present, and it still raises for an invalid state argument (existing validation preserved). Mutation-tested: reverting the source change makes the first test fail with the exact reportedNoMethodError; with the fix, full suite is green (bundle exec rake test, 9/9).🤖 Generated with Claude Code