Add deferred signal-handling (fixes #332). #334

Merged
merged 2 commits into from Mar 8, 2013

Projects

None yet

3 participants

@ged
Contributor
ged commented Mar 4, 2013

This uses a thread-local queue and a self-pipe so the code in the
trap blocks is properly re-entrant.

For details, see:

@ddollar
Owner
ddollar commented Mar 4, 2013

This seems like an alternative solution to #333. The reentrancy here seems desirable, especially since these signals tend to propagate.

@ged
Contributor
ged commented Mar 4, 2013

Yes, IMO introducing more threads is probably not the way to solve reentrancy problems.

ged added some commits Mar 4, 2013
@ged ged Add deferred signal-handling (fixes #332).
This uses a thread-local queue and a self-pipe so the code in the
trap blocks is properly re-entrant.

For details, see:

  * http://cr.yp.to/docs/selfpipe.html
  * http://blog.rubybestpractices.com/posts/ewong/016-Implementing-Signal-Handlers.html
5ab08c6
@ged ged Try to allow children to shut down gracefully
Since signals will no longer be handled once foreman goes into
`terminate_gracefully`, default signal handlers are restored so as
not to cause it to get stuck in an unTERMable state.

This necessitates not using the process group for signalling
except as a last resort, as foreman itself will receive the signals
it sends. This splits `killall` into two methods, one which
signals only processes foreman itself has started, and one which
signals all processes in the process group to try to clean up
more aggressively, and then reworks `terminate_gracefully` to use
them.
1691883
@ged
Contributor
ged commented Mar 4, 2013

It looks like the Travis build is failing because this patch restores the default signal handlers at the start of shutdown (as the queue won't be read again), and killing a process group with kill( "-#{signal}", Process.getpgrp ) sends the TERM to the sending process as well, so it terminates the foreman process.

A better behavior would be to split killing children gracefully from killing the whole process group so you can try the first, and then fall back to the second in the timeout. The second commit does this.

@ged
Contributor
ged commented Mar 5, 2013

The only Travis failure is under 2.0, and is an unrelated issue fixed by #335.

@crcastle
crcastle commented Mar 8, 2013

I can haz merge? (post #335 merge to make Travis pass)

@ddollar ddollar merged commit 9fe7ddb into ddollar:master Mar 8, 2013

1 check failed

default The Travis build failed
Details
@ged ged deleted the ged:reentrant_signal_handlers branch Mar 8, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment