Commit 3f6e3b45 authored by Chad Parry's avatar Chad Parry Committed by Facebook Github Bot 6

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
parent 2488a9d5
...@@ -55,7 +55,6 @@ void HHWheelTimer::Callback::setScheduled(HHWheelTimer* wheel, ...@@ -55,7 +55,6 @@ void HHWheelTimer::Callback::setScheduled(HHWheelTimer* wheel,
assert(wheel_ == nullptr); assert(wheel_ == nullptr);
assert(expiration_ == milliseconds(0)); assert(expiration_ == milliseconds(0));
wheelGuard_ = DestructorGuard(wheel);
wheel_ = wheel; wheel_ = wheel;
// Only update the now_ time if we're not in a timeout expired callback // Only update the now_ time if we're not in a timeout expired callback
...@@ -74,14 +73,14 @@ void HHWheelTimer::Callback::cancelTimeoutImpl() { ...@@ -74,14 +73,14 @@ void HHWheelTimer::Callback::cancelTimeoutImpl() {
hook_.unlink(); hook_.unlink();
wheel_ = nullptr; wheel_ = nullptr;
wheelGuard_ = folly::none;
expiration_ = milliseconds(0); expiration_ = milliseconds(0);
} }
HHWheelTimer::HHWheelTimer(folly::TimeoutManager* timeoutMananger, HHWheelTimer::HHWheelTimer(
std::chrono::milliseconds intervalMS, folly::TimeoutManager* timeoutMananger,
AsyncTimeout::InternalEnum internal, std::chrono::milliseconds intervalMS,
std::chrono::milliseconds defaultTimeoutMS) AsyncTimeout::InternalEnum internal,
std::chrono::milliseconds defaultTimeoutMS)
: AsyncTimeout(timeoutMananger, internal), : AsyncTimeout(timeoutMananger, internal),
interval_(intervalMS), interval_(intervalMS),
defaultTimeout_(defaultTimeoutMS), defaultTimeout_(defaultTimeoutMS),
...@@ -89,26 +88,17 @@ HHWheelTimer::HHWheelTimer(folly::TimeoutManager* timeoutMananger, ...@@ -89,26 +88,17 @@ HHWheelTimer::HHWheelTimer(folly::TimeoutManager* timeoutMananger,
count_(0), count_(0),
catchupEveryN_(DEFAULT_CATCHUP_EVERY_N), catchupEveryN_(DEFAULT_CATCHUP_EVERY_N),
expirationsSinceCatchup_(0), expirationsSinceCatchup_(0),
processingCallbacksGuard_(false) {} processingCallbacksGuard_(nullptr) {}
HHWheelTimer::~HHWheelTimer() { HHWheelTimer::~HHWheelTimer() {
CHECK(count_ == 0); // Ensure this gets done, but right before destruction finishes.
} auto destructionPublisherGuard = folly::makeGuard([&] {
// Inform the subscriber that this instance is doomed.
void HHWheelTimer::destroy() { if (processingCallbacksGuard_) {
if (getDestructorGuardCount() == count_) { *processingCallbacksGuard_ = true;
// Every callback holds a DestructorGuard. In this simple case, }
// All timeouts should already be gone. });
assert(count_ == 0); cancelAll();
// 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();
} }
void HHWheelTimer::scheduleTimeoutImpl(Callback* callback, void HHWheelTimer::scheduleTimeoutImpl(Callback* callback,
...@@ -172,15 +162,19 @@ bool HHWheelTimer::cascadeTimers(int bucket, int tick) { ...@@ -172,15 +162,19 @@ bool HHWheelTimer::cascadeTimers(int bucket, int tick) {
} }
void HHWheelTimer::timeoutExpired() noexcept { void HHWheelTimer::timeoutExpired() noexcept {
// If destroy() is called inside timeoutExpired(), delay actual destruction // If the last smart pointer for "this" is reset inside the callback's
// until timeoutExpired() returns // timeoutExpired(), then the guard will detect that it is time to bail from
DestructorGuard dg(this); // this method.
auto isDestroyed = false;
// If scheduleTimeout is called from a callback in this function, it may // If scheduleTimeout is called from a callback in this function, it may
// cause inconsistencies in the state of this object. As such, we need // cause inconsistencies in the state of this object. As such, we need
// to treat these calls slightly differently. // to treat these calls slightly differently.
processingCallbacksGuard_ = true; CHECK(!processingCallbacksGuard_);
processingCallbacksGuard_ = &isDestroyed;
auto reEntryGuard = folly::makeGuard([&] { auto reEntryGuard = folly::makeGuard([&] {
processingCallbacksGuard_ = false; if (!isDestroyed) {
processingCallbacksGuard_ = nullptr;
}
}); });
// timeoutExpired() can only be invoked directly from the event base loop. // timeoutExpired() can only be invoked directly from the event base loop.
...@@ -216,6 +210,12 @@ void HHWheelTimer::timeoutExpired() noexcept { ...@@ -216,6 +210,12 @@ void HHWheelTimer::timeoutExpired() noexcept {
cb->expiration_ = milliseconds(0); cb->expiration_ = milliseconds(0);
RequestContextScopeGuard rctx(cb->context_); RequestContextScopeGuard rctx(cb->context_);
cb->timeoutExpired(); 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) { if (count_ > 0) {
......
...@@ -25,8 +25,8 @@ ...@@ -25,8 +25,8 @@
#include <chrono> #include <chrono>
#include <cstddef> #include <cstddef>
#include <memory>
#include <list> #include <list>
#include <memory>
namespace folly { namespace folly {
...@@ -136,7 +136,6 @@ class HHWheelTimer : private folly::AsyncTimeout, ...@@ -136,7 +136,6 @@ class HHWheelTimer : private folly::AsyncTimeout,
void cancelTimeoutImpl(); void cancelTimeoutImpl();
HHWheelTimer* wheel_; HHWheelTimer* wheel_;
folly::Optional<DestructorGuard> wheelGuard_;
std::chrono::milliseconds expiration_; std::chrono::milliseconds expiration_;
typedef boost::intrusive::list_member_hook< typedef boost::intrusive::list_member_hook<
...@@ -174,18 +173,6 @@ class HHWheelTimer : private folly::AsyncTimeout, ...@@ -174,18 +173,6 @@ class HHWheelTimer : private folly::AsyncTimeout,
std::chrono::milliseconds defaultTimeoutMS = std::chrono::milliseconds defaultTimeoutMS =
std::chrono::milliseconds(-1)); 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 * Cancel all outstanding timeouts
* *
...@@ -327,7 +314,7 @@ class HHWheelTimer : private folly::AsyncTimeout, ...@@ -327,7 +314,7 @@ class HHWheelTimer : private folly::AsyncTimeout,
uint32_t catchupEveryN_; uint32_t catchupEveryN_;
uint32_t expirationsSinceCatchup_; uint32_t expirationsSinceCatchup_;
bool processingCallbacksGuard_; bool* processingCallbacksGuard_;
}; };
} // folly } // folly
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment