From b62f364d0a6a4413c1535dd5fbbfea288142fc02 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 30 Nov 2016 11:41:25 -0800 Subject: [PATCH] Fix shutdown semantics for client and server code. --- .../chttp2/client/chttp2_connector.c | 14 ++++-- .../chttp2/server/insecure/server_chttp2.c | 36 ++++++++------- .../server/secure/server_secure_chttp2.c | 44 ++++++++++--------- 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 43649376a16..6341682adee 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 9284d193570..bdf10de49e8 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 afdf93398f6..19aa22c7c43 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);