Implement WIN32 tor_cond_wait using condition variables #30187

Fix bug where running a relay on Windows would use 100% CPU
after some time. Makes Windows >= Vista the required Windows
version to build and run tor.

changes/bug30187

@@ -0,0 +1,5 @@
+  o Major bugfixes (relay, windows):
+    - Fix bug where running a relay on Windows would use 100%
+      CPU after some time. Makes Windows >= Vista the required
+      Windows version to build and run tor. Fixes bug 30187;
+      bugfix on 0.4.5.1-alpha. Patch by Daniel Pinto.

configure.ac

@@ -560,14 +560,14 @@ fi
 
 AH_BOTTOM([
 #ifdef _WIN32
-/* Defined to access windows functions and definitions for >=WinXP */
+/* Defined to access windows functions and definitions for >=WinVista */
 # ifndef WINVER
-#  define WINVER 0x0501
+#  define WINVER 0x0600
 # endif
 
-/* Defined to access _other_ windows functions and definitions for >=WinXP */
+/* Defined to access _other_ windows functions and definitions for >=WinVista */
 # ifndef _WIN32_WINNT
-#  define _WIN32_WINNT 0x0501
+#  define _WIN32_WINNT 0x0600
 # endif
 
 /* Defined to avoid including some windows headers as part of Windows.h */

src/lib/thread/compat_winthreads.c

@@ -10,18 +10,32 @@
  * functions.
  */
 
+#include "orconfig.h"
+
 #ifdef _WIN32
+/* For condition variable support */
+#ifndef WINVER
+#error "orconfig.h didn't define WINVER"
+#endif
+#ifndef _WIN32_WINNT
+#error "orconfig.h didn't define _WIN32_WINNT"
+#endif
+#if WINVER < 0x0600
+#error "winver too low"
+#endif
+#if _WIN32_WINNT < 0x0600
+#error "winver too low"
+#endif
 
 #include <windows.h>
 #include <process.h>
+#include <time.h>
+
 #include "lib/thread/threads.h"
 #include "lib/log/log.h"
 #include "lib/log/util_bug.h"
 #include "lib/log/win32err.h"
 
-/* This value is more or less total cargo-cult */
-#define SPIN_COUNT 2000
-
 /** Minimalist interface to run a void function in the background.  On
  * Unix calls fork, on win32 calls beginthread.  Returns -1 on failure.
  * func should not return, but rather should call spawn_exit.
@@ -64,45 +78,24 @@ tor_get_thread_id(void)
 int
 tor_cond_init(tor_cond_t *cond)
 {
-  memset(cond, 0, sizeof(tor_cond_t));
+  InitializeConditionVariable(&cond->cond);
-  if (InitializeCriticalSectionAndSpinCount(&cond->lock, SPIN_COUNT)==0) {
-    return -1;
-  }
-  if ((cond->event = CreateEvent(NULL,TRUE,FALSE,NULL)) == NULL) {
-    DeleteCriticalSection(&cond->lock);
-    return -1;
-  }
-  cond->n_waiting = cond->n_to_wake = cond->generation = 0;
   return 0;
 }
 void
 tor_cond_uninit(tor_cond_t *cond)
 {
-  DeleteCriticalSection(&cond->lock);
+  (void) cond;
-  CloseHandle(cond->event);
 }
 
-static void
-tor_cond_signal_impl(tor_cond_t *cond, int broadcast)
-{
-  EnterCriticalSection(&cond->lock);
-  if (broadcast)
-    cond->n_to_wake = cond->n_waiting;
-  else
-    ++cond->n_to_wake;
-  cond->generation++;
-  SetEvent(cond->event);
-  LeaveCriticalSection(&cond->lock);
-}
 void
 tor_cond_signal_one(tor_cond_t *cond)
 {
-  tor_cond_signal_impl(cond, 0);
+  WakeConditionVariable(&cond->cond);
 }
 void
 tor_cond_signal_all(tor_cond_t *cond)
 {
-  tor_cond_signal_impl(cond, 1);
+  WakeAllConditionVariable(&cond->cond);
 }
 
 int
@@ -152,66 +145,23 @@ int
 tor_cond_wait(tor_cond_t *cond, tor_mutex_t *lock_, const struct timeval *tv)
 {
   CRITICAL_SECTION *lock = &lock_->mutex;
-  int generation_at_start;
+  DWORD ms = INFINITE;
-  int waiting = 1;
+  if (tv) {
-  int result = -1;
+    ms = tv->tv_sec*1000 + (tv->tv_usec+999)/1000;
-  DWORD ms = INFINITE, ms_orig = INFINITE, startTime, endTime;
-  if (tv)
-    ms_orig = ms = tv->tv_sec*1000 + (tv->tv_usec+999)/1000;
-
-  EnterCriticalSection(&cond->lock);
-  ++cond->n_waiting;
-  generation_at_start = cond->generation;
-  LeaveCriticalSection(&cond->lock);
-
-  LeaveCriticalSection(lock);
-
-  startTime = GetTickCount();
-  do {
-    DWORD res;
-    res = WaitForSingleObject(cond->event, ms);
-    EnterCriticalSection(&cond->lock);
-    if (cond->n_to_wake &&
-        cond->generation != generation_at_start) {
-      --cond->n_to_wake;
-      --cond->n_waiting;
-      result = 0;
-      waiting = 0;
-      goto out;
-    } else if (res != WAIT_OBJECT_0) {
-      result = (res==WAIT_TIMEOUT) ? 1 : -1;
-      --cond->n_waiting;
-      waiting = 0;
-      goto out;
-    } else if (ms != INFINITE) {
-      endTime = GetTickCount();
-      if (startTime + ms_orig <= endTime) {
-        result = 1; /* Timeout */
-        --cond->n_waiting;
-        waiting = 0;
-        goto out;
-      } else {
-        ms = startTime + ms_orig - endTime;
   }
+
+  BOOL ok = SleepConditionVariableCS(&cond->cond, lock, ms);
+  if (!ok) {
+    DWORD err = GetLastError();
+    if (err == ERROR_TIMEOUT) {
+      return 1;
     }
-    /* If we make it here, we are still waiting. */
+    char *msg = format_win32_error(err);
-    if (cond->n_to_wake == 0) {
+    log_err(LD_GENERAL, "Error waiting for condition variable: %s", msg);
-      /* There is nobody else who should wake up; reset
+    tor_free(msg);
-       * the event. */
+    return -1;
-      ResetEvent(cond->event);
   }
-  out:
+  return 0;
-    LeaveCriticalSection(&cond->lock);
-  } while (waiting);
-
-  EnterCriticalSection(lock);
-
-  EnterCriticalSection(&cond->lock);
-  if (!cond->n_waiting)
-    ResetEvent(cond->event);
-  LeaveCriticalSection(&cond->lock);
-
-  return result;
 }
 
 void

src/lib/thread/threads.h

@@ -42,12 +42,7 @@ typedef struct tor_cond_t {
 #ifdef USE_PTHREADS
   pthread_cond_t cond;
 #elif defined(USE_WIN32_THREADS)
-  HANDLE event;
+  CONDITION_VARIABLE cond;
-
-  CRITICAL_SECTION lock;
-  int n_waiting;
-  int n_to_wake;
-  int generation;
 #else
 #error no known condition implementation.
 #endif /* defined(USE_PTHREADS) || ... */