Skip to content

Conversation

@florian-schunk
Copy link
Contributor

Description

The purpose of this PR is to remove the restriction that command timeouts only apply while the command is still waiting to be sent, so that a slow answer from redis also triggers the timeout.

However testing this code, I noticed some discrepancies between running the tests on my local machine and the pipeline, so I'm opening this PR now, before the development is fully finished to see how the pipeline processes the tests so far.

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

@nkaradzhov
Copy link
Collaborator

Hi @florian-schunk, apologies for the late reply. Im still loaded on other work and Im not sure when i will be able to take more thorough look at this.

Here is a scenario where this approach breaks: You send two commands (cmdA, cmdB). cmdA expires, server then replies for cmdA:

  +--------+             +--------+
  | Client |             | Redis  |
  +--------+             +--------+
       |                      |
       |------ cmdA --------->|
       |                      |
       |------ cmdB --------->|
       |                      |
       |↺ cmdA expired        |
       |                      |
       |<---- cmdA reply -----|
       |                      |

When we expire a command, we just remove it from the queue. That means we will have only cmdB remaining in the queue when we receive the response for cmdA.

And look at how we handle a response:

  #onReply(reply: ReplyUnion) {
    this.#waitingForReply.shift()!.resolve(reply);
  }

This means we will wrongly think the response for cmdA is the response for cmdB

Im not sure how many other corner cases will pop out, but we need to very carefully consider all posibilities as this has the potential to break stuff in a very bad way. ( hence why I was reluctant to work on this :D )

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