Skip to content

fix(client): keep decoding a chunk when a listener throws - #3486

Open
maxymlyskov wants to merge 1 commit into
redis:masterfrom
maxymlyskov:fix/keep-decoding-when-a-listener-throws
Open

maxymlyskov wants to merge 1 commit into
redis:masterfrom
maxymlyskov:fix/keep-decoding-when-a-listener-throws

Conversation

@maxymlyskov

@maxymlyskov maxymlyskov commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Description

When a pub/sub, MONITOR or invalidate listener throws, the client loses the rest of the socket chunk the message was in. The listeners run inside decoder.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 #onReply back, 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, with m1..m3 published together and m4 after, and a RESP3 client that subscribes and runs publish, get k1, get k2 in one go, then get k3:

# master
PROBE RESP2 subscriber: published m1..m4, received m1; errors: listener bug on m1 | Cannot read properties of undefined (reading 'resolve')
PROBE RESP3 one client: publish, get k1, get k2, then get k3 -> 1, "v3", still pending, still pending; errors: listener bug
# this branch
PROBE RESP2 subscriber: published m1..m4, received m1,m2,m3,m4; errors: listener bug on m1
PROBE RESP3 one client: publish, get k1, get k2, then get k3 -> 1, "v1", "v2", "v3"; errors: listener bug

The decoder now runs each callback in a try/catch, keeps decoding, and write() returns the errors. The data handler emits them as 'error' after write() returns, without a reset. The catch is in the decoder because every listener call goes through it. A protocol error from write() 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/client suite with CI's command fails the same 9 cases on master and on this branch on my Windows machine.


Checklist

  • Does npm test pass with this change (including linting)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

Note

Medium Risk
Changes core socket read/decode error handling for all connections; behavior is better isolated (listener vs protocol errors) but affects when error fires relative to other events in a chunk.

Overview
Fixes a bug where a throwing pub/sub, MONITOR, or invalidate listener 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 / onErrorReply callbacks in try/catch, keeps parsing the rest of the chunk, and write() 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 data handler uses that return value: it only resets the decoder when write() itself throws, and emits listener failures as error after 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.

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 nkaradzhov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants