Delay *SUBSCRIBE inside transactions

[joshleeb]

This change updates the Redis server to deny (P)SUBSCRIBE commands when attempting to execute inside a transaction.

Closes #2967

[madolson]

I wonder if anyone today is doing a multi-exec on a bunch of subscribes. I think a better solution would be to continue allowing subscribe in a multi-exec but deferring all of the published messages to the end. That should fix the resp corruption. @oranagra thoughts?

[oranagra]

considering that the default for redis-py pipeline() is transaction (very misleading IMHO), I think there's a chance that some people who wanted just to issue many subscription in a pipeline are getting a transaction by mistake, and this PR will break them.
while they might deserve that breakage and will easily fix their code, i'm not certain it's a wise move on our behalf.

We did recently add some logic to defer client side tracking invalidation messages to after EXEC responses, maybe that's something we need to do for PUBLISH as well, (even regardless of whether SUBSCRIBE was issues in the transaction or not).

@itamarhaber are you aware of any other considerations?

[itamarhaber]

Given redis-py's default, and the possible existence of this pattern in the wild regardless, I'd hate breaking it by denying/reordering. Per Oran's suggestion, deferring pushed PUBLISHed messages until after EXEC makes sense to me and shouldn't harm anyone (unless they dug that hole intentionally, in which case they deserve it).

[madolson]

Ok, @joshleeb given the consensus seems to be against this suggestion, despite what salvatore wrote, do you mind updating this PR so that we defer message publishing until the end of the subscribe? This is probably a more complex change, so no worries if you don't want to take it.

[joshleeb]

do you mind updating this PR so that we defer message publishing until the end of the subscribe?

Yep happy to give it a shot 👍

We did recently add some logic to defer client side tracking invalidation messages to after EXEC responses

It appears to me that this change was made in #9422. Is this the PR being referred to?

Related to deferring PUBLISHed messages until after EXEC (but perhaps to be addressed as a separate PR) in #2967 (comment) it is mentioned that

