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

Add command "Client Capa subv2" to change behavior for SUBSCRIBE and SSUBSCRIBE commands: one call with only one response #962

Open
wants to merge 9 commits into
base: unstable
Choose a base branch
from

Conversation

hwware
Copy link
Member

@hwware hwware commented Aug 28, 2024

Now for the command SUBSCRIBE and SSUBSCRIBE, if any client call with multiply channels, multiply responses come back.

image

It does not match with the Valkey command response logic, it causes client side to parse the response with extra effort on them. The problem exists long time.

Thus, to fix this design error, I add a new command parameter: client capa subv2
The command "client capa" is imported in Valkey 8, and I add a new parameter "subv2" for it
Note: subv2 is just a temporarily name, I have no idea which name is better, welcome any suggestion.

The update behavior as below: one call with one response

image

Note: for existing users, I think this is a break change. If any client chooses the behavior that one call with one response, the command "client capa subv2" can be called first.

@hwware hwware force-pushed the update-subscribe-command-2-one-reply branch from cbdafa8 to c4fff33 Compare August 28, 2024 14:50
Copy link

codecov bot commented Aug 28, 2024

Codecov Report

Attention: Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.67%. Comparing base (06cfe2c) to head (d7117c1).

Files with missing lines Patch % Lines
src/pubsub.c 95.65% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #962      +/-   ##
============================================
+ Coverage     70.61%   70.67%   +0.05%     
============================================
  Files           114      114              
  Lines         61740    61763      +23     
============================================
+ Hits          43599    43648      +49     
+ Misses        18141    18115      -26     
Files with missing lines Coverage Δ
src/networking.c 88.04% <100.00%> (-0.42%) ⬇️
src/server.h 100.00% <ø> (ø)
src/valkey-cli.c 55.66% <100.00%> (+0.12%) ⬆️
src/pubsub.c 96.23% <95.65%> (-0.14%) ⬇️

... and 15 files with indirect coverage changes

@hwware hwware changed the title Change behaviour for SUBSCRIBE and SSUBSCRIBE commands: one request with only one response Change behaviour for SUBSCRIBE and SSUBSCRIBE commands: one call with only one response Aug 28, 2024
@hwware hwware changed the title Change behaviour for SUBSCRIBE and SSUBSCRIBE commands: one call with only one response Change behavior for SUBSCRIBE and SSUBSCRIBE commands: one call with only one response Aug 28, 2024
@hwware hwware added the breaking-change Indicates a possible backwards incompatible change label Aug 28, 2024
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a design error from the beginning, and another design error when RESP3 was introduced, but it is too late to change it now.

This is a serious breaking change that will break all clients that support pubsub. This is very dangerous to do, considering there are many redis-compatible services out there and clients try to be compatible with not only valkey.

if (c->resp == 2)
addReply(c, shared.mbulkhdr[number]);
else
addReplyPushLen(c, number);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So with RESP3, this is still no normal in-band replies and one push message (instead of one per channel). This is still the main problem of [UN][P|S]SUBSCRIBE IMO.

To fix this behaviour, SUBSCRIBE should return an in-band reply. Either the list of channels as an array or map-reply, or just +OK and send the channels as push.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not want to import this change in Valkey 8.0. I wish we could do this change on Valkey 8.X or 9 version. Error need to be correct, never late ^_^

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we cannot fix it ever. Only maybe with a CLIENT CAPA or RESP4.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree with this being a potentially significant breaking change. I think maybe CLIENT CAPA could fix it (or RESP4).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I update the design, add a new parameter for client capa command, but the parameter name is just temporarily. If you have better name, please let me know or we can discuss it in the meeting

@zuiderkwast zuiderkwast added the major-decision-pending Major decision pending by TSC team label Aug 28, 2024
@hwware hwware force-pushed the update-subscribe-command-2-one-reply branch from c4fff33 to 3af8b90 Compare August 29, 2024 14:32
@enjoy-binbin enjoy-binbin added the release-notes This issue should get a line item in the release notes label Sep 4, 2024
@hwware hwware force-pushed the update-subscribe-command-2-one-reply branch 2 times, most recently from 30403a0 to 8bae942 Compare September 12, 2024 14:51
@hwware hwware changed the title Change behavior for SUBSCRIBE and SSUBSCRIBE commands: one call with only one response Add command "Client Capa subv2" to change behavior for SUBSCRIBE and SSUBSCRIBE commands: one call with only one response Sep 12, 2024
@hwware hwware added enhancement New feature or request and removed breaking-change Indicates a possible backwards incompatible change labels Sep 12, 2024
@hwware hwware force-pushed the update-subscribe-command-2-one-reply branch from 8bae942 to 79f7352 Compare September 16, 2024 17:25
@hwware hwware force-pushed the update-subscribe-command-2-one-reply branch from 79f7352 to f7ed3e2 Compare September 24, 2024 00:56
@zuiderkwast
Copy link
Contributor

@hwware What exactly is the problem with the old behaviour according to you?

According to the docs, when a client enters subscribed mode, the protocol changes semantics from a request-response protocol to a push protocol. The commands and the responses are no longer related to each other. This was for RESP2 and many commands are forbidden in subscribed mode to make sure the client can understand the responses. It is a limited number of possible responses. The client can receive pubsub messages and notifications that a channel is subscribed or unsubscribed, asynchronously, but not exactly as a reply to each command.

In RESP3, it is different, because here clients can call any command in subscribed mode. The reason for allowing this was that RESP3 has a push (out-of-band) type. The client just needs to know the very strange fact that SUBSCRIBE and UNSUBSCRIBE receive no in-band reply at all.

