Fix the bug that CLIENT REPLY OFF|SKIP cannot receive push notifications

[enjoy-binbin]

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

[oranagra]

What about other push notifications? E.g client side tracking.
I think we better handle it in networking.c since these functions that add a push message to the output buffer are not really a reply, and should not be skipped (no matter who calls them)

[enjoy-binbin]

I thought about modifying it in something like addReplyPubsubMessage or networking.c. But I can't think of any better way.

changing addReply* will cause a huge diff.

and I don't want to add extra flags to the client, some flags like, add a flag (PUSHING?) before push, clear the flag after push
then in networking.c, by checking the flag

so in the first place, i chooice this way, and then ignore the other push notifications first. maybe the client flag is the right way? do you have any other ideas?

[oranagra]

but why not put the exact same solution you implemented (backup and restore a flag) in addReplyPubsubMessage itself?

looking at the code now (i was AFK earlier), i see it's insufficient as well.
sendTrackingMessage calls addReplyPubsubMessage, but it also calls other networking.c primitives (like addReplyProto, addReplyBulkCBuffer).

also addReplyPushLen just adds the push header, and unlike addReplyPubsubMessage doesn't wrap the entire thing.

what i think we wanna do is:
add these flags addReplyPubsubMessage, but also in all places that call addReplyPushLen.
in addition we need some assertion that will help us catch bugs.
we can't just assert in addReplyPushLen that the flag is off, since the tests are unlikely to catch it, so maybe indeed a flag that disables the reply silencing flags (like PUSHING) is better.
maybe we can also make sure that any call to prepareClientToWrite when that client is not the executing_client, is done with the PUSHING flag set?

[enjoy-binbin]

so maybe indeed a flag that disables the reply silencing flags (like PUSHING) is better.

seems to be yes. i added the PUSHING flag.

maybe we can also make sure that any call to prepareClientToWrite when that client is not the executing_client, is done with the PUSHING flag set?

Is this the way to do it? (the one i commented)
the executing_client is only set in call, there are some addReply* (called by rejectCommandSds) in processCommand, in these cases, executing_client is not set, and it will crash

[oranagra]

LGTM. now maybe add a test or two?

[enjoy-binbin]

LGTM. now maybe add a test or two?

yean, the next step is to add tests, i will try to cover it