signal: Fix a potential Signal starvation based on creation order

* As observed in alexcrichton/tokio-signal#38, Signal instances can starve based on the order
they are created in, and this ordering appears to be platform/OS
specific
* The crux of the issue is that we woud only *attempt* to broadcast any
pending signals if we successfully read out at least one byte from the
global pipe.
* For reasons unclear to me, the affected Signal instance would get
woken up after the signal handler writes to the global pipe, but it
would immediately hit a WouldBlock error and give up, bypassing the
broadcast attempt (even though the pending flag was correctly set).
 - Maybe this has to do with OS specifics with how the bytes are
   delivered (or not), or with some complex interaction with tokio and
   the pipe registration. It seems fishy since strace logs didn't show
   the signal handler pipe write fail either, but I'm all out of ideas
* The fix appears simple: unconditionally attempt to broadcast any
pending signals *any* time a Driver instance is woken up.
* Since we perform an atomic check for each pending signal, we know that
each (coalesced) signal broadcast will happen at most once. If we were
supuriously woken up and no signals were pending, then nothing will be
yielded to any pollers of Signal
* The down side is that since each Signal instance polls a Driver
instance, each poll to Signal will essentially perform N atomic
operations (N = number of signals we support) in an attempt to broadcast
any pending signals.
 - However, we can revisit optimizing this better in the future

Fixes alexcrichton/tokio-signal#38

src/unix.rs

@@ -267,10 +267,10 @@ impl Future for Driver {
 
     fn poll(&mut self) -> Poll<(), ()> {
         // Drain the data from the pipe and maintain interest in getting more
-        let any_wakeup = self.drain();
-        if any_wakeup {
-            self.broadcast();
-        }
+        self.drain();
+        // Broadcast any signals which were received
+        self.broadcast();
+
         // This task just lives until the end of the event loop
         Ok(Async::NotReady)
     }
@@ -283,23 +283,21 @@ impl Driver {
         })
     }
 
-    /// Drain all data in the global receiver, returning whether data was to be
-    /// had.
+    /// Drain all data in the global receiver, ensuring we'll get woken up when
+    /// there is a write on the other end.
     ///
-    /// If this function returns `true` then some signal has been received since
-    /// we last checked, otherwise `false` indicates that no signal has been
-    /// received.
-    fn drain(&mut self) -> bool {
-        let mut received = false;
+    /// We do *NOT* use the existence of any read bytes as evidence a sigal was
+    /// received since the `pending` flags would have already been set if that
+    /// was the case. See #38 for more info.
+    fn drain(&mut self) {
         loop {
             match self.wakeup.read(&mut [0; 128]) {
                 Ok(0) => panic!("EOF on self-pipe"),
-                Ok(_) => received = true,
+                Ok(_) => {},
                 Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => break,
                 Err(e) => panic!("Bad read on self-pipe: {}", e),
             }
         }
-        received
     }
 
     /// Go through all the signals and broadcast everything.

tests/dropping_does_not_deregister_other_instances.rs

@@ -10,6 +10,8 @@ use futures::Future;
 use tokio_signal::unix::Signal;
 use std::time::{Duration, Instant};
 
+const TEST_SIGNAL: libc::c_int = libc::SIGUSR1;
+
 #[test]
 fn dropping_signal_does_not_deregister_any_other_instances() {
     // NB: Deadline requires a timer registration which is provided by
@@ -18,23 +20,18 @@ fn dropping_signal_does_not_deregister_any_other_instances() {
     let mut rt = tokio::runtime::current_thread::Runtime::new()
         .expect("failed to init runtime");
 
-    // FIXME(38): there appears to be a bug with the order these are created/destroyed
-    // If the duplicate signal is created first and dropped, it appears that `signal`
-    // will starve. This ordering also appears to be OS specific...
-    let first_signal = rt.block_on(Signal::new(libc::SIGUSR1))
-        .expect("failed to register duplicate signal");
-    let second_signal = rt.block_on(Signal::new(libc::SIGUSR1))
+    // NB: Testing for issue #38: signals should not starve based on ordering
+    let first_duplicate_signal = rt.block_on(Signal::new(TEST_SIGNAL))
+        .expect("failed to register first duplicate signal");
+    let signal = rt.block_on(Signal::new(TEST_SIGNAL))
         .expect("failed to register signal");
-    let (duplicate, signal) = if cfg!(target_os = "linux") {
-        (second_signal, first_signal)
-    } else {
-        // macOS
-        (first_signal, second_signal)
-    };
+    let second_duplicate_signal = rt.block_on(Signal::new(TEST_SIGNAL))
+        .expect("failed to register second duplicate signal");
 
-    drop(duplicate);
+    drop(first_duplicate_signal);
+    drop(second_duplicate_signal);
 
-    unsafe { assert_eq!(libc::kill(libc::getpid(), libc::SIGUSR1), 0); }
+    unsafe { assert_eq!(libc::kill(libc::getpid(), TEST_SIGNAL), 0); }
 
     let signal_future = signal.into_future()
         .map_err(|(e, _)| e);