[COMMIT seastar master] Merge 'future, future-utils: stop returning a variadic future from when_all_succeed'

[Commit Bot]

From: Piotr Sarna <sa...@scylladb.com>
Committer: Piotr Sarna <sa...@scylladb.com>
Branch: master

Merge 'future, future-utils: stop returning a variadic future from when_all_succeed'

from Avi.

when_all_succeed() can return a variadic future (if two or more
of its input futures are non-void). This is a problem since
variadic futures are deprecated and to be removed.

This series introduces a temporary solution, and an API-breaking complete
solution (with Seastar_API_LEVEL=4).

The temporary solution reduces code breakage with exising
1. if when_all_succeed() returned a nullary or unary future, nothing is
changed.
2. otherwise, instead of returning a variadic future, it returns a
future<when_all_succeed_tuple<T...>> instead of future<T...>.
3. The newly-introduced when_all_succeed_tuple<> template is
just a thin wrapper around std::tuple<>. The reason we need a wrapper
is that so then() knows to treat it specially.
4. If future::then() is called on a future carrying a
when_all_succeed_tuple, then it unpacks the tuple into the lambda's
parameters (same as the behavior for the variadic future it replaces).

The complete solution, with API_LEVEL=4, uses an ordinary std::tuple<>
for when_all_succeed() and does not have special cases for nullary
or unary tuples, or any hacks in .then(). Instead it relies on the
user to either accept a tuple in .then()'s continuation, or to use
the newly introduced .then_unpack().

* tag 'de-variadicate-when_all_succeed/v10' of https://github.com/avikivity/seastar:
future-util: make when_all_succeed() return a future<std::tuple<...>>
tests: perf: fair_queue_perf: prepare for when_all_succeed() returning a tuple
future: introduce future::then_unpack()
future, future-utils: stop returning a variadic future from when_all_succeed

---
diff --git a/CMakeLists.txt b/CMakeLists.txt
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -53,14 +53,14 @@ option (Seastar_SSTRING
ON)

set (Seastar_API_LEVEL
- "3"
+ "4"
CACHE
STRING
- "Seastar compatibility API level (2=server_socket::accept() returns accept_result, 3=make_file_output_stream(), make_file_data_sink() returns future<...>)")
+ "Seastar compatibility API level (2=server_socket::accept() returns accept_result, 3=make_file_output_stream(), make_file_data_sink() returns future<...>, 4=when_all_succeed returns future<std::tuple<>>)")

set_property (CACHE Seastar_API_LEVEL
PROPERTY
- STRINGS 2 3)
+ STRINGS 2 3 4)

#
# Add a dev build type.
diff --git a/doc/compatibility.md b/doc/compatibility.md
--- a/doc/compatibility.md
+++ b/doc/compatibility.md
@@ -99,6 +99,20 @@ exposed accidentally. These can also be removed or changed. Exposed
identifiers are documented using doxygen, but not all exposed
APIs are documented. In case of doubt, ask on the mailing list.

+
+API Level History
+=================
+
+|Level|Introduced |Mandatory|Description |
+|:---:|:---------:|:-------:| -------------------------------------------- |
+| 2 | 2019-07 | 2020-04 | Non-variadic futures in socket::accept() |
+| 3 | 2020-05 | | make_file_data_sink() closes file and returns a future<> |
+| 4 | 2020-06 | | Non-variadic futures in when_all_succeed() |
+
+
+Note: The "mandatory" column indicates when backwards compatibility
+support for the API preceding the new level was removed.
+
Implementation notes for API levels
===================================

diff --git a/include/seastar/core/future-util.hh b/include/seastar/core/future-util.hh
--- a/include/seastar/core/future-util.hh
+++ b/include/seastar/core/future-util.hh
@@ -1252,21 +1252,61 @@ struct tuple_to_future;

template<typename... Elements>
struct tuple_to_future<std::tuple<Elements...>> {
- using type = future<Elements...>;
- using promise_type = promise<Elements...>;
+#if SEASTAR_API_LEVEL < 4
+ using value_type = when_all_succeed_tuple<Elements...>;
+#else
+ using value_type = std::tuple<Elements...>;
+#endif
+ using type = future<value_type>;
+ using promise_type = promise<value_type>;
+
+ // Elements... all come from futures, so we know they are nothrow move
+ // constructible. `future` also has a static assertion to that effect.

static auto make_ready(std::tuple<Elements...> t) noexcept {
- auto create_future = [] (auto&&... args) {
- return make_ready_future<Elements...>(std::move(args)...);
- };
- return std::apply(create_future, std::move(t));
+ return make_ready_future<value_type>(value_type(std::move(t)));
+ }
+
+ static auto make_failed(std::exception_ptr excp) noexcept {
+ return seastar::make_exception_future<value_type>(std::move(excp));
+ }
+};
+
+#if SEASTAR_API_LEVEL < 4
+
+template<typename Element>
+struct tuple_to_future<std::tuple<Element>> {
+ using type = future<Element>;
+ using promise_type = promise<Element>;
+
+ // Element comes from a future, so we know it is nothrow move
+ // constructible. `future` also has a static assertion to that effect.
+
+ static auto make_ready(std::tuple<Element> t) noexcept {
+ return make_ready_future<Element>(std::get<0>(std::move(t)));
}

static auto make_failed(std::exception_ptr excp) noexcept {
- return seastar::make_exception_future<Elements...>(std::move(excp));
+ return seastar::make_exception_future<Element>(std::move(excp));
}
};

