io_buffer_t to store a promise, not a future, to satisfy TSan

io_buffer_t is a buffer that fills itself by reading from a file
descriptor (typically a pipe). When the file descriptor is widowed, the
operation completes, and it reports completion by marking a
`std::promise<void>`. The "main thread" waits for this by waiting on the
promise's future. However TSan was reporting that the future's destructor
races with its promise's wait method. It's not obvious if this is valid,
but we can fix it by keeping the promise alive until the io_buffer_t is
deallocated.

This fixes the TSan issues reported under
`complete_background_fillthread_and_take_buffer` for #7681 (but there
are other unresolved issues).
This commit is contained in:
ridiculousfish 2021-02-06 13:21:36 -08:00
parent 432f005859
commit 98b0ef532f
2 changed files with 10 additions and 11 deletions

View File

@ -95,12 +95,11 @@ void io_buffer_t::begin_filling(autoclose_fd_t fd) {
// indicates that the command substitution is done); in this case we will read until we get
// EAGAIN and then give up.
// Construct a promise that can go into our background thread.
// Construct a promise. We will fulfill it in our fill thread, and wait for it in
// complete_background_fillthread(). Note that TSan complains if the promise's dtor races with
// the future's call to wait(), so we store the promise, not just its future (#7681).
auto promise = std::make_shared<std::promise<void>>();
// Get the future associated with our promise.
// Note this should only ever be called once.
fillthread_waiter_ = promise->get_future();
this->fill_waiter_ = promise;
// Run our function to read until the receiver is closed.
// It's OK to capture 'this' by value because 'this' waits for the promise in its dtor.
@ -146,8 +145,8 @@ separated_buffer_t io_buffer_t::complete_background_fillthread_and_take_buffer()
// 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_ = std::future<void>{};
fill_waiter_->get_future().wait();
fill_waiter_.reset();
// Return our buffer, transferring ownership.
auto locked_buff = buffer_.acquire();

View File

@ -314,7 +314,7 @@ public:
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(); }
bool fillthread_running() const { return fill_waiter_.get() != nullptr; }
/// Buffer storing what we have read.
owning_lock<separated_buffer_t> buffer_;
@ -322,9 +322,9 @@ public:
/// Atomic flag indicating our fillthread should shut down.
relaxed_atomic_bool_t shutdown_fillthread_{false};
/// The future allowing synchronization with the background fillthread, if the fillthread is
/// running. The fillthread fulfills the corresponding promise when it exits.
std::future<void> fillthread_waiter_{};
/// A promise, allowing synchronization with the background fill operation.
/// The operation has a reference to this as well, and fulfills this promise when it exits.
std::shared_ptr<std::promise<void>> fill_waiter_{};
/// The item id of our background fillthread fd monitor item.
uint64_t item_id_{0};