Revert "async-threaded resolver: use ref counter"

This reverts commit 19226f9bb106347e21d1dd113f2e2aeff53ca925.

Due to flaky macos CI builds

Fixes #16880
Closes #16882

diff --git a/lib/asyn-ares.c b/lib/asyn-ares.c
index a8bcaddab83fe37ffcb6b03b9fa4008b8a25bdd7..2bd0ffa027a973fed3f9acaeef3128192d1c8c2f 100644 (file)
--- a/lib/asyn-ares.c
+++ b/lib/asyn-ares.c
@@ -976,15 +976,6 @@ CURLcode Curl_set_dns_local_ip6(struct Curl_easy *data,
   return CURLE_NOT_BUILT_IN;
 #endif
 }
-
-void Curl_resolver_set_result(struct Curl_easy *data,
-                              struct Curl_dns_entry *dnsentry)
-{
-  Curl_resolver_cancel(data);
-  data->state.async.dns = dnsentry;
-  data->state.async.done = TRUE;
-}
-
 #endif /* CURLRES_ARES */
 
 #endif /* USE_ARES */

diff --git a/lib/asyn-thread.c b/lib/asyn-thread.c
index 610c8fd167038aba9a6f2d3e434f7cb393e63367..c3edf7203303375db3dc96fc05a8f3d9a7f5f62a 100644 (file)
--- a/lib/asyn-thread.c
+++ b/lib/asyn-thread.c
@@ -143,34 +143,40 @@ void Curl_resolver_cancel(struct Curl_easy *data)
   destroy_async_data(data);
 }
 
+/* This function is used to init a threaded resolve */
+static bool init_resolve_thread(struct Curl_easy *data,
+                                const char *hostname, int port,
+                                const struct addrinfo *hints);
+
+
 static struct thread_sync_data *conn_thread_sync_data(struct Curl_easy *data)
 {
-  return data->state.async.thdata.tsd;
+  return &(data->state.async.thdata.tsd);
 }
 
 /* Destroy resolver thread synchronization data */
 static
 void destroy_thread_sync_data(struct thread_sync_data *tsd)
 {
-  if(tsd) {
-    DEBUGASSERT(!tsd->ref_count);
-    Curl_mutex_destroy(&tsd->mutx);
-    free(tsd->hostname);
-    if(tsd->res)
-      Curl_freeaddrinfo(tsd->res);
+  Curl_mutex_destroy(&tsd->mutx);
+
+  free(tsd->hostname);
+
+  if(tsd->res)
+    Curl_freeaddrinfo(tsd->res);
+
 #ifndef CURL_DISABLE_SOCKETPAIR
-    /*
-     * close one end of the socket pair (may be done in resolver thread);
-     * the other end (for reading) is always closed in the parent thread.
-     */
+  /*
+   * close one end of the socket pair (may be done in resolver thread);
+   * the other end (for reading) is always closed in the parent thread.
+   */
 #ifndef HAVE_EVENTFD
-    if(tsd->sock_pair[1] != CURL_SOCKET_BAD) {
-      wakeup_close(tsd->sock_pair[1]);
-    }
+  if(tsd->sock_pair[1] != CURL_SOCKET_BAD) {
+    wakeup_close(tsd->sock_pair[1]);
+  }
 #endif
 #endif
-    free(tsd);
-  }
+  memset(tsd, 0, sizeof(*tsd));
 }
 
 /* Initialize resolver thread synchronization data */
