Redis Client tracking pushes null invalidate upon flush

Flushing a Redis appears to create an "invalidate" message containing a null byte. Is this intentional? If so, it's undocumented.

Reproduction, using RESP3:

client tracking on
flushall

Message received is: >2\r\n$10\r\ninvalidate\r\n*1\r\n$1\r\n\0\r\n

It would be great to document RESP3 push messages in general, especially invalidate.

Comment From: madolson

It is intended behavior. On flushing all, or most of, the data we don't want to send invalidation messages for every key you might have tracked (That would be very expensive) so we send a single NULL message instead. I agree about the missing documentation. Might be better to create a request over in the documentation repo: https://github.com/redis-io/redis-doc

Comment From: lukepalmer

I actually think there might be a bug, and that the code below intends to send an empty string but instead sends a string with one null byte. This is good, because a null byte is a valid redis key that would otherwise be indistinguishable from the "invalidate everything" message.

Specifically, the call to sendTrackingMessage specifies length 1 but should instead use length 0.

Do you agree?

void trackingInvalidateKeysOnFlush(int dbid) {
    if (server.tracking_clients) {
        listNode *ln;
        listIter li;
        listRewind(server.clients,&li);
        while ((ln = listNext(&li)) != NULL) {
            client *c = listNodeValue(ln);
            if (c->flags & CLIENT_TRACKING) {
                sendTrackingMessage(c,"",1,0);
            }
        }
    }

Comment From: soloestoy

One problem.

This is good, because a null byte is a valid redis key that would otherwise be indistinguishable from the "invalidate everything" message.

User can use NULL key name, just like SET "" v, I'm not sure if some users do that, but it's valid. So if we send NULL message, it may be confused if it is FLUSHALL or a real key name?

Long before that, when we use slot to tracking, we user -1 to represent FLUSHALL, but now maybe we need a new channel.

Comment From: lukepalmer

@soloestoy you're right. I'm not sure how common empty string is going to be but completeness is important here. The null byte was actually not an edge case for me, because in some languages the None case of an Option type serializes to a null byte.

Maybe we need a push message that says "flush". In any case this sounds like a change rather than a bugfix, which means that I need to talk to maintainers for discussion on a fix.

Comment From: madolson

@soloestoy One possible fix is to send an actual "NULL" back, which has its own special character in RESPV2 and RESPV3. I think it's $-1/r/n and _\n. We should figure out if we think we'l need more special characters. Having to listen to multiple channels sounds like it will overly complicate client implementations.

Comment From: lukepalmer

@madolson I like that, it's simpler than what I thought of first. Happy to implement if people are on board.

Comment From: soloestoy

Yes @madolson you're right, I forgot that. The null object is here:

    /* The shared NULL depends on the protocol version. */
    shared.null[0] = NULL;
    shared.null[1] = NULL;
    shared.null[2] = createObject(OBJ_STRING,sdsnew("$-1\r\n"));
    shared.null[3] = createObject(OBJ_STRING,sdsnew("_\r\n"));

Comment From: madolson

@lukepalmer Sounds like that is probably the best fix for now, so I think you can go and implement it if you want.

Comment From: lukepalmer

I implemented null on invalidate and updated the pull request.

I'd love to get this merged if everyone likes this.