RESP3 PUSH support for MONITOR command

[KowalczykBartek]

Change MONITOR in RESP3 mode to report monitored commands using the RESP3 push type, instead of as a stream of simple string replies.

This makes it possible for a client to send commands while it's in monitoring mode. It also makes it possible to use MONITOR with clients like hiredis, that handle push replies by delegating them to a push-callback provided by the user.

Old format, a stream of replies on the form

+1733939274.692107 [0 127.0.0.1:51014] "get" "a"

New format, after sending HELLO 3 and MONITOR, the connection will receive push messages on the form

>2
$7
monitor
+1733939225.708882 [0 127.0.0.1:51014] "get" "a"

Additional change: If MONITOR is called again when already in monitoring mode, the command replies with an error instead of no reply. A command with no reply confuses clients. They're expecting a reply to each command.

Closes #1428.

[zuiderkwast]

Hello! Nice!

Clang-format complains about missing space after if and after comma.

[zuiderkwast]

You need to sign off the commits using git commit -s. See the description in CONTRIBUTING.md why we need this. See also the Details link at the "DCO" failing CI job for some help how to fix it.

[codecov]

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.83%. Comparing base (3eb8314) to head (182b66e).
Report is 13 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/server.c	33.33%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1426      +/-   ##
============================================
- Coverage     70.88%   70.83%   -0.06%     
============================================
  Files           119      119              
  Lines         64652    64701      +49     
============================================
  Hits          45831    45831              
- Misses        18821    18870      +49     

Files with missing lines	Coverage Δ
src/replication.c	87.49% <100.00%> (+0.23%)	⬆️
src/server.h	100.00% <ø> (ø)
src/server.c	87.47% <33.33%> (-0.10%)	⬇️

... and 48 files with indirect coverage changes

[zuiderkwast]

LGTM

@valkey-io/core-team Please approve (or not) the changed behaviour of MONITOR described in the top comment.

It's a potentially breaking change (unless we consider it a bugfix?) so we should release it in the next major release, Valkey 9, right?

[hpatro]

LGTM

[madolson]

It's a potentially breaking change (unless we consider it a bugfix?) so we should release it in the next major release, Valkey 9, right?

It's definitely a breaking change and definitely not a bug fix. Can we implement this not as a breaking change, maybe with an optional argument? I am very much against breaking changes when possible.

[zuiderkwast]

It's a potentially breaking change (unless we consider it a bugfix?) so we should release it in the next major release, Valkey 9, right?

It's definitely a breaking change and definitely not a bug fix. Can we implement this not as a breaking change, maybe with an optional argument? I am very much against breaking changes when possible.

It was meant as a rhetorical question. :) I did add the label.

I am very much against breaking changes when possible.

I am too. Why do I keep forgetting?

I guess there are two reasons why I thought this is OK:

1. I assumed nobody uses HELLO 3 and MONITOR together, because it doesn't work the way it should. (Here, I'm probably wrong. Clients do all sorts of weird things.)
2. I think this is an error in how RESP3 is used, yes a bug. RESP3 was supposed to solve the request-response model by adding the push type to be used for any reply that is not a one-to-one match to a command. As long as there are commands that don't follow this (MONITOR and all the SUBSCRIBE commands), then clients can't rely on this without special logic for each command. Maybe the opt-in we should use is hello 4, a "RESP bug fix release", which would also fix the subscribe commands to return an in-band +OK too. ...

[KowalczykBartek]

Reading the opinions and feelings I think its more a bug than a feature :D
antirez/RESP3#21
redis/redis#6426 (comment)
:)

But that's not a problem for me to push this flag's support (of course if I will be able to implement this :)), please just write me how it should looks like

[PingXie]

This is a good change but it is indeed breaking. Also I am curious about the rationale behind tying this behavior to REPS 3? Can we consider opt'ing in via a new CLIENT CAPA instead?

[zuiderkwast]

This is a good change but it is indeed breaking. Also I am curious about the rationale behind tying this behavior to REPS 3? Can we consider opt'ing in via a new CLIENT CAPA instead?

RESP2 doesn't have pushes.... It only makes sense for RESP3 and RESP3 is already opt-in.

I can see a few ways to proceed:

1. Opt-in using client capa (e.g. HELLO 3, CLIENT CAPA monitor-pushes, MONITOR)
2. Opt-in using hello (e.g. HELLO 4, MONITOR)
3. Opt-in using monitor parameter (e.g. HELLO 3, MONITOR PUSH)
4. Just do the breaking change (HELLO 3, MONITOR)

A CLIENT CAPA just for modifying the behavior of one command seems odd. Then, an optional parameter to MONITOR seems more clean.

[PingXie]

Thanks @zuiderkwast. Yeah, "client capa" is over kill. I like option 3.