Add getSemiFuture and storage of executor to FutureSplitter so that returned...

Add getSemiFuture and storage of executor to FutureSplitter so that returned future completes on the expected executor.

Summary:
Remove calls to Promise::getFuture in FutureSplitter test.

Add getSemiFuture to FutureSplitter for consistency with Promise.

Keep getFuture method but ensure that it completes on the same executor as the input future (or Inline if the executor on the input future was null, due to legacy code).

Reviewed By: yfeldblum

Differential Revision: D7105806

fbshipit-source-id: 63a919ba9dcf0f189782a3e2a362fad4d86ac91f

folly/futures/Future.h

@@ -48,6 +48,9 @@ class Future;
 template <class T>
 class SemiFuture;
 
+template <class T>
+class FutureSplitter;
+
 namespace futures {
 namespace detail {
 template <class T>
@@ -781,6 +784,8 @@ class Future : private futures::detail::FutureBase<T> {
   friend class Future;
   template <class>
   friend class SemiFuture;
+  template <class>
+  friend class FutureSplitter;
 
   using Base::setExecutor;
   using Base::throwIfInvalid;

folly/futures/FutureSplitter.h

@@ -45,8 +45,9 @@ class FutureSplitter {
    * Provide a way to split a Future<T>.
    */
   explicit FutureSplitter(Future<T>&& future)
-      : promise_(std::make_shared<SharedPromise<T>>()) {
-    future.then([promise = promise_](Try<T> && theTry) {
+      : promise_(std::make_shared<SharedPromise<T>>()),
+        e_(getExecutorFrom(future)) {
+    future.then([promise = promise_](Try<T>&& theTry) {
       promise->setTry(std::move(theTry));
     });
   }
@@ -58,11 +59,29 @@ class FutureSplitter {
     if (promise_ == nullptr) {
       throwNoFutureInSplitter();
     }
-    return promise_->getFuture();
+    return promise_->getSemiFuture().via(e_);
+  }
+
+  /**
+   * This can be called an unlimited number of times per FutureSplitter.
+   */
+  SemiFuture<T> getSemiFuture() {
+    if (promise_ == nullptr) {
+      throwNoFutureInSplitter();
+    }
+    return promise_->getSemiFuture();
   }
 
  private:
   std::shared_ptr<SharedPromise<T>> promise_;
+  Executor* e_ = nullptr;
+
+  static Executor* getExecutorFrom(Future<T>& f) {
+    // If the passed future had a null executor, use an inline executor
+    // to ensure that .via is safe
+    auto* e = f.getExecutor();
+    return e ? e : &folly::InlineExecutor::instance();
+  }
 };
 
 /**

folly/futures/test/FutureSplitterTest.cpp

@@ -21,7 +21,40 @@ using namespace folly;
 
 TEST(FutureSplitter, splitFutureSuccess) {
   Promise<int> p;
-  FutureSplitter<int> sp(p.getFuture());
+  folly::FutureSplitter<int> sp(
+      p.getSemiFuture().via(&folly::InlineExecutor::instance()));
+  auto f1 = sp.getFuture();
+  EXPECT_FALSE(f1.isReady());
+  p.setValue(1);
+  EXPECT_TRUE(f1.isReady());
+  EXPECT_TRUE(f1.hasValue());
+  auto f2 = sp.getFuture();
+  EXPECT_TRUE(f2.isReady());
+  EXPECT_TRUE(f2.hasValue());
+}
+
+TEST(FutureSplitter, splitFutureSuccessSemiFuture) {
+  Promise<int> p;
+  folly::FutureSplitter<int> sp(
+      p.getSemiFuture().via(&folly::InlineExecutor::instance()));
+  auto f1 = sp.getSemiFuture();
+  EXPECT_FALSE(f1.isReady());
+  p.setValue(1);
+  EXPECT_TRUE(f1.isReady());
+  EXPECT_TRUE(f1.hasValue());
+  auto f2 = sp.getSemiFuture();
+  EXPECT_TRUE(f2.isReady());
+  EXPECT_TRUE(f2.hasValue());
+}
+
+TEST(FutureSplitter, splitFutureSuccessNullExecutor) {
+  Promise<int> p;
+  auto sf = p.getSemiFuture();
+  // Double via because a null executor to SemiFuture.via is invalid but we
+  // are testing a situation where we have a FutureSplitter from a future with
+  // a null executor to account for legacy code.
+  auto f = std::move(sf).via(&folly::InlineExecutor::instance()).via(nullptr);
+  folly::FutureSplitter<int> sp(std::move(f));
   auto f1 = sp.getFuture();
   EXPECT_FALSE(f1.isReady());
   p.setValue(1);
@@ -34,8 +67,9 @@ TEST(FutureSplitter, splitFutureSuccess) {
 
 TEST(FutureSplitter, splitFutureCopyable) {
   Promise<int> p;
-  FutureSplitter<int> sp1(p.getFuture());
-  FutureSplitter<int> sp2(sp1);
+  folly::FutureSplitter<int> sp1(
+      p.getSemiFuture().via(&folly::InlineExecutor::instance()));
+  folly::FutureSplitter<int> sp2(sp1);
   auto f1 = sp1.getFuture();
   EXPECT_FALSE(f1.isReady());
   p.setValue(1);
@@ -44,7 +78,7 @@ TEST(FutureSplitter, splitFutureCopyable) {
   auto f2 = sp2.getFuture();
   EXPECT_TRUE(f2.isReady());
   EXPECT_TRUE(f2.hasValue());
-  FutureSplitter<int> sp3(sp1);
+  folly::FutureSplitter<int> sp3(sp1);
   auto f3 = sp3.getFuture();
   EXPECT_TRUE(f3.isReady());
   EXPECT_TRUE(f3.hasValue());
@@ -52,9 +86,10 @@ TEST(FutureSplitter, splitFutureCopyable) {
 
 TEST(FutureSplitter, splitFutureMovable) {
   Promise<int> p;
-  FutureSplitter<int> sp1(p.getFuture());
+  folly::FutureSplitter<int> sp1(
+      p.getSemiFuture().via(&folly::InlineExecutor::instance()));
   auto f1 = sp1.getFuture();
-  FutureSplitter<int> sp2(std::move(sp1));
+  folly::FutureSplitter<int> sp2(std::move(sp1));
   EXPECT_FALSE(f1.isReady());
   p.setValue(1);
   EXPECT_TRUE(f1.isReady());
@@ -62,7 +97,7 @@ TEST(FutureSplitter, splitFutureMovable) {
   auto f2 = sp2.getFuture();
   EXPECT_TRUE(f2.isReady());
   EXPECT_TRUE(f2.hasValue());
-  FutureSplitter<int> sp3(std::move(sp2));
+  folly::FutureSplitter<int> sp3(std::move(sp2));
   auto f3 = sp3.getFuture();
   EXPECT_TRUE(f3.isReady());
   EXPECT_TRUE(f3.hasValue());
@@ -70,8 +105,9 @@ TEST(FutureSplitter, splitFutureMovable) {
 
 TEST(FutureSplitter, splitFutureCopyAssignable) {
   Promise<int> p;
-  FutureSplitter<int> sp1(p.getFuture());
-  FutureSplitter<int> sp2{};
+  folly::FutureSplitter<int> sp1(
+      p.getSemiFuture().via(&folly::InlineExecutor::instance()));
+  folly::FutureSplitter<int> sp2{};
   sp2 = sp1;
   auto f1 = sp1.getFuture();
   EXPECT_FALSE(f1.isReady());
@@ -81,7 +117,7 @@ TEST(FutureSplitter, splitFutureCopyAssignable) {
   auto f2 = sp2.getFuture();
   EXPECT_TRUE(f2.isReady());
   EXPECT_TRUE(f2.hasValue());
-  FutureSplitter<int> sp3(sp1);
+  folly::FutureSplitter<int> sp3(sp1);
   auto f3 = sp3.getFuture();
   EXPECT_TRUE(f3.isReady());
   EXPECT_TRUE(f3.hasValue());
@@ -89,9 +125,10 @@ TEST(FutureSplitter, splitFutureCopyAssignable) {
 
 TEST(FutureSplitter, splitFutureMoveAssignable) {
   Promise<int> p;
-  FutureSplitter<int> sp1(p.getFuture());
+  folly::FutureSplitter<int> sp1(
+      p.getSemiFuture().via(&folly::InlineExecutor::instance()));
   auto f1 = sp1.getFuture();
-  FutureSplitter<int> sp2{};
+  folly::FutureSplitter<int> sp2{};
   sp2 = std::move(sp1);
   EXPECT_FALSE(f1.isReady());
   p.setValue(1);
@@ -100,7 +137,7 @@ TEST(FutureSplitter, splitFutureMoveAssignable) {
   auto f2 = sp2.getFuture();
   EXPECT_TRUE(f2.isReady());
   EXPECT_TRUE(f2.hasValue());
-  FutureSplitter<int> sp3(std::move(sp2));
+  folly::FutureSplitter<int> sp3(std::move(sp2));
   auto f3 = sp3.getFuture();
   EXPECT_TRUE(f3.isReady());
   EXPECT_TRUE(f3.hasValue());
@@ -108,7 +145,8 @@ TEST(FutureSplitter, splitFutureMoveAssignable) {
 
 TEST(FutureSplitter, splitFutureScope) {
   Promise<int> p;
-  auto pSP = std::make_unique<FutureSplitter<int>>(p.getFuture());
+  auto pSP = std::make_unique<folly::FutureSplitter<int>>(
+      p.getSemiFuture().via(&folly::InlineExecutor::instance()));
   auto f1 = pSP->getFuture();
   EXPECT_FALSE(f1.isReady());
   pSP.reset();
@@ -121,7 +159,8 @@ TEST(FutureSplitter, splitFutureScope) {
 
 TEST(FutureSplitter, splitFutureFailure) {
   Promise<int> p;
-  FutureSplitter<int> sp(p.getFuture());
+  folly::FutureSplitter<int> sp(
+      p.getSemiFuture().via(&folly::InlineExecutor::instance()));
   auto f1 = sp.getFuture();
   EXPECT_FALSE(f1.isReady());
   try {