[CORE-2814]: utils/file_io: fix double closing of ss::file in write_fully #18303
Conversation
ss::output_stream internally keeps track of in-flight exceptions. In the
close() method, it will close the underlying ss::file and rethrow the
exception.
ss::with_file_close_on_failure will too close the underlying ss::file if
the future fails, this can result in a double close triggering an
assertion like
```
seastar-prefix/src/seastar/include/seastar/core/future.hh:1917: future<T> seastar::promise<>::get_future() [T = void]: Assertion `!this->_future && this->_state && !this->_task' failed
```
This unit test shows how this assert could be triggered, if
ss::output_stream as an active exception:
```
SEASTAR_THREAD_TEST_CASE(test_with_file_close_on_failure) {
auto flags = ss::open_flags::rw | ss::open_flags::create
| ss::open_flags::truncate;
ss::with_file_close_on_failure(
ss::open_file_dma("/tmp/tmp.YuupbuphlR", flags),
[](ss::file f) mutable {
return f.close().then([] { throw "any value"; });
})
.get();
}
```
This commit moves out ss::output_stream::close() from
ss::with_file_close_on_failure.
The method is coroutinized for clarity.
|
new failures in https://buildkite.com/redpanda/redpanda/builds/48818#018f57e6-4fda-47d6-8ebe-2bb7d25c6bd8: new failures in https://buildkite.com/redpanda/redpanda/builds/48818#018f57f7-26f0-4de3-b203-6259257cbe13: new failures in https://buildkite.com/redpanda/redpanda/builds/48818#018f57f7-26f4-491e-a8b4-e1f710eb6a34: new failures in https://buildkite.com/redpanda/redpanda/builds/48818#018f57e6-4fd8-4008-95ad-27cc1ccc83c5: |
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48818#018f57f7-26f0-4de3-b203-6259257cbe13 |
|
failure is #14139 |
to test edit: " seastar - io_submit: Invalid argument" would need to make the function more unit friendly |
| }); | ||
| co_await write_iobuf_to_output_stream(std::move(buf), out).finally([&out] { | ||
| return out.close(); | ||
| }); |
There was a problem hiding this comment.
looks safe to me.
nit: I don't know if there is an idiomatic use of with_file_close_on_failure, but it seems like it would be:
auto f = co_await open();
co_await with_file_close_on_failure(
// all the things that could fail and we don't want hand crafted error handling.
make_file_output_stream.then(write_io_to_output_stream)...
);
co_await f.close();
There was a problem hiding this comment.
Ye, you just need to be sure that no path inside the lambda closes the file.
For the pr i copied how the function is used in the unit tests under seastar
|
failures are #14139 |
|
/backport v24.1.x |
|
/backport v23.3.x |
|
/backport v23.2.x |
ss::output_stream internally keeps track of in-flight exceptions. In the close() method, it will close the underlying ss::file and rethrow the exception.
ss::with_file_close_on_failure will too close the underlying ss::file if the future fails, this can result in a double close triggering an assertion like
This unit test shows how this assert could be triggered, if ss::output_stream as an active exception:
This commit moves out ss::output_stream::close() from ss::with_file_close_on_failure.
The method is coroutinized for clarity.
Fixes CORE-2814
Fixes #18286
Backports Required
Release Notes
Bug Fixes