Bug, Unicorn can drop signals in extreme conditions

Bug, Unicorn can drop signals in extreme conditions

[Steven Stewart-Gallus]

Hello,

While reading the article at
http://www.sitepoint.com/the-self-pipe-trick-explained/ I realized
that the signal handling code shown there and of the same type in your
application is broken.  As you have a single nonblocking pipe in your
application you can drop signals entirely (and not just fold multiple
signals of the same type into one).  The simplest fix is to use a pipe
for each individual type of signal or to just use pselect or similar.
I'm pretty sure that there is a way to use a single pipe but it seems
complicated and very difficult to implement correctly.

Thank you,
Steven Stewart-Gallus

Re: Bug, Unicorn can drop signals in extreme conditions

[Eric Wong]

Steven Stewart-Gallus <sstewartgallus00@mylangara.bc.ca> wrote:
> Hello,
> 
> While reading the article at
> http://www.sitepoint.com/the-self-pipe-trick-explained/ I realized
> that the signal handling code shown there and of the same type in your
> application is broken.  As you have a single nonblocking pipe in your
> application you can drop signals entirely (and not just fold multiple
> signals of the same type into one).  The simplest fix is to use a pipe
> for each individual type of signal or to just use pselect or similar.
> I'm pretty sure that there is a way to use a single pipe but it seems
> complicated and very difficult to implement correctly.

(I've Cc-ed Jesse for this)

I wasn't sure exactly what you were referring to, but now I see where
the sitepoint.com article makes calls in the wrong order:

     self_writer.write_nonblock('.') # XXX may fail!
     SIGNAL_QUEUE << signal

In contrast, unicorn enqueues the signal before attempting to write to
the pipe, so we don't care at all if the write fails.  It doesn't matter
if the non-blocking write fails due to the pipe being full at all; as
any existing data in the pipe is sufficient to cause the reader to wake
up.

The correct order would be:

     SIGNAL_QUEUE << signal
     self_writer.write_nonblock('.') # we don't care if this fails

Furthermore, this avoids a race condition in multi-threaded situations.
Order is critical, even outside of signal handlers, this ordering of
events is fundamental to correct usage of things like condition
variables and waitqueues.


Btw, MRI 1.9.3+ also uses the self-pipe trick internally, too (see
thread_pthread.c) for signals and thread switching.  Current versions
use two pipes, one for high-priority wakeups, and one for low-priority
wakeups.

And on a related note, using pselect/ppoll/epoll_pwait/signalfd-style
syscalls which affect the signal mask is not feasible with runtimes
which already implement a high-level signal handling API.  I ripped out
signalfd support from sleepy_penguin a few years back because it would
always conflict with the signal handling API in Ruby itself...

And eventfd is cheaper and usable in place of a self-pipe from Ruby, of
course(*), but I haven't convinced ruby-core it's worth the maintenance
effort for thread_pthread.c; so a conservative project like unicorn
won't use it, yet.

Anyways, thanks for bringing this to our attention.


(*) I use it in yet another horribly-named server :)

Re: Bug, Unicorn can drop signals in extreme conditions

[Steven Stewart-Gallus]

Hello,

Okay, my apologies, I misunderstood the signal handling code.  I
thought you were doing something like
self_writer.write_nonblock(signal) instead.  This is what I thought of
initially because I was still thinking in the context of C (where
signals are not deferred and allocating new space on a queue in a
signal handler would be unsafe) instead of in the context of Ruby
where Ruby's signals are deferred.  Incidentally, MRI's signal handler
implementation in signal.c appears to be broken in several ways but
that is a separate topic for a separate place.

Thank you,
Steven Stewart-Gallus