Notes on what *SUBSCRIBE returns (or doesn't return) #2327

zuiderkwast · 2023-02-16T10:42:16Z

See related discussion in redis/redis#11784.

netlify · 2023-02-16T10:42:19Z

👷 Deploy request for redis-doc pending review.

Name	Link
🔨 Latest commit	`1978cec`

itamarhaber

Thanks @zuiderkwast - I agree this helps.

Although these return nothing, I'd like to have a @return before the void's description for consistency. WDUT?
"Once the client enters..." - I know it is copied from subscribe, but I wonder about that statement's correctness when in RESP3. Perhaps we should make the distinction?

zuiderkwast · 2023-02-16T12:05:54Z

Although these return nothing, I'd like to have a @return before the void's description for consistency. WDUT?

Just a @return to add the heading? That's a good idea.

"Once the client enters..." - I know it is copied from subscribe, but I wonder about that statement's correctness when in RESP3. Perhaps we should make the distinction?

RESP3 doesn't have this limitation (if the void-or-error return value is handled correctly) but I think we can keep this around just like for SUBSCRIBE until we have a better recommendation. Better safe than sorry.

Would you like to formulate a better recommendation that I can paste here? 😀

I guess we would need to split this recommendation into one for RESP2 and one for RESP3, but I don't know if there is anywhere we can link to for more info about RESP3. Perhaps we can link to the HELLO command?

zuiderkwast · 2023-02-22T07:31:35Z

@itamarhaber would you like to have another look? I've added notes about RESP3.

ranshid · 2023-02-19T08:45:31Z

commands/psubscribe.md

@@ -7,3 +7,16 @@ Supported glob-style patterns:
 * `h[ae]llo` subscribes to `hello` and `hallo,` but not `hillo`

 Use `\` to escape special characters if you want to match them verbatim.
+
+Once the client enters the subscribed state it is not supposed to issue any other commands, except for additional `SUBSCRIBE`, `SSUBSCRIBE`, `PSUBSCRIBE`, `UNSUBSCRIBE`, `SUNSUBSCRIBE`, `PUNSUBSCRIBE`, `PING`, `RESET` and `QUIT` commands.


this is true only for resp2 right? maybe we should add that fact to the doc.

Thx for comment.

The next line (line 12) says "However, if RESP3 is used (see HELLO) it is possible for a client to issue any commands while in subscribed state.". Is this not clear enough? Should we explicitly mention RESP2 in the first sentence?

Since RESP2 is the default protocol, I thought we can mention the differences for RESP3 afterwards like this, but if you have better idea, I'm happy to change.

Btw, RESP3 is not even documented on this website yet. Itamar has a WIP PR #2120 adding the RESP3 protocol spec. Until that is merged, we don't have anywhere to link for more information about RESP3 types (push, etc.) so I think we can update these commands' docs again after the RESP3 PR is merged.

In general, we will need to figure out how we want to document the difference in RESP2 vs RESP3 for each command. For other commands we have no documentation at all for RESP3, such as that HGETALL returns a hash or that SUNION returns a set. IMO we should change this to 'returns a hash' and 'returns a set' and in the protocol spec explain how these are represented in RESP2 and RESP3, rather than explaining this on each command's doc page.

Wow to be honest I missed the following line since it does not apear in the published docs (AFAIK) I only stated it since I know currently people are missing that.

OK, I'll merge this as it is then?

ranshid · 2023-02-19T08:51:58Z

commands/subscribe.md

@@ -4,6 +4,13 @@ Once the client enters the subscribed state it is not supposed to issue any
 other commands, except for additional `SUBSCRIBE`, `SSUBSCRIBE`, `PSUBSCRIBE`, `UNSUBSCRIBE`, `SUNSUBSCRIBE`, 
 `PUNSUBSCRIBE`, `PING`, `RESET` and `QUIT` commands.


Maybe we would like to explain that this is only true for resp2?

…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 <oran@redislabs.com>

…IBE/SUNSUBSCRIBE (#11047) In unsubscribe related commands, we need to read the specified number of replies according to the number of parameters. These commands may return multiple RESP replies, and currently redis-cli only tries to read only one reply. Fixes #11046, this redis-cli bug seems to be there forever. Note that the [UN]SUBSCRIBE command response is a bit awkward see: redis/redis-doc#2327

…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 <oran@redislabs.com> (cherry picked from commit 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 <oran@redislabs.com> (cherry picked from commit 416842e)

…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 <oran@redislabs.com>

…IBE/SUNSUBSCRIBE (redis#11047) In unsubscribe related commands, we need to read the specified number of replies according to the number of parameters. These commands may return multiple RESP replies, and currently redis-cli only tries to read only one reply. Fixes redis#11046, this redis-cli bug seems to be there forever. Note that the [UN]SUBSCRIBE command response is a bit awkward see: redis/redis-doc#2327

…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 <oran@redislabs.com> (cherry picked from commit 416842e) (cherry picked from commit f8ae7a4)

…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 <oran@redislabs.com> (cherry picked from commit 416842e) (cherry picked from commit f8ae7a4) (cherry picked from commit 96814a32da61e5ed523864e00609a4aa6be065b3)

…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 <oran@redislabs.com> (cherry picked from commit 416842e) (cherry picked from commit f8ae7a4) (cherry picked from commit 96814a32da61e5ed523864e00609a4aa6be065b3)

…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 <oran@redislabs.com> (cherry picked from commit 416842e) (cherry picked from commit f8ae7a4)

…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 <oran@redislabs.com>

…IBE/SUNSUBSCRIBE (redis#11047) In unsubscribe related commands, we need to read the specified number of replies according to the number of parameters. These commands may return multiple RESP replies, and currently redis-cli only tries to read only one reply. Fixes redis#11046, this redis-cli bug seems to be there forever. Note that the [UN]SUBSCRIBE command response is a bit awkward see: redis/redis-doc#2327

Notes an what *SUBSCRIBE returns (or doesn't return)

265cb71

zuiderkwast requested a review from itamarhaber February 16, 2023 10:42

itamarhaber reviewed Feb 16, 2023

View reviewed changes

zuiderkwast added 3 commits February 16, 2023 13:21

Add @return heading

9f1f0fe

Add return info to *unsubscribe commands too

9b70f39

Add missing "the" in sentence

7478fea

zuiderkwast changed the title ~~Notes an what *SUBSCRIBE returns (or doesn't return)~~ Notes on what *SUBSCRIBE returns (or doesn't return) Feb 20, 2023

Add note about RESP3

1978cec

zuiderkwast added the to-be-merged should probably be merged soon label Feb 23, 2023

ranshid reviewed Feb 23, 2023

View reviewed changes

zuiderkwast merged commit 7310463 into redis:master Feb 23, 2023

zuiderkwast deleted the subscribe-return branch February 23, 2023 13:47

This was referenced Mar 7, 2023

[BUG] Subscribe hangs when RESP3 and push_cb are used redis/hiredis#1170

Open

Fix the bug that CLIENT REPLY OFF|SKIP cannot receive push notifications redis/redis#11875

Merged

oranagra mentioned this pull request Mar 12, 2023

redis-cli reads specified number of replies for UNSUBSCRIBE/PUNSUBSCRIBE/SUNSUBSCRIBE redis/redis#11047

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notes on what *SUBSCRIBE returns (or doesn't return) #2327

Notes on what *SUBSCRIBE returns (or doesn't return) #2327

zuiderkwast commented Feb 16, 2023

netlify bot commented Feb 16, 2023 •

edited

itamarhaber left a comment

zuiderkwast commented Feb 16, 2023

zuiderkwast commented Feb 22, 2023

ranshid Feb 19, 2023 •

edited

zuiderkwast Feb 23, 2023

ranshid Feb 23, 2023

zuiderkwast Feb 23, 2023

itamarhaber Feb 23, 2023

ranshid Feb 19, 2023

		@@ -4,6 +4,13 @@ Once the client enters the subscribed state it is not supposed to issue any
		other commands, except for additional `SUBSCRIBE`, `SSUBSCRIBE`, `PSUBSCRIBE`, `UNSUBSCRIBE`, `SUNSUBSCRIBE`,
		`PUNSUBSCRIBE`, `PING`, `RESET` and `QUIT` commands.

Notes on what *SUBSCRIBE returns (or doesn't return) #2327

Notes on what *SUBSCRIBE returns (or doesn't return) #2327

Conversation

zuiderkwast commented Feb 16, 2023

netlify bot commented Feb 16, 2023 • edited

👷 Deploy request for redis-doc pending review.

itamarhaber left a comment

Choose a reason for hiding this comment

zuiderkwast commented Feb 16, 2023

zuiderkwast commented Feb 22, 2023

ranshid Feb 19, 2023 • edited

Choose a reason for hiding this comment

zuiderkwast Feb 23, 2023

Choose a reason for hiding this comment

ranshid Feb 23, 2023

Choose a reason for hiding this comment

zuiderkwast Feb 23, 2023

Choose a reason for hiding this comment

itamarhaber Feb 23, 2023

Choose a reason for hiding this comment

ranshid Feb 19, 2023

Choose a reason for hiding this comment

netlify bot commented Feb 16, 2023 •

edited

ranshid Feb 19, 2023 •

edited