Skip to content

Wait until next tick before handling socket errors #1603

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jakepruitt
Copy link

Description

Per #1593, there is a currently a race condition where an error in a socket.write call can cause the error handler (which clears the command queue) to be fired before the write command is added to the write queue, causing a permanent break in the command queue.

While this situation is rare (it occurs once out of 20 tries to reproduce it) it causes significant pain on downstream consumers, which expects node-redis to return the correct responses. Currently, unless the client implements extra verification that the value returned from node-redis is correct, the client can be working with the completely wrong data in their application.

Description your pull request here

This change adds as little intrusion as possible - wrapping the on_error call in a process.nextTick() to ensure the assumption that the code makes about the socket error handling control flow will always be true. (The code currently assumes that the code inside internalSendCommand will execute to completion before any error handlers are called.)


Checklist

  • Does npm test pass with this change (including linting)?
  • Is the new or changed code fully tested?
    • Since this issue is so intermittent, I am not sure how to write an appropriate test. If anyone has advice, I would appreciate it.
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)? N/A

@OzQu
Copy link

OzQu commented Jun 15, 2022

I assume this can't be merged as is, because the repository has been upgraded quite a lot after this PR.
I assume the similar fix could be implemented in here

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