time: clean up redundant check in Wheel::poll()

[marcelo-gonzalez]

The condition checked in the and_then() call is the same as is checked in the match below, so we can clean it up by just matching on next_expiration() directly.

[marcelo-gonzalez]

also the comment looks like it referred to the code that existed before the PR that added it? https://github.com/tokio-rs/tokio/pull/2905/files#diff-f0d9f5707877769d0883b4982ca9788bd07fe6d4b7bee13549ad9ffe29475d73L141-L149

[hawkw]

is this actually equivalent? if expiration.deadline is greater than now, the and_then closure returns None, so the value of the variable expiration is None. in the match, if expiration is None, we call set_elapsed and skip popping from the list of pending timers. after this change, the value of expiration will now be Some when entering the match, and we will execute a different match arm that doesn't call set_elapsed, which changes the behavior. this doesn't seem correct to me...

[marcelo-gonzalez]

oh good call my bad!

[hawkw]

i believe what we actually should do is remove the Some(ref expiration) if expiration.deadline > now match arm, because it would previously never have executed at all (since when the deadline is greater than now, we set the value of expiration to None). this could potentially be replaced with a debug_assert!(expiration.deadline <= now) in the Some match arm...

[marcelo-gonzalez]

i believe what we actually should do is remove the Some(ref expiration) if expiration.deadline > now match arm, because it would previously never have executed at all (since when the deadline is greater than now, we set the value of expiration to None). this could potentially be replaced with a debug_assert!(expiration.deadline <= now) in the Some match arm...

@hawkw Yeah I just messed it up here... but I think what I should have written was

diff --git a/tokio/src/runtime/time/wheel/mod.rs b/tokio/src/runtime/time/wheel/mod.rs
index c3ba3643..bf13b7b2 100644
--- a/tokio/src/runtime/time/wheel/mod.rs
+++ b/tokio/src/runtime/time/wheel/mod.rs
@@ -148,23 +148,13 @@ impl Wheel {
                 return Some(handle);
             }
 
-            // under what circumstances is poll.expiration Some vs. None?
-            let expiration = self.next_expiration().and_then(|expiration| {
-                if expiration.deadline > now {
-                    None
-                } else {
-                    Some(expiration)
-                }
-            });
-
-            match expiration {
-                Some(ref expiration) if expiration.deadline > now => return None,
-                Some(ref expiration) => {
+            match self.next_expiration() {
+                Some(ref expiration) if expiration.deadline <= now => {
                     self.process_expiration(expiration);
 
                     self.set_elapsed(expiration.deadline);
                 }
-                None => {
+                _ => {
                     // in this case the poll did not indicate an expiration
                     // _and_ we were not able to find a next expiration in
                     // the current list of timers.  advance to the poll's

[hawkw]

Hmm, that diff also doesn't look like it actually implements the current behavior, as the case where the expiration's deadline is greater than now is still handled by the Some(ref expiration) match arm, which results in us processing expirations at that deadline (which is in the future).

Did you mean something like this?

            match expiration {
                Some(ref expiration) if expiration.deadline <= now => {
                    self.process_expiration(expiration);

                    self.set_elapsed(expiration.deadline);
                }
                _ => {
                    // in this case the poll did not indicate an expiration
                    // _and_ we were not able to find a next expiration in
                    // the current list of timers.  advance to the poll's
                    // current time and do nothing else.
                    self.set_elapsed(now);
                    break;
                }
            }

[hawkw]

Ah, yup, the edit you just made to the diff looks correct --- looks like we noticed the same thing :)

[marcelo-gonzalez]

oof yeah I must be low on sleep today, updated. That's what I meant indeed :D

[marcelo-gonzalez]

Ah, yup, the edit you just made to the diff looks correct --- looks like we noticed the same thing :)

@hawkw worth reopening this w/ that diff do you think?

[hawkw]

I think it's worth reopening, the current code is certainly a lot harder to understand than I think it needs to be (as illustrated by the amount of tries it took us to get this right...:slightly_smiling_face:)

[hawkw]

This looks good to me, thanks for cleaning this up!