From fc47c86b128103a362e4096d749fadf49d1c47f4 Mon Sep 17 00:00:00 2001 From: Karl Wiberg Date: Thu, 11 Apr 2019 10:31:24 +0200 Subject: [PATCH] NullSocketServer::Wait: Don't warn if we have to wait a long time for messages Make the warning timeout for Event::Wait configurable, and let NullSocketServer::Wait pass kForever to completely eliminate the warning. 3000 ms is a good default warning timeout for Event::Wait, but in some cases---such as when a message queue is waiting for a message to arrive---we don't want the warning, since a long wait isn't a reliable indicator that the system is deadlocked. It might just be that no one is posting messages. Bug: webrtc:10531 Change-Id: Ic5969b8bfedb96376bd6d6a72ba6a4591750a920 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/132017 Reviewed-by: Magnus Jedvert Commit-Queue: Karl Wiberg Cr-Commit-Position: refs/heads/master@{#27574} --- rtc_base/BUILD.gn | 1 + rtc_base/event.cc | 71 ++++++++++++++++++++++------------ rtc_base/event.h | 18 +++++++-- rtc_base/null_socket_server.cc | 5 ++- 4 files changed, 66 insertions(+), 29 deletions(-) diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index 6a2c5aaff5..05cecd178a 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -265,6 +265,7 @@ rtc_source_set("rtc_event") { ":checks", "synchronization:yield_policy", "system:warn_current_thread_is_deadlocked", + "//third_party/abseil-cpp/absl/types:optional", ] } } diff --git a/rtc_base/event.cc b/rtc_base/event.cc index 4b8e9ff752..67c8746205 100644 --- a/rtc_base/event.cc +++ b/rtc_base/event.cc @@ -21,6 +21,7 @@ #error "Must define either WEBRTC_WIN or WEBRTC_POSIX." #endif +#include "absl/types/optional.h" #include "rtc_base/checks.h" #include "rtc_base/synchronization/yield_policy.h" #include "rtc_base/system/warn_current_thread_is_deadlocked.h" @@ -50,9 +51,9 @@ void Event::Reset() { ResetEvent(event_handle_); } -bool Event::Wait(int milliseconds) { +bool Event::Wait(const int give_up_after_ms, int /*warn_after_ms*/) { ScopedYieldPolicy::YieldExecution(); - DWORD ms = (milliseconds == kForever) ? INFINITE : milliseconds; + const DWORD ms = give_up_after_ms == kForever ? INFINITE : give_up_after_ms; return (WaitForSingleObject(event_handle_, ms) == WAIT_OBJECT_0); } @@ -135,34 +136,54 @@ timespec GetTimespec(const int milliseconds_from_now) { } // namespace -bool Event::Wait(const int milliseconds) { - // Set a timeout for the given number of milliseconds, or 3000 ms if the - // caller asked for kForever. - const timespec ts = - GetTimespec(milliseconds == kForever ? 3000 : milliseconds); +bool Event::Wait(const int give_up_after_ms, const int warn_after_ms) { + // Instant when we'll log a warning message (because we've been waiting so + // long it might be a bug), but not yet give up waiting. nullopt if we + // shouldn't log a warning. + const absl::optional warn_ts = + warn_after_ms == kForever || + (give_up_after_ms != kForever && warn_after_ms > give_up_after_ms) + ? absl::nullopt + : absl::make_optional(GetTimespec(warn_after_ms)); + + // Instant when we'll stop waiting and return an error. nullopt if we should + // never give up. + const absl::optional give_up_ts = + give_up_after_ms == kForever + ? absl::nullopt + : absl::make_optional(GetTimespec(give_up_after_ms)); ScopedYieldPolicy::YieldExecution(); pthread_mutex_lock(&event_mutex_); - // Wait for the event to trigger or the timeout to expire, whichever comes - // first. - int error = 0; - while (!event_status_ && error == 0) { -#if USE_PTHREAD_COND_TIMEDWAIT_MONOTONIC_NP - error = - pthread_cond_timedwait_monotonic_np(&event_cond_, &event_mutex_, &ts); -#else - error = pthread_cond_timedwait(&event_cond_, &event_mutex_, &ts); -#endif - } - - if (milliseconds == kForever && error == ETIMEDOUT) { - // Our 3000 ms timeout expired, but the caller asked us to wait forever, so - // do that. - webrtc::WarnThatTheCurrentThreadIsProbablyDeadlocked(); - error = 0; + // Wait for `event_cond_` to trigger and `event_status_` to be set, with the + // given timeout (or without a timeout if none is given). + const auto wait = [&](const absl::optional timeout_ts) { + int error = 0; while (!event_status_ && error == 0) { - error = pthread_cond_wait(&event_cond_, &event_mutex_); + if (timeout_ts == absl::nullopt) { + error = pthread_cond_wait(&event_cond_, &event_mutex_); + } else { +#if USE_PTHREAD_COND_TIMEDWAIT_MONOTONIC_NP + error = pthread_cond_timedwait_monotonic_np(&event_cond_, &event_mutex_, + &*timeout_ts); +#else + error = + pthread_cond_timedwait(&event_cond_, &event_mutex_, &*timeout_ts); +#endif + } + } + return error; + }; + + int error; + if (warn_ts == absl::nullopt) { + error = wait(give_up_ts); + } else { + error = wait(warn_ts); + if (error == ETIMEDOUT) { + webrtc::WarnThatTheCurrentThreadIsProbablyDeadlocked(); + error = wait(give_up_ts); } } diff --git a/rtc_base/event.h b/rtc_base/event.h index 3f5f8b317c..584ad5d35a 100644 --- a/rtc_base/event.h +++ b/rtc_base/event.h @@ -34,9 +34,21 @@ class Event { void Set(); void Reset(); - // Wait for the event to become signaled, for the specified number of - // |milliseconds|. To wait indefinetly, pass kForever. - bool Wait(int milliseconds); + // Waits for the event to become signaled, but logs a warning if it takes more + // than `warn_after_ms` milliseconds, and gives up completely if it takes more + // than `give_up_after_ms` milliseconds. (If `warn_after_ms >= + // give_up_after_ms`, no warning will be logged.) Either or both may be + // `kForever`, which means wait indefinitely. + // + // Returns true if the event was signaled, false if there was a timeout or + // some other error. + bool Wait(int give_up_after_ms, int warn_after_ms); + + // Waits with the given timeout and a reasonable default warning timeout. + bool Wait(int give_up_after_ms) { + return Wait(give_up_after_ms, + give_up_after_ms == kForever ? 3000 : kForever); + } private: #if defined(WEBRTC_WIN) diff --git a/rtc_base/null_socket_server.cc b/rtc_base/null_socket_server.cc index f446b3d4c1..b2071e3baa 100644 --- a/rtc_base/null_socket_server.cc +++ b/rtc_base/null_socket_server.cc @@ -17,7 +17,10 @@ NullSocketServer::NullSocketServer() = default; NullSocketServer::~NullSocketServer() {} bool NullSocketServer::Wait(int cms, bool process_io) { - event_.Wait(cms); + // Wait with the given timeout. Do not log a warning if we end up waiting for + // a long time; that just means no one has any work for us, which is perfectly + // legitimate. + event_.Wait(/*give_up_after_ms=*/cms, /*warn_after_ms=*/Event::kForever); return true; }