FunctionScheduler - set running_ when it has actually started running

Summary:
When start() throws because of reasons (in my case it was std::system_error("Resource temporarily unavailalble") coming from std::thread constructo)
and during the exception propagation we will have to destroy FunctionScheduler then
thread_.join() in shutdown will throw again.
This diff sets running_ after the thread is created.

Reviewed By: yfeldblum

Differential Revision: D4469816

fbshipit-source-id: cde54dfbf39f04d3ea9dfa02a65295f5440e5ea4

diff --git a/folly/experimental/FunctionScheduler.cpp b/folly/experimental/FunctionScheduler.cpp
index 83fba55b1627cfdfc48da881cb929f90b0ab1a68..3677e53346a98d3cf865c40f50b985626c7a63dd 100644 (file)
--- a/folly/experimental/FunctionScheduler.cpp
+++ b/folly/experimental/FunctionScheduler.cpp
@@ -318,8 +318,6 @@ bool FunctionScheduler::start() {
     return false;
   }
 
-  running_ = true;
-
   VLOG(1) << "Starting FunctionScheduler with " << functions_.size()
           << " functions.";
   auto now = steady_clock::now();
@@ -334,6 +332,8 @@ bool FunctionScheduler::start() {
   std::make_heap(functions_.begin(), functions_.end(), fnCmp_);
 
   thread_ = std::thread([&] { this->run(); });
+  running_ = true;
+
   return true;
 }
 

diff --git a/folly/experimental/test/FunctionSchedulerTest.cpp b/folly/experimental/test/FunctionSchedulerTest.cpp
index 23293d2c5b2dd7b11bee14a9aebfc8db8d64a548..01d22d73fdb043fb25c0c69281055a708789353e 100644 (file)
--- a/folly/experimental/test/FunctionSchedulerTest.cpp
+++ b/folly/experimental/test/FunctionSchedulerTest.cpp
@@ -13,16 +13,19 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-#include <folly/experimental/FunctionScheduler.h>
-
 #include <algorithm>
 #include <atomic>
 #include <cassert>
 #include <random>
-#include <folly/Random.h>
 
+#include <folly/Random.h>
+#include <folly/experimental/FunctionScheduler.h>
 #include <folly/portability/GTest.h>
 
+#if defined(__linux__)
+#include <dlfcn.h>
+#endif
+
 using namespace folly;
 using std::chrono::milliseconds;
 
@@ -488,6 +491,60 @@ TEST(FunctionScheduler, cancelFunctionAndWait) {
   fs.shutdown();
 }
 
+#if defined(__linux__)
+namespace {
+/**
+ * A helper class that forces our pthread_create() wrapper to fail when
+ * an PThreadCreateFailure object exists.
+ */
+class PThreadCreateFailure {
+ public:
+  PThreadCreateFailure() {
+    ++forceFailure_;
+  }
+  ~PThreadCreateFailure() {
+    --forceFailure_;
+  }
+
+  static bool shouldFail() {
+    return forceFailure_ > 0;
+  }
+
+ private:
+  static std::atomic<int> forceFailure_;
+};
+
+std::atomic<int> PThreadCreateFailure::forceFailure_{0};
+} // unnamed namespce
+
+// Replace the system pthread_create() function with our own stub, so we can
+// trigger failures in the StartThrows() test.
+extern "C" int pthread_create(
+    pthread_t* thread,
+    const pthread_attr_t* attr,
+    void* (*start_routine)(void*),
+    void* arg) {
+  static const auto realFunction = reinterpret_cast<decltype(&pthread_create)>(
+      dlsym(RTLD_NEXT, "pthread_create"));
+  // For sanity, make sure we didn't find ourself,
+  // since that would cause infinite recursion.
+  CHECK_NE(realFunction, pthread_create);
+
+  if (PThreadCreateFailure::shouldFail()) {
+    errno = EINVAL;
+    return -1;
+  }
+  return realFunction(thread, attr, start_routine, arg);
+}
+
+TEST(FunctionScheduler, StartThrows) {
+  FunctionScheduler fs;
+  PThreadCreateFailure fail;
+  EXPECT_ANY_THROW(fs.start());
+  EXPECT_NO_THROW(fs.shutdown());
+}
+#endif
+
 TEST(FunctionScheduler, cancelAllFunctionsAndWait) {
   int total = 0;
   FunctionScheduler fs;