Allow a timer with pending callbacks to be destroyed

Summary:
This diff is part of a string of changes that is getting `HHWheelTimer` to use standard smart pointers.

D2617966 is being reverted. The `DestructorGuard` that was added for safety is not possible without intrusive reference counting.
That means that the `count_` (of pending callbacks) will still be correct, but there will be nothing to check it against. Because of that, I removed the assertions to make sure that the count of pending callbacks matches the count of active guards. Anyway, there were already ways to defeat that test, such as by creating a `DestructorGuard` from any client code.
As long as the assertions are gone, the `cancelAll` can be moved to the destructor. It only ever got run when `destroy` was called and `count_` was `0`, which means that `~HHWheelTimer` was also going to get called at the same time.

Reviewed By: djwatson

Differential Revision: D3349303

fbshipit-source-id: 433fa6605ee9921833328e2f82bd07b2e17e42c5

diff --git a/folly/io/async/HHWheelTimer.cpp b/folly/io/async/HHWheelTimer.cpp
index 20b9da8ee672a3bb8a09636533789b1cc4738be9..e16e4b39b93f686912d461b9a1400956f488edf8 100644 (file)
--- a/folly/io/async/HHWheelTimer.cpp
+++ b/folly/io/async/HHWheelTimer.cpp
@@ -55,7 +55,6 @@ void HHWheelTimer::Callback::setScheduled(HHWheelTimer* wheel,
   assert(wheel_ == nullptr);
   assert(expiration_ == milliseconds(0));
 
-  wheelGuard_ = DestructorGuard(wheel);
   wheel_ = wheel;
 
   // Only update the now_ time if we're not in a timeout expired callback
@@ -74,14 +73,14 @@ void HHWheelTimer::Callback::cancelTimeoutImpl() {
   hook_.unlink();
 
   wheel_ = nullptr;
-  wheelGuard_ = folly::none;
   expiration_ = milliseconds(0);
 }
 
-HHWheelTimer::HHWheelTimer(folly::TimeoutManager* timeoutMananger,
-                           std::chrono::milliseconds intervalMS,
-                           AsyncTimeout::InternalEnum internal,
-                           std::chrono::milliseconds defaultTimeoutMS)
+HHWheelTimer::HHWheelTimer(
+    folly::TimeoutManager* timeoutMananger,
+    std::chrono::milliseconds intervalMS,
+    AsyncTimeout::InternalEnum internal,
+    std::chrono::milliseconds defaultTimeoutMS)
     : AsyncTimeout(timeoutMananger, internal),
       interval_(intervalMS),
       defaultTimeout_(defaultTimeoutMS),
@@ -89,26 +88,17 @@ HHWheelTimer::HHWheelTimer(folly::TimeoutManager* timeoutMananger,
       count_(0),
       catchupEveryN_(DEFAULT_CATCHUP_EVERY_N),
       expirationsSinceCatchup_(0),
-      processingCallbacksGuard_(false) {}
+      processingCallbacksGuard_(nullptr) {}
 
 HHWheelTimer::~HHWheelTimer() {
-  CHECK(count_ == 0);
-}
-
-void HHWheelTimer::destroy() {
-  if (getDestructorGuardCount() == count_) {
-    // Every callback holds a DestructorGuard.  In this simple case,
-    // All timeouts should already be gone.
-    assert(count_ == 0);
-
-    // Clean them up in opt builds and move on
-    cancelAll();
-  }
-  // else, we are only marking pending destruction, but one or more
-  // HHWheelTimer::SharedPtr's (and possibly their timeouts) are still holding
-  // this HHWheelTimer.  We cannot assert that all timeouts have been cancelled,
-  // and will just have to wait for them all to complete on their own.
-  DelayedDestruction::destroy();
+  // Ensure this gets done, but right before destruction finishes.
+  auto destructionPublisherGuard = folly::makeGuard([&] {
+    // Inform the subscriber that this instance is doomed.
+    if (processingCallbacksGuard_) {
+      *processingCallbacksGuard_ = true;
+    }
+  });
+  cancelAll();
 }
 
 void HHWheelTimer::scheduleTimeoutImpl(Callback* callback,
@@ -172,15 +162,19 @@ bool HHWheelTimer::cascadeTimers(int bucket, int tick) {
 }
 
 void HHWheelTimer::timeoutExpired() noexcept {
-  // If destroy() is called inside timeoutExpired(), delay actual destruction
-  // until timeoutExpired() returns
-  DestructorGuard dg(this);
+  // If the last smart pointer for "this" is reset inside the callback's
+  // timeoutExpired(), then the guard will detect that it is time to bail from
+  // this method.
+  auto isDestroyed = false;
   // If scheduleTimeout is called from a callback in this function, it may
   // cause inconsistencies in the state of this object. As such, we need
   // to treat these calls slightly differently.
-  processingCallbacksGuard_ = true;
+  CHECK(!processingCallbacksGuard_);
+  processingCallbacksGuard_ = &isDestroyed;
   auto reEntryGuard = folly::makeGuard([&] {
-    processingCallbacksGuard_ = false;
+    if (!isDestroyed) {
+      processingCallbacksGuard_ = nullptr;
+    }
   });
 
   // timeoutExpired() can only be invoked directly from the event base loop.
@@ -216,6 +210,12 @@ void HHWheelTimer::timeoutExpired() noexcept {
       cb->expiration_ = milliseconds(0);
       RequestContextScopeGuard rctx(cb->context_);
       cb->timeoutExpired();
+      if (isDestroyed) {
+        // The HHWheelTimer itself has been destroyed. The other callbacks
+        // will have been cancelled from the destructor. Bail before causing
+        // damage.
+        return;
+      }
     }
   }
   if (count_ > 0) {

diff --git a/folly/io/async/HHWheelTimer.h b/folly/io/async/HHWheelTimer.h
index d708d0197b3ad3f442fc6718cdd3e9d22b02400c..0661a35e3118f44da86b2ac5d9e5c4e9dbb19933 100644 (file)
--- a/folly/io/async/HHWheelTimer.h
+++ b/folly/io/async/HHWheelTimer.h
@@ -25,8 +25,8 @@
 
 #include <chrono>
 #include <cstddef>
-#include <memory>
 #include <list>
+#include <memory>
 
 namespace folly {
 
@@ -136,7 +136,6 @@ class HHWheelTimer : private folly::AsyncTimeout,
     void cancelTimeoutImpl();
 
     HHWheelTimer* wheel_;
-    folly::Optional<DestructorGuard> wheelGuard_;
     std::chrono::milliseconds expiration_;
 
     typedef boost::intrusive::list_member_hook<
@@ -174,18 +173,6 @@ class HHWheelTimer : private folly::AsyncTimeout,
       std::chrono::milliseconds defaultTimeoutMS =
           std::chrono::milliseconds(-1));
 
-  /**
-   * Destroy the HHWheelTimer.
-   *
-   * A HHWheelTimer should only be destroyed when there are no more
-   * callbacks pending in the set. (If it helps you may use cancelAll() to
-   * cancel all pending timeouts explicitly before calling this.)
-   *
-   * However, it is OK to invoke this function (or via UniquePtr dtor) while
-   * there are outstanding DestructorGuard's or HHWheelTimer::SharedPtr's.
-   */
-  virtual void destroy();
-
   /**
    * Cancel all outstanding timeouts
    *
@@ -327,7 +314,7 @@ class HHWheelTimer : private folly::AsyncTimeout,
 
   uint32_t catchupEveryN_;
   uint32_t expirationsSinceCatchup_;
-  bool processingCallbacksGuard_;
+  bool* processingCallbacksGuard_;
 };
 
 } // folly