From 338d587f2af2249cf0333163d1c2a97b3dc32397 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Thu, 31 Mar 2022 20:36:29 -0700 Subject: [PATCH] Correct bug causing early teardown of fd_monitor fd_monitor is used when an external command pipes into a buffer, e.g. for command substitutions. It monitors the read end of the external command's pipe in the background, and fills the buffer as data arrives. fd_monitor is multiplexed, so multiple buffers can be monitored at once by a single thread. It may happen that there's no active buffer fill; in this case fd_monitor wants to keep its thread alive for a little bit in case a new one arrives. This is useful for e.g. handling loops where you run the same command multiple times. However there was a bug due to a refactoring which caused fd_monitor to exit too aggressively. This didn't affect correctness but it meant more thread creation and teardown. Fix this; this improves the aliases.fish benchmark by about 20 msec. No need to changelog this IMO. --- src/fd_monitor.cpp | 19 ++++++++++--------- src/fd_monitor.h | 2 +- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/fd_monitor.cpp b/src/fd_monitor.cpp index 25b745476..e5c40f110 100644 --- a/src/fd_monitor.cpp +++ b/src/fd_monitor.cpp @@ -117,7 +117,7 @@ void fd_monitor_t::run_in_background() { for (;;) { // Poke any items that need it. if (!pokelist.empty()) { - this->poke_in_background(std::move(pokelist)); + this->poke_in_background(pokelist); pokelist.clear(); } @@ -136,16 +136,15 @@ void fd_monitor_t::run_in_background() { timeout_usec = std::min(timeout_usec, item.usec_remaining(now)); } - // If we have only one item, it means that we are not actively monitoring any fds other than - // our self-pipe. In this case we wish to allow the thread to exit, but after a time, so we + // If we have no items, then we wish to allow the thread to exit, but after a time, so we // aren't spinning up and tearing down the thread repeatedly. - // Set a timeout of 16 msec; if nothing becomes readable by then we will exit. + // Set a timeout of 256 msec; if nothing becomes readable by then we will exit. // We refer to this as the wait-lap. - bool is_wait_lap = (items_.size() == 1); + bool is_wait_lap = (items_.size() == 0); if (is_wait_lap) { assert(timeout_usec == fd_monitor_item_t::kNoTimeout && "Should not have a timeout on wait-lap"); - timeout_usec = 16 * kUsecPerMsec; + timeout_usec = 256 * kUsecPerMsec; } // Call select(). @@ -170,7 +169,8 @@ void fd_monitor_t::run_in_background() { // Handle any changes if the change signaller was set. Alternatively this may be the wait // lap, in which case we might want to commit to exiting. - if (fds.test(change_signal_fd) || is_wait_lap) { + bool change_signalled = fds.test(change_signal_fd); + if (change_signalled || is_wait_lap) { // Clear the change signaller before processing incoming changes. change_signaller_.try_consume(); auto data = data_.acquire(); @@ -185,7 +185,8 @@ void fd_monitor_t::run_in_background() { pokelist = std::move(data->pokelist); data->pokelist.clear(); - if (data->terminate || (is_wait_lap && items_.empty())) { + if (data->terminate || + (is_wait_lap && items_.empty() && pokelist.empty() && !change_signalled)) { // Maybe terminate is set. // Alternatively, maybe we had no items, waited a bit, and still have no items. // It's important to do this while holding the lock, otherwise we race with new @@ -199,7 +200,7 @@ void fd_monitor_t::run_in_background() { } } -void fd_monitor_t::poke_in_background(poke_list_t pokelist) { +void fd_monitor_t::poke_in_background(const poke_list_t &pokelist) { ASSERT_IS_BACKGROUND_THREAD(); auto poker = [&pokelist](fd_monitor_item_t &item) { int fd = item.fd.fd(); diff --git a/src/fd_monitor.h b/src/fd_monitor.h index 7ac71da25..da21f41da 100644 --- a/src/fd_monitor.h +++ b/src/fd_monitor.h @@ -110,7 +110,7 @@ class fd_monitor_t { // Poke items in the pokelist, removing any items that close their FD. // The pokelist is consumed after this. // This is only called in the background thread. - void poke_in_background(poke_list_t pokelist); + void poke_in_background(const poke_list_t &pokelist); // The list of items to monitor. This is only accessed on the background thread. item_list_t items_{};