Clean up runBeforeLoop API

Summary: Clean up API per stepan's comments in D1641057: runBeforeLoop callbacks should be run before the if block, so runInLoop calls will work.

Test Plan: added unittest

Reviewed By: hans@fb.com

Subscribers: trunkagent, doug, alandau, bmatheny, njormrod, mshneer, folly-diffs@

FB internal diff: D1679773

Signature: t1:1679773:1415916488:f20dd98bf40dd4cb19a4b11875dc0c9fa3e7870e

Blame Revision: D1641057

diff --git a/folly/io/async/EventBase.cpp b/folly/io/async/EventBase.cpp
index 0e34c5e2c71b50fc6279c01aeec360d76a1c8c2f..6916ceaca57c88c863ff49b21f6a224e92c66c87 100644 (file)
--- a/folly/io/async/EventBase.cpp
+++ b/folly/io/async/EventBase.cpp
@@ -215,8 +215,8 @@ EventBase::~EventBase() {
     delete timeout;
   }
 
-  while (!noWaitLoopCallbacks_.empty()) {
-    delete &noWaitLoopCallbacks_.front();
+  while (!runBeforeLoopCallbacks_.empty()) {
+    delete &runBeforeLoopCallbacks_.front();
   }
 
   (void) runLoopCallbacks(false);
@@ -299,18 +299,19 @@ bool EventBase::loopBody(int flags) {
   while (!stop_) {
     ++nextLoopCnt_;
 
+    // Run the before loop callbacks
+    LoopCallbackList callbacks;
+    callbacks.swap(runBeforeLoopCallbacks_);
+
+    while(!callbacks.empty()) {
+      auto* item = &callbacks.front();
+      callbacks.pop_front();
+      item->runLoopCallback();
+    }
+
     // nobody can add loop callbacks from within this thread if
     // we don't have to handle anything to start with...
     if (blocking && loopCallbacks_.empty()) {
-      LoopCallbackList callbacks;
-      callbacks.swap(noWaitLoopCallbacks_);
-
-      while(!callbacks.empty()) {
-        auto* item = &callbacks.front();
-        callbacks.pop_front();
-        item->runLoopCallback();
-      }
-
       res = event_base_loop(evb_, EVLOOP_ONCE);
     } else {
       res = event_base_loop(evb_, EVLOOP_ONCE | EVLOOP_NONBLOCK);
@@ -499,7 +500,7 @@ void EventBase::runOnDestruction(LoopCallback* callback) {
 void EventBase::runBeforeLoop(LoopCallback* callback) {
   DCHECK(isInEventBaseThread());
   callback->cancelLoopCallback();
-  noWaitLoopCallbacks_.push_back(*callback);
+  runBeforeLoopCallbacks_.push_back(*callback);
 }
 
 bool EventBase::runInEventBaseThread(void (*fn)(void*), void* arg) {

diff --git a/folly/io/async/EventBase.h b/folly/io/async/EventBase.h
index 3da2169d88cbd5b38baffa843c57f4495ebef1b1..d18460715cb3efb77907448b6053a7e04ae6c2e1 100644 (file)
--- a/folly/io/async/EventBase.h
+++ b/folly/io/async/EventBase.h
@@ -262,6 +262,11 @@ class EventBase :
    */
   void runOnDestruction(LoopCallback* callback);
 
+  /**
+   * Adds a callback that will run immediately *before* the event loop.
+   * This is very similar to runInLoop(), but will not cause the loop to break:
+   * For example, this callback could be used to get loop times.
+   */
   void runBeforeLoop(LoopCallback* callback);
 
   /**
@@ -531,7 +536,7 @@ class EventBase :
   CobTimeout::List pendingCobTimeouts_;
 
   LoopCallbackList loopCallbacks_;
-  LoopCallbackList noWaitLoopCallbacks_;
+  LoopCallbackList runBeforeLoopCallbacks_;
   LoopCallbackList onDestructionCallbacks_;
 
   // This will be null most of the time, but point to currentCallbacks

diff --git a/folly/io/async/test/EventBaseTest.cpp b/folly/io/async/test/EventBaseTest.cpp
index ddf70a06652f1a206146346f0b85160b183b26b3..3dab11d7e148b7d53e41f39227c6b5a980ad4e2b 100644 (file)
--- a/folly/io/async/test/EventBaseTest.cpp
+++ b/folly/io/async/test/EventBaseTest.cpp
@@ -1540,3 +1540,26 @@ TEST(EventBaseTest, EventBaseThreadName) {
   ASSERT_EQ(0, strcmp("foo", name));
 #endif
 }
+
+TEST(TEventBaseTest, RunBeforeLoop) {
+  TEventBase base;
+  CountedLoopCallback cb(&base, 1, [&](){
+    base.terminateLoopSoon();
+  });
+  base.runBeforeLoop(&cb);
+  base.loopForever();
+  ASSERT_EQUAL(cb.getCount(), 0);
+}
+
+TEST(TEventBaseTest, RunBeforeLoopWait) {
+  TEventBase base;
+  CountedLoopCallback cb(&base, 1);
+  base.runAfterDelay([&](){
+      base.terminateLoopSoon();
+    }, 500);
+  base.runBeforeLoop(&cb);
+  base.loopForever();
+
+  // Check that we only ran once, and did not loop multiple times.
+  ASSERT_EQUAL(cb.getCount(), 0);
+}