Remove runInLoop
authorAndrii Grynenko <andrii@fb.com>
Fri, 10 Mar 2017 04:34:14 +0000 (20:34 -0800)
committerFacebook Github Bot <facebook-github-bot@users.noreply.github.com>
Fri, 10 Mar 2017 04:55:38 +0000 (20:55 -0800)
Summary: Existing runInLoop implementation was generally not safe (it did not capture the KeepAlive token). runInLoop() is mostly used in library code though, and often times it's ok not capture the KeepAlive token. It's better to just have the caller make the decision, rather then keep runInLoop() and make it always capture the KeepAlive token.

Reviewed By: yfeldblum

Differential Revision: D4684025

fbshipit-source-id: 65907bbe9c774e2a7b92580d0c0387d346b495d5

folly/fibers/EventBaseLoopController-inl.h
folly/io/async/VirtualEventBase.cpp
folly/io/async/VirtualEventBase.h

index 025341c..4c01ed0 100644 (file)
@@ -52,8 +52,8 @@ inline void EventBaseLoopControllerT<EventBaseT>::setFiberManager(
   fm_ = fm;
 }
 
-template <typename EventBaseT>
-inline void EventBaseLoopControllerT<EventBaseT>::schedule() {
+template <>
+inline void EventBaseLoopControllerT<folly::EventBase>::schedule() {
   if (eventBase_ == nullptr) {
     // In this case we need to postpone scheduling.
     awaitingScheduling_ = true;
@@ -64,6 +64,22 @@ inline void EventBaseLoopControllerT<EventBaseT>::schedule() {
   }
 }
 
+template <>
+inline void EventBaseLoopControllerT<folly::VirtualEventBase>::schedule() {
+  if (eventBase_ == nullptr) {
+    // In this case we need to postpone scheduling.
+    awaitingScheduling_ = true;
+  } else {
+    // Schedule it to run in current iteration.
+
+    if (!eventBaseKeepAlive_) {
+      eventBaseKeepAlive_ = eventBase_->getKeepAliveToken();
+    }
+    eventBase_->getEventBase().runInLoop(&callback_, true);
+    awaitingScheduling_ = false;
+  }
+}
+
 template <typename EventBaseT>
 inline void EventBaseLoopControllerT<EventBaseT>::cancel() {
   callback_.cancelLoopCallback();
@@ -72,6 +88,11 @@ inline void EventBaseLoopControllerT<EventBaseT>::cancel() {
 template <typename EventBaseT>
 inline void EventBaseLoopControllerT<EventBaseT>::runLoop() {
   if (!eventBaseKeepAlive_) {
+    // runLoop can be called twice if both schedule() and scheduleThreadSafe()
+    // were called.
+    if (!fm_->hasTasks()) {
+      return;
+    }
     eventBaseKeepAlive_ = eventBase_->getKeepAliveToken();
   }
   if (loopRunner_) {
index 688faf5..46c187b 100644 (file)
@@ -19,7 +19,6 @@ namespace folly {
 
 VirtualEventBase::VirtualEventBase(EventBase& evb) : evb_(evb) {
   evbLoopKeepAlive_ = evb_.getKeepAliveToken();
-  loopKeepAlive_ = getKeepAliveToken();
 }
 
 std::future<void> VirtualEventBase::destroy() {
index c67d2cb..a1194d8 100644 (file)
@@ -61,14 +61,6 @@ class VirtualEventBase : public folly::Executor, public folly::TimeoutManager {
    */
   void runOnDestruction(EventBase::LoopCallback* callback);
 
-  /**
-   * @see EventBase::runInLoop
-   */
-  template <typename F>
-  void runInLoop(F&& f, bool thisIteration = false) {
-    evb_.runInLoop(std::forward<F>(f), thisIteration);
-  }
-
   /**
    * VirtualEventBase destructor blocks until all tasks scheduled through its
    * runInEventBaseThread are complete.
@@ -129,6 +121,8 @@ class VirtualEventBase : public folly::Executor, public folly::TimeoutManager {
    * KeepAlive handle can be released from EventBase loop only.
    */
   KeepAlive getKeepAliveToken() override {
+    DCHECK(loopKeepAliveCount_ + loopKeepAliveCountAtomic_.load() > 0);
+
     if (evb_.inRunningEventBaseThread()) {
       ++loopKeepAliveCount_;
     } else {
@@ -163,11 +157,11 @@ class VirtualEventBase : public folly::Executor, public folly::TimeoutManager {
 
   EventBase& evb_;
 
-  ssize_t loopKeepAliveCount_{0};
+  ssize_t loopKeepAliveCount_{1};
   std::atomic<ssize_t> loopKeepAliveCountAtomic_{0};
   std::promise<void> destroyPromise_;
   std::future<void> destroyFuture_{destroyPromise_.get_future()};
-  KeepAlive loopKeepAlive_;
+  KeepAlive loopKeepAlive_{makeKeepAlive()};
 
   KeepAlive evbLoopKeepAlive_;