From ced56d492ff5ccca616603f145933b8dc39fc603 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 6 Feb 2021 22:05:43 -0800 Subject: [PATCH] Disable iothread pool wait-around under TSan The iothread pool has a feature where, if the thread is emptied, some threads will choose to wait around in case new work appears, up to a certain amount of time (500 msec). This prevents thrashing where new threads are rapidly created and destroyed as the user types. This is implemented via `std::condition_variable::wait_for`. However this function is not properly instrumented under Thread Sanitizer (see https://github.com/google/sanitizers/issues/1259) so TSan reports false positives. Just disable this feature under TSan. --- src/iothread.cpp | 21 ++++++++++++++++++++- src/topic_monitor.cpp | 4 ++-- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/iothread.cpp b/src/iothread.cpp index b5e145db4..a841250de 100644 --- a/src/iothread.cpp +++ b/src/iothread.cpp @@ -31,8 +31,26 @@ // which is too low, even tho the system can handle more than 64 threads. #define IO_MAX_THREADS 1024 +// iothread has a thread pool. Sometimes there's no work to do, but extant threads wait around for a +// while (on a condition variable) in case new work comes soon. However condition variables are not +// properly instrumented with Thread Sanitizer, so it fails to recognize when our mutex is locked. +// See https://github.com/google/sanitizers/issues/1259 +// When using TSan, disable the wait-around feature. +#if defined(__has_feature) +#if __has_feature(thread_sanitizer) +#define IOTHREAD_TSAN_WORKAROUND 1 +#endif +#endif +#ifdef __SANITIZE_THREAD__ +#define IOTHREAD_TSAN_WORKAROUND 1 +#endif + // The amount of time an IO thread many hang around to service requests, in milliseconds. +#ifdef IOTHREAD_TSAN_WORKAROUND +#define IO_WAIT_FOR_WORK_DURATION_MS 0 +#else #define IO_WAIT_FOR_WORK_DURATION_MS 500 +#endif using void_function_t = std::function; @@ -173,7 +191,8 @@ maybe_t thread_pool_t::dequeue_work_or_commit_to_exit() { auto data = this->req_data.acquire(); // If the queue is empty, check to see if we should wait. // We should wait if our exiting would drop us below the soft min. - if (data->request_queue.empty() && data->total_threads == this->soft_min_threads) { + if (data->request_queue.empty() && data->total_threads == this->soft_min_threads && + IO_WAIT_FOR_WORK_DURATION_MS > 0) { data->waiting_threads += 1; this->queue_cond.wait_for(data.get_lock(), std::chrono::milliseconds(IO_WAIT_FOR_WORK_DURATION_MS)); diff --git a/src/topic_monitor.cpp b/src/topic_monitor.cpp index 5711171bb..96350bd4c 100644 --- a/src/topic_monitor.cpp +++ b/src/topic_monitor.cpp @@ -17,11 +17,11 @@ // block) and use select() to poll it. #if defined(__has_feature) #if __has_feature(thread_sanitizer) -#define TOPIC_MONITOR_TSAN_WORKAROUND +#define TOPIC_MONITOR_TSAN_WORKAROUND 1 #endif #endif #ifdef __SANITIZE_THREAD__ -#define TOPIC_MONITOR_TSAN_WORKAROUND +#define TOPIC_MONITOR_TSAN_WORKAROUND 1 #endif wcstring generation_list_t::describe() const {