Revert "async-threaded resolver: use ref counter"

This reverts commit 19226f9bb1.

Due to flaky macos CI builds

Fixes #16880
Closes #16882

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 */

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) {
+  Curl_mutex_destroy(&tsd->mutx);
-    DEBUGASSERT(!tsd->ref_count);
+
-    Curl_mutex_destroy(&tsd->mutx);
+  free(tsd->hostname);
-    free(tsd->hostname);
+
-    if(tsd->res)
+  if(tsd->res)
-      Curl_freeaddrinfo(tsd->res);
+    Curl_freeaddrinfo(tsd->res);
+
 #ifndef CURL_DISABLE_SOCKETPAIR
-    /*
+  /*
-     * close one end of the socket pair (may be done in resolver 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.
+   * the other end (for reading) is always closed in the parent thread.
-     */
+   */
 #ifndef HAVE_EVENTFD
-    if(tsd->sock_pair[1] != CURL_SOCKET_BAD) {
+  if(tsd->sock_pair[1] != CURL_SOCKET_BAD) {
-      wakeup_close(tsd->sock_pair[1]);
+    wakeup_close(tsd->sock_pair[1]);
-    }
-#endif
-#endif
-    free(tsd);
   }
+#endif
+#endif
+  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);
+  memset(tsd, 0, sizeof(*tsd));
-  tsd = calloc(1, sizeof(*tsd));
-  if(!tsd)
-    return 0;
 
+  td->init = TRUE;
   tsd->port = port;
-#ifndef CURL_DISABLE_SOCKETPAIR
+  /* Treat the request as done until the thread actually starts so any early
-  tsd->sock_pair[0] = CURL_SOCKET_BAD;
+   * cleanup gets done properly.
-  tsd->sock_pair[1] = CURL_SOCKET_BAD;
+   */
-#endif
+  tsd->done = TRUE;
-  tsd->ref_count = 0;
-
 #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) {
+  if(tsd->done) {
-    /* Someone still waiting on our results. */
+    /* 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;
+  struct thread_data *td = arg;
-  bool all_gone;
+  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. */
+  if(tsd->done) {
-  --tsd->ref_count;
+    /* too late, gotta clean up the mess */
-  all_gone = !tsd->ref_count;;
+    Curl_mutex_release(&tsd->mutx);
-  Curl_mutex_release(&tsd->mutx);
-  if(all_gone)
     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) {
+    if(!done) {
-#ifndef CURL_DISABLE_SOCKETPAIR
+      Curl_thread_destroy(td->thread_hnd);
-      curl_socket_t sock_rd = td->tsd->sock_pair[0];
+    }
-#endif
+    else {
-      /* Release our reference to the data shared with the thread. */
+      if(td->thread_hnd != curl_thread_t_null)
-      Curl_mutex_acquire(&td->tsd->mutx);
+        Curl_thread_join(&td->thread_hnd);
-      --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) {
+      destroy_thread_sync_data(&td->tsd);
-        /* 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;
 #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))
+  if(!init_thread_sync_data(td, hostname, port, hints)) {
-    goto err_exit;
+    goto errno_exit;
-  DEBUGASSERT(td->tsd);
-
-  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);
-#else
-  td->thread_hnd = Curl_thread_create(gethostbyname_thread, td->tsd);
-#endif
-  if(td->thread_hnd == curl_thread_t_null) {
-    /* The thread never started, remove its reference that never happened. */
-    --td->tsd->ref_count;
-    err = errno;
-    Curl_mutex_release(&td->tsd->mutx);
-    goto err_exit;
   }
 
+  /* The thread will set this TRUE when complete. */
+  td->tsd.done = FALSE;
+
+#ifdef HAVE_GETADDRINFO
+  td->thread_hnd = Curl_thread_create(getaddrinfo_thread, td);
+#else
+  td->thread_hnd = Curl_thread_create(gethostbyname_thread, td);
+#endif
+
+  if(td->thread_hnd == curl_thread_t_null) {
+    /* The thread never started, so mark it as done here for proper cleanup. */
+    td->tsd.done = TRUE;
+    err = errno;
+    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);
+  Curl_mutex_acquire(&td->tsd.mutx);
-  if(!td->tsd)
+  done = td->tsd.done;
-    return CURLE_FAILED_INIT;
+  Curl_mutex_release(&td->tsd.mutx);
 
-  Curl_mutex_acquire(&td->tsd->mutx);
-  done = (td->tsd->ref_count == 1);
-  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)
 {

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
 

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. */
+#ifdef USE_CURL_ASYNC
-    Curl_resolver_set_result(data, dns);
+    data->state.async.dns = dns;
+    data->state.async.done = TRUE;
+#endif
     result = CURLE_OK;
     infof(data, "Hostname '%s' was found in DNS cache", hostname);
   }

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. */
+#ifdef USE_CURL_ASYNC
-      Curl_resolver_set_result(data, dns);
+      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. */
+#ifdef USE_CURL_ASYNC
-      Curl_resolver_set_result(data, dns);
+      data->state.async.dns = dns;
+      data->state.async.done = TRUE;
+#endif
       infof(data, "SOCKS5: hostname '%s' found", sx->hostname);
     }