Replace the future-splitting system

Summary:
The previous mechanism in SharedPromise doesn't work if the lifetime of the
SharedPromise object ends before the Future which it was splitting is
completed.

This variation on the same theme doesn't have that problem.

Reviewed By: spacedentist

Differential Revision: D4339670

fbshipit-source-id: f619762bff1390481715575b3e638ec26b5c4edd

diff --git a/folly/Makefile.am b/folly/Makefile.am
index 7e5a02842f18bd881ebe745e7eae1b88a0d16aa4..39c463673fd336ee9bbc0387b81c1673115c8577 100644 (file)
--- a/folly/Makefile.am
+++ b/folly/Makefile.am
@@ -161,6 +161,7 @@ nobase_follyinclude_HEADERS = \
        futures/Future.h \
        futures/Future-inl.h \
        futures/FutureException.h \
+       futures/FutureSplitter.h \
        futures/InlineExecutor.h \
        futures/ManualExecutor.h \
        futures/OpaqueCallbackShunt.h \

diff --git a/folly/futures/FutureException.h b/folly/futures/FutureException.h
index 57635424d84f9997ebbce5c6d7656f7b0bfe2e3c..3e30672db3605b68ae03aa3dbbc119ba3f181167 100644 (file)
--- a/folly/futures/FutureException.h
+++ b/folly/futures/FutureException.h
@@ -69,4 +69,7 @@ class PredicateDoesNotObtain : public FutureException {
   PredicateDoesNotObtain() : FutureException("Predicate does not obtain") {}
 };
 
+struct NoFutureInSplitter : FutureException {
+  NoFutureInSplitter() : FutureException("No Future in this FutureSplitter") {}
+};
 }

diff --git a/folly/futures/FutureSplitter.h b/folly/futures/FutureSplitter.h
new file mode 100644 (file)
index 0000000..b189d44
--- /dev/null
+++ b/folly/futures/FutureSplitter.h
@@ -0,0 +1,76 @@
+/*
+ * Copyright 2017 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <folly/futures/Future.h>
+#include <folly/futures/SharedPromise.h>
+
+namespace folly {
+
+/*
+ * FutureSplitter provides a `getFuture()' method which can be called multiple
+ * times, returning a new Future each time. These futures are called-back when
+ * the original Future passed to the FutureSplitter constructor completes. Calls
+ * to `getFuture()' after that time return a completed Future.
+ *
+ * Note that while the Futures from `getFuture()' depend on the completion of
+ * the original Future they do not inherit any other properties such as
+ * Executors passed to `via' etc.
+ */
+template <class T>
+class FutureSplitter {
+ public:
+  /**
+   * Default constructor for convenience only. It is an error to call
+   * `getFuture()` on a default-constructed FutureSplitter which has not had
+   * a correctly-constructed FutureSplitter copy- or move-assigned into it.
+   */
+  FutureSplitter() = default;
+
+  /**
+   * 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->setTry(std::move(theTry));
+    });
+  }
+
+  /**
+   * This can be called an unlimited number of times per FutureSplitter.
+   */
+  Future<T> getFuture() {
+    if (UNLIKELY(promise_ == nullptr)) {
+      throw NoFutureInSplitter();
+    }
+    return promise_->getFuture();
+  }
+
+ private:
+  std::shared_ptr<SharedPromise<T>> promise_;
+};
+
+/**
+ * Convenience function, allowing us to exploit template argument deduction to
+ * improve readability.
+ */
+template <class T>
+FutureSplitter<T> splitFuture(Future<T>&& future) {
+  return FutureSplitter<T>{std::move(future)};
+}
+}

diff --git a/folly/futures/SharedPromise-inl.h b/folly/futures/SharedPromise-inl.h
index fc45b4f4e56d7924b655eb0770a847f13dc10223..185062acdbcc01afbc5fd19ee543dadcc06a4b06 100644 (file)
--- a/folly/futures/SharedPromise-inl.h
+++ b/folly/futures/SharedPromise-inl.h
@@ -47,11 +47,6 @@ SharedPromise<T>& SharedPromise<T>::operator=(
   return *this;
 }
 
