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

diff --git a/folly/futures/detail/ThreadWheelTimekeeper.cpp b/folly/futures/detail/ThreadWheelTimekeeper.cpp
index f861af23aa649c329e04441fd073d3c1a2da06e2..294b71fa11789b3fdcf6ba10083b785fd04e8ef9 100644 (file)
--- a/folly/futures/detail/ThreadWheelTimekeeper.cpp
+++ b/folly/futures/detail/ThreadWheelTimekeeper.cpp
@@ -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() {
-      cancelTimeout();
-      delete this;
+      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);

diff --git a/folly/futures/test/TimekeeperTest.cpp b/folly/futures/test/TimekeeperTest.cpp
index ba792bb3de19c72d703e8482a7fc9e03695097c8..0fdac19a638eb326f7d556990e574fb4971d955e 100644 (file)
--- a/folly/futures/test/TimekeeperTest.cpp
+++ b/folly/futures/test/TimekeeperTest.cpp
@@ -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) {