asyn-thrdd: manage DEFERRED and locks better

- cancel thread waits until thread start is at least 5ms in the past
  to give it some time to get its cancellation setup in place
- cancel thread without holding the mutex. It's supposed to be an
  async operation, but better be safe
- set DEFERRED cancel state explicitly, should be default in a pthread,
  but better be safe

Closes #18350

diff --git a/lib/asyn-thrdd.c b/lib/asyn-thrdd.c
index c61d7ec600cd0d1639eee2e5f097484e015b1cef..f82eb29ba897c04c4abb90f691a14d07647a0a43 100644 (file)
--- a/lib/asyn-thrdd.c
+++ b/lib/asyn-thrdd.c
@@ -65,6 +65,7 @@
 #include "curl_threads.h"
 #include "select.h"
 #include "strdup.h"
+#include "curlx/wait.h"
 
 #ifdef USE_ARES
 #include <ares.h>
@@ -234,17 +235,9 @@ err_exit:
 static void async_thrd_cleanup(void *arg)
 {
   struct async_thrdd_addr_ctx *addr_ctx = arg;
-  Curl_thread_disable_cancel();
-  addr_ctx_unlink(&addr_ctx, NULL);
-}
 
-static bool asyn_thrd_start(struct async_thrdd_addr_ctx *addr_ctx)
-{
   Curl_thread_disable_cancel();
-  Curl_mutex_acquire(&addr_ctx->mutx);
-  ++addr_ctx->ref_count;
-  Curl_mutex_release(&addr_ctx->mutx);
-  return TRUE;
+  addr_ctx_unlink(&addr_ctx, NULL);
 }
 
 #ifdef HAVE_GETADDRINFO
