fix(client): keep decoding a chunk when a listener throws - #3486
Open
maxymlyskov wants to merge 1 commit into
Open
maxymlyskov wants to merge 1 commit into
maxymlyskov wants to merge 1 commit into
Conversation
Pub/sub, MONITOR and 'invalidate' listeners run inside Decoder.write(). When one threw, the socket data handler reset the decoder: the rest of the chunk was dropped, so later replies resolved the commands in front of their own, and the reset put plain #onReply back, which ended message delivery on a RESP2 subscriber and on MONITOR. The decoder now catches a callback's error, keeps decoding the chunk and returns the errors from write(). The data handler emits them as 'error' without resetting the decoder. A protocol error still resets it.
nkaradzhov
approved these changes
Oct 2, 2026
nkaradzhov
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for this fix, @maxymlyskov, and for the clear write-up and real-server repro. I traced the path and confirmed the bug. We'll merge this once CI passes.
One request for future contributions: for anything beyond a small, isolated fix, please open an issue first and wait for a maintainer to confirm the approach before you write code, as described in our contributing guide. Without a linked, agreed issue, we may pause review of a PR until the scope is discussed. Agreeing on scope early saves you rework and gets your PR reviewed faster.
Thanks again!
This branch has not been deployed
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.
Description
When a pub/sub, MONITOR or
invalidatelistener throws, the client loses the rest of the socket chunk the message was in. The listeners run insidedecoder.write(chunk), so the throw stops it mid-chunk, and the'data'handler resets the decoder and emits'error'. Every later frame in that chunk is dropped.The dropped replies leave their commands at the head of the queue, so every later reply goes to the wrong command until a reconnect. The reset also puts the plain
#onReplyback, which removes the RESP2 pub/sub handler and the MONITOR callback, so neither gets another message.Repro on Redis 8.2.2: a RESP2 subscriber whose listener throws on
m1, withm1..m3published together andm4after, and a RESP3 client that subscribes and runspublish,get k1,get k2in one go, thenget k3:The decoder now runs each callback in a try/catch, keeps decoding, and
write()returns the errors. The data handler emits them as'error'afterwrite()returns, without a reset. The catch is in the decoder because every listener call goes through it. A protocol error fromwrite()itself still resets the decoder as before.The
'error'now fires after the rest of the chunk is decoded, so later messages in that chunk are delivered first. Without an'error'listener the process still exits, as on master.The new decoder case and the three PubSub cases in index.spec.ts fail on master and pass here. The
packages/clientsuite with CI's command fails the same 9 cases on master and on this branch on my Windows machine.Checklist
npm testpass with this change (including linting)?Note
Medium Risk
Changes core socket read/decode error handling for all connections; behavior is better isolated (listener vs protocol errors) but affects when
errorfires relative to other events in a chunk.Overview
Fixes a bug where a throwing pub/sub, MONITOR, or
invalidatelistener aborted RESP decoding mid-chunk, which dropped later frames in the same TCP read and could misalign command replies until reconnect.The RESP decoder now wraps
onReply/onPush/onErrorReplycallbacks in try/catch, keeps parsing the rest of the chunk, andwrite()returns any collected listener errors (still throws on true protocol/decode failures). NULL replies are routed through the same path so a throwing reply handler no longer bypasses error collection.The client socket
datahandler uses that return value: it only resets the decoder whenwrite()itself throws, and emits listener failures aserrorafter the chunk is fully processed—so later messages and command replies in the same chunk still land correctly.Regression coverage adds a decoder unit test and integration tests for RESP2/3 pub/sub delivery and reply/command pairing when a listener throws.
Reviewed by Cursor Bugbot for commit 8164f60. Bugbot is set up for automated code reviews on this repo. Configure here.