Rethrow on unknown exceptions in fetch handler #16554
Conversation
There was a problem hiding this comment.
Then doesn't this raise again the possible problem with exception safety and gates? As I recall we "proved" that one function couldn't throw an exception after the gate was entered in part because the main function that could throw was surrounded by log_exceptions.
This wasn't clear to me: is the "catch present in the handler" the one in the fetch handler |
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44926#018d8c46-67f6-43b6-8c6e-dbea4de4aa52 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45214#018dce33-aba6-4769-97c8-39816bab5d92 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45214#018dcfa4-4b8a-44d3-96c7-3c4c8ac2c095 |
ballard26
force-pushed
the
fetch-impl-ll
branch
from
9578964
to
5287de6
Compare
Exceptions from outside the Kafka handler context can bubble up to the catch present in the handler. This seems to be the way some subsystems communicate issues with the requests. There is no current listing of what exceptions subsystems may throw, if/how to recover from these exceptions, or if a fetch should end as a result of the exception. Hence for the time being the fetch impl will revert to the behavior of further bubbling up unknown exceptions to outside of the handler context.
ballard26
force-pushed
the
fetch-impl-ll
branch
from
5287de6
to
a09d160
Compare
|
/backport v23.3.x |
|
|
||
| // Send abort signal to workers and wait for all workers to end before | ||
| // returning. | ||
| co_await abort_workers().finally( | ||
| [this] { return _workers_gate.close(); }); | ||
|
|
||
| if (_thrown_exception) { | ||
| std::rethrow_exception(_thrown_exception); |
There was a problem hiding this comment.
@ballard26 should this eat the exception pointer like std::rethrow_exception(std::exchange(_thrown_exception, {})); or is this sort of the end-of-the-road for the handler and we'll not be examining _thrown_exception again in the same context?
There was a problem hiding this comment.
We could, not sure what we'd gain by doing so. And at this point the exception thrown isn't caught by the fetch executor so during the stack unwinding the fetch executor and hence _thrown_exception will be freed so it shouldn't matter. Beyond that std::exception_ptr can roughly be treated as a shared pointer according to the standard and should remain valid even if rethrown.
There was a problem hiding this comment.
executor so during the stack unwinding the fetch executor and hence _thrown_exception will be freed so it shouldn't matter.
Ah, right. Thanks!
Exceptions from outside the Kafka handler context can bubble up to the catch present in the handler. This seems to be the way some subsystems communicate issues with the requests. There is no current listing of what exceptions subsystems may throw, if/how to recover from these exceptions, or if a fetch should end as a result of the exception. Hence for the time being the fetch impl will revert to the behavior of further bubbling up unknown exceptions to outside of the handler context.
Fixes #16532
Backports Required
Release Notes