Now, there are two things that make this more complicated:

  1. [S|P][UN]SUBSCRIBE can return an error. An error is in-band. An error can happen due to ACL rules and other reasons. For sharded-pubsub, it can even be a MOVED redirect or a CLUSTERDOWN error (slot not served). To check for errors, a client needs to wait for at least one 1) "subscribe" 2) "ch" 3) 1 out-of-band message to be sure there was no error.
  2. For sharded-pubsub, when a slot is migrated, the subscribers on the migrating node are automatically unsubscribed. They receive a message like 1) "sunsubscribe" 2) "ch" 3) 0 which is not matching any command the client has sent. It is just a spontaneous message.

Most of this is possible for clients to handle, even if it is very complex to handle error replies in subscribed mode, but one problem is not possible to solve. This one: #1066.

I have another idea that can solve the issue of matching request-response pairs:

CLIENT CAPA subscribe-ok: Makes the [S|P][UN]SUBSCRIBE return +OK on success and an error otherwise, so there is always exactly one in-band reply to each command. The notifications for each channel are received out-of-band as before.

@hwware hwware force-pushed the update-subscribe-command-2-one-reply branch from f7ed3e2 to c97985f Compare September 26, 2024 00:54
@hwware
Copy link
Member Author

hwware commented Sep 26, 2024

@hwware What exactly is the problem with the old behaviour according to you?

According to the docs, when a client enters subscribed mode, the protocol changes semantics from a request-response protocol to a push protocol. The commands and the responses are no longer related to each other. This was for RESP2 and many commands are forbidden in subscribed mode to make sure the client can understand the responses. It is a limited number of possible responses. The client can receive pubsub messages and notifications that a channel is subscribed or unsubscribed, asynchronously, but not exactly as a reply to each command.

In RESP3, it is different, because here clients can call any command in subscribed mode. The reason for allowing this was that RESP3 has a push (out-of-band) type. The client just needs to know the very strange fact that SUBSCRIBE and UNSUBSCRIBE receive no in-band reply at all.

Now, there are two things that make this more complicated:

  1. [S|P][UN]SUBSCRIBE can return an error. An error is in-band. An error can happen due to ACL rules and other reasons. For sharded-pubsub, it can even be a MOVED redirect or a CLUSTERDOWN error (slot not served). To check for errors, a client needs to wait for at least one 1) "subscribe" 2) "ch" 3) 1 out-of-band message to be sure there was no error.
  2. For sharded-pubsub, when a slot is migrated, the subscribers on the migrating node are automatically unsubscribed. They receive a message like 1) "sunsubscribe" 2) "ch" 3) 0 which is not matching any command the client has sent. It is just a spontaneous message.

Most of this is possible for clients to handle, even if it is very complex to handle error replies in subscribed mode, but one problem is not possible to solve. This one: #1066.

I have another idea that can solve the issue of matching request-response pairs:

CLIENT CAPA subscribe-ok: Makes the [S|P][UN]SUBSCRIBE return +OK on success and an error otherwise, so there is always exactly one in-band reply to each command. The notifications for each channel are received out-of-band as before.

Hi @zuiderkwast As what you said, the current behavior is that the protocol changes semantics from a request-response protocol to a push protocol if the client enters subscribed mode. The commands and the responses are no longer related to each other. Thus, the client could receive multiply responses from one command. It results the client side (such as Java side, or Javascript etc) need to check which mode or protocol the server current supports now. It causes the client side programming more complex.

Thus, in my PR, I think we should unify the request-response protocol and push protocol response way , make the response always matches with the command (1:1). We should try our best to decrease the client programming complexity. (Or by my pr, give the client one option)

@zuiderkwast
Copy link
Contributor

Thus, in my PR, I think we should unify the request-response protocol and push protocol response way , make the response always matches with the command (1:1). We should try our best to decrease the client programming complexity. (Or by my pr, give the client one option)

OK, but there are responses that can't be matched with a command, the messages sent by PUBLISH:

1) "message"
2) "ch"
3) "hello"

(And additionally the spontaneous sunsubscribe message which is sent by the server when a slot is deleted or migrated.)

That's why in RESP2 can never be 1:1 request-response protocol in pubsub. We need an out-of-band message for this, which we have in RESP3.

If we just use it correctly, pubsub can be a 1:1 where only in-band replies are matched with a command and push messages are out-of-band. Clients that want better semantics can use RESP3.

@zuiderkwast
Copy link
Contributor

CLIENT CAPA is not enough, because the client needs to know if the server supports the feature too. The client can check the response of HELLO to see the Valkey version. Can the client trust the version number to infer the subscribe behaviour?

Alternatives:

  • Use a command to enable this behaviour, such as CLIENT SUBSCRIBE-OK ON. If the server doesn't support this command, the client will receive an error reply. Similar to CLIENT TRACKING ON, CLIENT NO-TOUCH ON and CLIENT NO-EVICT ON.
  • Add new commands for subscribing and unsubscribing, such as SSUBSCRIBE2, PUNSUBSCRIBE2, etc. that return OK or ERR. It is many new commands though.

@hwware hwware force-pushed the update-subscribe-command-2-one-reply branch from c97985f to a0c0586 Compare October 7, 2024 06:24
@hwware hwware force-pushed the update-subscribe-command-2-one-reply branch from a0c0586 to d7117c1 Compare October 16, 2024 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request major-decision-pending Major decision pending by TSC team release-notes This issue should get a line item in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants