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 RedisReply leaving connection in unusable state if an exception o… #1396

Closed

Conversation

machindertech
Copy link
Contributor

…ccurs during initialization

This is required because if an exception is thrown in a constructor, the
destructor will not be called.

…ccurs during initialization

This is required because if an exception is thrown in a constructor, the
destructor will not be called.
@s-ludwig
Copy link
Member

AFAICS, drop() would only do something if reading a multi-reply failed. But the only way that could throw an exception is if the count is not a valid long string. But then then the reply length would still stay 0 and drop still wouldn't do anything.

I think the correct thing to do would be to instead close the connection, but only for the two cases of "unknown reply" and an invalid multi-reply length string. What do you think?

@machindertech
Copy link
Contributor Author

The issue we were having was that when an exception is thrown, the ctx.refCount stays incremented and then the next RedisReply fails on the assert(ctx.refCount == 0). Any kind of exception in initialize() will cause the assert to be triggered for the next reply.

@s-ludwig
Copy link
Member

Can you try the issue1412_redis_reply_exception branch? I've more or less implemented the approach outlined above there.

@machindertech
Copy link
Contributor Author

The issue1412_redis_reply_exception branch seems to fix the issues that we were having, so we're closing this pull request.

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

Successfully merging this pull request may close these issues.

2 participants