seomoz / qless

Queue / Pipeline Management
MIT License
294 stars 76 forks source link

Fix signal handling for Ruby 2 -- shutdown child processes #209

Closed evanbattaglia closed 9 years ago

evanbattaglia commented 9 years ago

Ruby 2 complains when using Mutex#synchronize within in a trap context. This is because, if the program flow is interrupted by a signal while the program is in a synchronize block, and synchronize is called from the trap context, syncrhonize will never return because it will wait for the same process/thread to release the block.

Here, in a trap context, if we fail to get the lock, I add the signal to a signal queue which will be run soon after the synchronize block in the normal program flow is finished. If we can get the lock, we should shutdown / process the signal immediately, because main program flow is probably just waiting for the child process and so can not timely check the signal queue.

Addresses #202

I have checked to make sure the spec I wrote passes with my new code and does not pass with Ruby 2.1.5 using the old code. Some other qless specs are failing for me in all branches, including master, but it looks like Travis is OK.

@myronmarston @danlecocq @benkirzhner @tpickett66

evanbattaglia commented 9 years ago

there seems to be an intermittent spec failure in travis https://travis-ci.org/seomoz/qless/jobs/50413427 https://travis-ci.org/seomoz/qless/jobs/42763100

evanbattaglia commented 9 years ago

Myron suggested using a thread-safe queue, and Vadim pointed out that signals can arrive in the middle of an operation on @signal_queue¸ potentially leaving @signal_queue in a weird state. I will change @signal_queue to a rhread-safe queue (if Ruby lets me use it in a trap context)

evanbattaglia commented 9 years ago

@vadim-moz I updated this to use a Thread-safe queue. Test manually and behavior still looks OK, and my spec still passes

evanbattaglia commented 9 years ago

Cool looks good. I agree it's nicer just adding the block to the queue!