Improve the DelayedDestruction smart pointer

Summary: There have been several ASAN bugs cropping up because the lifetime of an `HHWheelTimer` is not being manager properly. I think that people are too comfortable passing around a raw `HHWheelTimer*` even when it is difficult to prove correctness. The recommended solution used to be to create a `DestructorGuard` every time it was needed. There is enough friction there that not everyone is doing that like they should. We should make resource management easier---as easy as using raw pointers, in fact.

I've fixed the broken copy semantics of `DestructorGuard` and added the operators that allow it to be used as a smart pointer. I added the `IntrusivePtr` helper that can manage an arbitrary derived class of `DelayedDestructionBase`. Now, clients can all safely pass around an `IntrusivePtr` everywhere they used to use a raw pointer. They will get automatic resource management for free.

If you are not convinced that `DestructorGuard` should be changed, then note that the existing behavior is dangerously buggy. Consider the following innocent code that accidentally uses the implicitly-defined copy constructor:
  auto d = DestructorGuard(p);
This results in undefined behavior, because `p` can be accessed after it is destroyed! The bug happens because the default copy constructor copies the raw pointer but doesn't increment the count.

In a separate diff, I'll change all clients who pass around a raw `HHWheelTimer*` to use an `IntrusivePtr<HHWheelTimer>` instead. Then we can even entertain a long-term plan of switching from intrusive pointers to the standard `shared_ptr`.

Reviewed By: djwatson

Differential Revision: D2627941

fb-gh-sync-id: 58a934d64540d0bbab334adc4f23d31d507692da

diff --git a/folly/io/async/DelayedDestructionBase.h b/folly/io/async/DelayedDestructionBase.h
index c75b079b0bcfe54931f5e1875eba02d73b971bc7..3b9db7f9d7fc8a99602b27475a49d72d98524c7f 100644 (file)
--- a/folly/io/async/DelayedDestructionBase.h
+++ b/folly/io/async/DelayedDestructionBase.h
@@ -17,6 +17,10 @@
 #pragma once
 
 #include <assert.h>
+#include <cstddef>
+#include <memory>
+#include <type_traits>
+#include <utility>
 #include <boost/noncopyable.hpp>
 #include <functional>
 #include <glog/logging.h>
