Fix shutdown semantics for client and server code.

3 files changed, 54 insertions, 40 deletions

diff --git a/src/core/ext/transport/chttp2/client/chttp2_connector.c b/src/core/ext/transport/chttp2/client/chttp2_connector.c
index 43649376a1..6341682ade 100644
--- a/src/core/ext/transport/chttp2/client/chttp2_connector.c
+++ b/src/core/ext/transport/chttp2/client/chttp2_connector.c
@@ -107,15 +107,20 @@ static void on_handshake_done(grpc_exec_ctx *exec_ctx, void *arg,
     if (error == GRPC_ERROR_NONE) {
       error = GRPC_ERROR_CREATE("connector shutdown");
       // We were shut down after handshaking completed successfully, so
-      // shutdown the endpoint here.
+      // destroy the endpoint here.
+      // TODO(ctiller): It is currently necessary to shutdown endpoints
+      // before destroying them, even if we know that there are no
+      // pending read/write callbacks.  This should be fixed, at which
+      // point this can be removed.
       grpc_endpoint_shutdown(exec_ctx, args->endpoint);
+      grpc_endpoint_destroy(exec_ctx, args->endpoint);
+      grpc_channel_args_destroy(args->args);
+      grpc_slice_buffer_destroy(args->read_buffer);
+      gpr_free(args->read_buffer);
     } else {
       error = GRPC_ERROR_REF(error);
     }
     memset(c->result, 0, sizeof(*c->result));
-    grpc_endpoint_destroy(exec_ctx, args->endpoint);
-    grpc_channel_args_destroy(args->args);
-    gpr_free(args->read_buffer);
   } else {
     c->result->transport =
         grpc_create_chttp2_transport(exec_ctx, args->args, args->endpoint, 1);
@@ -232,6 +237,7 @@ grpc_connector *grpc_chttp2_connector_create(
     gpr_free(proxy_name);
   }
   if (security_connector != NULL) {
+// FIXME: this function call is not linked in for the insecure target!
     grpc_channel_security_connector_create_handshakers(
         exec_ctx, security_connector, c->handshake_mgr);
   }
diff --git a/src/core/ext/transport/chttp2/server/insecure/server_chttp2.c b/src/core/ext/transport/chttp2/server/insecure/server_chttp2.c
index 9284d19357..bdf10de49e 100644
--- a/src/core/ext/transport/chttp2/server/insecure/server_chttp2.c
+++ b/src/core/ext/transport/chttp2/server/insecure/server_chttp2.c
@@ -110,29 +110,33 @@ static void on_handshake_done(grpc_exec_ctx *exec_ctx, void *arg,
                               grpc_error *error) {
   grpc_handshaker_args *args = arg;
   server_connection_state *connection_state = args->user_data;
-  if (error != GRPC_ERROR_NONE) {
+  gpr_mu_lock(&connection_state->server_state->mu);
+  if (error != GRPC_ERROR_NONE || connection_state->server_state->shutdown) {
     const char *error_str = grpc_error_string(error);
     gpr_log(GPR_ERROR, "Handshaking failed: %s", error_str);
     grpc_error_free_string(error_str);
-    gpr_mu_lock(&connection_state->server_state->mu);
-  } else {
-    gpr_mu_lock(&connection_state->server_state->mu);
-    if (!connection_state->server_state->shutdown) {
-      grpc_transport *transport =
-          grpc_create_chttp2_transport(exec_ctx, args->args, args->endpoint, 0);
-      grpc_server_setup_transport(
-          exec_ctx, connection_state->server_state->server, transport,
-          connection_state->accepting_pollset,
-          grpc_server_get_channel_args(connection_state->server_state->server));
-      grpc_chttp2_transport_start_reading(exec_ctx, transport,
-                                          args->read_buffer);
-    } else {
-      // Need to destroy this here, because the server may have already
-      // gone away.
+    if (error == GRPC_ERROR_NONE) {
+      // We were shut down after handshaking completed successfully, so
+      // destroy the endpoint here.
+      // TODO(ctiller): It is currently necessary to shutdown endpoints
+      // before destroying them, even if we know that there are no
+      // pending read/write callbacks.  This should be fixed, at which
+      // point this can be removed.
+      grpc_endpoint_shutdown(exec_ctx, args->endpoint);
       grpc_endpoint_destroy(exec_ctx, args->endpoint);
+      grpc_channel_args_destroy(args->args);
       grpc_slice_buffer_destroy(args->read_buffer);
       gpr_free(args->read_buffer);
     }
+  } else {
+    grpc_transport *transport =
+        grpc_create_chttp2_transport(exec_ctx, args->args, args->endpoint, 0);
+    grpc_server_setup_transport(
+        exec_ctx, connection_state->server_state->server, transport,
+        connection_state->accepting_pollset,
+        grpc_server_get_channel_args(connection_state->server_state->server));
+    grpc_chttp2_transport_start_reading(exec_ctx, transport,
+                                        args->read_buffer);
     grpc_channel_args_destroy(args->args);
   }
   pending_handshake_manager_remove_locked(connection_state->server_state,
diff --git a/src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.c b/src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.c
index afdf93398f..19aa22c7c4 100644
--- a/src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.c
+++ b/src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.c
@@ -117,34 +117,38 @@ static void on_handshake_done(grpc_exec_ctx *exec_ctx, void *arg,
                               grpc_error *error) {
   grpc_handshaker_args *args = arg;
   server_secure_connection_state *connection_state = args->user_data;
-  if (error != GRPC_ERROR_NONE) {
+  gpr_mu_lock(&connection_state->server_state->mu);
+  if (error != GRPC_ERROR_NONE || connection_state->server_state->shutdown) {
     const char *error_str = grpc_error_string(error);
     gpr_log(GPR_ERROR, "Handshaking failed: %s", error_str);
     grpc_error_free_string(error_str);
-    gpr_mu_lock(&connection_state->server_state->mu);
-  } else {
-    gpr_mu_lock(&connection_state->server_state->mu);
-    if (!connection_state->server_state->shutdown) {
-      grpc_arg channel_arg = grpc_server_credentials_to_arg(
-          connection_state->server_state->creds);
-      grpc_channel_args *args_copy =
-          grpc_channel_args_copy_and_add(args->args, &channel_arg, 1);
-      grpc_transport *transport =
-          grpc_create_chttp2_transport(exec_ctx, args_copy, args->endpoint, 0);
-      grpc_server_setup_transport(
-          exec_ctx, connection_state->server_state->server, transport,
-          connection_state->accepting_pollset, args_copy);
-      grpc_channel_args_destroy(args_copy);
-      grpc_chttp2_transport_start_reading(exec_ctx, transport,
-                                          args->read_buffer);
-    } else {
-      // Need to destroy this here, because the server may have already
-      // gone away.
+    if (error == GRPC_ERROR_NONE) {
+      // We were shut down after handshaking completed successfully, so
+      // destroy the endpoint here.
+      // TODO(ctiller): It is currently necessary to shutdown endpoints
+      // before destroying them, even if we know that there are no
+      // pending read/write callbacks.  This should be fixed, at which
+      // point this can be removed.
+      grpc_endpoint_shutdown(exec_ctx, args->endpoint);
       grpc_endpoint_destroy(exec_ctx, args->endpoint);
+      grpc_channel_args_destroy(args->args);
       grpc_slice_buffer_destroy(args->read_buffer);
       gpr_free(args->read_buffer);
     }
+  } else {
+    grpc_arg channel_arg = grpc_server_credentials_to_arg(
+	connection_state->server_state->creds);
+    grpc_channel_args *args_copy =
+	grpc_channel_args_copy_and_add(args->args, &channel_arg, 1);
     grpc_channel_args_destroy(args->args);
+    grpc_transport *transport =
+	grpc_create_chttp2_transport(exec_ctx, args_copy, args->endpoint, 0);
+    grpc_server_setup_transport(
+	exec_ctx, connection_state->server_state->server, transport,
+	connection_state->accepting_pollset, args_copy);
+    grpc_channel_args_destroy(args_copy);
+    grpc_chttp2_transport_start_reading(exec_ctx, transport,
+					args->read_buffer);
   }
   pending_handshake_manager_remove_locked(connection_state->server_state,
                                           connection_state->handshake_mgr);