From: Maxim Georgiev Date: Sun, 28 May 2017 14:25:17 +0000 (-0700) Subject: Moving DestructorCheck from proxygen library to folly. X-Git-Tag: v2017.05.29.00~2 X-Git-Url: http://plrg.eecs.uci.edu/git/?p=folly.git;a=commitdiff_plain;h=c1a1a5e6039c13c55597fa942ce1c01ecfe2f6a3 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 --- diff --git a/CMakeLists.txt b/CMakeLists.txt index 97f9bcf9..4058e367 100755 --- a/CMakeLists.txt +++ b/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 diff --git a/folly/Makefile.am b/folly/Makefile.am index 3993b45f..00796bb4 100644 --- a/folly/Makefile.am +++ b/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 \ diff --git a/folly/io/async/DestructorCheck.h b/folly/io/async/DestructorCheck.h new file mode 100644 index 00000000..2d9930fb --- /dev/null +++ b/folly/io/async/DestructorCheck.h @@ -0,0 +1,131 @@ +/* + * 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_; +}; + +} diff --git a/folly/io/async/README.md b/folly/io/async/README.md index a6214dbd..0918b396 100644 --- a/folly/io/async/README.md +++ b/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. diff --git a/folly/io/async/test/DestructorCheckTest.cpp b/folly/io/async/test/DestructorCheckTest.cpp new file mode 100644 index 00000000..8c89947a --- /dev/null +++ b/folly/io/async/test/DestructorCheckTest.cpp @@ -0,0 +1,88 @@ +/* + * 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 +#include +#include + +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::Safety s(*d); + ASSERT_FALSE(s.destroyed()); + d.reset(); + ASSERT_TRUE(s.destroyed()); +} + +TEST(DestructorCheckTest, MultipleGuards) { + Derived d; + auto s1 = std::make_unique(d); + auto s2 = std::make_unique(d); + auto s3 = std::make_unique(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(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(); + auto s1 = std::make_unique(*d); + auto s2 = std::make_unique(*d); + auto s3 = std::make_unique(*d); + auto s4 = std::make_unique(*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()); +}