From 3f6e3b45bd73d9aa9827c7f680d2676a0a91f4c5 Mon Sep 17 00:00:00 2001 From: Chad Parry Date: Thu, 9 Jun 2016 15:07:58 -0700 Subject: [PATCH] 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 --- folly/io/async/HHWheelTimer.cpp | 58 ++++++++++++++++----------------- folly/io/async/HHWheelTimer.h | 17 ++-------- 2 files changed, 31 insertions(+), 44 deletions(-) diff --git a/folly/io/async/HHWheelTimer.cpp b/folly/io/async/HHWheelTimer.cpp index 20b9da8e..e16e4b39 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 d708d019..0661a35e 100644 --- a/folly/io/async/HHWheelTimer.h +++ b/folly/io/async/HHWheelTimer.h @@ -25,8 +25,8 @@ #include #include -#include #include +#include namespace folly { @@ -136,7 +136,6 @@ class HHWheelTimer : private folly::AsyncTimeout, void cancelTimeoutImpl(); HHWheelTimer* wheel_; - folly::Optional 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 -- 2.34.1