+template<>
+struct tuple_to_future<std::tuple<>> {
+ using type = future<>;
+ using promise_type = promise<>;
+
+ static auto make_ready(std::tuple<> t) noexcept {
+ return make_ready_future<>();
+ }
+
+ static auto make_failed(std::exception_ptr excp) noexcept {
+ return seastar::make_exception_future<>(std::move(excp));
+ }
+};
+
+#endif
+
template<typename... Futures>
class extract_values_from_futures_tuple {
static auto transform(std::tuple<Futures...> futures) noexcept {
diff --git a/include/seastar/core/future.hh b/include/seastar/core/future.hh
--- a/include/seastar/core/future.hh
+++ b/include/seastar/core/future.hh
@@ -573,6 +573,25 @@ struct continuation final : continuation_base_with_promise<Promise, T...> {
Func _func;
};

+#if SEASTAR_API_LEVEL < 4
+
+// This is an internal future<> payload for seastar::when_all_succeed(). It is used
+// to return a variadic future (when two or more of its input futures were non-void),
+// but with variadic futures deprecated and soon gone this is no longer possible.
+//
+// Instead, we use this tuple type, and future::then() knows to unpack it.
+//
+// The whole thing is temporary for a transition period.
+template <typename... T>
+struct when_all_succeed_tuple : std::tuple<T...> {
+ using std::tuple<T...>::tuple;
+ when_all_succeed_tuple(std::tuple<T...>&& t)
+ noexcept(std::is_nothrow_move_constructible<std::tuple<T...>>::value)
+ : std::tuple<T...>(std::move(t)) {}
+};
+
+#endif
+
namespace internal {

template <typename... T>
@@ -862,6 +881,13 @@ concept CanInvoke = std::invocable<Func, T...>;
template <typename Func, typename... T>
concept CanApply = CanInvoke<Func, T...>;

+template <typename Func, typename... T>
+concept CanApplyTuple
+ = sizeof...(T) == 1
+ && requires (Func func, std::tuple<T...> wrapped_val) {
+ { std::apply(func, std::get<0>(wrapped_val)) };
+ };
+
template <typename Func, typename Return, typename... T>
concept InvokeReturns = requires (Func f, T... args) {
{ f(std::forward<T>(args)...) } -> std::same_as<Return>;
@@ -1069,6 +1095,78 @@ struct warn_variadic_future<true> {
void check_deprecation() {}
};

+// This is a customization point for future::then()'s implementation.
+// It behaves differently when the future value type is a when_all_succeed_tuple
+// instantiation, indicating we need to unpack the tuple into multiple lambda
+// arguments.
+template <typename Future>
+struct call_then_impl;
+
+// Generic case - the input is not a future<when_all_succeed_tuple<...>>, so
+// we just forward everything to future::then_impl.
+template <typename... T>
+struct call_then_impl<future<T...>> {
+ template <typename Func>
+ using result_type = futurize_t<std::result_of_t<Func (T&&...)>>;
+
+ template <typename Func>
+ using func_type = result_type<Func> (T&&...);
+
+ template <typename Func>
+ static auto run(future<T...>& fut, Func&& func) noexcept {
+ return fut.then_impl(std::forward<Func>(func));
+ }
+};
+
+#if SEASTAR_API_LEVEL < 4
+
+// Special case: we unpack the tuple before calling the function
+template <typename... T>
+struct call_then_impl<future<when_all_succeed_tuple<T...>>> {
+ template <typename Func>
+ using result_type = futurize_t<std::result_of_t<Func (T&&...)>>;
+
+ template <typename Func>
+ using func_type = result_type<Func> (T&&...);
+
+ using was_tuple = when_all_succeed_tuple<T...>;
+ using std_tuple = std::tuple<T...>;
+
+ template <typename Func>
+ static auto run(future<was_tuple>& fut, Func&& func) noexcept {
+ // constructing func in the lambda can throw, but there's nothing we can do
+ // about it, similar to #84.
+ return fut.then_impl([func = std::forward<Func>(func)] (was_tuple&& t) mutable {
+ return std::apply(func, static_cast<std_tuple&&>(std::move(t)));
+ });
+ }
+};
+
+#endif
+
+template <typename Func, typename... Args>
+using call_then_impl_result_type = typename call_then_impl<future<Args...>>::template result_type<Func>;
+
+SEASTAR_CONCEPT(
+template <typename Func, typename... Args>
+concept CanInvokeWhenAllSucceed = requires {
+ typename call_then_impl_result_type<Func, Args...>;
+};
+)
+
+template <typename Func>
+struct result_of_apply {
+ // no "type" member if not a function call signature or not a tuple
+};
+
+template <typename Func, typename... T>
+struct result_of_apply<Func (std::tuple<T...>)> : std::result_of<Func (T...)> {
+ // Let std::result_of determine the result if the input is a tuple
+};
+
+template <typename Func, typename... T>
+using result_of_apply_t = typename result_of_apply<Func, T...>::type;
+
}

template <typename Promise, typename... T>
@@ -1133,6 +1231,7 @@ template <typename... T>
class SEASTAR_NODISCARD future : private internal::future_base, internal::warn_variadic_future<(sizeof...(T) > 1)> {
future_state<T...> _state;
static constexpr bool copy_noexcept = future_state<T...>::copy_noexcept;
+ using call_then_impl = internal::call_then_impl<future>;
private:
// This constructor creates a future that is not ready but has no
// associated promise yet. The use case is to have a less flexible
@@ -1313,19 +1412,54 @@ public:
/// unless it has failed.
/// \return a \c future representing the return value of \c func, applied
/// to the eventual value of this future.
- template <typename Func, typename Result = futurize_t<std::result_of_t<Func(T&&...)>>>
- SEASTAR_CONCEPT( requires std::invocable<Func, T...> )
+ template <typename Func, typename Result = futurize_t<typename call_then_impl::template result_type<Func>>>
+ SEASTAR_CONCEPT( requires std::invocable<Func, T...> || internal::CanInvokeWhenAllSucceed<Func, T...>)
Result
then(Func&& func) noexcept {
+ // The implementation of then() is customized via the call_then_impl helper
+ // template, in order to special case the results of when_all_succeed().
+ // when_all_succeed() used to return a variadic future, which is deprecated, so
+ // now it returns a when_all_succeed_tuple, which we intercept in call_then_impl,
+ // and treat it as a variadic future.
#ifndef SEASTAR_TYPE_ERASE_MORE
- return then_impl(std::move(func));
+ return call_then_impl::run(*this, std::move(func));
#else
- return then_impl(noncopyable_function<Result (T&&...)>([func = std::forward<Func>(func)] (T&&... args) mutable {
+ using func_type = typename call_then_impl::template func_type<Func>;
+ return call_then_impl::run(*this, noncopyable_function<func_type>([func = std::forward<Func>(func)] (auto&&... args) mutable {
return futurize_invoke(func, std::forward<decltype(args)>(args)...);
}));
#endif
}

+ /// \brief Schedule a block of code to run when the future is ready, unpacking tuples.
+ ///
+ /// Schedules a function (often a lambda) to run when the future becomes
+ /// available. The function is called with the result of this future's
+ /// computation as parameters. The return value of the function becomes
+ /// the return value of then(), itself as a future; this allows then()
+ /// calls to be chained.
+ ///
+ /// This member function is only available is the payload is std::tuple;
+ /// The tuple elements are passed as individual arguments to `func`, which
+ /// must have the same arity as the tuple.
+ ///
+ /// If the future failed, the function is not called, and the exception
+ /// is propagated into the return value of then().
+ ///
+ /// \param func - function to be called when the future becomes available,
+ /// unless it has failed.
+ /// \return a \c future representing the return value of \c func, applied
+ /// to the eventual value of this future.
+ template <typename Func, typename Result = futurize_t<internal::result_of_apply_t<Func (T...)>>>
+ SEASTAR_CONCEPT( requires (sizeof...(T) == 1) && ::seastar::CanApplyTuple<Func, T...>)
+ Result
+ then_unpack(Func&& func) noexcept {
+ return then([func = std::forward<Func>(func)] (T&&... tuple) mutable {
+ // sizeof...(tuple) is required to be 1
+ return std::apply(std::forward<Func>(func), std::move(tuple)...);
+ });
+ }
+
private:

// Keep this simple so that Named Return Value Optimization is used.
@@ -1577,7 +1711,9 @@ public:
/// Converts the future into a no-value \c future<>, by
/// ignoring any result. Exceptions are propagated unchanged.
future<> discard_result() noexcept {
- return then([] (T&&...) {});
+ // We need the generic variadic lambda, below, because then() behaves differently
+ // when value_type is when_all_succeed_tuple
+ return then([] (auto&&...) {});
}

/// \brief Handle the exception carried by this future.
@@ -1684,6 +1820,8 @@ private:
friend future<U...> current_exception_as_future() noexcept;
template <typename... U, typename V>
friend void internal::set_callback(future<U...>&, V*) noexcept;
+ template <typename Future>
+ friend struct internal::call_then_impl;
/// \endcond
};

diff --git a/tests/perf/fair_queue_perf.cc b/tests/perf/fair_queue_perf.cc
--- a/tests/perf/fair_queue_perf.cc
+++ b/tests/perf/fair_queue_perf.cc
@@ -94,7 +94,7 @@ PERF_TEST_F(perf_fair_queue, contended_local)
});
});

- return when_all_succeed(std::move(invokers), std::move(collectors));
+ return when_all_succeed(std::move(invokers), std::move(collectors)).discard_result();
}

PERF_TEST_F(perf_fair_queue, contended_shared)
@@ -119,7 +119,7 @@ PERF_TEST_F(perf_fair_queue, contended_shared)
shared_fq.notify_requests_finished(seastar::fair_queue_ticket{pending_ack, pending_ack});
return make_ready_future<>();
});
- return when_all_succeed(std::move(invokers), std::move(collectors));
+ return when_all_succeed(std::move(invokers), std::move(collectors)).discard_result();
}
PERF_TEST_F(perf_fair_queue, contended_shared_amortized)
{
@@ -143,5 +143,5 @@ PERF_TEST_F(perf_fair_queue, contended_shared_amortized)
shared_fq.notify_requests_finished(seastar::fair_queue_ticket{pending_ack, pending_ack});
return make_ready_future<>();
});
- return when_all_succeed(std::move(invokers), std::move(collectors));
+ return when_all_succeed(std::move(invokers), std::move(collectors)).discard_result();
}
diff --git a/tests/unit/futures_test.cc b/tests/unit/futures_test.cc
--- a/tests/unit/futures_test.cc
+++ b/tests/unit/futures_test.cc
@@ -977,7 +977,7 @@ SEASTAR_TEST_CASE(test_when_all_succeed_functions) {
return when_all_succeed(f, [] {
throw 42;
return make_ready_future<>();
- }, later()).then_wrapped([] (future<int> res) {
+ }, later()).then_wrapped([] (auto res) { // type of `res` changes when SESTAR_API_LEVEL < 3
BOOST_REQUIRE(res.available());
BOOST_REQUIRE(res.failed());
res.ignore_ready_future();
@@ -1115,6 +1115,12 @@ SEASTAR_TEST_CASE(test_shared_future_with_timeout) {
});
}

+#if SEASTAR_API_LEVEL < 4
+#define THEN_UNPACK then
+#else
+#define THEN_UNPACK then_unpack
+#endif
+
SEASTAR_TEST_CASE(test_when_all_succeed_tuples) {
return seastar::when_all_succeed(
make_ready_future<>(),
@@ -1123,7 +1129,7 @@ SEASTAR_TEST_CASE(test_when_all_succeed_tuples) {
make_ready_future<>(),
make_ready_future<int, sstring>(84, "hi"),
make_ready_future<bool>(true)
- ).then([] (sstring msg, int v, std::tuple<int, sstring> t, bool b) {
+ ).THEN_UNPACK([] (sstring msg, int v, std::tuple<int, sstring> t, bool b) {
BOOST_REQUIRE_EQUAL(msg, "hello world");
BOOST_REQUIRE_EQUAL(v, 42);
BOOST_REQUIRE_EQUAL(std::get<0>(t), 84);
@@ -1135,7 +1141,7 @@ SEASTAR_TEST_CASE(test_when_all_succeed_tuples) {
make_ready_future<sstring>("hello world"),
make_exception_future<int>(43),
make_ready_future<>()
- ).then([] (sstring, int) {
+ ).THEN_UNPACK([] (sstring, int) {
BOOST_FAIL("shouldn't reach");
return false;
}).handle_exception([] (auto excp) {
@@ -1346,3 +1352,15 @@ future<> func1() {
SEASTAR_THREAD_TEST_CASE(test_backtracing) {
func1().get();
}
+
+SEASTAR_THREAD_TEST_CASE(test_then_unpack) {
+ make_ready_future<std::tuple<>>().then_unpack([] () {
+ BOOST_REQUIRE(true);
+ }).get();
+ make_ready_future<std::tuple<int>>(std::tuple<int>(1)).then_unpack([] (int x) {
+ BOOST_REQUIRE(x == 1);
+ }).get();
+ make_ready_future<std::tuple<int, long>>(std::tuple<int, long>(1, 2)).then_unpack([] (int x, long y) {
+ BOOST_REQUIRE(x == 1 && y == 2);
+ }).get();
+}