From: Alex Yarmula <ayarmula@fb.com>
Date: Thu, 6 Oct 2016 15:51:33 +0000 (-0700)
Subject: Reverted commit D3979179
X-Git-Tag: v2016.10.10.00~7
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=b59ee6802a2454e854a52535d31598aa967e33c0;p=folly.git

Reverted commit D3979179

Summary:
When `delayed` is called on the Future, the underlying `futures::sleep` call runs on a timer thread, and the resulting callback is called on the same thread. Therefore, in the following sequence:

  f.via(&someExecutor).within(one_ms).then([&]() { /* [1] */ })

The code in [1] is not running in someExecutor. This can cause confusion by users of the library who expect the initial `via` to be sticky.

This change returns to the prior `Executor` after `delayed` is finished.

Reviewed By: yfeldblum

Differential Revision: D3979179

fbshipit-source-id: e1448f5603f0c9440490ae3bf0e670687f4179f3
---

diff --git a/folly/futures/Future-inl.h b/folly/futures/Future-inl.h
index 14ae5fc4..1cd4035e 100644
--- a/folly/futures/Future-inl.h
+++ b/folly/futures/Future-inl.h
@@ -972,11 +972,10 @@ Future<T> Future<T>::within(Duration dur, E e, Timekeeper* tk) {
 template <class T>
 Future<T> Future<T>::delayed(Duration dur, Timekeeper* tk) {
   return collectAll(*this, futures::sleep(dur, tk))
-      .then([](std::tuple<Try<T>, Try<Unit>> tup) {
-        Try<T>& t = std::get<0>(tup);
-        return makeFuture<T>(std::move(t));
-      })
-      .via(getExecutor());
+    .then([](std::tuple<Try<T>, Try<Unit>> tup) {
+      Try<T>& t = std::get<0>(tup);
+      return makeFuture<T>(std::move(t));
+    });
 }
 
 namespace detail {
diff --git a/folly/futures/test/TimekeeperTest.cpp b/folly/futures/test/TimekeeperTest.cpp
index e4aeba4d..664dd737 100644
--- a/folly/futures/test/TimekeeperTest.cpp
+++ b/folly/futures/test/TimekeeperTest.cpp
@@ -168,36 +168,20 @@ TEST(Timekeeper, chainedInterruptTest) {
   EXPECT_FALSE(test);
 }
 
-namespace {
-class ExecutorTester : public Executor {
- public:
-  void add(Func f) override {
-    count++;
-    f();
-  }
-  std::atomic<int> count{0};
+TEST(Timekeeper, executor) {
+  class ExecutorTester : public Executor {
+   public:
+    void add(Func f) override {
+      count++;
+      f();
+    }
+    std::atomic<int> count{0};
   };
-  }
-
-  TEST(Timekeeper, executor) {
-    auto f = makeFuture();
-    ExecutorTester tester;
-    f.via(&tester).within(one_ms).then([&]() {}).wait();
-    EXPECT_EQ(2, tester.count);
-  }
-
-  TEST(Timekeeper, executorSameAfterDelayed) {
-    ExecutorTester tester;
-    // make sure we're using the same Executor (tester) after delayed returns
-    // by comparing the executor count between invocations
-    makeFuture()
-        .via(&tester)
-        .delayed(one_ms)
-        .then([&]() { return tester.count.load(); })
-        .then([&](int countBefore) {
-          EXPECT_EQ(1, tester.count.load() - countBefore);
-        })
-        .wait();
+
+  auto f = makeFuture();
+  ExecutorTester tester;
+  f.via(&tester).within(one_ms).then([&](){}).wait();
+  EXPECT_EQ(2, tester.count);
 }
 
 // TODO(5921764)