From: Hannes Roth <hannesr@fb.com>
Date: Thu, 25 Jun 2015 15:38:14 +0000 (-0700)
Subject: (Wangle) Fix bug with CrappyExecutors, and bad PriorityExecutor
X-Git-Tag: v0.48.0~13
X-Git-Url: http://plrg.eecs.uci.edu/git/?a=commitdiff_plain;h=be98381373e5a727224f9fa74e45a067e67b54f0;p=folly.git

(Wangle) Fix bug with CrappyExecutors, and bad PriorityExecutor

Summary: 1) We forgot to `--attached_` if `x` throws an exception
2) `PriorityExecutor` didn't execute `Func`, causing leaks in the test (not a bug in Futures)
3) I moved up the initialization for an empty `Core` into the constructor to make it easier to see

Reviewed By: @jsedgwick

Differential Revision: D2187343
---

diff --git a/folly/futures/detail/Core.h b/folly/futures/detail/Core.h
index c0e09d09..d21d74ce 100644
--- a/folly/futures/detail/Core.h
+++ b/folly/futures/detail/Core.h
@@ -78,7 +78,7 @@ class Core {
   /// This must be heap-constructed. There's probably a way to enforce that in
   /// code but since this is just internal detail code and I don't know how
   /// off-hand, I'm punting.
-  Core() {}
+  Core() : result_(), fsm_(State::Start), attached_(2) {}
 
   explicit Core(Try<T>&& t)
     : result_(std::move(t)),
@@ -342,6 +342,7 @@ class Core {
           }, priority);
         }
       } catch (...) {
+        --attached_; // Account for extra ++attached_ before try
         result_ = Try<T>(exception_wrapper(std::current_exception()));
         callback_(std::move(*result_));
       }
@@ -364,10 +365,10 @@ class Core {
   char lambdaBuf_[lambdaBufSize];
   // place result_ next to increase the likelihood that the value will be
   // contained entirely in one cache line
-  folly::Optional<Try<T>> result_ {};
+  folly::Optional<Try<T>> result_;
   std::function<void(Try<T>&&)> callback_ {nullptr};
-  FSM<State> fsm_ {State::Start};
-  std::atomic<unsigned char> attached_ {2};
+  FSM<State> fsm_;
+  std::atomic<unsigned char> attached_;
   std::atomic<bool> active_ {true};
   std::atomic<bool> interruptHandlerSet_ {false};
   folly::MicroSpinLock interruptLock_ {0};
diff --git a/folly/futures/test/ViaTest.cpp b/folly/futures/test/ViaTest.cpp
index 053499c3..77466ac4 100644
--- a/folly/futures/test/ViaTest.cpp
+++ b/folly/futures/test/ViaTest.cpp
@@ -191,7 +191,7 @@ TEST(Via, chain3) {
 struct PriorityExecutor : public Executor {
   void add(Func f) override {}
 
-  void addWithPriority(Func, int8_t priority) override {
+  void addWithPriority(Func f, int8_t priority) override {
     int mid = getNumPriorities() / 2;
     int p = priority < 0 ?
             std::max(0, mid + priority) :
@@ -205,6 +205,7 @@ struct PriorityExecutor : public Executor {
     } else if (p == 2) {
       count2++;
     }
+    f();
   }
 
   uint8_t getNumPriorities() const override {