[PATCH] rpc stream: do not abort stream queue if stream connection was closed without error

[Gleb Natapov]

queue::abort() drops all queued packets and report an error to a
consumer. If stream connection completes normally we want the consumer
to get all the data without errors, so abort the queue only in case of
an error. Otherwise the queue will wait to be consumed. Since closing
the stream involves sending a special EOS packet the consumer should not
hang since the queue will not be empty.

Fixes: #2612

diff --git a/src/rpc/rpc.cc b/src/rpc/rpc.cc
index 5b859aed117..8844260eb97 100644
--- a/src/rpc/rpc.cc
+++ b/src/rpc/rpc.cc
@@ -1020,9 +1020,9 @@ namespace rpc {
log_exception(*this, log_level::debug, "fail to connect", ep);
}
}
+ _stream_queue.abort(ep);
}
_error = true;
- _stream_queue.abort(std::make_exception_ptr(stream_closed()));
return stop_send_loop(ep).then_wrapped([this] (future<> f) {
f.ignore_ready_future();
_outstanding.clear();
@@ -1241,10 +1241,10 @@ future<> server::connection::send_unknown_verb_reply(std::optional<rpc_clock_typ
ep = f.get_exception();
log_exception(*this, log_level::error,
format("server{} connection dropped", is_stream() ? " stream" : "").c_str(), ep);
+ _stream_queue.abort(ep);
}
_fd.shutdown_input();
_error = true;
- _stream_queue.abort(std::make_exception_ptr(stream_closed()));
return stop_send_loop(ep).then_wrapped([this] (future<> f) {
f.ignore_ready_future();
get_server()._conns.erase(get_connection_id());
--
Gleb.

[Kamil Braun]

Pavel please review

--
You received this message because you are subscribed to the Google Groups "seastar-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev...@googlegroups.com.
To view this discussion visit https://groups.google.com/d/msgid/seastar-dev/Z4PE4w3Oah2oCC2u%40scylladb.com.

[Tomasz Grabiec]

Looks good, but the unit test is missing.

On Sun, Jan 12, 2025 at 2:34 PM 'Gleb Natapov' via seastar-dev <seast...@googlegroups.com> wrote:

[Gleb Natapov]

On Tue, Jan 14, 2025 at 09:59:56PM +0100, Tomasz Grabiec wrote:
> Looks good, but the unit test is missing.
>

That's not easy to write a reliable reproducer without error injection
which, iirc, we do not have in seastar. Without error injection the problem
only happens in debug mode since continuation re-ordering is needed to
hit it and even then it was very rare in the scylladb reproducer that we
have.

--
Gleb.

[Tomasz Grabiec]

On Wed, Jan 15, 2025 at 9:21 AM Gleb Natapov <gl...@scylladb.com> wrote:

On Tue, Jan 14, 2025 at 09:59:56PM +0100, Tomasz Grabiec wrote:
> Looks good, but the unit test is missing.
>

That's not easy to write a reliable reproducer without error injection
which, iirc, we do not have in seastar. Without error injection the problem
only happens in debug mode since continuation re-ordering is needed to
hit it and even then it was very rare in the scylladb reproducer that we
have.

Ok, merged.