From 53bd69330ca12c8d430997283402a50cbe307e2f Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 1 Dec 2016 08:00:38 -0800 Subject: Code review changes. --- .../ext/client_channel/http_connect_handshaker.c | 3 +++ .../chttp2/client/insecure/channel_create.c | 23 ++++++++++-------- .../chttp2/client/secure/secure_channel_create.c | 27 ++++++++++++---------- src/core/lib/channel/handshaker.h | 2 +- 4 files changed, 33 insertions(+), 22 deletions(-) (limited to 'src') diff --git a/src/core/ext/client_channel/http_connect_handshaker.c b/src/core/ext/client_channel/http_connect_handshaker.c index 6a34e390bc..e0b717dac5 100644 --- a/src/core/ext/client_channel/http_connect_handshaker.c +++ b/src/core/ext/client_channel/http_connect_handshaker.c @@ -228,6 +228,9 @@ static void on_read_done(grpc_exec_ctx* exec_ctx, void* arg, // Success. Invoke handshake-done callback. grpc_exec_ctx_sched(exec_ctx, handshaker->on_handshake_done, error, NULL); done: + // Set shutdown to true so that subsequent calls to + // http_connect_handshaker_shutdown() do nothing. + handshaker->shutdown = true; gpr_mu_unlock(&handshaker->mu); http_connect_handshaker_unref(exec_ctx, handshaker); } diff --git a/src/core/ext/transport/chttp2/client/insecure/channel_create.c b/src/core/ext/transport/chttp2/client/insecure/channel_create.c index 00b272de27..d42e6e98aa 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.c @@ -39,6 +39,7 @@ #include #include #include +#include #include "src/core/ext/client_channel/client_channel.h" #include "src/core/ext/client_channel/http_connect_handshaker.h" @@ -60,6 +61,8 @@ typedef struct { grpc_connector base; gpr_refcount refs; + char *server_name; + grpc_closure *notify; grpc_connect_in_args args; grpc_connect_out_args *result; @@ -82,7 +85,7 @@ static void connector_unref(grpc_exec_ctx *exec_ctx, grpc_connector *con) { connector *c = (connector *)con; if (gpr_unref(&c->refs)) { /* c->initial_string_buffer does not need to be destroyed */ - grpc_handshake_manager_destroy(exec_ctx, c->handshake_mgr); + gpr_free(c->server_name); gpr_free(c); } } @@ -107,6 +110,7 @@ static void on_handshake_done(grpc_exec_ctx *exec_ctx, void *arg, grpc_closure *notify = c->notify; c->notify = NULL; grpc_exec_ctx_sched(exec_ctx, notify, GRPC_ERROR_REF(error), NULL); + grpc_handshake_manager_destroy(exec_ctx, c->handshake_mgr); } static void connected(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { @@ -123,6 +127,14 @@ static void connected(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { grpc_endpoint_write(exec_ctx, tcp, &c->initial_string_buffer, &c->initial_string_sent); } else { + c->handshake_mgr = grpc_handshake_manager_create(); + char *proxy_name = grpc_get_http_proxy_server(); + if (proxy_name != NULL) { + grpc_handshake_manager_add( + c->handshake_mgr, + grpc_http_connect_handshaker_create(proxy_name, c->server_name)); + gpr_free(proxy_name); + } grpc_handshake_manager_do_handshake( exec_ctx, c->handshake_mgr, tcp, c->args.channel_args, c->args.deadline, NULL /* acceptor */, on_handshake_done, c); @@ -174,14 +186,7 @@ static grpc_subchannel *client_channel_factory_create_subchannel( memset(c, 0, sizeof(*c)); c->base.vtable = &connector_vtable; gpr_ref_init(&c->refs, 1); - c->handshake_mgr = grpc_handshake_manager_create(); - char *proxy_name = grpc_get_http_proxy_server(); - if (proxy_name != NULL) { - grpc_handshake_manager_add( - c->handshake_mgr, - grpc_http_connect_handshaker_create(proxy_name, args->server_name)); - gpr_free(proxy_name); - } + c->server_name = gpr_strdup(args->server_name); grpc_subchannel *s = grpc_subchannel_create(exec_ctx, &c->base, args); grpc_connector_unref(exec_ctx, &c->base); return s; diff --git a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c index b4a30f94fc..93023af275 100644 --- a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c +++ b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c @@ -39,6 +39,7 @@ #include #include #include +#include #include "src/core/ext/client_channel/client_channel.h" #include "src/core/ext/client_channel/http_connect_handshaker.h" @@ -63,6 +64,7 @@ typedef struct { gpr_refcount refs; grpc_channel_security_connector *security_connector; + char *server_name; grpc_closure *notify; grpc_connect_in_args args; @@ -92,7 +94,7 @@ static void connector_unref(grpc_exec_ctx *exec_ctx, grpc_connector *con) { if (gpr_unref(&c->refs)) { /* c->initial_string_buffer does not need to be destroyed */ grpc_channel_args_destroy(c->tmp_args); - grpc_handshake_manager_destroy(exec_ctx, c->handshake_mgr); + gpr_free(c->server_name); gpr_free(c); } } @@ -146,11 +148,20 @@ static void on_handshake_done(grpc_exec_ctx *exec_ctx, void *arg, exec_ctx, c->security_connector, args->endpoint, args->read_buffer, c->args.deadline, on_secure_handshake_done, c); } + grpc_handshake_manager_destroy(exec_ctx, c->handshake_mgr); } static void on_initial_connect_string_sent(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { connector *c = arg; + c->handshake_mgr = grpc_handshake_manager_create(); + char *proxy_name = grpc_get_http_proxy_server(); + if (proxy_name != NULL) { + grpc_handshake_manager_add( + c->handshake_mgr, + grpc_http_connect_handshaker_create(proxy_name, c->server_name)); + gpr_free(proxy_name); + } grpc_handshake_manager_do_handshake( exec_ctx, c->handshake_mgr, c->connecting_endpoint, c->args.channel_args, c->args.deadline, NULL /* acceptor */, on_handshake_done, c); @@ -173,9 +184,8 @@ static void connected(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { grpc_endpoint_write(exec_ctx, tcp, &c->initial_string_buffer, &c->initial_string_sent); } else { - grpc_handshake_manager_do_handshake( - exec_ctx, c->handshake_mgr, tcp, c->args.channel_args, - c->args.deadline, NULL /* acceptor */, on_handshake_done, c); + // Start handshaking. + on_initial_connect_string_sent(exec_ctx, c, error); } } else { memset(c->result, 0, sizeof(*c->result)); @@ -252,14 +262,7 @@ static grpc_subchannel *client_channel_factory_create_subchannel( memset(c, 0, sizeof(*c)); c->base.vtable = &connector_vtable; c->security_connector = f->security_connector; - c->handshake_mgr = grpc_handshake_manager_create(); - char *proxy_name = grpc_get_http_proxy_server(); - if (proxy_name != NULL) { - grpc_handshake_manager_add( - c->handshake_mgr, - grpc_http_connect_handshaker_create(proxy_name, args->server_name)); - gpr_free(proxy_name); - } + c->server_name = gpr_strdup(args->server_name); gpr_mu_init(&c->mu); gpr_ref_init(&c->refs, 1); grpc_subchannel *s = grpc_subchannel_create(exec_ctx, &c->base, args); diff --git a/src/core/lib/channel/handshaker.h b/src/core/lib/channel/handshaker.h index 2e1f543512..ebbc1ff7f3 100644 --- a/src/core/lib/channel/handshaker.h +++ b/src/core/lib/channel/handshaker.h @@ -133,7 +133,7 @@ void grpc_handshake_manager_shutdown(grpc_exec_ctx* exec_ctx, /// Invokes handshakers in the order they were added. /// Takes ownership of \a endpoint, and then passes that ownership to /// the \a on_handshake_done callback. -/// Does NOT take ownership of \a args. Instead, makes a copy before +/// Does NOT take ownership of \a channel_args. Instead, makes a copy before /// invoking the first handshaker. /// \a acceptor will be NULL for client-side handshakers. /// -- cgit v1.2.3