From bb76b0f7bf1eabf0b66dea063d617e5c6d8de56f Mon Sep 17 00:00:00 2001 From: Phil Willoughby Date: Wed, 15 Feb 2017 03:37:16 -0800 Subject: [PATCH] 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 --- folly/Makefile.am | 1 + folly/futures/FutureException.h | 3 + folly/futures/FutureSplitter.h | 76 ++++++++++++ folly/futures/SharedPromise-inl.h | 5 - folly/futures/SharedPromise.h | 7 -- folly/futures/test/FutureSplitterTest.cpp | 137 ++++++++++++++++++++++ folly/futures/test/SharedPromiseTest.cpp | 30 ----- 7 files changed, 217 insertions(+), 42 deletions(-) create mode 100644 folly/futures/FutureSplitter.h create mode 100644 folly/futures/test/FutureSplitterTest.cpp diff --git a/folly/Makefile.am b/folly/Makefile.am index 7e5a0284..39c46367 100644 --- 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 57635424..3e30672d 100644 --- 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 index 00000000..b189d44a --- /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 +#include + +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 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. + */ + explicit FutureSplitter(Future&& future) + : promise_(std::make_shared>()) { + future.then([promise = promise_](Try && theTry) { + promise->setTry(std::move(theTry)); + }); + } + + /** + * This can be called an unlimited number of times per FutureSplitter. + */ + Future getFuture() { + if (UNLIKELY(promise_ == nullptr)) { + throw NoFutureInSplitter(); + } + return promise_->getFuture(); + } + + private: + std::shared_ptr> promise_; +}; + +/** + * Convenience function, allowing us to exploit template argument deduction to + * improve readability. + */ +template +FutureSplitter splitFuture(Future&& future) { + return FutureSplitter{std::move(future)}; +} +} diff --git a/folly/futures/SharedPromise-inl.h b/folly/futures/SharedPromise-inl.h index fc45b4f4..185062ac 100644 --- a/folly/futures/SharedPromise-inl.h +++ b/folly/futures/SharedPromise-inl.h @@ -47,11 +47,6 @@ SharedPromise& SharedPromise::operator=( return *this; } -template -SharedPromise::SharedPromise(Future future) { - future.then(&SharedPromise::setTry, this); -} - template size_t SharedPromise::size() { std::lock_guard g(mutex_); diff --git a/folly/futures/SharedPromise.h b/folly/futures/SharedPromise.h index bf1c096e..51a4a179 100644 --- a/folly/futures/SharedPromise.h +++ b/folly/futures/SharedPromise.h @@ -46,13 +46,6 @@ public: SharedPromise(SharedPromise&&) noexcept; SharedPromise& operator=(SharedPromise&&) noexcept; - /** - * Provide a way to split a Future. 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); - /** * 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 index 00000000..c14e30c0 --- /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 +#include + +using namespace folly; + +TEST(FutureSplitter, splitFutureSuccess) { + Promise p; + FutureSplitter 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 p; + FutureSplitter sp1(p.getFuture()); + FutureSplitter 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 sp3(sp1); + auto f3 = sp3.getFuture(); + EXPECT_TRUE(f3.isReady()); + EXPECT_TRUE(f3.hasValue()); +} + +TEST(FutureSplitter, splitFutureMovable) { + Promise p; + FutureSplitter sp1(p.getFuture()); + auto f1 = sp1.getFuture(); + FutureSplitter 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 sp3(std::move(sp2)); + auto f3 = sp3.getFuture(); + EXPECT_TRUE(f3.isReady()); + EXPECT_TRUE(f3.hasValue()); +} + +TEST(FutureSplitter, splitFutureCopyAssignable) { + Promise p; + FutureSplitter sp1(p.getFuture()); + FutureSplitter 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 sp3(sp1); + auto f3 = sp3.getFuture(); + EXPECT_TRUE(f3.isReady()); + EXPECT_TRUE(f3.hasValue()); +} + +TEST(FutureSplitter, splitFutureMoveAssignable) { + Promise p; + FutureSplitter sp1(p.getFuture()); + auto f1 = sp1.getFuture(); + FutureSplitter 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 sp3(std::move(sp2)); + auto f3 = sp3.getFuture(); + EXPECT_TRUE(f3.isReady()); + EXPECT_TRUE(f3.hasValue()); +} + +TEST(FutureSplitter, splitFutureScope) { + Promise p; + auto pSP = make_unique>(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 p; + FutureSplitter 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 a449bf60..3cb878dd 100644 --- 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 p; - SharedPromise 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 p; - SharedPromise 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()); -} -- 2.34.1