@@ -57,28 +61,143 @@ class DelayedDestructionBase : private boost::noncopyable {
   class DestructorGuard {
    public:
 
-    explicit DestructorGuard(DelayedDestructionBase* dd) : dd_(dd) {
-      ++dd_->guardCount_;
-      assert(dd_->guardCount_ > 0); // check for wrapping
+    explicit DestructorGuard(DelayedDestructionBase* dd = nullptr) :
+        dd_(dd) {
+      if (dd_ != nullptr) {
+        ++dd_->guardCount_;
+        assert(dd_->guardCount_ > 0); // check for wrapping
+      }
+    }
+
+    DestructorGuard(const DestructorGuard& dg) :
+        DestructorGuard(dg.dd_) {
     }
 
-    DestructorGuard(const DestructorGuard& dg) : dd_(dg.dd_) {
-      ++dd_->guardCount_;
-      assert(dd_->guardCount_ > 0); // check for wrapping
+    DestructorGuard(DestructorGuard&& dg) noexcept :
+        dd_(dg.dd_) {
+      dg.dd_ = nullptr;
+    }
+
+    DestructorGuard& operator =(DestructorGuard dg) noexcept {
+      std::swap(dd_, dg.dd_);
+      return *this;
+    }
+
+    DestructorGuard& operator =(DelayedDestructionBase* dd) {
+      *this = DestructorGuard(dd);
+      return *this;
     }
 
     ~DestructorGuard() {
-      assert(dd_->guardCount_ > 0);
-      --dd_->guardCount_;
-      if (dd_->guardCount_ == 0) {
-        dd_->onDestroy_(true);
+      if (dd_ != nullptr) {
+        assert(dd_->guardCount_ > 0);
+        --dd_->guardCount_;
+        if (dd_->guardCount_ == 0) {
+          dd_->onDestroy_(true);
+        }
       }
     }
 
+    DelayedDestructionBase* get() const {
+      return dd_;
+    }
+
+    explicit operator bool() const {
+      return dd_ != nullptr;
+    }
+
    private:
     DelayedDestructionBase* dd_;
   };
 
+  /**
+   * This smart pointer is a convenient way to manage a concrete
+   * DelayedDestructorBase child. It can replace the equivalent raw pointer and
+   * provide automatic memory management.
+   */
+  template <typename AliasType>
+  class IntrusivePtr : private DestructorGuard {
+    template <typename CopyAliasType>
+    friend class IntrusivePtr;
+   public:
+    template <typename... Args>
+    static IntrusivePtr<AliasType> make(Args&&... args) {
+      return {new AliasType(std::forward<Args>(args)...)};
+    }
+
+    IntrusivePtr() = default;
+    IntrusivePtr(const IntrusivePtr&) = default;
+    IntrusivePtr(IntrusivePtr&&) noexcept = default;
+
+    template <typename CopyAliasType, typename =
+      typename std::enable_if<
+        std::is_convertible<CopyAliasType*, AliasType*>::value
+      >::type>
+    IntrusivePtr(const IntrusivePtr<CopyAliasType>& copy) :
+        DestructorGuard(copy) {
+    }
+
+    template <typename CopyAliasType, typename =
+      typename std::enable_if<
+        std::is_convertible<CopyAliasType*, AliasType*>::value
+      >::type>
+    IntrusivePtr(IntrusivePtr<CopyAliasType>&& copy) :
+        DestructorGuard(std::move(copy)) {
+    }
+
+    explicit IntrusivePtr(AliasType* dd) :
+        DestructorGuard(dd) {
+    }
+
+    // Copying from a unique_ptr is safe because if the upcast to
+    // DelayedDestructionBase works, then the instance is already using
+    // intrusive ref-counting.
+    template <typename CopyAliasType, typename Deleter, typename =
+      typename std::enable_if<
+        std::is_convertible<CopyAliasType*, AliasType*>::value
+      >::type>
+    explicit IntrusivePtr(const std::unique_ptr<CopyAliasType, Deleter>& copy) :
+        DestructorGuard(copy.get()) {
+    }
+
+    IntrusivePtr& operator =(const IntrusivePtr&) = default;
+    IntrusivePtr& operator =(IntrusivePtr&&) noexcept = default;
+
+    template <typename CopyAliasType, typename =
+      typename std::enable_if<
+        std::is_convertible<CopyAliasType*, AliasType*>::value
+      >::type>
+    IntrusivePtr& operator =(IntrusivePtr<CopyAliasType> copy) noexcept {
+      DestructorGuard::operator =(copy);
+      return *this;
+    }
+
+    IntrusivePtr& operator =(AliasType* dd) {
+      DestructorGuard::operator =(dd);
+      return *this;
+    }
+
+    void reset(AliasType* dd = nullptr) {
+      *this = dd;
+    }
+
+    AliasType* get() const {
+      return static_cast<AliasType *>(DestructorGuard::get());
+    }
+
+    AliasType& operator *() const {
+      return *get();
+    }
+
+    AliasType* operator ->() const {
+      return get();
+    }
+
+    explicit operator bool() const {
+      return DestructorGuard::operator bool();
+    }
+  };
+
  protected:
   DelayedDestructionBase()
     : guardCount_(0) {}
@@ -115,4 +234,72 @@ class DelayedDestructionBase : private boost::noncopyable {
    */
   uint32_t guardCount_;
 };
+
+inline bool operator ==(
+    const DelayedDestructionBase::DestructorGuard& left,
+    const DelayedDestructionBase::DestructorGuard& right) {
+  return left.get() == right.get();
+}
+inline bool operator !=(
+    const DelayedDestructionBase::DestructorGuard& left,
+    const DelayedDestructionBase::DestructorGuard& right) {
+  return left.get() != right.get();
+}
+inline bool operator ==(
+    const DelayedDestructionBase::DestructorGuard& left,
+    std::nullptr_t right) {
+  return left.get() == right;
+}
+inline bool operator ==(
+    std::nullptr_t left,
+    const DelayedDestructionBase::DestructorGuard& right) {
+  return left == right.get();
+}
+inline bool operator !=(
+    const DelayedDestructionBase::DestructorGuard& left,
+    std::nullptr_t right) {
+  return left.get() != right;
+}
+inline bool operator !=(
+    std::nullptr_t left,
+    const DelayedDestructionBase::DestructorGuard& right) {
+  return left != right.get();
+}
+
+template <typename LeftAliasType, typename RightAliasType>
+inline bool operator ==(
+    const DelayedDestructionBase::IntrusivePtr<LeftAliasType>& left,
+    const DelayedDestructionBase::IntrusivePtr<RightAliasType>& right) {
+  return left.get() == right.get();
+}
+template <typename LeftAliasType, typename RightAliasType>
+inline bool operator !=(
+    const DelayedDestructionBase::IntrusivePtr<LeftAliasType>& left,
+    const DelayedDestructionBase::IntrusivePtr<RightAliasType>& right) {
+  return left.get() != right.get();
+}
+template <typename LeftAliasType>
+inline bool operator ==(
+    const DelayedDestructionBase::IntrusivePtr<LeftAliasType>& left,
+    std::nullptr_t right) {
+  return left.get() == right;
+}
+template <typename RightAliasType>
+inline bool operator ==(
+    std::nullptr_t left,
+    const DelayedDestructionBase::IntrusivePtr<RightAliasType>& right) {
+  return left == right.get();
+}
+template <typename LeftAliasType>
+inline bool operator !=(
+    const DelayedDestructionBase::IntrusivePtr<LeftAliasType>& left,
+    std::nullptr_t right) {
+  return left.get() != right;
+}
+template <typename RightAliasType>
+inline bool operator !=(
+    std::nullptr_t left,
+    const DelayedDestructionBase::IntrusivePtr<RightAliasType>& right) {
+  return left != right.get();
+}
 } // folly

diff --git a/folly/io/async/HHWheelTimer.h b/folly/io/async/HHWheelTimer.h
index 963af518e6269f2f94a787bb7f5b3356e01b9601..17a74d54eb61597e87f1ee2d9fffe48720673c30 100644 (file)
--- a/folly/io/async/HHWheelTimer.h
+++ b/folly/io/async/HHWheelTimer.h
@@ -59,11 +59,16 @@ namespace folly {
 class HHWheelTimer : private folly::AsyncTimeout,
                      public folly::DelayedDestruction {
  public:
-  typedef std::unique_ptr<HHWheelTimer, Destructor> UniquePtr;
+  // This type has always been a misnomer, because it is not a unique pointer.
+  using UniquePtr = IntrusivePtr<HHWheelTimer>;
 
   template <typename... Args>
   static UniquePtr newTimer(Args&&... args) {
-    return UniquePtr(new HHWheelTimer(std::forward<Args>(args)...));
+    std::unique_ptr<HHWheelTimer, Destructor> instance(
+        new HHWheelTimer(std::forward<Args>(args)...));
+    // Avoid the weird semantics of the Destructor by managing ownership
+    // entirely from the IntrusivePtr.
+    return UniquePtr(instance);
   }
 
   /**