Relax HHWheelTimer::destroy assertion to accommodate SharedPtr

Summary:
HHWheelTimer's auto-pointers are kind of funny.  You can do something like this:

```
HHWheelTimer::UniquePtr p = ...;
// create a SharedPtr from UniquePtr
HHWheelTimer::SharedPtr s(p);
// create another SharedPtr from raw ptr
HHWheelTimer::SharedPtr s(p.get());
// No problem.

If you do this:

HHWheelTimer::SharedPtr p = ....;
// this leaks
```

Why?  Because SharedPtr is only have of std::shared_ptr.  It's the refcounting half.  But when the last SharedPtr goes out of scope, it **does not** invoke HHWheelTimer::destroy().

So code like this is possible/expected:

```
MySweetObj::MySweetObj(HHWheelTimer::SharedPtr s) {
  s_ = s;
  s_.scheduleTimeout(a, b);
}

{
  HHWheelTimer::UniquePtr p = ...;

  obj = MySweetObj(p)

  // But what if I decide to kill p:
  p.reset();
}
```

Since MySweetObj takes a SharedPtr and holds it, it can reasonbly expect that it can schedule timeouts on it, and the HHWheelTimer will not be deleted until it releases the SharedPtr.  This is true, but the above code would hit the assert that count_ == 0.

Instead, relase the check that count_ == 0 only if there are no extra outstanding SharedPtrs.

Reviewed By: viswanathgs, chadparry

Differential Revision: D2908729

fb-gh-sync-id: 9abd4a7d692fe952c5514dbb8d85dfbad95a3cac
shipit-source-id: 9abd4a7d692fe952c5514dbb8d85dfbad95a3cac

diff --git a/folly/io/async/HHWheelTimer.cpp b/folly/io/async/HHWheelTimer.cpp
index e104ca6fe3e3976a2a64e420b2c7a7a966b4203f..89460b90008ed4b5f32a8343fd7392cafeecdced 100644 (file)
--- a/folly/io/async/HHWheelTimer.cpp
+++ b/folly/io/async/HHWheelTimer.cpp
@@ -96,8 +96,18 @@ HHWheelTimer::~HHWheelTimer() {
 }
 
 void HHWheelTimer::destroy() {
-  assert(count_ == 0);
-  cancelAll();
+  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();
 }
 

diff --git a/folly/io/async/HHWheelTimer.h b/folly/io/async/HHWheelTimer.h
index 70dbcc60d6c7dddfb34480a7ca34fb09a4a15330..8a60e2fdc741a3b5a21d051c0280a9fdf51587bb 100644 (file)
--- a/folly/io/async/HHWheelTimer.h
+++ b/folly/io/async/HHWheelTimer.h
@@ -180,6 +180,9 @@ class HHWheelTimer : private folly::AsyncTimeout,
    * 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();
 

diff --git a/folly/io/async/test/HHWheelTimerTest.cpp b/folly/io/async/test/HHWheelTimerTest.cpp
index 3b6d4d8e7be2ab78f22f7f014dd7e4ed1098e14d..c8f5a4059731b61809d94f38245d8fc621822b68 100644 (file)
--- a/folly/io/async/test/HHWheelTimerTest.cpp
+++ b/folly/io/async/test/HHWheelTimerTest.cpp
@@ -453,3 +453,40 @@ TEST_F(HHWheelTimerTest, cancelAll) {
   EXPECT_EQ(1, t.cancelAll());
   EXPECT_EQ(1, tt.canceledTimestamps.size());
 }
+
+TEST_F(HHWheelTimerTest, SharedPtr) {
+  HHWheelTimer::UniquePtr t(new HHWheelTimer(&eventBase, milliseconds(1)));
+
+  TestTimeout t1;
+  TestTimeout t2;
+  TestTimeout t3;
+
+  ASSERT_EQ(t->count(), 0);
+
+  t->scheduleTimeout(&t1, milliseconds(5));
+  t->scheduleTimeout(&t2, milliseconds(5));
+
+  HHWheelTimer::SharedPtr s(t);
+
+  s->scheduleTimeout(&t3, milliseconds(10));
+
+  ASSERT_EQ(t->count(), 3);
+
+  // Kill the UniquePtr, but the SharedPtr keeps it alive
+  t.reset();
+
+  TimePoint start;
+  eventBase.loop();
+  TimePoint end;
+
+  ASSERT_EQ(t1.timestamps.size(), 1);
+  ASSERT_EQ(t2.timestamps.size(), 1);
+  ASSERT_EQ(t3.timestamps.size(), 1);
+
+  ASSERT_EQ(s->count(), 0);
+
+  T_CHECK_TIMEOUT(start, t1.timestamps[0], milliseconds(5));
+  T_CHECK_TIMEOUT(start, t2.timestamps[0], milliseconds(5));
+  T_CHECK_TIMEOUT(start, t3.timestamps[0], milliseconds(10));
+  T_CHECK_TIMEOUT(start, end, milliseconds(10));
+}