Revert "Fix shutdown of closed fd when c-ares opens a second fd"

1 files changed, 32 insertions, 33 deletions

diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.cc b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.cc
index 07f2e2efba..ebe2c4c41c 100644
--- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.cc
+++ b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.cc
@@ -21,7 +21,6 @@
 #if GRPC_ARES == 1 && defined(GRPC_POSIX_SOCKET_ARES_EV_DRIVER)
 
 #include <ares.h>
-#include <string.h>
 #include <sys/ioctl.h>
 
 #include "src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver.h"
@@ -56,8 +55,8 @@ typedef struct fd_node {
   bool readable_registered;
   /** if the writable closure has been registered */
   bool writable_registered;
-  /** if the fd has been shutdown yet from grpc iomgr perspective */
-  bool already_shutdown;
+  /** if the fd is being shut down */
+  bool shutting_down;
 } fd_node;
 
 struct grpc_ares_ev_driver {
@@ -102,26 +101,25 @@ static void fd_node_destroy(fd_node* fdn) {
   gpr_log(GPR_DEBUG, "delete fd: %d", grpc_fd_wrapped_fd(fdn->fd));
   GPR_ASSERT(!fdn->readable_registered);
   GPR_ASSERT(!fdn->writable_registered);
-  GPR_ASSERT(fdn->already_shutdown);
   gpr_mu_destroy(&fdn->mu);
-  /* TODO: we need to pass a non-null "release_fd" parameter to
-   * grpc_fd_orphan because "epollsig" iomgr will close the fd
-   * even if "already_closed" is true, and it only leaves it open
-   * if "release_fd" is non-null. This is unlike the rest of the
-   * pollers, should this be changed within epollsig? */
-  int dummy_release_fd;
   /* c-ares library has closed the fd inside grpc_fd. This fd may be picked up
      immediately by another thread, and should not be closed by the following
      grpc_fd_orphan. */
-  grpc_fd_orphan(fdn->fd, nullptr, &dummy_release_fd, true /* already_closed */,
+  grpc_fd_orphan(fdn->fd, nullptr, nullptr, true /* already_closed */,
                  "c-ares query finished");
   gpr_free(fdn);
 }
 
-static void fd_node_shutdown_locked(fd_node* fdn, const char* reason) {
-  if (!fdn->already_shutdown) {
-    fdn->already_shutdown = true;
-    grpc_fd_shutdown(fdn->fd, GRPC_ERROR_CREATE_FROM_STATIC_STRING(reason));
+static void fd_node_shutdown(fd_node* fdn) {
+  gpr_mu_lock(&fdn->mu);
+  fdn->shutting_down = true;
+  if (!fdn->readable_registered && !fdn->writable_registered) {
+    gpr_mu_unlock(&fdn->mu);
+    fd_node_destroy(fdn);
+  } else {
+    grpc_fd_shutdown(
+        fdn->fd, GRPC_ERROR_CREATE_FROM_STATIC_STRING("c-ares fd shutdown"));
+    gpr_mu_unlock(&fdn->mu);
   }
 }
 
@@ -129,10 +127,7 @@ grpc_error* grpc_ares_ev_driver_create(grpc_ares_ev_driver** ev_driver,
                                        grpc_pollset_set* pollset_set) {
   *ev_driver = static_cast<grpc_ares_ev_driver*>(
       gpr_malloc(sizeof(grpc_ares_ev_driver)));
-  ares_options opts;
-  memset(&opts, 0, sizeof(opts));
-  opts.flags |= ARES_FLAG_STAYOPEN;
-  int status = ares_init_options(&(*ev_driver)->channel, &opts, ARES_OPT_FLAGS);
+  int status = ares_init(&(*ev_driver)->channel);
   gpr_log(GPR_DEBUG, "grpc_ares_ev_driver_create");
   if (status != ARES_SUCCESS) {
     char* err_msg;
@@ -169,9 +164,8 @@ void grpc_ares_ev_driver_shutdown(grpc_ares_ev_driver* ev_driver) {
   ev_driver->shutting_down = true;
   fd_node* fn = ev_driver->fds;
   while (fn != nullptr) {
-    gpr_mu_lock(&fn->mu);
-    fd_node_shutdown_locked(fn, "grpc_ares_ev_driver_shutdown");
-    gpr_mu_unlock(&fn->mu);
+    grpc_fd_shutdown(fn->fd, GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+                                 "grpc_ares_ev_driver_shutdown"));
     fn = fn->next;
   }
   gpr_mu_unlock(&ev_driver->mu);
@@ -208,7 +202,14 @@ static void on_readable_cb(void* arg, grpc_error* error) {
   gpr_mu_lock(&fdn->mu);
   const int fd = grpc_fd_wrapped_fd(fdn->fd);
   fdn->readable_registered = false;
+  if (fdn->shutting_down && !fdn->writable_registered) {
+    gpr_mu_unlock(&fdn->mu);
+    fd_node_destroy(fdn);
+    grpc_ares_ev_driver_unref(ev_driver);
+    return;
+  }
   gpr_mu_unlock(&fdn->mu);
+
   gpr_log(GPR_DEBUG, "readable on %d", fd);
   if (error == GRPC_ERROR_NONE) {
     do {
@@ -235,7 +236,14 @@ static void on_writable_cb(void* arg, grpc_error* error) {
   gpr_mu_lock(&fdn->mu);
   const int fd = grpc_fd_wrapped_fd(fdn->fd);
   fdn->writable_registered = false;
+  if (fdn->shutting_down && !fdn->readable_registered) {
+    gpr_mu_unlock(&fdn->mu);
+    fd_node_destroy(fdn);
+    grpc_ares_ev_driver_unref(ev_driver);
+    return;
+  }
   gpr_mu_unlock(&fdn->mu);
+
   gpr_log(GPR_DEBUG, "writable on %d", fd);
   if (error == GRPC_ERROR_NONE) {
     ares_process_fd(ev_driver->channel, ARES_SOCKET_BAD, fd);
@@ -280,7 +288,7 @@ static void grpc_ares_notify_on_event_locked(grpc_ares_ev_driver* ev_driver) {
           fdn->ev_driver = ev_driver;
           fdn->readable_registered = false;
           fdn->writable_registered = false;
-          fdn->already_shutdown = false;
+          fdn->shutting_down = false;
           gpr_mu_init(&fdn->mu);
           GRPC_CLOSURE_INIT(&fdn->read_closure, on_readable_cb, fdn,
                             grpc_schedule_on_exec_ctx);
@@ -321,16 +329,7 @@ static void grpc_ares_notify_on_event_locked(grpc_ares_ev_driver* ev_driver) {
   while (ev_driver->fds != nullptr) {
     fd_node* cur = ev_driver->fds;
     ev_driver->fds = ev_driver->fds->next;
-    gpr_mu_lock(&cur->mu);
-    fd_node_shutdown_locked(cur, "c-ares fd shutdown");
-    if (!cur->readable_registered && !cur->writable_registered) {
-      gpr_mu_unlock(&cur->mu);
-      fd_node_destroy(cur);
-    } else {
-      cur->next = new_list;
-      new_list = cur;
-      gpr_mu_unlock(&cur->mu);
-    }
+    fd_node_shutdown(cur);
   }
   ev_driver->fds = new_list;
   // If the ev driver has no working fd, all the tasks are done.