Fix rpc server exception handling #2917
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
emaxerrno
reviewed
Nov 9, 2021
src/v/rpc/simple_protocol.cc
Outdated
| @@ -33,6 +33,9 @@ struct server_context_impl final : streaming_context { | |||
| ~server_context_impl() override { res.probe().request_completed(); } | |||
| const header& get_header() const final { return hdr; } | |||
| void signal_body_parse() final { pr.set_value(); } | |||
| void body_parse_exception(const std::exception_ptr& e) final { | |||
| pr.set_exception(e); | |||
There was a problem hiding this comment.
Exception ptr can be copied. Don’t you want to move the exception?
There was a problem hiding this comment.
what is the point of std::move(e) here? exception_ptr is trivially copyable
There was a problem hiding this comment.
exception_ptr has semantics similar to shared_ptr, there is no need to copy it.
Data are always set together with header. We may leverage that fact and create appropriate constructor. Signed-off-by: Michal Maslanka <michal@vectorized.io>
Our RPC implementation support propagating server errors to the client. For this we use a `meta` field in RPC response header. The status that is included into `header::meta` field indicates whether the request is successful or its execution failed on the server side. When status indicates an error response body is empty. Fixed an error caused by the fact that we tried to parse response body before we checked the status flag. This caused parsing error and broken promise exception on the client side. Signed-off-by: Michal Maslanka <michal@vectorized.io>
Added field to response of a service that is supposed to thrown an exception during execution. This way we trigger situation in which response that contains error in header has different payload size than successful response. Signed-off-by: Michal Maslanka <michal@vectorized.io>
mmaslankaprv
force-pushed
the
fix-rpc-server-exception-handling
branch
from
66f9a18
to
58507ca
Compare
dotnwat
reviewed
Nov 9, 2021
mmaslankaprv
force-pushed
the
fix-rpc-server-exception-handling
branch
from
58507ca
to
82a9c2f
Compare
mmaslankaprv
force-pushed
the
fix-rpc-server-exception-handling
branch
from
82a9c2f
to
fc51875
Compare
Streaming context promise must be set with exception when rpc body parsing failed. This way we will forward exception to rpc transport read dispatch loop. Not setting an exception in body parsing promise would lead to `broken_promise` exception since the promise would be deleted before setting its value Signed-off-by: Michal Maslanka <michal@vectorized.io>
mmaslankaprv
force-pushed
the
fix-rpc-server-exception-handling
branch
from
fc51875
to
6332a91
Compare
dotnwat
approved these changes
Nov 10, 2021
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cover letter
Our RPC implementation support propagating server errors to the client.
For this we use a
metafield in RPC response header. The status thatis included into
header::metafield indicates whether the request issuccessful or its execution failed on the server side. When status
indicates an error response body is empty.
Fixed an error caused by the fact that we tried to parse response body
before we checked the status flag. This caused parsing error and broken
promise exception on the client side.