@@ -180,20 +186,16 @@ int init_thread_sync_data(struct thread_data *td,
                           int port,
                           const struct addrinfo *hints)
 {
-  struct thread_sync_data *tsd;
+  struct thread_sync_data *tsd = &td->tsd;
 
-  DEBUGASSERT(!td->tsd);
-  tsd = calloc(1, sizeof(*tsd));
-  if(!tsd)
-    return 0;
+  memset(tsd, 0, sizeof(*tsd));
 
+  td->init = TRUE;
   tsd->port = port;
-#ifndef CURL_DISABLE_SOCKETPAIR
-  tsd->sock_pair[0] = CURL_SOCKET_BAD;
-  tsd->sock_pair[1] = CURL_SOCKET_BAD;
-#endif
-  tsd->ref_count = 0;
-
+  /* Treat the request as done until the thread actually starts so any early
+   * cleanup gets done properly.
+   */
+  tsd->done = TRUE;
 #ifdef HAVE_GETADDRINFO
   DEBUGASSERT(hints);
   tsd->hints = *hints;
@@ -220,9 +222,6 @@ int init_thread_sync_data(struct thread_data *td,
   if(!tsd->hostname)
     goto err_exit;
 
-  td->init = TRUE;
-  td->tsd = tsd;
-  tsd->ref_count = 1;
   return 1;
 
 err_exit:
@@ -267,10 +266,10 @@ unsigned int
 #endif
 CURL_STDCALL getaddrinfo_thread(void *arg)
 {
-  struct thread_sync_data *tsd = arg;
+  struct thread_data *td = arg;
+  struct thread_sync_data *tsd = &td->tsd;
   char service[12];
   int rc;
-  bool all_gone;
 
   msnprintf(service, sizeof(service), "%d", tsd->port);
 
@@ -286,8 +285,12 @@ CURL_STDCALL getaddrinfo_thread(void *arg)
   }
 
   Curl_mutex_acquire(&tsd->mutx);
-  if(tsd->ref_count > 1) {
-    /* Someone still waiting on our results. */
+  if(tsd->done) {
+    /* too late, gotta clean up the mess */
+    Curl_mutex_release(&tsd->mutx);
+    destroy_thread_sync_data(tsd);
+  }
+  else {
 #ifndef CURL_DISABLE_SOCKETPAIR
     if(tsd->sock_pair[1] != CURL_SOCKET_BAD) {
 #ifdef HAVE_EVENTFD
@@ -302,13 +305,9 @@ CURL_STDCALL getaddrinfo_thread(void *arg)
       }
     }
 #endif
+    tsd->done = TRUE;
+    Curl_mutex_release(&tsd->mutx);
   }
-  /* thread gives up its reference to the shared data now. */
-  --tsd->ref_count;
-  all_gone = !tsd->ref_count;
-  Curl_mutex_release(&tsd->mutx);
-  if(all_gone)
-    destroy_thread_sync_data(tsd);
 
   return 0;
 }
@@ -326,8 +325,8 @@ unsigned int
 #endif
 CURL_STDCALL gethostbyname_thread(void *arg)
 {
-  struct thread_sync_data *tsd = arg;
-  bool all_gone;
+  struct thread_data *td = arg;
+  struct thread_sync_data *tsd = &td->tsd;
 
   tsd->res = Curl_ipv4_resolve_r(tsd->hostname, tsd->port);
 
@@ -338,12 +337,15 @@ CURL_STDCALL gethostbyname_thread(void *arg)
   }
 
   Curl_mutex_acquire(&tsd->mutx);
-  /* thread gives up its reference to the shared data now. */
-  --tsd->ref_count;
-  all_gone = !tsd->ref_count;;
-  Curl_mutex_release(&tsd->mutx);
-  if(all_gone)
+  if(tsd->done) {
+    /* too late, gotta clean up the mess */
+    Curl_mutex_release(&tsd->mutx);
     destroy_thread_sync_data(tsd);
+  }
+  else {
+    tsd->done = TRUE;
+    Curl_mutex_release(&tsd->mutx);
+  }
 
   return 0;
 }
