From 98b0ef532f3a7faf81d11679280834340e2149fa Mon Sep 17 00:00:00 2001
From: ridiculousfish <rf@fishshell.com>
Date: Sat, 6 Feb 2021 13:21:36 -0800
Subject: [PATCH] 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).
---
 src/io.cpp | 13 ++++++-------
 src/io.h   |  8 ++++----
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/src/io.cpp b/src/io.cpp
index 742f6bf41..f8f91d648 100644
--- a/src/io.cpp
+++ b/src/io.cpp
@@ -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();
diff --git a/src/io.h b/src/io.h
index bb8d38e2d..e29eec16d 100644
--- a/src/io.h
+++ b/src/io.h
@@ -314,7 +314,7 @@ class io_buffer_t {
     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 @@ class io_buffer_t {
     /// 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};