@@ -258,10 +251,7 @@ static bool asyn_thrd_start(struct async_thrdd_addr_ctx *addr_ctx)
 static CURL_THREAD_RETURN_T CURL_STDCALL getaddrinfo_thread(void *arg)
 {
   struct async_thrdd_addr_ctx *addr_ctx = arg;
-  int rc;
-
-  if(!asyn_thrd_start(addr_ctx))
-    return 1;
+  bool do_abort;
 
 /* clang complains about empty statements and the pthread_cleanup* macros
  * are pretty ill defined. */
@@ -269,13 +259,20 @@ static CURL_THREAD_RETURN_T CURL_STDCALL getaddrinfo_thread(void *arg)
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wextra-semi-stmt"
 #endif
+
+  Curl_thread_cancel_deferred();
   Curl_thread_push_cleanup(async_thrd_cleanup, addr_ctx);
+  Curl_thread_disable_cancel();
 
-  {
+  Curl_mutex_acquire(&addr_ctx->mutx);
+  do_abort = addr_ctx->do_abort;
+  Curl_mutex_release(&addr_ctx->mutx);
+
+  if(!do_abort) {
     char service[12];
+    int rc;
 
     Curl_thread_enable_cancel();
-
 #ifdef DEBUGBUILD
     Curl_resolve_test_delay();
 #endif
@@ -283,18 +280,18 @@ static CURL_THREAD_RETURN_T CURL_STDCALL getaddrinfo_thread(void *arg)
 
     rc = Curl_getaddrinfo_ex(addr_ctx->hostname, service,
                              &addr_ctx->hints, &addr_ctx->res);
-    Curl_thread_disable_cancel();
-  }
 
-  if(rc) {
-    addr_ctx->sock_error = SOCKERRNO ? SOCKERRNO : rc;
-    if(addr_ctx->sock_error == 0)
-      addr_ctx->sock_error = RESOLVER_ENOMEM;
-  }
-  else {
-    Curl_addrinfo_set_port(addr_ctx->res, addr_ctx->port);
+    if(rc) {
+      addr_ctx->sock_error = SOCKERRNO ? SOCKERRNO : rc;
+      if(addr_ctx->sock_error == 0)
+        addr_ctx->sock_error = RESOLVER_ENOMEM;
+    }
+    else {
+      Curl_addrinfo_set_port(addr_ctx->res, addr_ctx->port);
+    }
   }
 
+  Curl_thread_disable_cancel();
   Curl_thread_pop_cleanup();
 #if defined(__clang__)
 #pragma clang diagnostic pop
@@ -312,10 +309,7 @@ static CURL_THREAD_RETURN_T CURL_STDCALL getaddrinfo_thread(void *arg)
 static CURL_THREAD_RETURN_T CURL_STDCALL gethostbyname_thread(void *arg)
 {
   struct async_thrdd_addr_ctx *addr_ctx = arg;
-  bool all_gone;
-
-  if(!asyn_thrd_start(addr_ctx))
-    return 1;
+  bool do_abort;
 
 /* clang complains about empty statements and the pthread_cleanup* macros
  * are pretty ill defined. */
@@ -323,23 +317,30 @@ static CURL_THREAD_RETURN_T CURL_STDCALL gethostbyname_thread(void *arg)
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wextra-semi-stmt"
 #endif
+
+  Curl_thread_cancel_deferred();
   Curl_thread_push_cleanup(async_thrd_cleanup, addr_ctx);
-  {
+  Curl_thread_disable_cancel();
+
+  Curl_mutex_acquire(&addr_ctx->mutx);
+  do_abort = addr_ctx->do_abort;
+  Curl_mutex_release(&addr_ctx->mutx);
+  if(!do_abort) {
+
     Curl_thread_enable_cancel();
 #ifdef DEBUGBUILD
     Curl_resolve_test_delay();
 #endif
 
     addr_ctx->res = Curl_ipv4_resolve_r(addr_ctx->hostname, addr_ctx->port);
-    Curl_thread_disable_cancel();
-  }
-
-  if(!addr_ctx->res) {
-    addr_ctx->sock_error = SOCKERRNO;
-    if(addr_ctx->sock_error == 0)
-      addr_ctx->sock_error = RESOLVER_ENOMEM;
+    if(!addr_ctx->res) {
+      addr_ctx->sock_error = SOCKERRNO;
+      if(addr_ctx->sock_error == 0)
+        addr_ctx->sock_error = RESOLVER_ENOMEM;
+    }
   }
 
+  Curl_thread_disable_cancel();
   Curl_thread_pop_cleanup();
 #if defined(__clang__)
 #pragma clang diagnostic pop
@@ -478,8 +479,7 @@ static bool async_thrdd_init(struct Curl_easy *data,
   thrdd->addr = addr_ctx;
 
   /* passing addr_ctx to the thread adds a reference */
-  Curl_mutex_acquire(&addr_ctx->mutx);
-  DEBUGASSERT(addr_ctx->ref_count == 1);
+  addr_ctx->ref_count = 2;
   addr_ctx->start = curlx_now();
 
 #ifdef HAVE_GETADDRINFO
@@ -490,12 +490,11 @@ static bool async_thrdd_init(struct Curl_easy *data,
 
   if(addr_ctx->thread_hnd == curl_thread_t_null) {
     /* The thread never started */
+    addr_ctx->ref_count = 1;
     addr_ctx->thrd_done = TRUE;
-    Curl_mutex_release(&addr_ctx->mutx);
     err = errno;
     goto err_exit;
   }
-  Curl_mutex_release(&addr_ctx->mutx);
 
 #ifdef USE_HTTPSRR_ARES
   if(async_rr_start(data))
@@ -523,6 +522,7 @@ static void async_thrdd_shutdown(struct Curl_easy *data)
     return;
 
   Curl_mutex_acquire(&addr_ctx->mutx);
+  addr_ctx->do_abort = TRUE;
   done = addr_ctx->thrd_done;
 #ifndef CURL_DISABLE_SOCKETPAIR
   /* We are no longer interested in wakeups */
@@ -534,8 +534,13 @@ static void async_thrdd_shutdown(struct Curl_easy *data)
   Curl_mutex_release(&addr_ctx->mutx);
 
   DEBUGASSERT(addr_ctx->thread_hnd != curl_thread_t_null);
-  if(!done) {
-    CURL_TRC_DNS(data, "attempt to cancel resolve thread");
+  if(!done && (addr_ctx->thread_hnd != curl_thread_t_null)) {
+    timediff_t alive_ms = curlx_timediff(curlx_now(), addr_ctx->start);
+    /* give thread a startup chance to get cancel mode, etc. set up
+     * before we cancel it. */
+    if(alive_ms < 5)
+      curlx_wait_ms(5 - alive_ms);
+    CURL_TRC_DNS(data, "cancelling resolve thread");
     (void)Curl_thread_cancel(&addr_ctx->thread_hnd);
   }
 }
@@ -555,18 +560,12 @@ static CURLcode asyn_thrdd_await(struct Curl_easy *data,
       async_thrdd_shutdown(data);
 
     CURL_TRC_DNS(data, "resolve, wait for thread to finish");
-    if(Curl_thread_join(&addr_ctx->thread_hnd)) {
-#ifdef DEBUGBUILD
-      Curl_mutex_acquire(&addr_ctx->mutx);
-      DEBUGASSERT(addr_ctx->ref_count == 1);
-      DEBUGASSERT(addr_ctx->thrd_done);
-      Curl_mutex_release(&addr_ctx->mutx);
-#endif
-      if(entry)
-        result = Curl_async_is_resolved(data, entry);
-    }
-    else
+    if(!Curl_thread_join(&addr_ctx->thread_hnd)) {
       DEBUGASSERT(0);
+    }
+
+    if(entry)
+      result = Curl_async_is_resolved(data, entry);
   }
 
   data->state.async.done = TRUE;

diff --git a/lib/asyn.h b/lib/asyn.h
index 8408d9edf34c6c2879a14306cecbb87829ba9137..7863042bbe37f74f5a72bcb0c8d1887853393754 100644 (file)
--- a/lib/asyn.h
+++ b/lib/asyn.h
@@ -194,6 +194,7 @@ struct async_thrdd_addr_ctx {
   int sock_error;
   int ref_count;
   BIT(thrd_done);
+  BIT(do_abort);
 };
 
 /* Context for threaded resolver */

diff --git a/lib/curl_threads.h b/lib/curl_threads.h
index 115277c00eaa72c9bac018eebf81ce8e2316ef29..e066c0901f980d2ff52f487ec6918434c96de2e5 100644 (file)
--- a/lib/curl_threads.h
+++ b/lib/curl_threads.h
@@ -75,11 +75,14 @@ int Curl_thread_cancel(curl_thread_t *hnd);
     pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL)
 #define Curl_thread_disable_cancel()     \
     pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, NULL)
+#define Curl_thread_cancel_deferred()     \
+    pthread_setcanceltype(PTHREAD_CANCEL_DEFERRED, NULL)
 #else
 #define Curl_thread_push_cleanup(a,b)   ((void)a,(void)b)
 #define Curl_thread_pop_cleanup()       Curl_nop_stmt
 #define Curl_thread_enable_cancel()     Curl_nop_stmt
 #define Curl_thread_disable_cancel()    Curl_nop_stmt
+#define Curl_thread_cancel_deferred()   Curl_nop_stmt
 #endif
 
 #endif /* USE_THREADS_POSIX || USE_THREADS_WIN32 */