From: Alexey Spiridonov <lesha@fb.com>
Date: Tue, 16 May 2017 21:05:49 +0000 (-0700)
Subject: Simplify ThreadedRepeatingFunctionRunner by requiring classes that contain it to... 
X-Git-Tag: v2017.05.22.00~12
X-Git-Url: http://plrg.eecs.uci.edu/git/?p=folly.git;a=commitdiff_plain;h=1b546da9faec9c673bf637aeaca1bc604857cbc1

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
---

diff --git a/folly/experimental/ThreadedRepeatingFunctionRunner.cpp b/folly/experimental/ThreadedRepeatingFunctionRunner.cpp
index 56a15c9e..a309f01a 100644
--- a/folly/experimental/ThreadedRepeatingFunctionRunner.cpp
+++ b/folly/experimental/ThreadedRepeatingFunctionRunner.cpp
@@ -23,20 +23,13 @@ namespace folly {
 ThreadedRepeatingFunctionRunner::ThreadedRepeatingFunctionRunner() {}
 
 ThreadedRepeatingFunctionRunner::~ThreadedRepeatingFunctionRunner() {
-  stopAndWarn("ThreadedRepeatingFunctionRunner");
-}
-
-void ThreadedRepeatingFunctionRunner::stopAndWarn(
-    const std::string& class_of_destructor) {
   if (stopImpl()) {
     LOG(ERROR)
         << "ThreadedRepeatingFunctionRunner::stop() should already have been "
-        << "called, since the " << class_of_destructor << " destructor is now "
-        << "running. This is unsafe because it means that its threads "
-        << "may be accessing class state that was already destroyed "
-        << "(e.g. derived class members, or members that were declared after "
-        << "the " << class_of_destructor << ") .";
-    stop();
+        << "called, since we are now in the Runner's destructor. This is "
+        << "because it means that its threads may be accessing object state "
+        << "that was already destroyed -- e.g. members that were declared "
+        << "after the ThreadedRepeatingFunctionRunner.";
   }
 }
 
diff --git a/folly/experimental/ThreadedRepeatingFunctionRunner.h b/folly/experimental/ThreadedRepeatingFunctionRunner.h
index 878da728..75122dae 100644
--- a/folly/experimental/ThreadedRepeatingFunctionRunner.h
+++ b/folly/experimental/ThreadedRepeatingFunctionRunner.h
@@ -51,32 +51,38 @@ namespace folly {
  * if you want to have ThreadedRepeatingFunctionRunner as a member of your
  * 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:
  *   //
- *   //   (1) If you added some, and had any subsequent initialization (e.g.
- *   //       derived class constructors), 'this' would not be fully
- *   //       constructed when the worker threads came up, causing
- *   //       heisenbugs.
+ *   //   (1) If you first added some threads, and then had additional
+ *   //       initialization (e.g. derived class constructors), `this` might
+ *   //       not be fully constructed by the time the function threads
+ *   //       started running, causing heisenbugs.
  *   //
- *   //   (2) Also, if your constructor threw after thread creation, the
- *   //       class destructor would not be invoked, potentially leaving the
+ *   //   (2) If your constructor threw after thread creation, the class
+ *   //       destructor would not be invoked, potentially leaving the
  *   //       threads running too long.
  *   //
- *   // It's better to have explicit two-step initialization, or to lazily
- *   // add threads the first time they are needed.
+ *   // It is much safer to have explicit two-step initialization, or to
+ *   // lazily add threads the first time they are needed.
  *   MyClass() : count_(0) {}
  *
  *   // You must stop the threads as early as possible in the destruction
- *   // process (or even before).  In the case of a class hierarchy, the
- *   // final class MUST always call stop() as the first thing in its
- *   // destructor -- otherwise, the worker threads may access already-
+ *   // process (or even before).  If MyClass had derived classes, the final
+ *   // derived class MUST always call stop() as the first thing in its
+ *   // destructor -- otherwise, the worker threads might access already-
  *   // destroyed state.
  *   ~MyClass() {
- *     // if MyClass is abstract:
- *     threads_.stopAndWarn("MyClass");
- *     // Otherwise:
- *     threads_.stop();
+ *     threads_.stop();  // Stop threads BEFORE destroying any state they use.
  *   }
  *
  *   // See the constructor for why two-stage initialization is preferred.
@@ -91,7 +97,7 @@ namespace folly {
  *
  * private:
  *   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_;
  * };
  */
@@ -111,14 +117,6 @@ class ThreadedRepeatingFunctionRunner final {
    */
   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
    * calls for a duration returned by `f`.  Optionally waits for