Moving DestructorCheck from proxygen library to folly.

Summary:
allow-large-files
Moving DestructorCheck from proxygen library to folly.

Reviewed By: djwatson

Differential Revision: D5110897

fbshipit-source-id: 02a6cf6336000e8c36484e75d2da820588baf2df

CMakeLists.txt

@@ -386,6 +386,7 @@ if (BUILD_TESTS)
       TEST async_timeout_test SOURCES AsyncTimeoutTest.cpp
       TEST AsyncUDPSocketTest SOURCES AsyncUDPSocketTest.cpp
       TEST DelayedDestructionTest SOURCES DelayedDestructionTest.cpp
+      TEST DestructorCheckTest SOURCES DestructorCheckTest.cpp
       TEST DelayedDestructionBaseTest SOURCES DelayedDestructionBaseTest.cpp
       TEST EventBaseTest SOURCES EventBaseTest.cpp
       TEST EventBaseLocalTest SOURCES EventBaseLocalTest.cpp

folly/Makefile.am

@@ -241,6 +241,7 @@ nobase_follyinclude_HEADERS = \
 	io/async/DecoratedAsyncTransportWrapper.h \
 	io/async/DelayedDestructionBase.h \
 	io/async/DelayedDestruction.h \
+	io/async/DestructorCheck.h \
 	io/async/EventBase.h \
 	io/async/EventBaseLocal.h \
 	io/async/EventBaseManager.h \

folly/io/async/DestructorCheck.h

+/*
+ * Copyright 2017 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
+
+namespace folly {
+
+/**
+ * DestructorCheck is a helper class that helps to detect if a tracked object
+ * was deleted.
+ * This is useful for objects that request callbacks from other components.
+ *
+ * Classes needing this functionality should:
+ * - derive from DestructorCheck
+ *
+ * Callback context can be extended with an instance of DestructorCheck::Safety
+ * object initialized with a reference to the object dereferenced from the
+ * callback.  Once the callback is invoked, it can use this safety object to
+ * check if the object was not deallocated yet before dereferencing it.
+ *
+ * DestructorCheck does not perform any locking.  It is intended to be used
+ * only from a single thread.
+ *
+ * Example:
+ *
+ * class AsyncFoo : public DestructorCheck {
+ *  public:
+ *   ~AsyncFoo();
+ *   // awesome async code with circuitous deletion paths
+ *   void async1();
+ *   void async2();
+ * };
+ *
+ * righteousFunc(AsyncFoo& f) {
+ *   DestructorCheck::Safety safety(f);
+ *
+ *   f.async1(); // might have deleted f, oh noes
+ *   if (!safety.destroyed()) {
+ *     // phew, still there
+ *     f.async2();
+ *   }
+ * }
+ */
+
+class DestructorCheck {
+ public:
+  virtual ~DestructorCheck() {
+    rootGuard_.setAllDestroyed();
+  }
+
+  class Safety;
+
+  class ForwardLink {
+   // These methods are mostly private because an outside caller could violate
+   // the integrity of the linked list.
+   private:
+    void setAllDestroyed() {
+      for (auto guard = next_; guard; guard = guard->next_) {
+        guard->setDestroyed();
+      }
+    }
+
+    // This is used to maintain the double-linked list. An intrusive list does
+    // not require any heap allocations, like a standard container would. This
+    // isolation of next_ in its own class means that the DestructorCheck can
+    // easily hold a next_ pointer without needing to hold a prev_ pointer.
+    // DestructorCheck never needs a prev_ pointer because it is the head node
+    // and this is a special list where the head never moves and never has a
+    // previous node.
+    Safety* next_{nullptr};
+
+    friend DestructorCheck::~DestructorCheck();
+    friend class Safety;
+  };
+
+  // See above example for usage
+  class Safety : public ForwardLink {
+   public:
+    explicit Safety(DestructorCheck& destructorCheck) {
+      // Insert this node at the head of the list.
+      prev_ = &destructorCheck.rootGuard_;
+      next_ = prev_->next_;
+      if (next_ != nullptr) {
+        next_->prev_ = this;
+      }
+      prev_->next_ = this;
+     }
+
+    ~Safety() {
+      if (!destroyed()) {
+        // Remove this node from the list.
+        prev_->next_ = next_;
+        if (next_ != nullptr) {
+          next_->prev_ = prev_;
+        }
+      }
+    }
+
+    bool destroyed() const {
+      return prev_ == nullptr;
+    }
+
+   private:
+    void setDestroyed() {
+      prev_ = nullptr;
+    }
+
+    // This field is used to maintain the double-linked list. If the root has
+    // been destroyed then the field is set to the nullptr sentinel value.
+    ForwardLink* prev_;
+
+    friend class ForwardLink;
+  };
+
+ private:
+  ForwardLink rootGuard_;
+};
+
+}

