Skip to content
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

Clarification: Expected behavior of calling QUIT on a reconnecting client #2341

Open
JPricey opened this issue Dec 8, 2022 · 1 comment
Open
Labels

Comments

@JPricey
Copy link
Contributor

JPricey commented Dec 8, 2022

Hi,
What is the expected behavior of calling quit on a redis client which isOpen but not isReady, such as for a client which is attempting to reconnect to redis?
Currently it seems that ClientClosedError is raised immediately, even when the offline queue is enabled. Since in the normal case quit attempts to finish other commands which are in progress first before closing a connection, I might expect the client to wait until a reconnect attempt finishes after quit is called to either:

  • run queued commands and then disconnect, if the reconnect was a success
  • flush all queued commands and disconnect permanently, if the reconnect attempt failed

Thanks!

Environment:

  • Node Redis Version: 4.5.1
@JPricey JPricey added the Bug label Dec 8, 2022
@leibale
Copy link
Contributor

leibale commented Jan 24, 2023

Calling .quit() on a client that is reconnecting will wait for the client to reconnect -> execute the commands in the queue (if the offline queue is disabled, the queue will be empty) -> run the QUIT command and close the socket (TBH I haven't tried it now. We can actually forcefully close the socket if the offline queue is disabled since the queue is empty anyway, but I'm not sure if optimizing for this case is necessary...

The only exception is if you call .quit() from the error listener, which runs before the commands in the queue are flushed, therefore the QUIT command will reject with an "ECONNREFUSED" error instead of being executed on the new socket. I think that the order should be swapped - first of all flush the commands on the queue, only then emit the error (https://github.com/redis/node-redis/blob/master/packages/client/lib/client/index.ts#L278).. WDUT?

@leibale leibale mentioned this issue Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants