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.

Sign up for GitHub

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

Jump to bottom

[BUG] CLIENT REPLY OFF|SKIP would disable push notifications #11874

Closed
jonnyomerredis opened this issue Mar 2, 2023 · 3 comments · Fixed by #11875
Closed

[BUG] CLIENT REPLY OFF|SKIP would disable push notifications #11874

jonnyomerredis opened this issue Mar 2, 2023 · 3 comments · Fixed by #11875
Labels
state:help-wanted No member is currently implementing this change

Comments

@jonnyomerredis
Copy link
Contributor

jonnyomerredis commented Mar 2, 2023 •
edited
Loading

Describe the bug
If client uses client reply off they will not get push notifications.
If client uses client reply skip then they will not get push notifications until the next command's reply was skipped.

To reproduce

Client 1:

root@3ea0f2ab5df9:/bionic# telnet localhost 6379
Trying ::1...
Connected to localhost.
Escape character is '^]'.
hello 3
// ...

subscribe channel
// ...
client reply off

After that, if client 2 uses publish channel xyz then client 1 will not get any notifications.

This also works with client reply skip, but after client 1 sends another command, it will get notifications as usual.

Expected behavior
CLIENT REPLY should only block command replies and not push notifications

Additional information
Observed on Redis 7.0.5, might have existed before.
@oranagra oranagra added the state:help-wanted No member is currently implementing this change label Mar 2, 2023
@oranagra oranagra moved this to Todo in Redis 7.2 Mar 2, 2023
@enjoy-binbin
Copy link
Collaborator

a simple patch seems to work 9ed1e19
@oranagra WDYT?

enjoy-binbin added a commit to enjoy-binbin/redis that referenced this issue Mar 2, 2023
@enjoy-binbin
Fix the bug that CLIENT REPLY OFF|SKIP cannot receive Pub/Sub messages
de7ce6f 
This bug seems to be there forever, CLIENT REPLY OFF|SKIP will
mark the client with CLIENT_REPLY_OFF or CLIENT_REPLY_SKIP flags.
With these flags, prepareClientToWrite called by addReply* will
return C_ERR directly. So the client can't receive the Pub/Sub
messages.

In this PR, we clear these flags when sending Pub/Sub messages,
and restore them after sending if any.

Fixes redis#11874
@enjoy-binbin enjoy-binbin mentioned this issue Mar 2, 2023
Merged
@soloestoy
Copy link
Collaborator

seems this is just a wrong usage, or if I miss something?

@oranagra
Copy link
Member

oranagra commented Mar 7, 2023

@soloestoy it may be an odd usage, but not sure a misuse.
for instance a RESP3 client subscribed to KSN, or requested client side tracking on certain keys, now issues a command it doesn't want the reply for, so it uses SKIP but then misses some push messages as well (if some other client got executed between the SKIP and the next command of this client).

