🧵Refactor receive responses thread sync#694
Merged
nevans merged 7 commits intoJun 8, 2026
Merged
Conversation
I'm not sure why this was outside the `synchronize`...
There's no good reason to release the lock only to immediately reacquire it for exception handling. That might allow commands to be started (or continued) while still propagating irrelevant response errors. By moving the `rescue` clause inside the `synchronize` block, `begin` and `end` are now superfluous and can be removed. But that makes the diff seem much larger than it should, so I'm saving it for a later branch.
Although it wasn't obvious at first glance, because nearly every possible error is caught by a `rescue Exception` clause, this is effectively no different from what was already happening. This structure makes that much more obvious. Also, by moving this code into the `ensure`, it allows us to simplify error handling elsewhere, by simply raising or re-raising an exception. This also lets us move `state_logout!` inside the `synchronize` block.
By resetting `@exception` inside the synchronize block at the end of the loop, we don't need to release and immediately reacquire the lock each time through the loop.
The receiver thread now enters the logout state _before_ signaling the conditional variables for the last time. This is less surprising than doing it afterward, and it also makes some of the other `state_logout!` calls redundant.
With this change, an EOFError `@exception` may be assigned a receiver thread backtrace. Previously, it might've been raised in the client thread which, would set the backtrace for that client thread.
This delays setting `@exception` until it can be synchronized with everything else in the `ensure` block. NOTE: this could be simplified further: drop the `rescue` clause and use `$!` in the `ensure` clause. But that seems to trigger a TruffleRuby bug: truffleruby/truffleruby#4308
nevans
deleted the
refactor-receive-responses-thread-sync-without-exception-global-in-ensure
branch
This was referenced Jun 8, 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
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.
Related to #692, but that handles code that executes in client thread(s) and this handles code that executes in the receiver thread.
Notable changes:
The receiver loop only calls
synchronizeonce per loop, to handle the response.@exceptionat the start is moved to the end (unless connection_closed).The logout state is now entered before signaling the conditional variables for the last time. This is less surprising than doing it afterward, and it also makes some of the other
state_logout!calls redundant.connection_stateis relatively new, and it is not expected that this difference should impact much code anyway.An
EOFErrormay be assigned a receiver thread backtrace, similarly to other exceptions that are raised by response reading or parsing. This is significantly improved by 🧵🥅 Reraise receiver thread errors with caller's backtrace #691.Simplified exception handling by simply letting the response reader and response parser exceptions bubble up, which allows us to assign
@exceptionin the synchronize block inside the ensure clause. This makes the loop much simpler to understand, and now we don't release and immediately re-acquire the lock (with slightly inconsistent state in the middle).NOTE: This is slightly more complex than how 🧵Refactor receive responses thread sync #693 handles it, because it uses a workaround to avoid triggering this TruffleRuby bug: Strange Exception#backtrace seen from different thread truffleruby/truffleruby#4308.