diff --git a/folly/io/async/HHWheelTimer.cpp b/folly/io/async/HHWheelTimer.cpp index 20b9da8ee672a3bb8a09636533789b1cc4738be9..e16e4b39b93f686912d461b9a1400956f488edf8 100644 --- 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 --- 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