Don't check for thread running in terminateLoopSoon()

Summary:
Consider this use case:
thread 1:
make an evb
create and start thread 2
do some work
evb->terminateLoopSoon()
join thread 2 with a timeout
thread 2:
do some initialization
evb->loop()
Normally, this all works fine.  However, if the work thread 1 has to do is sufficiently small, and no external input occurs (one of the thing the evb is doing is listening on a socket), then sometimes, terminateLoopSoon() happens before loop() is called (or at least, before loopThread_.store() happens). isRunning() in terminateLoopSoon() is false, and so stop_ is never set, and event_base_loopbreak() is never called. The join times out, and thread 2 is still running.  Removing the isRunning() check gives the desired behavior.

Test Plan:
In my ad hoc tests, this fix caused my threads to exit when
I wanted them to exit in a situation like the one above.

Reviewed By: andrewcox@fb.com

FB internal diff: D1455885

Tasks: 2057547

diff --git a/folly/io/async/EventBase.cpp b/folly/io/async/EventBase.cpp
index 9415e05a6c59cbeee0fd946551c101eafe29d638..cae583fee18b152d3985e39cd4d56a6461e0ac0f 100644 (file)
--- a/folly/io/async/EventBase.cpp
+++ b/folly/io/async/EventBase.cpp
@@ -393,10 +393,6 @@ bool EventBase::bumpHandlingTime() {
 void EventBase::terminateLoopSoon() {
   VLOG(5) << "EventBase(): Received terminateLoopSoon() command.";
 
-  if (!isRunning()) {
-    return;
-  }
-
   // Set stop to true, so the event loop will know to exit.
   // TODO: We should really use an atomic operation here with a release
   // barrier.

diff --git a/folly/io/async/EventBase.h b/folly/io/async/EventBase.h
index c6223077c3a3f51be931e0d42f0260e5eec02f3b..2406f02b285ee6907a9ce45fd3097556b4c94c08 100644 (file)
--- a/folly/io/async/EventBase.h
+++ b/folly/io/async/EventBase.h
@@ -184,7 +184,11 @@ class EventBase : private boost::noncopyable, public TimeoutManager {
    * to wake up and return in the EventBase loop thread.  terminateLoopSoon()
    * may also be called from the loop thread itself (for example, a
    * EventHandler or AsyncTimeout callback may call terminateLoopSoon() to
-   * cause the loop to exit after the callback returns.)
+   * cause the loop to exit after the callback returns.)  If the loop is not
+   * running, this will cause the next call to loop to terminate soon after
+   * starting.  If a loop runs out of work (and so terminates on its own)
+   * concurrently with a call to terminateLoopSoon(), this may cause a race
+   * condition.
    *
    * Note that the caller is responsible for ensuring that cleanup of all event
    * callbacks occurs properly.  Since terminateLoopSoon() causes the loop to

diff --git a/folly/io/test/EventBaseTest.cpp b/folly/io/test/EventBaseTest.cpp
new file mode 100644 (file)
index 0000000..b7020c8
--- /dev/null
+++ b/folly/io/test/EventBaseTest.cpp
@@ -0,0 +1,58 @@
+/*
+ * Copyright 2014 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 <unistd.h>
+#include <thread>
+
+#include <gtest/gtest.h>
+
+#include <folly/io/async/EventBase.h>
+#include <folly/io/async/EventHandler.h>
+
+using namespace folly;
+
+class PipeHandler : public EventHandler {
+public:
+  PipeHandler(EventBase* eventBase, int fd)
+    : EventHandler(eventBase, fd) {}
+
+  void handlerReady(uint16_t events) noexcept {
+    abort();
+  }
+};
+
+TEST(EventBase, StopBeforeLoop) {
+  EventBase evb;
+
+  // Give the evb something to do.
+  int p[2];
+  ASSERT_EQ(0, pipe(p));
+  PipeHandler handler(&evb, p[0]);
+  handler.registerHandler(EventHandler::READ);
+
+  // It's definitely not running yet
+  evb.terminateLoopSoon();
+
+  // let it run, it should exit quickly.
+  std::thread t([&] { evb.loop(); });
+  t.join();
+
+  handler.unregisterHandler();
+  close(p[0]);
+  close(p[1]);
+
+  SUCCEED();
+}