Commit 03439afb authored by Dave Watson's avatar Dave Watson Committed by Pavlo Kushnir

Fix cancel in ThreadWheelTimeKeeper

Summary: This is actually a bug, future.cancel() doesn't work with the current THreadWheelTimekeeper, because cancel() only works from the eventBase thread.

Test Plan: added unittest.  Crashes before, passes now

Reviewed By: hans@fb.com

Subscribers: doug, folly-diffs@, jsedgwick, yfeldblum, chalfant

FB internal diff: D2091531

Signature: t1:2091531:1432224024:4aa5dd71de15b1344034a414d47c97ffaba68949
parent 496a1139
......@@ -27,9 +27,9 @@ namespace {
// Our Callback object for HHWheelTimer
struct WTCallback : public folly::HHWheelTimer::Callback {
// Only allow creation by this factory, to ensure heap allocation.
static WTCallback* create() {
static WTCallback* create(EventBase* base) {
// optimization opportunity: memory pool
return new WTCallback();
return new WTCallback(base);
}
Future<void> getFuture() {
......@@ -37,9 +37,11 @@ namespace {
}
protected:
EventBase* base_;
Promise<void> promise_;
explicit WTCallback() {
explicit WTCallback(EventBase* base)
: base_(base) {
promise_.setInterruptHandler(
std::bind(&WTCallback::interruptHandler, this));
}
......@@ -50,8 +52,10 @@ namespace {
}
void interruptHandler() {
base_->runInEventBaseThread([=] {
cancelTimeout();
delete this;
});
}
};
......@@ -78,7 +82,7 @@ ThreadWheelTimekeeper::~ThreadWheelTimekeeper() {
}
Future<void> ThreadWheelTimekeeper::after(Duration dur) {
auto cob = WTCallback::create();
auto cob = WTCallback::create(&eventBase_);
auto f = cob->getFuture();
eventBase_.runInEventBaseThread([=]{
wheelTimer_->scheduleTimeout(cob, dur);
......
......@@ -25,6 +25,7 @@ using Duration = folly::Duration;
std::chrono::milliseconds const one_ms(1);
std::chrono::milliseconds const awhile(10);
std::chrono::seconds const too_long(10);
std::chrono::steady_clock::time_point now() {
return std::chrono::steady_clock::now();
......@@ -154,6 +155,11 @@ TEST(Timekeeper, onTimeoutVoid) {
// just testing compilation here
}
TEST(Timekeeper, interruptDoesntCrash) {
auto f = futures::sleep(too_long);
f.cancel();
}
// TODO(5921764)
/*
TEST(Timekeeper, onTimeoutPropagates) {
......
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