Commit 596d6f6d authored by Chad Parry's avatar Chad Parry Committed by facebook-github-bot-9

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
parent 6762f08b
...@@ -17,6 +17,10 @@ ...@@ -17,6 +17,10 @@
#pragma once #pragma once
#include <assert.h> #include <assert.h>
#include <cstddef>
#include <memory>
#include <type_traits>
#include <utility>
#include <boost/noncopyable.hpp> #include <boost/noncopyable.hpp>
#include <functional> #include <functional>
#include <glog/logging.h> #include <glog/logging.h>
...@@ -57,28 +61,143 @@ class DelayedDestructionBase : private boost::noncopyable { ...@@ -57,28 +61,143 @@ class DelayedDestructionBase : private boost::noncopyable {
class DestructorGuard { class DestructorGuard {
public: public:
explicit DestructorGuard(DelayedDestructionBase* dd) : dd_(dd) { explicit DestructorGuard(DelayedDestructionBase* dd = nullptr) :
++dd_->guardCount_; dd_(dd) {
assert(dd_->guardCount_ > 0); // check for wrapping 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_) { DestructorGuard(DestructorGuard&& dg) noexcept :
++dd_->guardCount_; dd_(dg.dd_) {
assert(dd_->guardCount_ > 0); // check for wrapping 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() { ~DestructorGuard() {
assert(dd_->guardCount_ > 0); if (dd_ != nullptr) {
--dd_->guardCount_; assert(dd_->guardCount_ > 0);
if (dd_->guardCount_ == 0) { --dd_->guardCount_;
dd_->onDestroy_(true); if (dd_->guardCount_ == 0) {
dd_->onDestroy_(true);
}
} }
} }
DelayedDestructionBase* get() const {
return dd_;
}
explicit operator bool() const {
return dd_ != nullptr;
}
private: private:
DelayedDestructionBase* dd_; 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: protected:
DelayedDestructionBase() DelayedDestructionBase()
: guardCount_(0) {} : guardCount_(0) {}
...@@ -115,4 +234,72 @@ class DelayedDestructionBase : private boost::noncopyable { ...@@ -115,4 +234,72 @@ class DelayedDestructionBase : private boost::noncopyable {
*/ */
uint32_t guardCount_; 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 } // folly
...@@ -59,11 +59,16 @@ namespace folly { ...@@ -59,11 +59,16 @@ namespace folly {
class HHWheelTimer : private folly::AsyncTimeout, class HHWheelTimer : private folly::AsyncTimeout,
public folly::DelayedDestruction { public folly::DelayedDestruction {
public: 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> template <typename... Args>
static UniquePtr newTimer(Args&&... 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);
} }
/** /**
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment