From: Alan Frindell <afrind@fb.com>
Date: Tue, 9 Feb 2016 17:44:21 +0000 (-0800)
Subject: Relax HHWheelTimer::destroy assertion to accommodate SharedPtr
X-Git-Tag: deprecate-dynamic-initializer~85
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=5e5cf95a1602ea0edafbc2f04a9ae03ec611007f;p=folly.git

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 e104ca6f..89460b90 100644
--- 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 70dbcc60..8a60e2fd 100644
--- 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 3b6d4d8e..c8f5a405 100644
--- 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));
+}