From 21d4b2d930642b2b8d272352dfa982b5102efd1e Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 18 Nov 2016 09:53:41 -0800 Subject: [PATCH] Pass client channel factory and server name via channel args. --- include/grpc/impl/codegen/grpc_types.h | 2 + src/core/ext/client_channel/client_channel.c | 48 +++++-------- src/core/ext/client_channel/client_channel.h | 7 -- .../chttp2/client/insecure/channel_create.c | 50 ++++++++------ .../client/secure/secure_channel_create.c | 67 ++++++++++++------- 5 files changed, 93 insertions(+), 81 deletions(-) diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index 8c5215faee8..5ecc5ba0435 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -220,6 +220,8 @@ typedef struct { #define GRPC_ARG_LB_ADDRESSES "grpc.lb_addresses" /** The grpc_socket_mutator instance that set the socket options. A pointer. */ #define GRPC_ARG_SOCKET_MUTATOR "grpc.socket_mutator" +/** Client channel factory. Not intended for external use. */ +#define GRPC_ARG_CLIENT_CHANNEL_FACTORY "grpc.client_channel_factory" /** \} */ /** Result of a grpc call. If the caller satisfies the prerequisites of a diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index b66fed4b88a..a607f73c04c 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -44,6 +44,7 @@ #include #include "src/core/ext/client_channel/lb_policy_registry.h" +#include "src/core/ext/client_channel/resolver_registry.h" #include "src/core/ext/client_channel/subchannel.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/connected_channel.h" @@ -454,20 +455,31 @@ static void cc_init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { channel_data *chand = elem->channel_data; - memset(chand, 0, sizeof(*chand)); - GPR_ASSERT(args->is_last); GPR_ASSERT(elem->filter == &grpc_client_channel_filter); - + // Initialize data members. gpr_mu_init(&chand->mu); + chand->owning_stack = args->channel_stack; grpc_closure_init(&chand->on_resolver_result_changed, on_resolver_result_changed, chand); - chand->owning_stack = args->channel_stack; - + chand->interested_parties = grpc_pollset_set_create(); grpc_connectivity_state_init(&chand->state_tracker, GRPC_CHANNEL_IDLE, "client_channel"); - chand->interested_parties = grpc_pollset_set_create(); + // Record client channel factory. + const grpc_arg *arg = grpc_channel_args_find(args->channel_args, + GRPC_ARG_CLIENT_CHANNEL_FACTORY); + GPR_ASSERT(arg != NULL); + GPR_ASSERT(arg->type == GRPC_ARG_POINTER); + grpc_client_channel_factory_ref(arg->value.pointer.p); + chand->client_channel_factory = arg->value.pointer.p; + // Instantiate resolver. + arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_SERVER_NAME); + GPR_ASSERT(arg != NULL); + GPR_ASSERT(arg->type == GRPC_ARG_STRING); + chand->resolver = grpc_resolver_create(arg->value.string, args->channel_args); +// FIXME: return failure instead of asserting + GPR_ASSERT(chand->resolver != NULL); } /* Destructor for channel_data */ @@ -1080,30 +1092,6 @@ const grpc_channel_filter grpc_client_channel_filter = { "client-channel", }; -void grpc_client_channel_finish_initialization( - grpc_exec_ctx *exec_ctx, grpc_channel_stack *channel_stack, - grpc_resolver *resolver, - grpc_client_channel_factory *client_channel_factory) { - /* post construction initialization: set the transport setup pointer */ - GPR_ASSERT(client_channel_factory != NULL); - grpc_channel_element *elem = grpc_channel_stack_last_element(channel_stack); - channel_data *chand = elem->channel_data; - gpr_mu_lock(&chand->mu); - GPR_ASSERT(!chand->resolver); - chand->resolver = resolver; - GRPC_RESOLVER_REF(resolver, "channel"); - if (!grpc_closure_list_empty(chand->waiting_for_config_closures) || - chand->exit_idle_when_lb_policy_arrives) { - chand->started_resolving = true; - GRPC_CHANNEL_STACK_REF(chand->owning_stack, "resolver"); - grpc_resolver_next(exec_ctx, resolver, &chand->resolver_result, - &chand->on_resolver_result_changed); - } - chand->client_channel_factory = client_channel_factory; - grpc_client_channel_factory_ref(client_channel_factory); - gpr_mu_unlock(&chand->mu); -} - grpc_connectivity_state grpc_client_channel_check_connectivity_state( grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, int try_to_connect) { channel_data *chand = elem->channel_data; diff --git a/src/core/ext/client_channel/client_channel.h b/src/core/ext/client_channel/client_channel.h index ab5a84fdfbe..9ba012865cf 100644 --- a/src/core/ext/client_channel/client_channel.h +++ b/src/core/ext/client_channel/client_channel.h @@ -47,13 +47,6 @@ extern const grpc_channel_filter grpc_client_channel_filter; -/* Post-construction initializer to give the client channel its resolver - and factory. */ -void grpc_client_channel_finish_initialization( - grpc_exec_ctx *exec_ctx, grpc_channel_stack *channel_stack, - grpc_resolver *resolver, - grpc_client_channel_factory *client_channel_factory); - grpc_connectivity_state grpc_client_channel_check_connectivity_state( grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, int try_to_connect); 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 8e03fd82c11..d448b909920 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.c @@ -42,7 +42,6 @@ #include "src/core/ext/client_channel/client_channel.h" #include "src/core/ext/client_channel/http_connect_handshaker.h" -#include "src/core/ext/client_channel/resolver_registry.h" #include "src/core/ext/transport/chttp2/transport/chttp2_transport.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/compress_filter.h" @@ -195,20 +194,7 @@ static grpc_channel *client_channel_factory_create_channel( grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *cc_factory, const char *target, grpc_client_channel_type type, const grpc_channel_args *args) { - grpc_channel *channel = - grpc_channel_create(exec_ctx, target, args, GRPC_CLIENT_CHANNEL, NULL); - grpc_resolver *resolver = grpc_resolver_create(target, args); - if (!resolver) { - GRPC_CHANNEL_INTERNAL_UNREF(exec_ctx, channel, - "client_channel_factory_create_channel"); - return NULL; - } - - grpc_client_channel_finish_initialization( - exec_ctx, grpc_channel_get_channel_stack(channel), resolver, cc_factory); - GRPC_RESOLVER_UNREF(exec_ctx, resolver, "create_channel"); - - return channel; + return grpc_channel_create(exec_ctx, target, args, GRPC_CLIENT_CHANNEL, NULL); } static const grpc_client_channel_factory_vtable client_channel_factory_vtable = @@ -219,6 +205,21 @@ static const grpc_client_channel_factory_vtable client_channel_factory_vtable = static grpc_client_channel_factory client_channel_factory = { &client_channel_factory_vtable}; +static void *cc_factory_arg_copy(void *cc_factory) { + return cc_factory; +} + +static void cc_factory_arg_destroy(void *cc_factory) {} + +static int cc_factory_arg_cmp(void *cc_factory1, void *cc_factory2) { + if (cc_factory1 < cc_factory2) return -1; + if (cc_factory1 > cc_factory2) return 1; + return 0; +} + +static const grpc_arg_pointer_vtable cc_factory_arg_vtable = { + cc_factory_arg_copy, cc_factory_arg_destroy, cc_factory_arg_cmp}; + /* Create a client channel: Asynchronously: - resolve target - connect to it (trying alternatives as presented) @@ -231,15 +232,26 @@ grpc_channel *grpc_insecure_channel_create(const char *target, "grpc_insecure_channel_create(target=%p, args=%p, reserved=%p)", 3, (target, args, reserved)); GPR_ASSERT(!reserved); - grpc_client_channel_factory *factory = (grpc_client_channel_factory *)&client_channel_factory; + // Add channel args containing the server name and client channel factory. + grpc_arg new_args[2]; + new_args[0].type = GRPC_ARG_STRING; + new_args[0].key = GRPC_ARG_SERVER_NAME; + new_args[0].value.string = (char *)target; + new_args[1].type = GRPC_ARG_POINTER; + new_args[1].key = GRPC_ARG_CLIENT_CHANNEL_FACTORY; + new_args[1].value.pointer.p = factory; + new_args[1].value.pointer.vtable = &cc_factory_arg_vtable; + grpc_channel_args *args_copy = + grpc_channel_args_copy_and_add(args, new_args, GPR_ARRAY_SIZE(new_args)); + // Create channel. grpc_channel *channel = client_channel_factory_create_channel( - &exec_ctx, factory, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, args); - + &exec_ctx, factory, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, args_copy); + // Clean up. + grpc_channel_args_destroy(args_copy); grpc_client_channel_factory_unref(&exec_ctx, factory); grpc_exec_ctx_finish(&exec_ctx); - return channel != NULL ? channel : grpc_lame_client_channel_create( target, GRPC_STATUS_INTERNAL, "Failed to create client channel"); 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 04c88a2d368..b53e637fc25 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 @@ -42,7 +42,6 @@ #include "src/core/ext/client_channel/client_channel.h" #include "src/core/ext/client_channel/http_connect_handshaker.h" -#include "src/core/ext/client_channel/resolver_registry.h" #include "src/core/ext/transport/chttp2/transport/chttp2_transport.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/handshaker.h" @@ -273,20 +272,7 @@ static grpc_channel *client_channel_factory_create_channel( grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *cc_factory, const char *target, grpc_client_channel_type type, const grpc_channel_args *args) { - client_channel_factory *f = (client_channel_factory *)cc_factory; - grpc_channel *channel = - grpc_channel_create(exec_ctx, target, args, GRPC_CLIENT_CHANNEL, NULL); - grpc_resolver *resolver = grpc_resolver_create(target, args); - if (resolver != NULL) { - grpc_client_channel_finish_initialization( - exec_ctx, grpc_channel_get_channel_stack(channel), resolver, &f->base); - GRPC_RESOLVER_UNREF(exec_ctx, resolver, "create"); - } else { - GRPC_CHANNEL_INTERNAL_UNREF(exec_ctx, channel, - "client_channel_factory_create_channel"); - channel = NULL; - } - return channel; + return grpc_channel_create(exec_ctx, target, args, GRPC_CLIENT_CHANNEL, NULL); } static const grpc_client_channel_factory_vtable client_channel_factory_vtable = @@ -294,6 +280,28 @@ static const grpc_client_channel_factory_vtable client_channel_factory_vtable = client_channel_factory_create_subchannel, client_channel_factory_create_channel}; +static void *cc_factory_arg_copy(void *cc_factory) { + client_channel_factory_ref(cc_factory); + return cc_factory; +} + +static void cc_factory_arg_destroy(void *cc_factory) { + // TODO(roth): remove local exec_ctx when + // https://github.com/grpc/grpc/pull/8705 is merged + grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; + client_channel_factory_unref(&exec_ctx, cc_factory); + grpc_exec_ctx_finish(&exec_ctx); +} + +static int cc_factory_arg_cmp(void *cc_factory1, void *cc_factory2) { + if (cc_factory1 < cc_factory2) return -1; + if (cc_factory1 > cc_factory2) return 1; + return 0; +} + +static const grpc_arg_pointer_vtable cc_factory_arg_vtable = { + cc_factory_arg_copy, cc_factory_arg_destroy, cc_factory_arg_cmp}; + /* Create a secure client channel: Asynchronously: - resolve target - connect to it (trying alternatives as presented) @@ -326,14 +334,6 @@ grpc_channel *grpc_secure_channel_create(grpc_channel_credentials *creds, return grpc_lame_client_channel_create( target, GRPC_STATUS_INTERNAL, "Failed to create security connector."); } - grpc_arg connector_arg = - grpc_security_connector_to_arg(&security_connector->base); - grpc_channel_args *new_args = grpc_channel_args_copy_and_add( - new_args_from_connector != NULL ? new_args_from_connector : args, - &connector_arg, 1); - if (new_args_from_connector != NULL) { - grpc_channel_args_destroy(new_args_from_connector); - } // Create client channel factory. client_channel_factory *f = gpr_malloc(sizeof(*f)); memset(f, 0, sizeof(*f)); @@ -342,13 +342,30 @@ grpc_channel *grpc_secure_channel_create(grpc_channel_credentials *creds, GRPC_SECURITY_CONNECTOR_REF(&security_connector->base, "grpc_secure_channel_create"); f->security_connector = security_connector; + // Add channel args containing the server name, client channel + // factory, and security connector. + grpc_arg new_args[3]; + new_args[0].type = GRPC_ARG_STRING; + new_args[0].key = GRPC_ARG_SERVER_NAME; + new_args[0].value.string = (char *)target; + new_args[1].type = GRPC_ARG_POINTER; + new_args[1].key = GRPC_ARG_CLIENT_CHANNEL_FACTORY; + new_args[1].value.pointer.p = f; + new_args[1].value.pointer.vtable = &cc_factory_arg_vtable; + new_args[2] = grpc_security_connector_to_arg(&security_connector->base); + grpc_channel_args *args_copy = grpc_channel_args_copy_and_add( + new_args_from_connector != NULL ? new_args_from_connector : args, + new_args, GPR_ARRAY_SIZE(new_args)); + if (new_args_from_connector != NULL) { + grpc_channel_args_destroy(new_args_from_connector); + } // Create channel. grpc_channel *channel = client_channel_factory_create_channel( - &exec_ctx, &f->base, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, new_args); + &exec_ctx, &f->base, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, args_copy); // Clean up. GRPC_SECURITY_CONNECTOR_UNREF(&f->security_connector->base, "secure_client_channel_factory_create_channel"); - grpc_channel_args_destroy(new_args); + grpc_channel_args_destroy(args_copy); grpc_client_channel_factory_unref(&exec_ctx, &f->base); grpc_exec_ctx_finish(&exec_ctx); return channel; /* may be NULL */