diff --git a/src/builtin_eval.cpp b/src/builtin_eval.cpp index e9b0512b3..73941e89e 100644 --- a/src/builtin_eval.cpp +++ b/src/builtin_eval.cpp @@ -74,12 +74,12 @@ maybe_t builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **arg // deallocate to close. ios.clear(); if (stdout_fill) { - std::shared_ptr output = io_bufferfill_t::finish(std::move(stdout_fill)); - streams.out.append_narrow_buffer(output->take_buffer()); + separated_buffer_t output = io_bufferfill_t::finish(std::move(stdout_fill)); + streams.out.append_narrow_buffer(std::move(output)); } if (stderr_fill) { - std::shared_ptr errput = io_bufferfill_t::finish(std::move(stderr_fill)); - streams.err.append_narrow_buffer(errput->take_buffer()); + separated_buffer_t errput = io_bufferfill_t::finish(std::move(stderr_fill)); + streams.err.append_narrow_buffer(std::move(errput)); } return status; } diff --git a/src/exec.cpp b/src/exec.cpp index 9677fb591..c4a235f43 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -690,8 +690,8 @@ static launch_result_t exec_block_or_func_process(parser_t &parser, const std::s // Remove our write pipe and forget it. This may close the pipe, unless another thread has // claimed it (background write) or another process has inherited it. io_chain.remove(block_output_bufferfill); - auto block_output_buffer = io_bufferfill_t::finish(std::move(block_output_bufferfill)); - buffer_contents = block_output_buffer->take_buffer().newline_serialized(); + buffer_contents = + io_bufferfill_t::finish(std::move(block_output_bufferfill)).newline_serialized(); } run_internal_process_or_short_circuit(parser, j, p, std::move(buffer_contents), @@ -1110,7 +1110,7 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, return STATUS_CMD_ERROR; } eval_res_t eval_res = parser.eval(cmd, io_chain_t{bufferfill}, job_group, block_type_t::subst); - separated_buffer_t buffer = io_bufferfill_t::finish(std::move(bufferfill))->take_buffer(); + separated_buffer_t buffer = io_bufferfill_t::finish(std::move(bufferfill)); if (buffer.discarded()) { *break_expand = true; return STATUS_READ_TOO_MUCH; diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 7bab36104..89fbb97aa 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -1302,7 +1302,7 @@ static void test_1_cancellation(const wchar_t *src) { pthread_kill(thread, SIGINT); }); eval_res_t res = parser_t::principal_parser().eval(src, io_chain_t{filler}); - auto buffer = io_bufferfill_t::finish(std::move(filler))->take_buffer(); + separated_buffer_t buffer = io_bufferfill_t::finish(std::move(filler)); if (buffer.size() != 0) { err(L"Expected 0 bytes in out_buff, but instead found %lu bytes, for command %ls\n", buffer.size(), src); diff --git a/src/io.cpp b/src/io.cpp index afbca1d91..cb951ea47 100644 --- a/src/io.cpp +++ b/src/io.cpp @@ -136,7 +136,7 @@ void io_buffer_t::begin_filling(autoclose_fd_t fd) { this->item_id_ = fd_monitor().add(std::move(item)); } -void io_buffer_t::complete_background_fillthread() { +separated_buffer_t io_buffer_t::complete_background_fillthread_and_take_buffer() { // Mark that our fillthread is done, then wake it up. ASSERT_IS_MAIN_THREAD(); assert(fillthread_running() && "Should have a fillthread"); @@ -147,7 +147,13 @@ void io_buffer_t::complete_background_fillthread() { // Wait for the fillthread to fulfill its promise, and then clear the future so we know we no // longer have one. fillthread_waiter_.wait(); - fillthread_waiter_ = {}; + fillthread_waiter_ = std::future{}; + + // Return our buffer, transferring ownership. + auto locked_buff = buffer_.acquire(); + separated_buffer_t result = std::move(*locked_buff); + locked_buff->clear(); + return result; } shared_ptr io_bufferfill_t::create(const fd_set_t &conflicts, size_t buffer_limit, @@ -173,7 +179,7 @@ shared_ptr io_bufferfill_t::create(const fd_set_t &conflicts, s return std::make_shared(target, std::move(pipes->write), buffer); } -std::shared_ptr io_bufferfill_t::finish(std::shared_ptr &&filler) { +separated_buffer_t io_bufferfill_t::finish(std::shared_ptr &&filler) { // The io filler is passed in. This typically holds the only instance of the write side of the // pipe used by the buffer's fillthread (except for that side held by other processes). Get the // buffer out of the bufferfill and clear the shared_ptr; this will typically widow the pipe. @@ -181,8 +187,7 @@ std::shared_ptr io_bufferfill_t::finish(std::shared_ptrbuffer(); filler.reset(); - buffer->complete_background_fillthread(); - return buffer; + return buffer->complete_background_fillthread_and_take_buffer(); } io_buffer_t::~io_buffer_t() { diff --git a/src/io.h b/src/io.h index 59807cd81..1504509d7 100644 --- a/src/io.h +++ b/src/io.h @@ -75,7 +75,7 @@ class separated_buffer_t { void operator=(const separated_buffer_t &) = delete; /// We may be moved. - /// Note this leaves the moved-from value in a bogus state, until clear() is called on it. + /// Note this leaves the moved-from value in a bogus state until clear() is called on it. separated_buffer_t(separated_buffer_t &&rhs) = default; separated_buffer_t &operator=(separated_buffer_t &&) = default; @@ -303,7 +303,7 @@ class io_bufferfill_t final : public io_data_t { /// Reset the receiver (possibly closing the write end of the pipe), and complete the fillthread /// of the buffer. \return the buffer. - static std::shared_ptr finish(std::shared_ptr &&filler); + static separated_buffer_t finish(std::shared_ptr &&filler); }; class output_stream_t; @@ -316,16 +316,6 @@ public: ~io_buffer_t(); - /// Take the underlying buffer, transferring ownership to the caller. - /// This should only be called after the fillthread operation is complete. - separated_buffer_t take_buffer() { - assert(!fillthread_running() && "Cannot access buffer during background fill"); - auto locked_buff = buffer_.acquire(); - separated_buffer_t result = std::move(*locked_buff); - locked_buff->clear(); - return result; - } - /// Append a string to the buffer. void append(std::string &&str, separation_type_t type = separation_type_t::inferred) { buffer_.acquire()->append(std::move(str), type); @@ -343,8 +333,8 @@ public: /// Begin the fill operation, reading from the given fd in the background. void begin_filling(autoclose_fd_t readfd); - /// End the background fillthread operation. - void complete_background_fillthread(); + /// End the background fillthread operation, and return the buffer, transferring ownership. + separated_buffer_t complete_background_fillthread_and_take_buffer(); /// Helper to return whether the fillthread is running. bool fillthread_running() const { return fillthread_waiter_.valid(); }