Skip to content

Fix #1870: TypeError in PubSub #2016

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 7, 2022
Merged

Conversation

jhiesey
Copy link
Contributor

@jhiesey jhiesey commented Mar 1, 2022

Description

Fix race that causes #1870.

The root cause of that issue is a race in updating PubSubState that causes the parser to be in string mode instead of buffer mode when a PubSub 'message' event arrives from the server in the same data chunk as the 'subscribe' reply arrives.

The sequence of events triggering the issue:

  1. Application code calls .subscribe() on the client object
  2. The redis server returns single chunk containing both 'subscribe' and 'message' replies in the same buffer that goes into RedisCommandsQueue.parseResponse
  3. The parser calls returnReply with the reply 'subscribe'. This goes to this line:
    this.#shiftWaitingForReply().resolve();
  4. shiftWaitingForReply removes the 'subscribe' message from this.#waitingForReply, then calls #setReturnBuffers. Since this.#waitingForReply is now empty and this.#pubSubState.subscribed hasn't been updated yet, it turns buffer mode off (switches to string mode)
  5. Once shiftWaitingForReply returns, .resolve() gets called here:
    this.#shiftWaitingForReply().resolve();
    This sets this.#pubSubState.subscribed to 1 here:
    pubSubState.subscribed += channelsCounter;
  6. The parser calls returnReply again with the reply 'message', but it's now in string mode, so this Buffer.equals() call throws:
    if (RedisCommandsQueue.#PUB_SUB_MESSAGES.message.equals(reply[0])) {

If the 'subscribe' and 'message' replies come in separate chunks, the 'message' reply will turn buffer mode back on through this line, preventing the problem:

The fix is to also enable buffer mode on the parser in setReturnBuffers if this.#pubSubState.subscribing is nonzero, which avoids this race. As far as I can tell this is correct behavior.

I have added a test which passes reliably with this patch, and (on my machine) fails without the patch. Since it is testing for a race, it may sometimes pass even without this patch.


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)?

@jhiesey jhiesey marked this pull request as ready for review March 1, 2022 10:06
This test is for a race condition; it should reliably pass now,
but is not guaranteed to fail with the previous version.
@jhiesey jhiesey force-pushed the fix-pubsub-type-error branch from c145891 to e9f9f9f Compare March 1, 2022 19:09
@leibale
Copy link
Contributor

leibale commented Mar 2, 2022

@jhiesey nice catch! :)

@codecov-commenter
Copy link

codecov-commenter commented Mar 2, 2022

Codecov Report

Merging #2016 (e9f9f9f) into master (00b58e8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2016   +/-   ##
=======================================
  Coverage   94.94%   94.94%           
=======================================
  Files         358      358           
  Lines        3343     3344    +1     
  Branches      409      410    +1     
=======================================
+ Hits         3174     3175    +1     
  Misses         85       85           
  Partials       84       84           
Impacted Files Coverage Δ
packages/client/lib/client/commands-queue.ts 80.39% <100.00%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00b58e8...e9f9f9f. Read the comment docs.

@leibale leibale merged commit f79e14c into redis:master Mar 7, 2022
@jhiesey jhiesey deleted the fix-pubsub-type-error branch September 1, 2022 19:14
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.

3 participants