unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help
 help / color / mirror / code / Atom feed
* Bug, Unicorn can drop signals in extreme conditions
@ 2015-02-16  2:19 Steven Stewart-Gallus
  2015-02-18  9:15 ` Eric Wong
  0 siblings, 1 reply; 3+ messages in thread
From: Steven Stewart-Gallus @ 2015-02-16  2:19 UTC (permalink / raw)
  To: unicorn-public

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Bug, Unicorn can drop signals in extreme conditions
  2015-02-16  2:19 Bug, Unicorn can drop signals in extreme conditions Steven Stewart-Gallus
@ 2015-02-18  9:15 ` Eric Wong
  2015-02-18 17:27   ` Steven Stewart-Gallus
  0 siblings, 1 reply; 3+ messages in thread
From: Eric Wong @ 2015-02-18  9:15 UTC (permalink / raw)
  To: Steven Stewart-Gallus; +Cc: unicorn-public, Jesse Storimer

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 :)

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Bug, Unicorn can drop signals in extreme conditions
  2015-02-18  9:15 ` Eric Wong
@ 2015-02-18 17:27   ` Steven Stewart-Gallus
  0 siblings, 0 replies; 3+ messages in thread
From: Steven Stewart-Gallus @ 2015-02-18 17:27 UTC (permalink / raw)
  To: Eric Wong; +Cc: unicorn-public, Jesse Storimer

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2015-02-18 17:27 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-16  2:19 Bug, Unicorn can drop signals in extreme conditions Steven Stewart-Gallus
2015-02-18  9:15 ` Eric Wong
2015-02-18 17:27   ` Steven Stewart-Gallus

Code repositories for project(s) associated with this inbox:

	../../../unicorn.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).