Normally only a few selected commands are allowed in subscribed mode, but because this is only checked on receive of a command, which is inside a transaction for you and thus only queued, not executed, Redis will happily execute it on EXEC (at which point it does not recheck your client's status)

Is this something we want to change as well? That is, in CLIENT_MULTI mode we will allow *SUBSCRIBE commands to be queued and after which the server should reject queuing any commands that are not part of the commands allowed in CLIENT_PUBSUB mode until an UNSUBSCRIBE command is queued.

[oranagra]

We did recently add some logic to defer client side tracking invalidation messages to after EXEC responses

It appears to me that this change was made in #9422. Is this the PR being referred to?

yes

Related to deferring PUBLISHed messages until after EXEC (but perhaps to be addressed as a separate PR) in #2967 (comment) it is mentioned that

Normally only a few selected commands are allowed in subscribed mode, but because this is only checked on receive of a command, which is inside a transaction for you and thus only queued, not executed, Redis will happily execute it on EXEC (at which point it does not recheck your client's status)

Is this something we want to change as well? That is, in CLIENT_MULTI mode we will allow *SUBSCRIBE commands to be queued and after which the server should reject queuing any commands that are not part of the commands allowed in CLIENT_PUBSUB mode until an UNSUBSCRIBE command is queued.

well, if we change that, maybe there's no need to defer the publish for later.
maybe that's the mode "correct" solution here, and i suppose that's something we don't mind breaking (unlike rejecting SUBSCRIBE inside MULTI).

there are already some checks in execCommand that are a reflection of a portion of the checks in processCommand.
e.g. the call to ACLCheckAllPerm.
maybe it's time to extract these checks to a separate function and let it be called from both places, in which case the check for CLIENT_PUBSUB should be moved there.

[madolson]

Agree with Oran, let's create a new wrapper around "call" that performs the common logic, that seems like a better solution.

[oranagra]

there is another problem that can be resolved by a similar fix: #9993

[zuiderkwast]

If this (deferring effects of commands until exec) is the start of a trend, then perhaps CONFIG SET should also be deferred too? (mainly for consistency, not that it's needed now that we have multi-config set).

[madolson]

@joshleeb You still taking a look into this?

[joshleeb]

Yep, sorry for the delay. I’ve updated the PR to add a checkedCall function (happy to go with a better name) that wraps call with the common checks. At this stage the only check is ensuring only the subset of pubsub commands are called in the CLIENT_PUBSUB context. IIUC, updating the call sites (in processCommand and execCommand) now fixes #2967 in that PUBLISH cannot be called after SUBSCRIBE in a MULTI.

I was struggling to test this by adding to the existing unit tests as they seem to reject calling SUBSCRIBE in a MULTI, as with the redis-cli. So I have written a small script to confirm the above.

Test script

import socket


def msg(commands):
    cs = commands.split(' ')
    ms = ['*' + str(len(cs))]
    for s in cs:
        ms.append('$' + str(len(s)))
        ms.append(s)
    return '\r\n'.join(ms) + '\r\n'


sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
sock.connect(('127.0.0.1', 6379))

for c in ['multi', 'subscribe foo', 'publish foo asdf', 'exec']:
    sock.send(msg(c))
    parts = sock.recv(1024).split('\r\n')

    print(c)
    print(list(filter(None, parts)))

Which produced the output

multi
['+OK']
subscribe foo
['+QUEUED']
publish foo asdf
['+QUEUED']
exec
['*2', '*3', '$9', 'subscribe', '$3', 'foo', ':1', "-ERR Can't execute 'publish': only (P|S)SUBSCRIBE / (P|S)UNSUBSCRIBE / PING / QUIT / RESET are allowed in this context"]

As for pulling out other common checks, I’m happy to do more in this PR or in follow ups. Another commonality which has been mentioned is ACL checks. Though with these I have opted not to pull that out as (in processCommand) it is performed before actions such as cluster redirection and client evictions. Moving these to the checkedCall wrapper would move the ACL check to after these actions which I thought may introduce some unwanted effects from a security standpoint. But of course, happy to take guidance on this as well (as the Redis codebase is still very new to me 🙂)

[ranshid]

@joshleeb It has been lingering long time without a review. Sorry about that and thank you for this fix!
Please tell if you plan to continue working on this fix.
regarding your solution:
this will not issue error during the queuing but rather during the execution. I think it would have been better to identify the subscription during the queuing, which suggests we should still handle this inside processCommand.
In case we do plan to add it during command call, maybe a handle inside the call() function is fine. scripts already prevent running subscribe, and modules will soon have the ability to subscribe to channels without using call(), but still I suggest maybe avoid extra wrapping and just include this in call() function.

[oranagra]

@ranshid have you considered this #9928 (comment) ?

[ranshid]

@ranshid have you considered this #9928 (comment) ?

@oranagra I also looked at #9928 but I think this will not break the current clients running many subscriptions since it is O.K to perform more subscribes when in subscribed mode.
I think that failing during exec is possible but might cause some inconsistency since part of the transaction was already performed.

[oranagra]

not sure you saw the exact comment i referred to.
the issue i mentioned is that redis.py users may get a transaction by mistake when trying to send a bunch of command as a pipeline, so such a change will break existing apps.
instead i suggested to keep allowing subscription during multi-exec, but postponing all publish broadcasts to after the exec is over.
am i missing anything?

[ranshid]

not sure you saw the exact comment i referred to. the issue i mentioned is that redis.py users may get a transaction by mistake when trying to send a bunch of command as a pipeline, so such a change will break existing apps. instead i suggested to keep allowing subscription during multi-exec, but postponing all publish broadcasts to after the exec is over. am i missing anything?

I think I understand that. I think that supporting ill handling of some clients is no reason to invest in supporting it, much more that the same clients will probably break or produce some other wrong behavior running other commands (like blocking commands) in that pipeline. I understand redis-py specifically can disable transactions for pipelines def pipeline(self, transaction=False).
In any case IMO we can decide to postpone merging the fix for next major version and give clients heads-up to be ready.

[joshleeb]

Please tell if you plan to continue working on this fix.

Yep still happy to work on this. I'll aim to dive back into it this week.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}