[PATCH] rpc: Take care of client::send() future in send_helper

[Pavel Emelyanov]

The send_helper::operator() waits for two futures to resolve -- the
sending one and receiving-the-response one. The latter future result
is then propagated back to caller.

However, the former future may also resolve into exception sometimes,
and this exception should be correctly short-circuited.

Nowadays this doesn't really happen, because .send() only resolved
into exception if it noticed error on entry and the helper didn't let
this happen by checking the error early itself. After 8c82d889 (rpc:
Send messages instantly) exceptions started popping up asynchronously.

tests: scylla.unit(dev), rpc(dev)

Signed-off-by: Pavel Emelyanov <xe...@scylladb.com>
---
include/seastar/rpc/rpc_impl.hh | 1 +
tests/unit/rpc_test.cc | 5 +++++
2 files changed, 6 insertions(+)

diff --git a/include/seastar/rpc/rpc_impl.hh b/include/seastar/rpc/rpc_impl.hh
index e74e2e2d..2a1509d1 100644
--- a/include/seastar/rpc/rpc_impl.hh
+++ b/include/seastar/rpc/rpc_impl.hh
@@ -478,6 +478,7 @@ auto send_helper(MsgType xt, signature<Ret (InArgs...)> xsig) {
// prepare reply handler, if return type is now_wait_type this does nothing, since no reply will be sent
using wait = wait_signature_t<Ret>;
return when_all(dst.send(std::move(data), timeout, cancel), wait_for_reply<Serializer>(wait(), timeout, cancel, dst, msg_id, sig)).then([] (auto r) {
+ std::get<0>(r).ignore_ready_future();
return std::move(std::get<1>(r)); // return future of wait_for_reply
});
}
diff --git a/tests/unit/rpc_test.cc b/tests/unit/rpc_test.cc
index 134d7e5c..69e6d597 100644
--- a/tests/unit/rpc_test.cc
+++ b/tests/unit/rpc_test.cc
@@ -365,7 +365,12 @@ SEASTAR_TEST_CASE(test_rpc_connect_abort) {
test_rpc_proto::client c1(env.proto(), {}, env.make_socket(), ipv4_addr());
env.register_handler(1, []() { return make_ready_future<>(); }).get();
auto f = env.proto().make_client<void ()>(1);
+ auto fut = f(c1);
c1.stop().get0();
+ try {
+ fut.get0();
+ BOOST_REQUIRE(false);
+ } catch (...) {}
try {
f(c1).get0();
BOOST_REQUIRE(false);
--
2.20.1

[Commit Bot]

From: Pavel Emelyanov' via seastar-dev <seast...@googlegroups.com>
Committer: Avi Kivity <a...@scylladb.com>
Branch: master

rpc: Take care of client::send() future in send_helper

The send_helper::operator() waits for two futures to resolve -- the
sending one and receiving-the-response one. The latter future result
is then propagated back to caller.

However, the former future may also resolve into exception sometimes,
and this exception should be correctly short-circuited.

Nowadays this doesn't really happen, because .send() only resolved
into exception if it noticed error on entry and the helper didn't let
this happen by checking the error early itself. After 8c82d889 (rpc:
Send messages instantly) exceptions started popping up asynchronously.

tests: scylla.unit(dev), rpc(dev)

Signed-off-by: Pavel Emelyanov <xe...@scylladb.com>

Message-Id: <20220621164747...@scylladb.com>

---
diff --git a/include/seastar/rpc/rpc_impl.hh b/include/seastar/rpc/rpc_impl.hh

--- a/include/seastar/rpc/rpc_impl.hh
+++ b/include/seastar/rpc/rpc_impl.hh
@@ -478,6 +478,7 @@ auto send_helper(MsgType xt, signature<Ret (InArgs...)> xsig) {
// prepare reply handler, if return type is now_wait_type this does nothing, since no reply will be sent
using wait = wait_signature_t<Ret>;
return when_all(dst.send(std::move(data), timeout, cancel), wait_for_reply<Serializer>(wait(), timeout, cancel, dst, msg_id, sig)).then([] (auto r) {
+ std::get<0>(r).ignore_ready_future();
return std::move(std::get<1>(r)); // return future of wait_for_reply
});
}
diff --git a/tests/unit/rpc_test.cc b/tests/unit/rpc_test.cc

[Avi Kivity]

Isn't it better to make the two sequential?


when_all() adds an extra allocation + task to collect things and create
a result tuple. Starting the receive earlier won't make it ever happen
before send() completes (and in any case we wait for both).

[Pavel Emelyanov]

The wait_for_reply<>() is sometimes instantly ready future. In that case, will

return send().then([] { return make_ready_future<>(); })

still be better (not worse at least) than

return when_all(send(), make_ready_future<>());

?

[Avi Kivity]

Yes.


First, usually send() returns immediately, and then it doesn't need to
allocate anything or create any task. So the cost of chaining is the
cost of the second call (which can be 0 for no_wait).


If send() blocks, then we'll need to allocate a continuation for the
second task. But when_all() also allocates a continuation in case send()
blocks, in order to convert its result into a tuple future. So it ends
up the same.


If both block, then when_all() allocates one continuation and has two
tasks - one to collect each result.


If neither block, then they are the same, but probably more instructions
for tuple mangling.