Make DestructorGuard inherit from a base

Summary: There are more use cases that the Destruction/Guard pattern can be
used than current DelayedDestruction provides. This diff makes the pattern more
general (remove self destruct) and lets DelayedDestruction derive from that.
The functionalities of DelayedDestruction is kept unchanged.

I leave destroy(), Destructor class, and destroyPending_ in DelayedDestruction
since they are not required by our CallbackGuard in proxygen.

I add a shouldDestruct() function to allow customized conditions on when to
call destructor.

I haven't made destroyNow() a std::function since I only need it to be set at
instatiation time. If there is any other use case that needs destroyNow() to be
a std::function, I can do that as well.

Reviewed By: @afrind

Differential Revision: D2202575

folly/Makefile.am

@@ -193,6 +193,7 @@ nobase_follyinclude_HEADERS = \
 	io/async/AsyncSocketBase.h \
 	io/async/AsyncSSLSocket.h \
 	io/async/AsyncSocketException.h \
+	io/async/DelayedDestructionBase.h \
 	io/async/DelayedDestruction.h \
 	io/async/EventBase.h \
 	io/async/EventBaseManager.h \

folly/io/async/DelayedDestruction.h

@@ -16,9 +16,9 @@
 
 #pragma once
 
-#include <boost/noncopyable.hpp>
-#include <inttypes.h>
-#include <assert.h>
+#include <folly/io/async/DelayedDestructionBase.h>
+
+#include <glog/logging.h>
 
 namespace folly {
 
@@ -39,23 +39,8 @@ namespace folly {
  * DelayedDestruction does not perform any locking.  It is intended to be used
  * only from a single thread.
  */
-class DelayedDestruction : private boost::noncopyable {
+class DelayedDestruction : public DelayedDestructionBase {
  public:
-  /**
-   * Helper class to allow DelayedDestruction classes to be used with
-   * std::shared_ptr.
-   *
-   * This class can be specified as the destructor argument when creating the
-   * shared_ptr, and it will destroy the guarded class properly when all
-   * shared_ptr references are released.
-   */
-  class Destructor {
-   public:
-    void operator()(DelayedDestruction* dd) const {
-      dd->destroy();
-    }
-  };
-
   /**
    * destroy() requests destruction of the object.
    *
@@ -66,67 +51,34 @@ class DelayedDestruction : private boost::noncopyable {
   virtual void destroy() {
     // If guardCount_ is not 0, just set destroyPending_ to delay
     // actual destruction.
-    if (guardCount_ != 0) {
+    VLOG(4) << "DelayedDestruction::destroy()";
+    if (getDestructorGuardCount() != 0) {
       destroyPending_ = true;
     } else {
-      destroyNow(false);
+      onDestroy_(false);
     }
   }
 
   /**
-   * Classes should create a DestructorGuard object on the stack in any
-   * function that may invoke callback functions.
+   * Helper class to allow DelayedDestruction classes to be used with
+   * std::shared_ptr.
    *
-   * The DestructorGuard prevents the guarded class from being destroyed while
-   * it exists.  Without this, the callback function could delete the guarded
-   * object, causing problems when the callback function returns and the
-   * guarded object's method resumes execution.
+   * This class can be specified as the destructor argument when creating the
+   * shared_ptr, and it will destroy the guarded class properly when all
+   * shared_ptr references are released.
    */
-  class DestructorGuard {
+  class Destructor {
    public:
-
-    explicit DestructorGuard(DelayedDestruction* dd) : dd_(dd) {
-      ++dd_->guardCount_;
-      assert(dd_->guardCount_ > 0); // check for wrapping
-    }
-
-    DestructorGuard(const DestructorGuard& dg) : dd_(dg.dd_) {
-      ++dd_->guardCount_;
-      assert(dd_->guardCount_ > 0); // check for wrapping
-    }
-
-    ~DestructorGuard() {
-      assert(dd_->guardCount_ > 0);
-      --dd_->guardCount_;
-      if (dd_->guardCount_ == 0 && dd_->destroyPending_) {
-        dd_->destroyPending_ = false;
-        dd_->destroyNow(true);
-      }
+    void operator()(DelayedDestruction* dd) const {
+      dd->destroy();
     }
-
-   private:
-    DelayedDestruction* dd_;
   };
 
- protected:
-  /**
-   * destroyNow() is invoked to actually destroy the object, after destroy()
-   * has been called and no more DestructorGuard objects exist.  By default it
-   * calls "delete this", but subclasses may override this behavior.
-   *
-   * @param delayed  This parameter is true if destruction was delayed because
-   *                 of a DestructorGuard object, or false if destroyNow() is
-   *                 being called directly from destroy().
-   */
-  virtual void destroyNow(bool delayed) {
-    delete this;
-    (void)delayed; // prevent unused variable warnings
+  bool getDestroyPending() const {
+    return destroyPending_;
   }
 
-  DelayedDestruction()
-    : guardCount_(0)
-    , destroyPending_(false) {}
-
+ protected:
   /**
    * Protected destructor.
    *
@@ -145,28 +97,21 @@ class DelayedDestruction : private boost::noncopyable {
    */
   virtual ~DelayedDestruction() = default;
 
-  /**
-   * Get the number of DestructorGuards currently protecting this object.
-   *
-   * This is primarily intended for debugging purposes, such as asserting
-   * that an object has at least 1 guard.
-   */
-  uint32_t getDestructorGuardCount() const {
-    return guardCount_;
+  DelayedDestruction()
+    : destroyPending_(false) {
+
+    onDestroy_ = [this] (bool delayed) {
+      // check if it is ok to destroy now
+      if (delayed && !destroyPending_) {
+        return;
+      }
+      VLOG(4) << "DelayedDestruction::onDestroy_ delayed=" << delayed;
+      delete this;
+      (void)delayed; // prevent unused variable warnings
+    };
   }
 
  private:
-  /**
-   * guardCount_ is incremented by DestructorGuard, to indicate that one of
-   * the DelayedDestruction object's methods is currently running.
-   *
-   * If destroy() is called while guardCount_ is non-zero, destruction will
-   * be delayed until guardCount_ drops to 0.  This allows DelayedDestruction
-   * objects to invoke callbacks without having to worry about being deleted
-   * before the callback returns.
-   */
-  uint32_t guardCount_;
-
   /**
    * destroyPending_ is set to true if destoy() is called while guardCount_ is
    * non-zero.

folly/io/async/DelayedDestructionBase.h

+/*
+ * Copyright 2015 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <assert.h>
+#include <boost/noncopyable.hpp>
+#include <functional>
+#include <glog/logging.h>
+#include <inttypes.h>
+
+namespace folly {
+
+/**
+ * DelayedDestructionBase is a helper class to ensure objects are not deleted
+ * while they still have functions executing in a higher stack frame.
+ *
+ * This is useful for objects that invoke callback functions, to ensure that a
+ * callback does not destroy the calling object.
+ *
+ * Classes needing this functionality should:
+ * - derive from DelayedDestructionBase directly
+ * - pass a callback to onDestroy_ which'll be called before the object is
+ *   going to be destructed
+ * - create a DestructorGuard object on the stack in each public method that
+ *   may invoke a callback
+ *
+ * DelayedDestructionBase does not perform any locking.  It is intended to be
+ * used only from a single thread.
+ */
+class DelayedDestructionBase : private boost::noncopyable {
+ public:
+  virtual ~DelayedDestructionBase() = default;
+
+  /**
+   * Classes should create a DestructorGuard object on the stack in any
+   * function that may invoke callback functions.
+   *
+   * The DestructorGuard prevents the guarded class from being destroyed while
+   * it exists.  Without this, the callback function could delete the guarded
+   * object, causing problems when the callback function returns and the
+   * guarded object's method resumes execution.
+   */
+  class DestructorGuard {
+   public:
+
+    explicit DestructorGuard(DelayedDestructionBase* dd) : dd_(dd) {
+      ++dd_->guardCount_;
+      assert(dd_->guardCount_ > 0); // check for wrapping
+    }
+
+    DestructorGuard(const DestructorGuard& dg) : dd_(dg.dd_) {
+      ++dd_->guardCount_;
+      assert(dd_->guardCount_ > 0); // check for wrapping
+    }
+
+    ~DestructorGuard() {
+      assert(dd_->guardCount_ > 0);
+      --dd_->guardCount_;
+      if (dd_->guardCount_ == 0) {
+        dd_->onDestroy_(true);
+      }
+    }
+
+   private:
+    DelayedDestructionBase* dd_;
+  };
+
+ protected:
+  DelayedDestructionBase()
+    : guardCount_(0) {}
+
+  /**
+   * Get the number of DestructorGuards currently protecting this object.
+   *
+   * This is primarily intended for debugging purposes, such as asserting
+   * that an object has at least 1 guard.
+   */
+  uint32_t getDestructorGuardCount() const {
+    return guardCount_;
+  }
+
+  /**
+   * Implement onDestroy_ in subclasses.
+   * onDestroy_() is invoked when the object is potentially being destroyed.
+   *
+   * @param delayed  This parameter is true if destruction was delayed because
+   *                 of a DestructorGuard object, or false if onDestroy_() is
+   *                 being called directly from the destructor.
+   */
+  std::function<void(bool)> onDestroy_;
+
+ private:
+  /**
+   * guardCount_ is incremented by DestructorGuard, to indicate that one of
+   * the DelayedDestructionBase object's methods is currently running.
+   *
+   * If the destructor is called while guardCount_ is non-zero, destruction
+   * will be delayed until guardCount_ drops to 0.  This allows
+   * DelayedDestructionBase objects to invoke callbacks without having to worry
+   * about being deleted before the callback returns.
+   */
+  uint32_t guardCount_;
+};
+} // folly

folly/io/async/test/DelayedDestructionBaseTest.cpp

+/*
+ * Copyright 2015 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#include <folly/io/async/DelayedDestructionBase.h>
+
+#include <functional>
+#include <gtest/gtest.h>
+#include <list>
+#include <vector>
+
+using namespace folly;
+
+class DestructionOnCallback : public DelayedDestructionBase {
+ public:
+  DestructionOnCallback() : state_(0), deleted_(false) {
+    onDestroy_ = [this] (bool delayed) {
+      deleted_ = true;
+      delete this;
+      (void)delayed; // prevent unused variable warnings
+    };
+  }
+
+  void onComplete(int n, int& state) {
+    DestructorGuard dg(this);
+    for (auto i = n; i >= 0; --i) {
+      onStackedComplete(i);
+    }
+    state = state_;
+  }
+
+  void setOnDestroy(std::function<void(bool)> onDestroy) {
+    onDestroy_ = onDestroy;
+  }
+
+  int state() const { return state_; }
+  bool deleted() const { return deleted_; }
+
+ protected:
+  void onStackedComplete(int recur) {
+    DestructorGuard dg(this);
+    ++state_;
+    if (recur <= 0) {
+      return;
+    }
+    onStackedComplete(--recur);
+  }
+ private:
+  int state_;
+  bool deleted_;
+};
+
+struct DelayedDestructionBaseTest : public ::testing::Test {
+};
+
+TEST_F(DelayedDestructionBaseTest, basic) {
+  DestructionOnCallback* d = new DestructionOnCallback();
+  EXPECT_NE(d, nullptr);
+  int32_t state;
+  d->onComplete(3, state);
+  EXPECT_EQ(state, 10); // 10 = 6 + 3 + 1
+  EXPECT_EQ(d->deleted(), true);
+}
+
+TEST_F(DelayedDestructionBaseTest, destructFromContainer) {
+  std::list<DestructionOnCallback> l;
+  l.emplace_back();
+  l.back().setOnDestroy([&] (bool delayed) {
+    l.erase(l.begin());
+    (void)delayed;
+  });
+  EXPECT_NE(l.size(), 0);
+  int32_t state;
+  l.back().onComplete(3, state);
+  EXPECT_EQ(state, 10); // 10 = 6 + 3 + 1
+  EXPECT_EQ(l.size(), 0);
+}

folly/io/async/test/UndelayedDestruction.h

@@ -57,7 +57,18 @@ class UndelayedDestruction : public TDD {
   // behavior for non-destructible types, which is unfortunate.)
   template<typename ...Args>
   explicit UndelayedDestruction(Args&& ...args)
-    : TDD(std::forward<Args>(args)...) {}
+    : TDD(std::forward<Args>(args)...) {
+      this->TDD::onDestroy_ = [&, this] (bool delayed) {
+        if (delayed && !this->TDD::getDestroyPending()) {
+          return;
+        }
+        // Do nothing.  This will always be invoked from the call to destroy
+        // inside our destructor.
+        assert(!delayed);
+        // prevent unused variable warnings when asserts are compiled out.
+        (void)delayed;
+      };
+  }
 
   /**
    * Public destructor.
@@ -89,14 +100,6 @@ class UndelayedDestruction : public TDD {
     this->TDD::destroy();
   }
 
-  virtual void destroyNow(bool delayed) {
-    // Do nothing.  This will always be invoked from the call to destroy inside
-    // our destructor.
-    assert(!delayed);
-    // prevent unused variable warnings when asserts are compiled out.
-    (void)delayed;
-  }
-
  private:
   // Forbidden copy constructor and assignment operator
   UndelayedDestruction(UndelayedDestruction const &) = delete;