@github-project-automation github-project-automation bot moved this from Todo to Done in Redis 7.2 Mar 12, 2023
oranagra added a commit that referenced this issue Mar 12, 2023
@enjoy-binbin @oranagra
Fix the bug that CLIENT REPLY OFF|SKIP cannot receive push notificati…
416842e 
…ons (#11875)

This bug seems to be there forever, CLIENT REPLY OFF|SKIP will
mark the client with CLIENT_REPLY_OFF or CLIENT_REPLY_SKIP flags.
With these flags, prepareClientToWrite called by addReply* will
return C_ERR directly. So the client can't receive the Pub/Sub
messages and any other push notifications, e.g client side tracking.

In this PR, we adding a CLIENT_PUSHING flag, disables the reply
silencing flags. When adding push replies, set the flag, after the reply,
clear the flag. Then add the flag check in prepareClientToWrite.

Fixes #11874

Note, the SUBSCRIBE command response is a bit awkward,
see redis/redis-doc#2327

Co-authored-by: Oran Agra <[email protected]>
oranagra pushed a commit that referenced this issue Mar 20, 2023
@enjoy-binbin @oranagra
Fix the bug that CLIENT REPLY OFF|SKIP cannot receive push notificati…
f8ae7a4 
…ons (#11875)

This bug seems to be there forever, CLIENT REPLY OFF|SKIP will
mark the client with CLIENT_REPLY_OFF or CLIENT_REPLY_SKIP flags.
With these flags, prepareClientToWrite called by addReply* will
return C_ERR directly. So the client can't receive the Pub/Sub
messages and any other push notifications, e.g client side tracking.

In this PR, we adding a CLIENT_PUSHING flag, disables the reply
silencing flags. When adding push replies, set the flag, after the reply,
clear the flag. Then add the flag check in prepareClientToWrite.

Fixes #11874

Note, the SUBSCRIBE command response is a bit awkward,
see redis/redis-doc#2327

Co-authored-by: Oran Agra <[email protected]>
(cherry picked from commit 416842e)
oranagra pushed a commit that referenced this issue Apr 17, 2023
@enjoy-binbin @oranagra
Fix the bug that CLIENT REPLY OFF|SKIP cannot receive push notificati…
61cf011 
…ons (#11875)

This bug seems to be there forever, CLIENT REPLY OFF|SKIP will
mark the client with CLIENT_REPLY_OFF or CLIENT_REPLY_SKIP flags.
With these flags, prepareClientToWrite called by addReply* will
return C_ERR directly. So the client can't receive the Pub/Sub
messages and any other push notifications, e.g client side tracking.

In this PR, we adding a CLIENT_PUSHING flag, disables the reply
silencing flags. When adding push replies, set the flag, after the reply,
clear the flag. Then add the flag check in prepareClientToWrite.

Fixes #11874

Note, the SUBSCRIBE command response is a bit awkward,
see redis/redis-doc#2327

Co-authored-by: Oran Agra <[email protected]>
(cherry picked from commit 416842e)
(cherry picked from commit f8ae7a4)
(cherry picked from commit 96814a32da61e5ed523864e00609a4aa6be065b3)
oranagra pushed a commit that referenced this issue Apr 17, 2023
@enjoy-binbin @oranagra
Fix the bug that CLIENT REPLY OFF|SKIP cannot receive push notificati…
7afd172 
…ons (#11875)

This bug seems to be there forever, CLIENT REPLY OFF|SKIP will
mark the client with CLIENT_REPLY_OFF or CLIENT_REPLY_SKIP flags.
With these flags, prepareClientToWrite called by addReply* will
return C_ERR directly. So the client can't receive the Pub/Sub
messages and any other push notifications, e.g client side tracking.

In this PR, we adding a CLIENT_PUSHING flag, disables the reply
silencing flags. When adding push replies, set the flag, after the reply,
clear the flag. Then add the flag check in prepareClientToWrite.

Fixes #11874

Note, the SUBSCRIBE command response is a bit awkward,
see redis/redis-doc#2327

Co-authored-by: Oran Agra <[email protected]>
(cherry picked from commit 416842e)
(cherry picked from commit f8ae7a4)
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this issue Jul 31, 2023
@enjoy-binbin @oranagra
Fix the bug that CLIENT REPLY OFF|SKIP cannot receive push notificati…
f538726 
…ons (redis#11875)

This bug seems to be there forever, CLIENT REPLY OFF|SKIP will
mark the client with CLIENT_REPLY_OFF or CLIENT_REPLY_SKIP flags.
With these flags, prepareClientToWrite called by addReply* will
return C_ERR directly. So the client can't receive the Pub/Sub
messages and any other push notifications, e.g client side tracking.

In this PR, we adding a CLIENT_PUSHING flag, disables the reply
silencing flags. When adding push replies, set the flag, after the reply,
clear the flag. Then add the flag check in prepareClientToWrite.

Fixes redis#11874

Note, the SUBSCRIBE command response is a bit awkward,
see redis/redis-doc#2327

Co-authored-by: Oran Agra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
state:help-wanted No member is currently implementing this change
Projects
Redis 7.2
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix the bug that CLIENT REPLY OFF|SKIP cannot receive push notifications enjoy-binbin/redis
4 participants
@oranagra @enjoy-binbin @soloestoy @jonnyomerredis