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 <magjed@webrtc.org>
Commit-Queue: Karl Wiberg <kwiberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27574}

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",
     ]
   }
 }

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) {
+bool Event::Wait(const int give_up_after_ms, const int warn_after_ms) {
-  // Set a timeout for the given number of milliseconds, or 3000 ms if the
+  // Instant when we'll log a warning message (because we've been waiting so
-  // caller asked for kForever.
+  // long it might be a bug), but not yet give up waiting. nullopt if we
-  const timespec ts =
+  // shouldn't log a warning.
-      GetTimespec(milliseconds == kForever ? 3000 : milliseconds);
+  const absl::optional<timespec> 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<timespec> 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
+  // Wait for `event_cond_` to trigger and `event_status_` to be set, with the
-  // first.
+  // given timeout (or without a timeout if none is given).
-  int error = 0;
+  const auto wait = [&](const absl::optional<timespec> timeout_ts) {
-  while (!event_status_ && error == 0) {
+    int 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;
     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);
     }
   }
 

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
+  // Waits for the event to become signaled, but logs a warning if it takes more
-  // |milliseconds|.  To wait indefinetly, pass kForever.
+  // than `warn_after_ms` milliseconds, and gives up completely if it takes more
-  bool Wait(int milliseconds);
+  // 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)

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;
 }