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: corruption of client during interrupt #945
Conversation
test/transactions_test.rb
Outdated
begin | ||
Redis::Pipeline::Multi.stubs(:new).raises(Interrupt) | ||
r.multi {} | ||
rescue Interrupt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to use assert_raises Interrupt
as to ensure that your stub is indeed called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger, changed.
lib/redis.rb
Outdated
@@ -2445,7 +2445,7 @@ def pipelined | |||
yield(self) | |||
original.call_pipeline(@client) | |||
ensure | |||
@client = original | |||
@client = _client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not totally sure this is correct. I'll need to have a look later, but this always restore the original client, so if you nest two pipeline/multi, that might cause issues. That being said I'm not so sure such nesting is properly handled either...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@byroot I think if you take a look at synchronize
it references the same instance variable @client
and passes it in as either _client
or client
in pipelined
and multi
(respectively). Given that is the same value and all the nested calls would reference the same instance value via the synchronize
call I think it checks out. Test suite passes too which is a good sign. Happy to help track it down if I am wrong though, I don't spend too much time down in redis.rb
def synchronize
mon_synchronize { yield(@client) }
end
Obviously could do some variable name cleanup and remove duplication but thought to keep the changeset minimal until a maintainer could take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you take a look at
synchronize
Thanks, that's exactly the kind of thing I didn't have fresh in mind and meant to check tomorrow. So yeah,, LGTM.
could do some variable name cleanup and remove duplication
Please do.
until a maintainer could take a look
One is talking with you right now 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Please squash your commits together and I'll merge.
f5baec8
to
61c36e0
Compare
@byroot ok i tried to give you a few options to pick from .... haha. I guess you liked the last one then? I just squashed them to the original commit and flattened it to follow your other commits for the project. Feel free to modify as you need. I don't mind. |
Sorry, I didn't notice. I don't have commits push notifications on, it's way too noisy. Anything last version was fine. |
Thanks for the patch. |
Due to the fact that
original
is hoisted to nil and its assignmentis not the first operation interrupts can corrupt the redis client
pointer.
This commit addresses issue by using a reference guaranteed to
be set correctly to reassign the client in the ensure block
Steps to reproduce
pipelined
ormulti
(Timeout.timeout, sigint, etc, pick your poison)original = @client
(see test cases for example)Expected behavior
The library is resilient to interrupts and the redis client can continue to accept commands afterwards
Actual behavior
The underlying
client
is net to nil and is corrupted forever after that raising undefined method callsSystem configuration
Ruby version: 2.7.1
Gem version: 4.2.1