use thread.exit for feedback receiver stop #113

Closed
wants to merge 1 commit into
from

2 participants

@tehael

No description provided.

@ileitch
Owner

What's wrong with the current approach?

@tehael

The call to interrupt_sleep from outside the thread where interruptable_sleep was called does not interrupt the the sleep.

What I found is that the instance variable @_sleep_interrupt is not set for the feedback receiver instance but for the thread, and therefore interrupt_sleep called from the context of the feedback receiver instance does nothing.
The call to stop on the feedback receiver waits for the thread to finish (@thread.join) and this woul only happen if the loop in the thread breaks or an exception is raised in the loop. The instance variable @stop used to break the loop is not defined when the thread starts so from the threads point of view it will never be so it can never be true.

Why I came to this was, that the rapns daemon did not shut down when a TERM signal was sent. It stops after the second TERM signal, but without cleaning up the pid file (exit 1 on line 112 of daemon.rb). So i tried to figure out why the process didn't stop and figured out it was call to stop on the feedback receiver which took "forever" and kept the process alive.

So the easiest fix for to stop the thread was to call exit on the thread.

Kind regards,
Thomas

P.S.: I'll fix the broken tests

@ileitch
Owner

The thread block is bound to the scope in which it is created, which is the FeedbackReceiver instance. Even though @stop is not set prior to the block creation, it shouldn't matter.

What version of Ruby are you using?

@tehael

I'm using ruby-1.9.3-pX

The problem ist not the @stop instance variable, but the @_sleep_interrupt instance variable set in the context of the Thread.new block, because the call to interrupt_sleep is done in the context or the notification receiver, where @_sleep_interrupt was never set, therefore doing nothing, i.e. not interrupting the interruptable sleep initiated in the thread.
kind regards, thomas

@tehael

I will send a new pull request when tests are ready.

@tehael tehael closed this Mar 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment