diff --git a/src/unix.rs b/src/unix.rs index b4467e264..eb27f572b 100644 --- a/src/unix.rs +++ b/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. diff --git a/tests/dropping_does_not_deregister_other_instances.rs b/tests/dropping_does_not_deregister_other_instances.rs index c0df714e6..16cab40ac 100644 --- a/tests/dropping_does_not_deregister_other_instances.rs +++ b/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);