From 9c238385f06da1f8c251a4f4a616e1eaa94119ef Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 6 Feb 2021 14:39:07 -0800 Subject: [PATCH] Fix binary_semaphore_t under non-Linux TSan Under non-Linux builds, binary_semaphore is implemented with a self-pipe. When TSan is active we mark the pipe as non-blocking as TSan cannot interrupt read (but can interrupt select). However we weren't properly testing for EAGAIN leading to an assertion failure. Allow looping on EAGAIN. --- src/topic_monitor.cpp | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/topic_monitor.cpp b/src/topic_monitor.cpp index 83fb1ab25..5711171bb 100644 --- a/src/topic_monitor.cpp +++ b/src/topic_monitor.cpp @@ -56,6 +56,7 @@ binary_semaphore_t::binary_semaphore_t() : sem_ok_(false) { } binary_semaphore_t::~binary_semaphore_t() { + // We never use sem_t on Mac. The #ifdef avoids deprecation warnings. #ifndef __APPLE__ if (sem_ok_) (void)sem_destroy(&sem_); #endif @@ -92,21 +93,22 @@ void binary_semaphore_t::wait() { if (res < 0) die(L"sem_wait"); } else { int fd = pipes_.read.fd(); -#ifdef TOPIC_MONITOR_TSAN_WORKAROUND - // Under tsan our notifying pipe is non-blocking, so we would busy-loop on the read() call - // until data is available (that is, fish would use 100% cpu while waiting for processes). - // The select prevents that. - fd_set fds; - FD_ZERO(&fds); - FD_SET(fd, &fds); - (void)select(fd + 1, &fds, nullptr, nullptr, nullptr /* timeout */); -#endif // We must read exactly one byte. for (;;) { +#ifdef TOPIC_MONITOR_TSAN_WORKAROUND + // Under tsan our notifying pipe is non-blocking, so we would busy-loop on the read() + // call until data is available (that is, fish would use 100% cpu while waiting for + // processes). The select prevents that. + fd_set fds; + FD_ZERO(&fds); + FD_SET(fd, &fds); + (void)select(fd + 1, &fds, nullptr, nullptr, nullptr /* timeout */); +#endif uint8_t ignored; auto amt = read(fd, &ignored, sizeof ignored); if (amt == 1) break; - if (amt < 0 && errno != EINTR) die(L"read"); + // EAGAIN should only be returned in TSan case. + if (amt < 0 && errno != EINTR && errno != EAGAIN) die(L"read"); } } }