Commit 1b546da9 authored by Alexey Spiridonov's avatar Alexey Spiridonov Committed by Facebook Github Bot

Simplify ThreadedRepeatingFunctionRunner by requiring classes that contain it to be final

Summary: It is pretty confusing to inherit from classes that manage threads. See the docblocks in this diff.

Reviewed By: yfeldblum

Differential Revision: D4973498

fbshipit-source-id: 2fcf1ddf68ef46d4d78a9b40f304262064862715
parent 1a1ce79e
...@@ -23,20 +23,13 @@ namespace folly { ...@@ -23,20 +23,13 @@ namespace folly {
ThreadedRepeatingFunctionRunner::ThreadedRepeatingFunctionRunner() {} ThreadedRepeatingFunctionRunner::ThreadedRepeatingFunctionRunner() {}
ThreadedRepeatingFunctionRunner::~ThreadedRepeatingFunctionRunner() { ThreadedRepeatingFunctionRunner::~ThreadedRepeatingFunctionRunner() {
stopAndWarn("ThreadedRepeatingFunctionRunner");
}
void ThreadedRepeatingFunctionRunner::stopAndWarn(
const std::string& class_of_destructor) {
if (stopImpl()) { if (stopImpl()) {
LOG(ERROR) LOG(ERROR)
<< "ThreadedRepeatingFunctionRunner::stop() should already have been " << "ThreadedRepeatingFunctionRunner::stop() should already have been "
<< "called, since the " << class_of_destructor << " destructor is now " << "called, since we are now in the Runner's destructor. This is "
<< "running. This is unsafe because it means that its threads " << "because it means that its threads may be accessing object state "
<< "may be accessing class state that was already destroyed " << "that was already destroyed -- e.g. members that were declared "
<< "(e.g. derived class members, or members that were declared after " << "after the ThreadedRepeatingFunctionRunner.";
<< "the " << class_of_destructor << ") .";
stop();
} }
} }
......
...@@ -51,32 +51,38 @@ namespace folly { ...@@ -51,32 +51,38 @@ namespace folly {
* if you want to have ThreadedRepeatingFunctionRunner as a member of your * if you want to have ThreadedRepeatingFunctionRunner as a member of your
* class. A reasonable pattern looks like this: * class. A reasonable pattern looks like this:
* *
* struct MyClass { * // Your class must be `final` because inheriting from a class with
* // threads can cause all sorts of subtle issues:
* // - Your base class might start threads that attempt to access derived
* // class state **before** that state was constructed.
* // - Your base class's destructor will only be able to stop threads
* // **after** the derived class state was destroyed -- and that state
* // might be accessed by the threads.
* // In short, any derived class would have to do work to manage the
* // threads itself, which makes inheritance a poor means of composition.
* struct MyClass final {
* // Note that threads are NOT added in the constructor, for two reasons: * // Note that threads are NOT added in the constructor, for two reasons:
* // * //
* // (1) If you added some, and had any subsequent initialization (e.g. * // (1) If you first added some threads, and then had additional
* // derived class constructors), 'this' would not be fully * // initialization (e.g. derived class constructors), `this` might
* // constructed when the worker threads came up, causing * // not be fully constructed by the time the function threads
* // heisenbugs. * // started running, causing heisenbugs.
* // * //
* // (2) Also, if your constructor threw after thread creation, the * // (2) If your constructor threw after thread creation, the class
* // class destructor would not be invoked, potentially leaving the * // destructor would not be invoked, potentially leaving the
* // threads running too long. * // threads running too long.
* // * //
* // It's better to have explicit two-step initialization, or to lazily * // It is much safer to have explicit two-step initialization, or to
* // add threads the first time they are needed. * // lazily add threads the first time they are needed.
* MyClass() : count_(0) {} * MyClass() : count_(0) {}
* *
* // You must stop the threads as early as possible in the destruction * // You must stop the threads as early as possible in the destruction
* // process (or even before). In the case of a class hierarchy, the * // process (or even before). If MyClass had derived classes, the final
* // final class MUST always call stop() as the first thing in its * // derived class MUST always call stop() as the first thing in its
* // destructor -- otherwise, the worker threads may access already- * // destructor -- otherwise, the worker threads might access already-
* // destroyed state. * // destroyed state.
* ~MyClass() { * ~MyClass() {
* // if MyClass is abstract: * threads_.stop(); // Stop threads BEFORE destroying any state they use.
* threads_.stopAndWarn("MyClass");
* // Otherwise:
* threads_.stop();
* } * }
* *
* // See the constructor for why two-stage initialization is preferred. * // See the constructor for why two-stage initialization is preferred.
...@@ -91,7 +97,7 @@ namespace folly { ...@@ -91,7 +97,7 @@ namespace folly {
* *
* private: * private:
* std::atomic<int> count_; * std::atomic<int> count_;
* // Declared last because the threads' functions access other members. * // CAUTION: Declare last since the threads access other members of `this`.
* ThreadedRepeatingFunctionRunner threads_; * ThreadedRepeatingFunctionRunner threads_;
* }; * };
*/ */
...@@ -111,14 +117,6 @@ class ThreadedRepeatingFunctionRunner final { ...@@ -111,14 +117,6 @@ class ThreadedRepeatingFunctionRunner final {
*/ */
void stop(); void stop();
/**
* Must be called at the TOP of the destructor of any abstract class that
* contains ThreadedRepeatingFunctionRunner (directly or through a
* parent). Any non-abstract class destructor must instead stop() at the
* top.
*/
void stopAndWarn(const std::string& class_of_destructor);
/** /**
* Run your noexcept function `f` in a background loop, sleeping between * Run your noexcept function `f` in a background loop, sleeping between
* calls for a duration returned by `f`. Optionally waits for * calls for a duration returned by `f`. Optionally waits for
......
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