Prevent premature timeouts

Summary:
The current implementation may fire timeouts prematurely due to them being put in a bucket that we're about to run. In particular, timeouts that span `WHEEL_SIZE-1` (2.55-2.56s with default interval of 10ms) have very high likelihood of being fired immediately if we have another callback already scheduled.

The issue is that we may use a bucket for the next epoch of wheel before the previous epoch drained it. This diff fixes it by using next unprocessed bucket as a base.

Reviewed By: yfeldblum, djwatson

Differential Revision: D13541502

fbshipit-source-id: 963139e77615750820a63274a1e21929e11184f1

folly/io/async/HHWheelTimer.cpp

@@ -105,9 +105,10 @@ HHWheelTimer::~HHWheelTimer() {
 void HHWheelTimer::scheduleTimeoutImpl(
     Callback* callback,
     std::chrono::milliseconds timeout,
+    int64_t nextTickToProcess,
     int64_t nextTick) {
   int64_t due = timeToWheelTicks(timeout) + nextTick;
-  int64_t diff = due - nextTick;
+  int64_t diff = due - nextTickToProcess;
   CallbackList* list;
 
   auto bi = makeBitIterator(bitmap_.begin());
@@ -128,7 +129,7 @@ void HHWheelTimer::scheduleTimeoutImpl(
     /* in largest slot */
     if (diff > LARGEST_SLOT) {
       diff = LARGEST_SLOT;
-      due = diff + nextTick;
+      due = diff + nextTickToProcess;
     }
     list = &buckets_[3][(due >> 3 * WHEEL_BITS) & WHEEL_MASK];
   }
@@ -140,14 +141,21 @@ void HHWheelTimer::scheduleTimeout(
     std::chrono::milliseconds timeout) {
   // Cancel the callback if it happens to be scheduled already.
   callback->cancelTimeout();
-
   callback->requestContext_ = RequestContext::saveContext();
 
   count_++;
 
   callback->setScheduled(this, timeout);
   auto nextTick = calcNextTick();
-  scheduleTimeoutImpl(callback, timeout, nextTick);
+
+  // It's possible that the wheel timeout is already scheduled for earlier
+  // tick, but it didn't run yet. In such case, we must use the lowest of them
+  // to avoid using the wrong slot.
+  int64_t baseTick = nextTick;
+  if (isScheduled()) {
+    baseTick = std::min(expireTick_, nextTick);
+  }
+  scheduleTimeoutImpl(callback, timeout, baseTick, nextTick);
 
   /* If we're calling callbacks, timer will be reset after all
    * callbacks are called.
@@ -169,7 +177,8 @@ bool HHWheelTimer::cascadeTimers(int bucket, int tick) {
   while (!cbs.empty()) {
     auto* cb = &cbs.front();
     cbs.pop_front();
-    scheduleTimeoutImpl(cb, cb->getTimeRemaining(getCurTime()), calcNextTick());
+    scheduleTimeoutImpl(
+        cb, cb->getTimeRemaining(getCurTime()), lastTick_, calcNextTick());
   }
 
   // If tick is zero, timeoutExpired will cascade the next bucket.

folly/io/async/HHWheelTimer.h

@@ -300,9 +300,21 @@ class HHWheelTimer : private folly::AsyncTimeout,
 
   int64_t calcNextTick();
 
+  /**
+   * Schedule a given timeout by putting it into the appropriate bucket of the
+   * wheel.
+   *
+   * @param callback           Callback to fire after `timeout`
+   * @param timeout            Interval after which the `callback` should be
+   *                           fired.
+   * @param nextTickToProcess  next tick that was not processed by the timer
+   *                           yet. Can be less than nextTick if we're lagging.
+   * @nextTick                 next tick based on the actual time
+   */
   void scheduleTimeoutImpl(
       Callback* callback,
       std::chrono::milliseconds timeout,
+      int64_t nextTickToProcess,
       int64_t nextTick);
   void scheduleNextTimeout(int64_t nextTick);
 

folly/io/async/test/HHWheelTimerTest.cpp

@@ -475,3 +475,23 @@ TEST_F(HHWheelTimerTest, GetTimeRemaining) {
   ASSERT_EQ(t.count(), 0);
   T_CHECK_TIMEOUT(start, end, milliseconds(10));
 }
+
+TEST_F(HHWheelTimerTest, prematureTimeout) {
+  StackWheelTimer t(&eventBase, milliseconds(10));
+  TestTimeout t1;
+  TestTimeout t2;
+  // Schedule the timeout for the nextTick of timer
+  t.scheduleTimeout(&t1, std::chrono::milliseconds(1));
+  // Make sure that time is past that tick.
+  ::usleep(10000);
+  // Schedule the timeout for the +255 tick, due to sleep above it will overlap
+  // with what would be ran on the next timeoutExpired of the timer.
+  auto timeout = std::chrono::milliseconds(2555);
+  t.scheduleTimeout(&t2, std::chrono::milliseconds(2555));
+  auto start = std::chrono::steady_clock::now();
+  eventBase.loop();
+  ASSERT_EQ(t2.timestamps.size(), 1);
+  auto elapsedMs = std::chrono::duration_cast<std::chrono::milliseconds>(
+      t2.timestamps[0].getTime() - start);
+  EXPECT_GE(elapsedMs.count(), timeout.count());
+}