search

mloughran / em-hiredis

Eventmachine redis client
MIT License
221 stars 63 forks source link

fixed a race condition #10

Closed asmuth closed 12 years ago

asmuth commented 12 years ago

I got a lot of "Replies out of sync"-errors when running under high load. This fixed it for me.

mloughran commented 12 years ago

That's really quite strange - those lines should always happen together if you're running a single threaded application - it shouldn't be possible to get a race condition. I wonder whether you have multiple threads, each one making calls to the same em-hiredis client?

asmuth commented 12 years ago

Yes, I think this is the piece of code:

https://github.com/paulasmuth/fnordmetric/blob/v0.5/lib/fnordmetric/worker.rb#L24

mloughran commented 12 years ago

I would either use a separate redis client for the blpop operation (especially if you want to use this redis client in other parts of your code), or call the recursive method after the hincrby. Otherwise your event processing will be blocked by the blpop. Let me know if this helps. However I still don't see why you'd be getting out of sync replies...

asmuth commented 12 years ago

Mh, I tried both, but neither helped and that instance of the client isn't used anywhere else. (The blpop is supposed to block, because it pops the next job from the queue). If you like, you can add me on Skype and I'll demonstrate the problem and that the switch of send_command and @defs.push seemingly fixes it. Also, I can't find the reason to issue the command to redis before storing the callback? What am I missing? :)

asmuth commented 12 years ago

Hey, I found out, that the "Replies out of sync" only happens, when the call is made from inside a Yajl::Parser.on_parse_complete-callback. I stopped using the stream parser and everything is working fine without the change, so I guess I should close the pull request. Thanks! :)