search

redis / hiredis-rb

Ruby wrapper for hiredis
BSD 3-Clause "New" or "Revised" License
319 stars 90 forks source link

hiredis-rb is not GC-safe #42

Open findchris opened 8 years ago

findchris commented 8 years ago

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.

badboy commented 8 years ago

Thanks for the report. I'm quite busy this and the coming week, but I will look into this.

findchris commented 8 years ago

Thanks @badboy - At first read, does the addition of volatile to the VALUEs make sense?

findchris commented 8 years ago

@badboy - Get a chance to look at this?

findchris commented 8 years ago

@badboy Is there anyone else I can @tag here to get a set of eyes on this issue?

badboy commented 8 years ago

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.

badboy commented 8 years ago

@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)

findchris commented 8 years ago

@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.

findchris commented 8 years ago

@badboy and @ohler55 - Any chance to check this out, or is it dying on the vine here?

ohler55 commented 8 years ago

Glad to get involved. Do you have a set of changes? Need a some help and a PR?

badboy commented 8 years ago

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.

ohler55 commented 8 years ago

ok, I'll put together some change and attempt to put together a test. It will be a step at a time.

badboy commented 8 years ago

Thanks in advance!

findchris commented 8 years ago

Thanks guys; I really appreciate you stepping up on this @ohler55 :clap:

ohler55 commented 8 years ago

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.

findchris commented 8 years ago

I can lock production to a particular commit, which should work.

ohler55 commented 8 years ago

It would help a lot. I put up a PR. Maybe batboy can help get everything squared away on branches.

findchris commented 8 years ago

Thanks @ohler55.
@badboy Can you look at https://github.com/redis/hiredis-rb/pull/43?

findchris commented 8 years ago

@badboy Did you get a chance to review #43?

findchris commented 8 years ago

Checking in @badboy #squeakywheel

badboy commented 8 years ago

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.

findchris commented 8 years ago

Totally understandable. Enjoy your holiday!

findchris commented 7 years ago

This is still an issue for us.

@badboy - Did you get a chance to look at this issue?

badboy commented 7 years ago

I merged, but never pushed a new release. Will take care of that ASAP

mberlanda commented 5 years ago

Hey there! Did anyone managed to reproduce the issue described by @findchris since the latest release? Is it still open?

tonydehnke commented 4 years ago

Bump - what is the status of this @badboy?

Should this be closes or is this still an issue?

badboy commented 4 years ago

I'm not working on hiredis-rb anymore.