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.
It would be better to use assert_raises Interrupt as to ensure that your stub is indeed called.
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.
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.
@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.
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 😉
trcarden
force-pushed
the
fix/client_nil_on_interrupt
branch
from
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
originalis 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
pipelinedormulti(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
clientis net to nil and is corrupted forever after that raising undefined method callsSystem configuration
Ruby version: 2.7.1
Gem version: 4.2.1