-template <class T>
-SharedPromise<T>::SharedPromise(Future<T> future) {
-  future.then(&SharedPromise<T>::setTry, this);
-}
-
 template <class T>
 size_t SharedPromise<T>::size() {
   std::lock_guard<std::mutex> g(mutex_);

diff --git a/folly/futures/SharedPromise.h b/folly/futures/SharedPromise.h
index bf1c096e0a395118775353edfb13684094f95408..51a4a179aa70fb85789fb11e8f57571f1f985696 100644 (file)
--- a/folly/futures/SharedPromise.h
+++ b/folly/futures/SharedPromise.h
@@ -46,13 +46,6 @@ public:
   SharedPromise(SharedPromise<T>&&) noexcept;
   SharedPromise& operator=(SharedPromise<T>&&) noexcept;
 
-  /**
-   * Provide a way to split a Future<T>. Note that while the Futures from
-   * `getFuture()' depend on the completion of the parameter Future they do not
-   * inherit any other properties such as Executor's passed to `via' etc.
-   */
-  explicit SharedPromise(Future<T>);
-
   /**
    * Return a Future tied to the shared core state. Unlike Promise::getFuture,
    * this can be called an unlimited number of times per SharedPromise.

diff --git a/folly/futures/test/FutureSplitterTest.cpp b/folly/futures/test/FutureSplitterTest.cpp
new file mode 100644 (file)
index 0000000..c14e30c
--- /dev/null
+++ b/folly/futures/test/FutureSplitterTest.cpp
@@ -0,0 +1,137 @@
+/*
+ * Copyright 2017 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <folly/futures/FutureSplitter.h>
+#include <folly/portability/GTest.h>
+
+using namespace folly;
+
+TEST(FutureSplitter, splitFutureSuccess) {
+  Promise<int> p;
+  FutureSplitter<int> sp(p.getFuture());
+  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, splitFutureCopyable) {
+  Promise<int> p;
+  FutureSplitter<int> sp1(p.getFuture());
+  FutureSplitter<int> sp2(sp1);
+  auto f1 = sp1.getFuture();
+  EXPECT_FALSE(f1.isReady());
+  p.setValue(1);
+  EXPECT_TRUE(f1.isReady());
+  EXPECT_TRUE(f1.hasValue());
+  auto f2 = sp2.getFuture();
+  EXPECT_TRUE(f2.isReady());
+  EXPECT_TRUE(f2.hasValue());
+  FutureSplitter<int> sp3(sp1);
+  auto f3 = sp3.getFuture();
+  EXPECT_TRUE(f3.isReady());
+  EXPECT_TRUE(f3.hasValue());
+}
+
+TEST(FutureSplitter, splitFutureMovable) {
+  Promise<int> p;
+  FutureSplitter<int> sp1(p.getFuture());
+  auto f1 = sp1.getFuture();
+  FutureSplitter<int> sp2(std::move(sp1));
+  EXPECT_FALSE(f1.isReady());
+  p.setValue(1);
+  EXPECT_TRUE(f1.isReady());
+  EXPECT_TRUE(f1.hasValue());
+  auto f2 = sp2.getFuture();
+  EXPECT_TRUE(f2.isReady());
+  EXPECT_TRUE(f2.hasValue());
+  FutureSplitter<int> sp3(std::move(sp2));
+  auto f3 = sp3.getFuture();
+  EXPECT_TRUE(f3.isReady());
+  EXPECT_TRUE(f3.hasValue());
+}
+
+TEST(FutureSplitter, splitFutureCopyAssignable) {
+  Promise<int> p;
+  FutureSplitter<int> sp1(p.getFuture());
+  FutureSplitter<int> sp2{};
+  sp2 = sp1;
+  auto f1 = sp1.getFuture();
+  EXPECT_FALSE(f1.isReady());
+  p.setValue(1);
+  EXPECT_TRUE(f1.isReady());
+  EXPECT_TRUE(f1.hasValue());
+  auto f2 = sp2.getFuture();
+  EXPECT_TRUE(f2.isReady());
+  EXPECT_TRUE(f2.hasValue());
+  FutureSplitter<int> sp3(sp1);
+  auto f3 = sp3.getFuture();
+  EXPECT_TRUE(f3.isReady());
+  EXPECT_TRUE(f3.hasValue());
+}
+
+TEST(FutureSplitter, splitFutureMoveAssignable) {
+  Promise<int> p;
+  FutureSplitter<int> sp1(p.getFuture());
+  auto f1 = sp1.getFuture();
+  FutureSplitter<int> sp2{};
+  sp2 = std::move(sp1);
+  EXPECT_FALSE(f1.isReady());
+  p.setValue(1);
+  EXPECT_TRUE(f1.isReady());
+  EXPECT_TRUE(f1.hasValue());
+  auto f2 = sp2.getFuture();
+  EXPECT_TRUE(f2.isReady());
+  EXPECT_TRUE(f2.hasValue());
+  FutureSplitter<int> sp3(std::move(sp2));
+  auto f3 = sp3.getFuture();
+  EXPECT_TRUE(f3.isReady());
+  EXPECT_TRUE(f3.hasValue());
+}
+
+TEST(FutureSplitter, splitFutureScope) {
+  Promise<int> p;
+  auto pSP = make_unique<FutureSplitter<int>>(p.getFuture());
+  auto f1 = pSP->getFuture();
+  EXPECT_FALSE(f1.isReady());
+  pSP.reset();
+  EXPECT_NO_THROW(EXPECT_FALSE(f1.isReady()));
+  p.setValue(1);
+  EXPECT_TRUE(f1.isReady());
+  EXPECT_TRUE(f1.hasValue());
+  EXPECT_EQ(1, f1.get());
+}
+
+TEST(FutureSplitter, splitFutureFailure) {
+  Promise<int> p;
+  FutureSplitter<int> sp(p.getFuture());
+  auto f1 = sp.getFuture();
+  EXPECT_FALSE(f1.isReady());
+  try {
+    throw std::runtime_error("Oops");
+  } catch (...) {
+    p.setException(exception_wrapper(std::current_exception()));
+  }
+  EXPECT_TRUE(f1.isReady());
+  EXPECT_TRUE(f1.hasException());
+  auto f2 = sp.getFuture();
+  EXPECT_TRUE(f2.isReady());
+  EXPECT_TRUE(f2.hasException());
+}

diff --git a/folly/futures/test/SharedPromiseTest.cpp b/folly/futures/test/SharedPromiseTest.cpp
index a449bf60026a0854abbc146b242da525962e7e9c..3cb878ddc0565baf6460bb3d046679cf060da91a 100644 (file)
--- a/folly/futures/test/SharedPromiseTest.cpp
+++ b/folly/futures/test/SharedPromiseTest.cpp
@@ -125,33 +125,3 @@ TEST(SharedPromise, interruptHandler) {
   f.cancel();
   EXPECT_TRUE(flag);
 }
-
-TEST(SharedPromise, splitFutureSuccess) {
-  Promise<int> p;
-  SharedPromise<int> sp(p.getFuture());
-  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(SharedPromise, splitFutureFailure) {
-  Promise<int> p;
-  SharedPromise<int> sp(p.getFuture());
-  auto f1 = sp.getFuture();
-  EXPECT_FALSE(f1.isReady());
-  try {
-    throw std::runtime_error("Oops");
-  } catch (const std::exception& e) {
-    p.setException(exception_wrapper(std::current_exception(), e));
-  }
-  EXPECT_TRUE(f1.isReady());
-  EXPECT_TRUE(f1.hasException());
-  auto f2 = sp.getFuture();
-  EXPECT_TRUE(f2.isReady());
-  EXPECT_TRUE(f2.hasException());
-}