Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

execute() doesn't send EXEC if no commands are queued #1233

Closed
brianmaissy opened this issue Oct 23, 2019 · 1 comment
Closed

execute() doesn't send EXEC if no commands are queued #1233

brianmaissy opened this issue Oct 23, 2019 · 1 comment

Comments

@brianmaissy
Copy link

Version: redis 5.0.4, redis-py 3.3.6
Platform: Python 3.6.8 on Ubuntu 18.04
Description: When calling execute() on a pipeline (created with transaction=True), the EXEC is never sent if no commands are queued. While at first glance this makes sense as an optimization (why send an empty pipeline to the server?), in fact the EXEC might still have a purpose: to check if any of the WATCHed keys have since been modified.

For example, under normal conditions:

pipe = r.pipeline()
pipe.watch('foo')
r.set('foo', 'bar')
pipe.multi()
pipe.get('bar')
pipe.execute()  # raises WatchError

However, if we don't run the pipe.get('bar'), the EXEC never gets sent:

pipe = r.pipeline()
pipe.watch('foo')
r.set('foo', 'bar')
pipe.multi()
pipe.execute()  # returns []

It's easy to see that this is caused by this short-circuit check on line 3673 of client.py:
https://github.com/andymccurdy/redis-py/blob/fa0b0392c86126cae1a264197dddab647fa37821/redis/client.py#L3670-L3674
Which, based on the git blame, looks like it was done based on the discussion here: #362

But it seems to me that if pipe.watch() has been called, we can't short circuit and must run the EXEC to see if the transaction succeeded or failed.

@andymccurdy
Copy link
Contributor

Interesting, I had not really considered the use case of using multi/exec to just watch keys. If you want to change that check to something like if not stack and not self.watching: and then write a test that demonstrates that behavior works as expected I'll merge it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants