Remove unnecesary loop breaks from EventBase

Summary: This logic makes it impossible to reason about order between io events, internal and external events. The comment also seems outdated.

Reviewed By: yfeldblum

Differential Revision: D19785249

fbshipit-source-id: 62eee5427a3c47cccba6cb33c2ca88af82767bed

folly/futures/test/SemiFutureTest.cpp

@@ -450,7 +450,6 @@ TEST(SemiFuture, MakeFutureFromSemiFutureReturnSemiFuture) {
   EXPECT_FALSE(future.isReady());
   p.setValue(42);
   e.loopOnce();
-  e.loopOnce();
   EXPECT_TRUE(future.isReady());
   ASSERT_EQ(future.value(), 42);
   ASSERT_EQ(result, 42);

folly/io/async/EventBase.cpp

@@ -132,19 +132,6 @@ class EventBase::FunctionRunner
     : public NotificationQueue<EventBase::Func>::Consumer {
  public:
   void messageAvailable(Func&& msg) noexcept override {
-    // In libevent2, internal events do not break the loop.
-    // Most users would expect loop(), followed by runInEventBaseThread(),
-    // to break the loop and check if it should exit or not.
-    // To have similar bejaviour to libevent1.4, tell the loop to break here.
-    // Note that loop() may still continue to loop, but it will also check the
-    // stop_ flag as well as runInLoop callbacks, etc.
-    getEventBase()->getBackend()->eb_event_base_loopbreak();
-
-    if (!msg) {
-      // terminateLoopSoon() sends a null message just to
-      // wake up the loop.  We can ignore these messages.
-      return;
-    }
     msg();
   }
 };
@@ -574,24 +561,15 @@ void EventBase::terminateLoopSoon() {
   // Set stop to true, so the event loop will know to exit.
   stop_.store(true, std::memory_order_relaxed);
 
-  // Call event_base_loopbreak() so that libevent will exit the next time
-  // around the loop.
-  evb_->eb_event_base_loopbreak();
-
   // If terminateLoopSoon() is called from another thread,
   // the EventBase thread might be stuck waiting for events.
   // In this case, it won't wake up and notice that stop_ is set until it
   // receives another event.  Send an empty frame to the notification queue
   // so that the event loop will wake up even if there are no other events.
-  //
-  // We don't care about the return value of trySendFrame().  If it fails
-  // this likely means the EventBase already has lots of events waiting
-  // anyway.
   try {
-    queue_->putMessage(nullptr);
+    queue_->putMessage([&] { evb_->eb_event_base_loopbreak(); });
   } catch (...) {
-    // We don't care if putMessage() fails.  This likely means
-    // the EventBase already has lots of events waiting anyway.
+    // putMessage() can only fail when the queue is draining in ~EventBase.
   }
 }
 

folly/io/async/test/EventBaseTestLib.h

@@ -1721,6 +1721,33 @@ TYPED_TEST_P(EventBaseTest, AlwaysEnqueueCallbackOrderTest) {
   EXPECT_EQ(num, 2);
 }
 
+TYPED_TEST_P(EventBaseTest1, InternalExternalCallbackOrderTest) {
+  size_t counter = 0;
+
+  FOLLY_SKIP_IF_NULLPTR_BACKEND(evb);
+
+  std::vector<size_t> calls;
+
+  folly::Function<void(size_t)> runInLoopRecursive = [&](size_t left) {
+    evb.runInLoop([&, left]() mutable {
+      calls.push_back(counter++);
+      if (--left == 0) {
+        evb.terminateLoopSoon();
+        return;
+      }
+      runInLoopRecursive(left);
+    });
+  };
+
+  evb.runInEventBaseThread([&] { runInLoopRecursive(5); });
+  for (size_t i = 0; i < 49; ++i) {
+    evb.runInEventBaseThread([&] { ++counter; });
+  }
+  evb.loopForever();
+
+  EXPECT_EQ(std::vector<size_t>({9, 20, 31, 42, 53}), calls);
+}
+
 ///////////////////////////////////////////////////////////////////////////
 // Tests for latency calculations
 ///////////////////////////////////////////////////////////////////////////
@@ -2317,6 +2344,7 @@ REGISTER_TYPED_TEST_CASE_P(
     RunOnDestructionBasic,
     RunOnDestructionCancelled,
     RunOnDestructionAfterHandleDestroyed,
-    RunOnDestructionAddCallbackWithinCallback);
+    RunOnDestructionAddCallbackWithinCallback,
+    InternalExternalCallbackOrderTest);
 } // namespace test
 } // namespace folly