hiredis-rb
hiredis-rb copied to clipboard
redis
hiredis-rb is not GC-safe
Hi there.
While investigating a GC-related issue (https://github.com/ohler55/oj/issues/265#issuecomment-147554809), @ohler55 mentioned that he looked at the hiredis-rb C code and saw some stuff that wasn't GC safe and might be the source of the bug I'm investigating (NotImplementedError: method ... called on terminated object). He pointed out that:
Ruby checks the stack for VALUE types which are Ruby object references. Compilers often optimize code by placing variable in registers which are invisible to Ruby as far as checking for wether an object is still being referenced. The work around for this is to make sure all VALUE local variable are volatile. That keeps them out of registers and on the stack.
See the linked-to github issue above, but I'm only seeing this issue a few times per day on an app that gets significant traffic, so it's tough to reproduce, but all signs point to it being GC-related.
I look forward to your reply here.
Thanks for the report. I'm quite busy this and the coming week, but I will look into this.
Thanks @badboy - At first read, does the addition of volatile to the VALUEs make sense?
@badboy Is there anyone else I can @tag here to get a set of eyes on this issue?
Hey. Currently I'm the only maintainer, so there's no one else to take a look.
But I did and it seems there is a macro to use (RB_GC_GUARD) on those VALUEs instead of making it explicitely volatile.
I'm trying to find the right solution today.
@findchris Do you have a test case that triggers the bug reliably? Would be nice to know that any fix we apply actually solves the problem (the other option for me would be to read the assembly and I'm really not good at that)
@badboy I don't have a test case, sadly. It seems to be GC-related, and I have been able to reproduce consistently.
@ohler55 Did this comment make sense to you? I just don't know enough to respond intelligently.
@badboy and @ohler55 - Any chance to check this out, or is it dying on the vine here?
Glad to get involved. Do you have a set of changes? Need a some help and a PR?
I don't have any code yet. I can't even reproduce it, which makes it near to impossible to know if any change would actually cover the bug. I appreciate all help, so if you have more insight or know your way around in Ruby extensions (I really don't :D) please tell so.
ok, I'll put together some change and attempt to put together a test. It will be a step at a time.
I am not having much success reproducing the failure here. @findchris, can you run tests on changes we make to verify when the changes work? You can be our tester. Less than ideal but it should be enough.
It would help a lot. I put up a PR. Maybe batboy can help get everything squared away on branches.
Thanks @ohler55.
@badboy Can you look at https://github.com/redis/hiredis-rb/pull/43?
I'm sorry this is post-poned so long, but I have to shift it a bit again (upcoming holiday and I just need a break from this). I take a fresh look when I'm back in 2 weeks.
This is still an issue for us.
@badboy - Did you get a chance to look at this issue?
Hey there! Did anyone managed to reproduce the issue described by @findchris since the latest release? Is it still open?
Bump - what is the status of this @badboy?
Should this be closes or is this still an issue?
