Incorrect handling of commands in pub/sub state with RESP3

[stolyaroleh]

hiredis misbehaves when you upgrade the connection to RESP3 and then try to run commands other than SUBSCRIBE/UNSUBSCRIBE.

Both Redis source code and documentation seem to agree that it should be possible to run other commands. For example, the following should work:

HELLO 3
SUBSCRIBE foo
PUBLISH foo bar
PING
GET baz
UNSUBSCRIBE FOO

Unfortunately, when trying to run this, I trigger the following assertion:

/* No more regular callbacks and no errors, the context *must* be subscribed or monitoring. */
assert((c->flags & REDIS_SUBSCRIBED || c->flags & REDIS_MONITORING));

...because here, hiredis does not push user callbacks into replies when in pub/sub mode:

if (c->flags & REDIS_SUBSCRIBED) {
    /* This will likely result in an error reply, but it needs to be
     * received and passed to the callback. */   // <- no longer true with RESP3
    if (__redisPushCallback(&ac->sub.invalid,&cb) != REDIS_OK)
        goto oom;
} else {
    if (__redisPushCallback(&ac->replies,&cb) != REDIS_OK)
        goto oom;
}

Additionally, here, it assumes that all regular callbacks are handled before entering pub/sub mode. I think this is also incorrect, since it should be possible to receive a reply to a command interleaved with subscription messages with RESP3.

I think that, in the async case, this can be fixed by:

• keeping track of current connection protocol in redisContext
• conditionally allowing other commands depending on protocol in __redisAsyncCommand
• reordering the code in redisProcessCallbacks to first handle all push messages, and then all others

I would like to help you with this, but I wanted to ask for some preliminary feedback on how a fix should look like.
I haven't tried the above using the synchronous API yet.

[michael-grunder]

Thanks for the report. I'll try to replicate the behavior locally. The tricky part about any fix is probably making sure we don't break existing code.

[capathida]

How is it going with this problem?