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

Fix the case where an error reply is received before any callbacks are registered #43

Merged
merged 2 commits into from
May 29, 2011

Conversation

bmatheny
Copy link
Contributor

In an async context, if an error reply is received from the server before any callbacks have been registered, the callback processor (redisProcessCallbacks) assumes it's a subscribed context. This instance (error before callback registered) results in a failed assertion. This can be replicated by modifying the example-libevent.c main function as specified here: https://gist.github.com/988962

You'll see something like:

connectCallback: connected...
connectCallback: connected...
Assertion failed: (c->flags & REDIS_SUBSCRIBED), function redisProcessCallbacks, file async.c, line 377.
[1]    71052 abort      ./hiredis-example-libevent

The diff detects the condition where an error was received and handles it appropriately.

@bmatheny
Copy link
Contributor Author

One note, in order to replicate the error with that gist you would need to set maxclients to 1 for redis (the error is triggered by two clients connecting).

@pietern
Copy link
Contributor

pietern commented May 24, 2011

Thanks, never thought of this scenario. Let's think some more before merging. In this particular case Redis sends an error before the client sends anything and the code path you fixed is taken, other replies will always be sent after a request. What happens when a error reply is received after an invalid command is called, while the context is subscribed? Will it correctly trigger that callback instead of taking this code path (on the road, can't check myself)?

@bmatheny
Copy link
Contributor Author

I think you're right, updated in 63dcf9b to check the context flag.

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

Successfully merging this pull request may close these issues.

2 participants