@@ -357,9 +359,11 @@ static void destroy_async_data(struct Curl_easy *data)
 {
   struct Curl_async *async = &data->state.async;
   struct thread_data *td = &async->thdata;
-
   if(td->init) {
     bool done;
+#ifndef CURL_DISABLE_SOCKETPAIR
+    curl_socket_t sock_rd = td->tsd.sock_pair[0];
+#endif
 
 #ifdef USE_HTTPSRR_ARES
     if(td->channel) {
@@ -367,32 +371,24 @@ static void destroy_async_data(struct Curl_easy *data)
       td->channel = NULL;
     }
 #endif
+    /*
+     * if the thread is still blocking in the resolve syscall, detach it and
+     * let the thread do the cleanup...
+     */
+    Curl_mutex_acquire(&td->tsd.mutx);
+    done = td->tsd.done;
+    td->tsd.done = TRUE;
+    Curl_mutex_release(&td->tsd.mutx);
 
-    if(td->tsd) {
-#ifndef CURL_DISABLE_SOCKETPAIR
-      curl_socket_t sock_rd = td->tsd->sock_pair[0];
-#endif
-      /* Release our reference to the data shared with the thread. */
-      Curl_mutex_acquire(&td->tsd->mutx);
-      --td->tsd->ref_count;
-      CURL_TRC_DNS(data, "resolve, destroy async data, shared ref=%d",
-                   td->tsd->ref_count);
-      done = !td->tsd->ref_count;
-      Curl_mutex_release(&td->tsd->mutx);
-
-      if(!done) {
-        /* thread is still running. Detach the thread, it will
-         * trigger the cleanup when it releases its reference. */
-        Curl_thread_destroy(td->thread_hnd);
-      }
-      else {
-        /* thread has released its reference, join it and
-         * release the memory we shared with it. */
-        if(td->thread_hnd != curl_thread_t_null)
-          Curl_thread_join(&td->thread_hnd);
-        destroy_thread_sync_data(td->tsd);
-      }
-      td->tsd = NULL;
+    if(!done) {
+      Curl_thread_destroy(td->thread_hnd);
+    }
+    else {
+      if(td->thread_hnd != curl_thread_t_null)
+        Curl_thread_join(&td->thread_hnd);
+
+      destroy_thread_sync_data(&td->tsd);
+    }
 #ifndef CURL_DISABLE_SOCKETPAIR
     /*
      * ensure CURLMOPT_SOCKETFUNCTION fires CURL_POLL_REMOVE
@@ -401,10 +397,10 @@ static void destroy_async_data(struct Curl_easy *data)
     Curl_multi_will_close(data, sock_rd);
     wakeup_close(sock_rd);
 #endif
-    }
 
     td->init = FALSE;
   }
+
 }
 
 #ifdef USE_HTTPSRR_ARES
@@ -441,51 +437,41 @@ static bool init_resolve_thread(struct Curl_easy *data,
   int err = ENOMEM;
   struct Curl_async *async = &data->state.async;
 
-  if(async->done && td->tsd) {
-    CURL_TRC_DNS(data, "starting new resolve, with previous not cleaned up"
-                 " for '%s:%d'", td->tsd->hostname, td->tsd->port);
-    destroy_async_data(data);
-    DEBUGASSERT(!td->tsd);
-  }
-
   async->port = port;
   async->done = FALSE;
   async->dns = NULL;
   td->thread_hnd = curl_thread_t_null;
   td->start = Curl_now();
 
-  if(!init_thread_sync_data(td, hostname, port, hints))
-    goto err_exit;
-  DEBUGASSERT(td->tsd);
+  if(!init_thread_sync_data(td, hostname, port, hints)) {
+    goto errno_exit;
+  }
+
+  /* The thread will set this TRUE when complete. */
+  td->tsd.done = FALSE;
 
-  Curl_mutex_acquire(&td->tsd->mutx);
-  DEBUGASSERT(td->tsd->ref_count == 1);
-  /* passing td->tsd to the thread adds a reference */
-  ++td->tsd->ref_count;
 #ifdef HAVE_GETADDRINFO
-  td->thread_hnd = Curl_thread_create(getaddrinfo_thread, td->tsd);
+  td->thread_hnd = Curl_thread_create(getaddrinfo_thread, td);
 #else
-  td->thread_hnd = Curl_thread_create(gethostbyname_thread, td->tsd);
+  td->thread_hnd = Curl_thread_create(gethostbyname_thread, td);
 #endif
+
   if(td->thread_hnd == curl_thread_t_null) {
-    /* The thread never started, remove its reference that never happened. */
-    --td->tsd->ref_count;
+    /* The thread never started, so mark it as done here for proper cleanup. */
+    td->tsd.done = TRUE;
     err = errno;
-    Curl_mutex_release(&td->tsd->mutx);
     goto err_exit;
   }
-
 #ifdef USE_HTTPSRR_ARES
   if(resolve_httpsrr(data, async))
     infof(data, "Failed HTTPS RR operation");
 #endif
-  Curl_mutex_release(&td->tsd->mutx);
-  CURL_TRC_DNS(data, "resolve thread started for of %s:%d", hostname, port);
   return TRUE;
 
 err_exit:
-  CURL_TRC_DNS(data, "resolve thread failed init: %d", err);
   destroy_async_data(data);
+
+errno_exit:
   CURL_SETERRNO(err);
   return FALSE;
 }
@@ -505,7 +491,6 @@ static CURLcode thread_wait_resolv(struct Curl_easy *data,
   DEBUGASSERT(td);
   DEBUGASSERT(td->thread_hnd != curl_thread_t_null);
 
-  CURL_TRC_DNS(data, "resolve, wait for thread to finish");
   /* wait for the thread to resolve the name */
   if(Curl_thread_join(&td->thread_hnd)) {
     if(entry)
@@ -586,15 +571,10 @@ CURLcode Curl_resolver_is_resolved(struct Curl_easy *data,
   (void)Curl_ares_perform(td->channel, 0); /* ignore errors */
 #endif
 
-  DEBUGASSERT(td->tsd);
-  if(!td->tsd)
-    return CURLE_FAILED_INIT;
-
-  Curl_mutex_acquire(&td->tsd->mutx);
-  done = (td->tsd->ref_count == 1);
-  Curl_mutex_release(&td->tsd->mutx);
+  Curl_mutex_acquire(&td->tsd.mutx);
+  done = td->tsd.done;
+  Curl_mutex_release(&td->tsd.mutx);
 
-  CURL_TRC_DNS(data, "resolve, thread %sfinished", done ? "" : "not ");
   if(done) {
     CURLcode result = td->result;
     getaddrinfo_complete(data);
@@ -664,9 +644,9 @@ int Curl_resolver_getsock(struct Curl_easy *data, curl_socket_t *socks)
   }
 #endif
 #ifndef CURL_DISABLE_SOCKETPAIR
-  if(td->init && td->tsd) {
+  if(td->init) {
     /* return read fd to client for polling the DNS resolution status */
-    socks[socketi] = td->tsd->sock_pair[0];
+    socks[socketi] = td->tsd.sock_pair[0];
     ret_val |= GETSOCK_READSOCK(socketi);
   }
   else
@@ -724,7 +704,6 @@ struct Curl_addrinfo *Curl_resolver_getaddrinfo(struct Curl_easy *data,
   int pf = PF_INET;
   *waitp = 0; /* default to synchronous response */
 
-  CURL_TRC_DNS(data, "init threaded resolve of %s:%d", hostname, port);
 #ifdef CURLRES_IPV6
   if((data->conn->ip_version != CURL_IPRESOLVE_V4) && Curl_ipv6works(data)) {
     /* The stack seems to be IPv6-enabled */
@@ -753,14 +732,6 @@ struct Curl_addrinfo *Curl_resolver_getaddrinfo(struct Curl_easy *data,
 
 #endif /* !HAVE_GETADDRINFO */
 
-void Curl_resolver_set_result(struct Curl_easy *data,
-                              struct Curl_dns_entry *dnsentry)
-{
-  destroy_async_data(data);
-  data->state.async.dns = dnsentry;
-  data->state.async.done = TRUE;
-}
-
 CURLcode Curl_set_dns_servers(struct Curl_easy *data,
                               char *servers)
 {

diff --git a/lib/asyn.h b/lib/asyn.h
index ab7a973d1526408ee59445e8614131eb170163b0..481c869bdb963abb6bf27040dce2ae8f8f07fb0f 100644 (file)
--- a/lib/asyn.h
+++ b/lib/asyn.h
@@ -51,7 +51,7 @@ struct thread_sync_data {
 #endif
   int port;
   int sock_error;
-  int ref_count;
+  bool done;
 };
 
 struct thread_data {
@@ -59,7 +59,7 @@ struct thread_data {
   unsigned int poll_interval;
   timediff_t interval_end;
   struct curltime start;
-  struct thread_sync_data *tsd;
+  struct thread_sync_data tsd;
   CURLcode result; /* CURLE_OK or error handling response */
 #if defined(USE_HTTPSRR) && defined(USE_ARES)
   struct Curl_https_rrinfo hinfo;
@@ -227,13 +227,6 @@ struct Curl_addrinfo *Curl_resolver_getaddrinfo(struct Curl_easy *data,
                                                 int port,
                                                 int *waitp);
 
-/*
- * Set `dnsentry` as result of resolve operation, replacing any
- * ongoing resolve attempts.
- */
-void Curl_resolver_set_result(struct Curl_easy *data,
-                              struct Curl_dns_entry *dnsentry);
-
 #ifndef CURLRES_ASYNCH
 /* convert these functions if an asynch resolver is not used */
 #define Curl_resolver_cancel(x) Curl_nop_stmt
@@ -244,7 +237,6 @@ void Curl_resolver_set_result(struct Curl_easy *data,
 #define Curl_resolver_init(x,y) CURLE_OK
 #define Curl_resolver_global_init() CURLE_OK
 #define Curl_resolver_global_cleanup() Curl_nop_stmt
-#define Curl_resolver_set_result(x,y) Curl_nop_stmt
 #define Curl_resolver_cleanup(x) Curl_nop_stmt
 #endif
 

diff --git a/lib/multi.c b/lib/multi.c
index 2426f69d448e47e540c38eb78f4c3cfa25362a4a..09afe804c0bfaab94b72467236f2f9b0d54d37ab 100644 (file)
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -2086,8 +2086,10 @@ static CURLMcode state_resolving(struct Curl_multi *multi,
   dns = Curl_fetch_addr(data, hostname, conn->primary.remote_port);
 
   if(dns) {
-    /* Tell a possibly async resolver we no longer need the results. */
-    Curl_resolver_set_result(data, dns);
+#ifdef USE_CURL_ASYNC
+    data->state.async.dns = dns;
+    data->state.async.done = TRUE;
+#endif
     result = CURLE_OK;
     infof(data, "Hostname '%s' was found in DNS cache", hostname);
   }

diff --git a/lib/socks.c b/lib/socks.c
index 431f9d57bd02fe32f401945bdd4f62d60ebd69ed..3a88f0b38679d377261a136a6518d4c6f5be62e6 100644 (file)
--- a/lib/socks.c
+++ b/lib/socks.c
@@ -344,8 +344,10 @@ static CURLproxycode do_SOCKS4(struct Curl_cfilter *cf,
     dns = Curl_fetch_addr(data, sx->hostname, conn->primary.remote_port);
 
     if(dns) {
-      /* Tell a possibly async resolver we no longer need the results. */
-      Curl_resolver_set_result(data, dns);
+#ifdef USE_CURL_ASYNC
+      data->state.async.dns = dns;
+      data->state.async.done = TRUE;
+#endif
       infof(data, "Hostname '%s' was found", sx->hostname);
       sxstate(sx, data, CONNECT_RESOLVED);
     }
@@ -812,8 +814,10 @@ CONNECT_REQ_INIT:
     dns = Curl_fetch_addr(data, sx->hostname, sx->remote_port);
 
     if(dns) {
-      /* Tell a possibly async resolver we no longer need the results. */
-      Curl_resolver_set_result(data, dns);
+#ifdef USE_CURL_ASYNC
+      data->state.async.dns = dns;
+      data->state.async.done = TRUE;
+#endif
       infof(data, "SOCKS5: hostname '%s' found", sx->hostname);
     }