folly/io/async/README.md

@@ -293,6 +293,30 @@ type.  It's also pretty easy to forget to add a DestructorGuard in
 code that calls callbacks.  But it is well worth it to avoid queuing
 callbacks, and the improved P99 times as a result.
 
+### DestructorCheck
+
+Often for an object requesting callbacks from other components (timer,
+socket connect, etc.) there is a chance that the requestor will be
+deallocated before it'll receive the callback.  One of the ways to avoid
+dereferencing the deallocated object from callbacks is to derive the
+object from DelayedDestruction, and add a delayed destruction guard
+to the callback context.  In case if keeping the object around until
+all the requested callbacks fire is too expensive, or if the callback
+requestor can't have private destructor (it's allocated on the stack,
+or as a member of a larger object), DestructorCheck can be used.
+DestructorCheck is not affecting object life time. It helps other
+component to detect safely that the tracked object was deallocated.
+
+The object requesting the callback must be derived from DestructorCheck.
+The callback context should contain an instance of
+DestructorCheck::Safety object initialized with a reference to the
+object requesting the callback.  Safety object can be captured by value
+in the callback lambda, or explicitly added to a predefined callback
+context class. Multiple instances of Safety object can be instantiated
+for the same tracked object.  Once the callback is invoked, before
+dereferencing the requester object, callback code should make sure that
+`destroyed()` method for the corresponding Safety object returns false.
+
 ### EventBaseManager
 
 DANGEROUS.

folly/io/async/test/DestructorCheckTest.cpp

+/*
+ * Copyright 2017 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.
+ */
+
+#include <folly/Memory.h>
+#include <folly/io/async/DestructorCheck.h>
+#include <folly/portability/GTest.h>
+
+using namespace folly;
+using namespace testing;
+
+class Derived : public DestructorCheck { };
+
+TEST(DestructorCheckTest, WithoutGuard) {
+  Derived d;
+}
+
+TEST(DestructorCheckTest, SingleGuard) {
+  Derived d;
+  Derived::Safety s(d);
+  ASSERT_FALSE(s.destroyed());
+}
+
+TEST(DestructorCheckTest, SingleGuardDestroyed) {
+  auto d = std::make_unique<Derived>();
+  Derived::Safety s(*d);
+  ASSERT_FALSE(s.destroyed());
+  d.reset();
+  ASSERT_TRUE(s.destroyed());
+}
+
+TEST(DestructorCheckTest, MultipleGuards) {
+  Derived d;
+  auto s1 = std::make_unique<Derived::Safety>(d);
+  auto s2 = std::make_unique<Derived::Safety>(d);
+  auto s3 = std::make_unique<Derived::Safety>(d);
+
+  // Remove the middle of the list.
+  ASSERT_FALSE(s2->destroyed());
+  s2.reset();
+
+  // Add in a link after a removal has occurred.
+  auto s4 = std::make_unique<Derived::Safety>(d);
+
+  // Remove the beginning of the list.
+  ASSERT_FALSE(s1->destroyed());
+  s1.reset();
+  // Remove the end of the list.
+  ASSERT_FALSE(s4->destroyed());
+  s4.reset();
+  // Remove the last remaining of the list.
+  ASSERT_FALSE(s3->destroyed());
+  s3.reset();
+}
+
+TEST(DestructorCheckTest, MultipleGuardsDestroyed) {
+  auto d = std::make_unique<Derived>();
+  auto s1 = std::make_unique<Derived::Safety>(*d);
+  auto s2 = std::make_unique<Derived::Safety>(*d);
+  auto s3 = std::make_unique<Derived::Safety>(*d);
+  auto s4 = std::make_unique<Derived::Safety>(*d);
+
+  // Remove something from the list.
+  ASSERT_FALSE(s2->destroyed());
+  s2.reset();
+
+  ASSERT_FALSE(s1->destroyed());
+  ASSERT_FALSE(s3->destroyed());
+  ASSERT_FALSE(s4->destroyed());
+
+  d.reset();
+
+  ASSERT_TRUE(s1->destroyed());
+  ASSERT_TRUE(s3->destroyed());
+  ASSERT_TRUE(s4->destroyed());
+}