Commit b7e2f57b authored by Lee Howes's avatar Lee Howes Committed by Facebook Github Bot

=delete Future::onError

Summary:
Replace Future::onError with Future::thenError:
 * to remove ambiguous typing
 * to ensure that the executor is not lost and the returned Future is still bound to an executor

This diff finally removes the legacy onError.

See:
https://fb.workplace.com/groups/fbcode/permalink/2002251863144976/
for details.

Reviewed By: yfeldblum, Orvid

Differential Revision: D15034702

fbshipit-source-id: 17faf37e79c0a31ad25c02542f09a8293e693899
parent e9440ced
...@@ -275,10 +275,11 @@ class CrappyExecutor : public Executor { ...@@ -275,10 +275,11 @@ class CrappyExecutor : public Executor {
TEST(Executor, CrappyExecutor) { TEST(Executor, CrappyExecutor) {
CrappyExecutor x; CrappyExecutor x;
bool flag = false; bool flag = false;
auto f = folly::via(&x).onError([&](std::runtime_error& e) { auto f = folly::via(&x).thenError(
EXPECT_STREQ("bad", e.what()); folly::tag_t<std::runtime_error>{}, [&](std::runtime_error&& e) {
flag = true; EXPECT_STREQ("bad", e.what());
}); flag = true;
});
EXPECT_TRUE(flag); EXPECT_TRUE(flag);
} }
......
...@@ -128,7 +128,7 @@ class FutureDAG : public std::enable_shared_from_this<FutureDAG> { ...@@ -128,7 +128,7 @@ class FutureDAG : public std::enable_shared_from_this<FutureDAG> {
nodes[handle].promise.setTry(std::move(t)); nodes[handle].promise.setTry(std::move(t));
}); });
}) })
.onError([this, handle](exception_wrapper ew) { .thenError([this, handle](exception_wrapper ew) {
nodes[handle].promise.setException(std::move(ew)); nodes[handle].promise.setException(std::move(ew));
}); });
} }
......
...@@ -1284,53 +1284,6 @@ Future<Unit> Future<T>::then() && { ...@@ -1284,53 +1284,6 @@ Future<Unit> Future<T>::then() && {
return std::move(*this).thenValue([](T&&) {}); return std::move(*this).thenValue([](T&&) {});
} }
// onError where the callback returns T
template <class T>
template <class F>
typename std::enable_if<
!is_invocable<F, exception_wrapper>::value &&
!futures::detail::Extract<F>::ReturnsFuture::value,
Future<T>>::type
Future<T>::onError(F&& func) && {
typedef typename futures::detail::Extract<F>::FirstArg Exn;
static_assert(
std::is_same<typename futures::detail::Extract<F>::RawReturn, T>::value,
"Return type of onError callback must be T or Future<T>");
// NOTE: Removes the executor to maintain historical behaviour.
return std::move(*this)
.thenError(
tag_t<Exn>{},
[func = std::forward<F>(func)](auto&& ex) mutable {
return std::forward<F>(func)(ex);
})
.via(&InlineExecutor::instance());
}
// onError where the callback returns Future<T>
template <class T>
template <class F>
typename std::enable_if<
!is_invocable<F, exception_wrapper>::value &&
futures::detail::Extract<F>::ReturnsFuture::value,
Future<T>>::type
Future<T>::onError(F&& func) && {
static_assert(
std::is_same<typename futures::detail::Extract<F>::Return, Future<T>>::
value,
"Return type of onError callback must be T or Future<T>");
typedef typename futures::detail::Extract<F>::FirstArg Exn;
// NOTE: Removes the executor to maintain historical behaviour.
return std::move(*this)
.thenError(
tag_t<Exn>{},
[func = std::forward<F>(func)](auto&& ex) mutable {
return std::forward<F>(func)(ex);
})
.via(&InlineExecutor::instance());
}
template <class T> template <class T>
template <class F> template <class F>
Future<T> Future<T>::ensure(F&& func) && { Future<T> Future<T>::ensure(F&& func) && {
...@@ -1351,43 +1304,6 @@ Future<T> Future<T>::onTimeout(Duration dur, F&& func, Timekeeper* tk) && { ...@@ -1351,43 +1304,6 @@ Future<T> Future<T>::onTimeout(Duration dur, F&& func, Timekeeper* tk) && {
}); });
} }
template <class T>
template <class F>
typename std::enable_if<
is_invocable<F, exception_wrapper>::value &&
futures::detail::Extract<F>::ReturnsFuture::value,
Future<T>>::type
Future<T>::onError(F&& func) && {
static_assert(
std::is_same<typename futures::detail::Extract<F>::Return, Future<T>>::
value,
"Return type of onError callback must be T or Future<T>");
// NOTE: Removes the executor to maintain historical behaviour.
return std::move(*this)
.thenError(std::forward<F>(func))
.via(&InlineExecutor::instance());
}
// onError(exception_wrapper) that returns T
template <class T>
template <class F>
typename std::enable_if<
is_invocable<F, exception_wrapper>::value &&
!futures::detail::Extract<F>::ReturnsFuture::value,
Future<T>>::type
Future<T>::onError(F&& func) && {
static_assert(
std::is_same<typename futures::detail::Extract<F>::Return, Future<T>>::
value,
"Return type of onError callback must be T or Future<T>");
// NOTE: Removes the executor to maintain historical behaviour.
return std::move(*this)
.thenError(std::forward<F>(func))
.via(&InlineExecutor::instance());
}
template <class Func> template <class Func>
auto via(Executor::KeepAlive<> x, Func&& func) -> Future< auto via(Executor::KeepAlive<> x, Func&& func) -> Future<
typename isFutureOrSemiFuture<decltype(std::declval<Func>()())>::Inner> { typename isFutureOrSemiFuture<decltype(std::declval<Func>()())>::Inner> {
......
...@@ -1469,7 +1469,7 @@ class Future : private futures::detail::FutureBase<T> { ...@@ -1469,7 +1469,7 @@ class Future : private futures::detail::FutureBase<T> {
!is_invocable<F, exception_wrapper>::value && !is_invocable<F, exception_wrapper>::value &&
!futures::detail::Extract<F>::ReturnsFuture::value, !futures::detail::Extract<F>::ReturnsFuture::value,
Future<T>>::type Future<T>>::type
onError(F&& func) &&; onError(F&& func) && = delete;
/// Overload of onError where the error continuation returns a Future<T> /// Overload of onError where the error continuation returns a Future<T>
/// ///
...@@ -1489,7 +1489,7 @@ class Future : private futures::detail::FutureBase<T> { ...@@ -1489,7 +1489,7 @@ class Future : private futures::detail::FutureBase<T> {
!is_invocable<F, exception_wrapper>::value && !is_invocable<F, exception_wrapper>::value &&
futures::detail::Extract<F>::ReturnsFuture::value, futures::detail::Extract<F>::ReturnsFuture::value,
Future<T>>::type Future<T>>::type
onError(F&& func) &&; onError(F&& func) && = delete;
/// Overload of onError that takes exception_wrapper and returns Future<T> /// Overload of onError that takes exception_wrapper and returns Future<T>
/// ///
...@@ -1509,7 +1509,7 @@ class Future : private futures::detail::FutureBase<T> { ...@@ -1509,7 +1509,7 @@ class Future : private futures::detail::FutureBase<T> {
is_invocable<F, exception_wrapper>::value && is_invocable<F, exception_wrapper>::value &&
futures::detail::Extract<F>::ReturnsFuture::value, futures::detail::Extract<F>::ReturnsFuture::value,
Future<T>>::type Future<T>>::type
onError(F&& func) &&; onError(F&& func) && = delete;
/// Overload of onError that takes exception_wrapper and returns T /// Overload of onError that takes exception_wrapper and returns T
/// ///
...@@ -1529,16 +1529,15 @@ class Future : private futures::detail::FutureBase<T> { ...@@ -1529,16 +1529,15 @@ class Future : private futures::detail::FutureBase<T> {
is_invocable<F, exception_wrapper>::value && is_invocable<F, exception_wrapper>::value &&
!futures::detail::Extract<F>::ReturnsFuture::value, !futures::detail::Extract<F>::ReturnsFuture::value,
Future<T>>::type Future<T>>::type
onError(F&& func) &&; onError(F&& func) && = delete;
template <class R, class... Args> template <class R, class... Args>
Future<T> onError(R (&func)(Args...)) && { Future<T> onError(R (&func)(Args...)) && = delete;
return std::move(*this).onError(&func);
}
// clang-format off // clang-format off
template <class F> template <class F>
[[deprecated("ERROR: use rvalue-qualified fn, eg, std::move(future).onError(...)")]] [[deprecated(
"onError loses the attached executor and is weakly typed. Please move to thenError instead.")]]
Future<T> onError(F&& func) & = delete; Future<T> onError(F&& func) & = delete;
/// func is like std::function<void()> and is executed unconditionally, and /// func is like std::function<void()> and is executed unconditionally, and
...@@ -1559,7 +1558,7 @@ class Future : private futures::detail::FutureBase<T> { ...@@ -1559,7 +1558,7 @@ class Future : private futures::detail::FutureBase<T> {
Future<T> ensure(F&& func) &&; Future<T> ensure(F&& func) &&;
// clang-format on // clang-format on
/// Like onError, but for timeouts. example: /// Like thenError, but for timeouts. example:
/// ///
/// Future<int> f = makeFuture<int>(42) /// Future<int> f = makeFuture<int>(42)
/// .delayed(long_time) /// .delayed(long_time)
......
...@@ -26,7 +26,7 @@ using namespace folly; ...@@ -26,7 +26,7 @@ using namespace folly;
namespace { namespace {
/*** /***
* The basic premise is to check that the callback passed to then or onError * The basic premise is to check that the callback passed to then or thenError
* is destructed before wait returns on the resulting future. * is destructed before wait returns on the resulting future.
* *
* The approach is to use callbacks where the destructor sleeps 500ms and then * The approach is to use callbacks where the destructor sleeps 500ms and then
...@@ -102,110 +102,123 @@ TEST_F(CallbackLifetimeTest, thenReturnsFutureThrows) { ...@@ -102,110 +102,123 @@ TEST_F(CallbackLifetimeTest, thenReturnsFutureThrows) {
EXPECT_EQ(1, *c); EXPECT_EQ(1, *c);
} }
TEST_F(CallbackLifetimeTest, onErrorTakesExnReturnsValueMatch) { TEST_F(CallbackLifetimeTest, thenErrorTakesExnReturnsValueMatch) {
auto c = mkC(); auto c = mkC();
via(&executor) via(&executor)
.thenValue(raise) .thenValue(raise)
.onError([_ = mkCGuard(c)](std::exception&) {}) .thenError(folly::tag_t<std::exception>{}, [_ = mkCGuard(c)](auto&&) {})
.wait(); .wait();
EXPECT_EQ(1, *c); EXPECT_EQ(1, *c);
} }
TEST_F(CallbackLifetimeTest, onErrorTakesExnReturnsValueMatchThrows) { TEST_F(CallbackLifetimeTest, thenErrorTakesExnReturnsValueMatchThrows) {
auto c = mkC(); auto c = mkC();
via(&executor) via(&executor)
.thenValue(raise) .thenValue(raise)
.onError([_ = mkCGuard(c)](std::exception&) { raise(); }) .thenError(
folly::tag_t<std::exception>{},
[_ = mkCGuard(c)](auto&&) { raise(); })
.wait(); .wait();
EXPECT_EQ(1, *c); EXPECT_EQ(1, *c);
} }
TEST_F(CallbackLifetimeTest, onErrorTakesExnReturnsValueWrong) { TEST_F(CallbackLifetimeTest, thenErrorTakesExnReturnsValueWrong) {
auto c = mkC(); auto c = mkC();
via(&executor) via(&executor)
.thenValue(raise) .thenValue(raise)
.onError([_ = mkCGuard(c)](std::logic_error&) {}) .thenError(folly::tag_t<std::logic_error>{}, [_ = mkCGuard(c)](auto&&) {})
.wait(); .wait();
EXPECT_EQ(1, *c); EXPECT_EQ(1, *c);
} }
TEST_F(CallbackLifetimeTest, onErrorTakesExnReturnsValueWrongThrows) { TEST_F(CallbackLifetimeTest, thenErrorTakesExnReturnsValueWrongThrows) {
auto c = mkC(); auto c = mkC();
via(&executor) via(&executor)
.thenValue(raise) .thenValue(raise)
.onError([_ = mkCGuard(c)](std::logic_error&) { raise(); }) .thenError(
folly::tag_t<std::logic_error>{},
[_ = mkCGuard(c)](auto&&) { raise(); })
.wait(); .wait();
EXPECT_EQ(1, *c); EXPECT_EQ(1, *c);
} }
TEST_F(CallbackLifetimeTest, onErrorTakesExnReturnsFutureMatch) { TEST_F(CallbackLifetimeTest, thenErrorTakesExnReturnsFutureMatch) {
auto c = mkC(); auto c = mkC();
via(&executor) via(&executor)
.thenValue(raise) .thenValue(raise)
.onError([_ = mkCGuard(c)](std::exception&) { return makeFuture(); }) .thenError(
folly::tag_t<std::exception>{},
[_ = mkCGuard(c)](auto&&) { return makeFuture(); })
.wait(); .wait();
EXPECT_EQ(1, *c); EXPECT_EQ(1, *c);
} }
TEST_F(CallbackLifetimeTest, onErrorTakesExnReturnsFutureMatchThrows) { TEST_F(CallbackLifetimeTest, thenErrorTakesExnReturnsFutureMatchThrows) {
auto c = mkC(); auto c = mkC();
via(&executor) via(&executor)
.thenValue(raise) .thenValue(raise)
.onError([_ = mkCGuard(c)](std::exception&) { return raiseFut(); }) .thenError(
folly::tag_t<std::exception>{},
[_ = mkCGuard(c)](auto&&) { return raiseFut(); })
.wait(); .wait();
EXPECT_EQ(1, *c); EXPECT_EQ(1, *c);
} }
TEST_F(CallbackLifetimeTest, onErrorTakesExnReturnsFutureWrong) { TEST_F(CallbackLifetimeTest, thenErrorTakesExnReturnsFutureWrong) {
auto c = mkC(); auto c = mkC();
via(&executor) via(&executor)
.thenValue(raise) .thenValue(raise)
.onError([_ = mkCGuard(c)](std::logic_error&) { return makeFuture(); }) .thenError(
folly::tag_t<std::logic_error>{},
[_ = mkCGuard(c)](auto&&) { return makeFuture(); })
.wait(); .wait();
EXPECT_EQ(1, *c); EXPECT_EQ(1, *c);
} }
TEST_F(CallbackLifetimeTest, onErrorTakesExnReturnsFutureWrongThrows) { TEST_F(CallbackLifetimeTest, thenErrorTakesExnReturnsFutureWrongThrows) {
auto c = mkC(); auto c = mkC();
via(&executor) via(&executor)
.thenValue(raise) .thenValue(raise)
.onError([_ = mkCGuard(c)](std::logic_error&) { return raiseFut(); }) .thenError(
folly::tag_t<std::logic_error>{},
[_ = mkCGuard(c)](auto&&) { return raiseFut(); })
.wait(); .wait();
EXPECT_EQ(1, *c); EXPECT_EQ(1, *c);
} }
TEST_F(CallbackLifetimeTest, onErrorTakesWrapReturnsValue) { TEST_F(CallbackLifetimeTest, thenErrorTakesWrapReturnsValue) {
auto c = mkC(); auto c = mkC();
via(&executor) via(&executor)
.thenValue(raise) .thenValue(raise)
.onError([_ = mkCGuard(c)](exception_wrapper&&) {}) .thenError([_ = mkCGuard(c)](exception_wrapper&&) {})
.wait(); .wait();
EXPECT_EQ(1, *c); EXPECT_EQ(1, *c);
} }
TEST_F(CallbackLifetimeTest, onErrorTakesWrapReturnsValueThrows) { TEST_F(CallbackLifetimeTest, thenErrorTakesWrapReturnsValueThrows) {
auto c = mkC(); auto c = mkC();
via(&executor) via(&executor)
.thenValue(raise) .thenValue(raise)
.onError([_ = mkCGuard(c)](exception_wrapper&&) { raise(); }) .thenError([_ = mkCGuard(c)](exception_wrapper&&) { raise(); })
.wait(); .wait();
EXPECT_EQ(1, *c); EXPECT_EQ(1, *c);
} }
TEST_F(CallbackLifetimeTest, onErrorTakesWrapReturnsFuture) { TEST_F(CallbackLifetimeTest, thenErrorTakesWrapReturnsFuture) {
auto c = mkC(); auto c = mkC();
via(&executor) via(&executor)
.thenValue(raise) .thenValue(raise)
.onError([_ = mkCGuard(c)](exception_wrapper&&) { return makeFuture(); }) .thenError(
[_ = mkCGuard(c)](exception_wrapper&&) { return makeFuture(); })
.wait(); .wait();
EXPECT_EQ(1, *c); EXPECT_EQ(1, *c);
} }
TEST_F(CallbackLifetimeTest, onErrorTakesWrapReturnsFutureThrows) { TEST_F(CallbackLifetimeTest, thenErrorTakesWrapReturnsFutureThrows) {
auto c = mkC(); auto c = mkC();
via(&executor) via(&executor)
.thenValue(raise) .thenValue(raise)
.onError([_ = mkCGuard(c)](exception_wrapper&&) { return raiseFut(); }) .thenError([_ = mkCGuard(c)](exception_wrapper&&) { return raiseFut(); })
.wait(); .wait();
EXPECT_EQ(1, *c); EXPECT_EQ(1, *c);
} }
This diff is collapsed.
...@@ -52,8 +52,8 @@ class ThrowingExecutor : public folly::Executor { ...@@ -52,8 +52,8 @@ class ThrowingExecutor : public folly::Executor {
TEST(SelfDestruct, throwingExecutor) { TEST(SelfDestruct, throwingExecutor) {
ThrowingExecutor executor; ThrowingExecutor executor;
auto* p = new Promise<int>(); auto* p = new Promise<int>();
auto future = auto future = p->getFuture().via(&executor).thenError(
p->getFuture().via(&executor).onError([p](ThrowingExecutorError const&) { folly::tag_t<ThrowingExecutorError>{}, [p](auto const&) {
delete p; delete p;
return 456; return 456;
}); });
...@@ -71,7 +71,9 @@ TEST(SelfDestruct, throwingInlineExecutor) { ...@@ -71,7 +71,9 @@ TEST(SelfDestruct, throwingInlineExecutor) {
delete p; delete p;
throw ThrowingExecutorError("callback throws"); throw ThrowingExecutorError("callback throws");
}) })
.onError([](ThrowingExecutorError const&) { return 456; }); .thenError(
folly::tag_t<ThrowingExecutorError>{},
[](auto const&) { return 456; });
p->setValue(123); p->setValue(123);
EXPECT_EQ(456, std::move(future).get()); EXPECT_EQ(456, std::move(future).get());
} }
...@@ -68,7 +68,9 @@ TEST(Times, success) { ...@@ -68,7 +68,9 @@ TEST(Times, success) {
auto thunk = makeThunk(ps, interrupt, ps_mutex); auto thunk = makeThunk(ps, interrupt, ps_mutex);
auto f = folly::times(3, thunk) auto f = folly::times(3, thunk)
.thenValue([&](auto&&) mutable { complete = true; }) .thenValue([&](auto&&) mutable { complete = true; })
.onError([&](FutureException& /* e */) { failure = true; }); .thenError(folly::tag_t<FutureException>{}, [&](auto&& /* e */) {
failure = true;
});
popAndFulfillPromise(ps, ps_mutex); popAndFulfillPromise(ps, ps_mutex);
EXPECT_FALSE(complete); EXPECT_FALSE(complete);
...@@ -94,7 +96,9 @@ TEST(Times, failure) { ...@@ -94,7 +96,9 @@ TEST(Times, failure) {
auto thunk = makeThunk(ps, interrupt, ps_mutex); auto thunk = makeThunk(ps, interrupt, ps_mutex);
auto f = folly::times(3, thunk) auto f = folly::times(3, thunk)
.thenValue([&](auto&&) mutable { complete = true; }) .thenValue([&](auto&&) mutable { complete = true; })
.onError([&](FutureException& /* e */) { failure = true; }); .thenError(folly::tag_t<FutureException>{}, [&](auto&& /* e */) {
failure = true;
});
popAndFulfillPromise(ps, ps_mutex); popAndFulfillPromise(ps, ps_mutex);
EXPECT_FALSE(complete); EXPECT_FALSE(complete);
...@@ -122,7 +126,9 @@ TEST(Times, interrupt) { ...@@ -122,7 +126,9 @@ TEST(Times, interrupt) {
auto thunk = makeThunk(ps, interrupt, ps_mutex); auto thunk = makeThunk(ps, interrupt, ps_mutex);
auto f = folly::times(3, thunk) auto f = folly::times(3, thunk)
.thenValue([&](auto&&) mutable { complete = true; }) .thenValue([&](auto&&) mutable { complete = true; })
.onError([&](FutureException& /* e */) { failure = true; }); .thenError(folly::tag_t<FutureException>{}, [&](auto&& /* e */) {
failure = true;
});
EXPECT_EQ(0, interrupt); EXPECT_EQ(0, interrupt);
......
...@@ -70,7 +70,9 @@ TEST(WhileDo, success) { ...@@ -70,7 +70,9 @@ TEST(WhileDo, success) {
auto thunk = makeThunk(ps, interrupt, ps_mutex); auto thunk = makeThunk(ps, interrupt, ps_mutex);
auto f = folly::whileDo(pred, thunk) auto f = folly::whileDo(pred, thunk)
.thenValue([&](auto&&) mutable { complete = true; }) .thenValue([&](auto&&) mutable { complete = true; })
.onError([&](FutureException& /* e */) { failure = true; }); .thenError(folly::tag_t<FutureException>{}, [&](auto&& /* e */) {
failure = true;
});
popAndFulfillPromise(ps, ps_mutex); popAndFulfillPromise(ps, ps_mutex);
EXPECT_FALSE(complete); EXPECT_FALSE(complete);
...@@ -98,7 +100,9 @@ TEST(WhileDo, failure) { ...@@ -98,7 +100,9 @@ TEST(WhileDo, failure) {
auto thunk = makeThunk(ps, interrupt, ps_mutex); auto thunk = makeThunk(ps, interrupt, ps_mutex);
auto f = folly::whileDo(pred, thunk) auto f = folly::whileDo(pred, thunk)
.thenValue([&](auto&&) mutable { complete = true; }) .thenValue([&](auto&&) mutable { complete = true; })
.onError([&](FutureException& /* e */) { failure = true; }); .thenError(folly::tag_t<FutureException>{}, [&](auto&& /* e */) {
failure = true;
});
popAndFulfillPromise(ps, ps_mutex); popAndFulfillPromise(ps, ps_mutex);
EXPECT_FALSE(complete); EXPECT_FALSE(complete);
...@@ -128,7 +132,9 @@ TEST(WhileDo, interrupt) { ...@@ -128,7 +132,9 @@ TEST(WhileDo, interrupt) {
auto thunk = makeThunk(ps, interrupt, ps_mutex); auto thunk = makeThunk(ps, interrupt, ps_mutex);
auto f = folly::whileDo(pred, thunk) auto f = folly::whileDo(pred, thunk)
.thenValue([&](auto&&) mutable { complete = true; }) .thenValue([&](auto&&) mutable { complete = true; })
.onError([&](FutureException& /* e */) { failure = true; }); .thenError(folly::tag_t<FutureException>{}, [&](auto&& /* e */) {
failure = true;
});
EXPECT_EQ(0, interrupt); EXPECT_EQ(0, interrupt);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment