time: refactor to avoid double validation

The fix in the last commit was surgical, but resulted in duplicating the
valid-duration check. This commit adjusts Entry::new and
Registration::new to return a result whose error type conveys the
precise nature of the error, so that validation logic doesn't need to be
duplicated.

tokio/src/time/delay.rs

@@ -16,8 +16,7 @@ use std::task::{self, Poll};
 /// Canceling a delay is done by dropping the returned future. No additional
 /// cleanup work is required.
 pub fn delay_until(deadline: Instant) -> Delay {
-    let registration = Registration::new(deadline, Duration::from_millis(0));
-    Delay { registration }
+    Delay::new_timeout(deadline, Duration::from_millis(0))
 }
 
 /// Wait until `duration` has elapsed.
@@ -50,8 +49,20 @@ pub struct Delay {
 
 impl Delay {
     pub(crate) fn new_timeout(deadline: Instant, duration: Duration) -> Delay {
-        let registration = Registration::new(deadline, duration);
-        Delay { registration }
+        // Creating a registration can fail because a) the timer was not set up,
+        // b) the timer is at capacity, or c) the requested deadline exceeds the
+        // maximum duration that this timer can handle. Both (a) and (b) are
+        // likely the result of programmer error. A user couldn't really handle
+        // the error if we passed it ownward, so we just panic.
+        //
+        // (c) is arguably an error that could be handled gracefully by the
+        // user, but backwards compatibility prevents us from returning a proper
+        // error here. Plus, in the future, the timer implementation may not
+        // have a maximum duration at all.
+        match Registration::new(deadline, duration) {
+            Ok(registration) => Delay { registration },
+            Err(e) => panic!("timer error: {}", e),
+        }
     }
 
     /// Returns the instant at which the future will complete.
@@ -82,15 +93,10 @@ impl Future for Delay {
     type Output = ();
 
     fn poll(self: Pin<&mut Self>, cx: &mut task::Context<'_>) -> Poll<Self::Output> {
-        // `poll_elapsed` can return an error in two cases:
-        //
-        // - AtCapacity: this is a pathlogical case where far too many
-        //   delays have been scheduled.
-        // - Shutdown: No timer has been setup, which is a mis-use error.
-        //
-        // Both cases are extremely rare, and pretty accurately fit into
-        // "logic errors", so we just panic in this case. A user couldn't
-        // really do much better if we passed the error onwards.
+        // `poll_elapsed` will only return an error if no timer has been set
+        // up, which is almost certainly the result of programmer error. A
+        // user couldn't really handle the error if we passed it onward, so we
+        // just panic.
         match ready!(self.registration.poll_elapsed(cx)) {
             Ok(()) => Poll::Ready(()),
             Err(e) => panic!("timer error: {}", e),

tokio/src/time/driver/entry.rs

@@ -2,7 +2,6 @@ use crate::loom::sync::atomic::AtomicU64;
 use crate::sync::AtomicWaker;
 use crate::time::driver::{Handle, Inner};
 use crate::time::{Duration, Error, Instant};
-use crate::time::wheel::MAX_DURATION;
 
 use std::cell::UnsafeCell;
 use std::ptr;
@@ -105,31 +104,23 @@ const ERROR: u64 = u64::MAX;
 // ===== impl Entry =====
 
 impl Entry {
-    pub(crate) fn new(deadline: Instant, duration: Duration) -> Arc<Entry> {
+    pub(crate) fn new(deadline: Instant, duration: Duration) -> Result<Arc<Entry>, Error> {
         let inner = Handle::current().inner().unwrap();
-        let entry: Entry;
 
         // Increment the number of active timeouts
-        if inner.increment().is_err() {
-            entry = Entry::new2(deadline, duration, Weak::new(), ERROR)
+        inner.increment()?;
+
+        let when = inner.normalize_deadline(deadline);
+        let state = if when <= inner.elapsed() {
+            ELAPSED
         } else {
-            let when = inner.normalize_deadline(deadline);
-            let state = if when <= inner.elapsed() {
-                ELAPSED
-            } else if when - inner.elapsed() > MAX_DURATION {
-                panic!("timer duration exceeds maximum duration");
-            } else {
-                when
-            };
-            entry = Entry::new2(deadline, duration, Arc::downgrade(&inner), state);
-        }
+            when
+        };
 
-        let entry = Arc::new(entry);
-        if inner.queue(&entry).is_err() {
-            entry.error();
-        }
+        let entry = Arc::new(Entry::new2(deadline, duration, Arc::downgrade(&inner), state));
+        inner.queue(&entry)?;
 
-        entry
+        Ok(entry)
     }
 
     /// Only called by `Registration`

tokio/src/time/driver/registration.rs

@@ -14,10 +14,10 @@ pub(crate) struct Registration {
 }
 
 impl Registration {
-    pub(crate) fn new(deadline: Instant, duration: Duration) -> Registration {
-        Registration {
-            entry: Entry::new(deadline, duration),
-        }
+    pub(crate) fn new(deadline: Instant, duration: Duration) -> Result<Registration, Error> {
+        Ok(Registration {
+            entry: Entry::new(deadline, duration)?,
+        })
     }
 
     pub(crate) fn deadline(&self) -> Instant {

tokio/src/time/error.rs

@@ -28,6 +28,7 @@ pub struct Error(Kind);
 enum Kind {
     Shutdown,
     AtCapacity,
+    Invalid,
 }
 
 impl Error {
@@ -56,6 +57,19 @@ impl Error {
             _ => false,
         }
     }
+
+    /// Create an error representing a misconfigured timer.
+    pub fn invalid() -> Error {
+        Error(Invalid)
+    }
+
+    /// Returns `true` if the error was caused by the timer being misconfigured.
+    pub fn is_invalid(&self) -> bool {
+        match self.0 {
+            Kind::Invalid => true,
+            _ => false,
+        }
+    }
 }
 
 impl error::Error for Error {}
@@ -66,6 +80,7 @@ impl fmt::Display for Error {
         let descr = match self.0 {
             Shutdown => "timer is shutdown",
             AtCapacity => "timer is at capacity and cannot create a new entry",
+            Invalid => "timer duration exceeds max duration",
         };
         write!(fmt, "{}", descr)
     }

tokio/src/time/wheel/mod.rs

@@ -43,7 +43,7 @@ pub(crate) struct Wheel<T> {
 const NUM_LEVELS: usize = 6;
 
 /// The maximum duration of a delay
-pub(crate) const MAX_DURATION: u64 = 1 << (6 * NUM_LEVELS);
+const MAX_DURATION: u64 = 1 << (6 * NUM_LEVELS);
 
 #[derive(Debug)]
 pub(crate) enum InsertError {