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

Pipelines seem to be false-y #994

Closed
anderson-dan-w opened this issue Jun 7, 2018 · 3 comments
Closed

Pipelines seem to be false-y #994

anderson-dan-w opened this issue Jun 7, 2018 · 3 comments

Comments

@anderson-dan-w
Copy link

This is a non-blocking issue, since there's a clear work-around, but I just found it surprising:

r = redis.Redis()
pipe = r.pipeline()
pipe  ## Pipeline<ConnectionPool<Connection<host=127.0.0.1,port=6379,db=0>>>
pipe or True  ## True

## workaround:
pipe if pipe is not None else True

Is there a particular reason pipeline objects should appear false-y? My inclination is that this doesn't pass the Principle of Least Surprise (but maybe I'm missing something).

@etiktin
Copy link

etiktin commented Dec 3, 2019

I encountered the same surprising behavior.
I was refactoring some code so it can be used with an existing connection (Redis/Pipeline) or without one, in which case it crates a new connection. Something like:

def create_session(session_id: str, ..., r: redis.Redis = None):
    r = r or get_connection()
    # Prepare/serialize the data...
    # Write it to Redis...

After the refactor I ran my test suite and noticed failures and saw a bunch of redis.WatchError exceptions in the logs. I then ran redis-cli monitor to see what commands were received by Redis and in what order. There I noticed that all the commands that were suppose to be executed inside a transaction (between the MULTI and EXEC commands), were executed prior to MULTI. Meaning the actions were not executed in a transaction. When I debugged the code, I noticed that when a pipeline was passed to a function like the one above, r = r or get_connection(), a new connection was returned (the object id was different), so the operation was executed outside of the pipeline.
It was easy to fix it (just replaced the line with if r is None: r = get_connection()), but I was really surprised by the behavior and it was time consuming to figure it out.

I expected a Pipeline instance to always be truthy since it inherits from Redis which is always truthy, but instead its truth value is based on if pipe.command_stack is empty or not. This happens because in commit 5827683 the __len__ method was added to Pipeline so you could do len(pipe) and see how many commands are waiting to be executed.
Note: if a Python object doesn't have a __bool__ method (or __nonzero__ in Python 2), but has a __len__ method, the truth value will be based on that (reference).

The documentation didn't mention anything about being able to run len(pipe), and no explicit warning about the truthiness of a pipe was given.

Pipeline is a a Redis client (inheritance) that has a command_stack (composition). So the fact that pipelines behave as the command_stack (the truthiness of an empty container is false), is unexpected.
I'm not sure what was wrong with just using len(pipe.command_stack) or if pipe.command_stack: ..., that this change was needed but I guess it just seemed like a harmless little tweak.

IMO, for now it's probably best to just document the current behavior, since people might be relaying on it in the wild. But long term (maybe next major version) I think the __len__ method should be removed from Pipeline.

@etiktin
Copy link

etiktin commented Dec 3, 2019

CC: @andymccurdy.

@andymccurdy
Copy link
Contributor

Thanks for reporting this. This was unintended behavior. Pipelines will now always be True when evaluating their boolean representation.

haimjether pushed a commit to jether-energy/kombu that referenced this issue May 14, 2023
The tasks are replicating because _remove_from_indices()
removes only from 'unacked_index', but keeps task in 'unacked' - so effectively replicating the task.

The issue is in pyredis pipe always being False as described here: redis/redis-py#994 and causing out of order execution of orders in multi block.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants