From 0e48a9af490c5c48802ca1f3faab36e022cfca49 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 8 Sep 2016 14:14:39 -0700 Subject: [PATCH 1/9] Move LB policy instantiation from resolvers into client_channel. --- src/core/ext/client_config/client_channel.c | 29 ++++++++---- src/core/ext/client_config/client_channel.h | 13 ++--- src/core/ext/client_config/resolver_factory.h | 5 +- .../ext/client_config/resolver_registry.c | 4 +- .../ext/client_config/resolver_registry.h | 3 +- src/core/ext/client_config/resolver_result.c | 47 ++++++++++--------- src/core/ext/client_config/resolver_result.h | 19 +++++--- .../ext/resolver/dns/native/dns_resolver.c | 27 ++--------- .../ext/resolver/sockaddr/sockaddr_resolver.c | 18 +------ .../chttp2/client/insecure/channel_create.c | 6 +-- .../client/secure/secure_channel_create.c | 6 +-- 11 files changed, 81 insertions(+), 96 deletions(-) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index 61e012578e0..2e6f253d386 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -42,6 +42,7 @@ #include #include +#include "src/core/ext/client_config/lb_policy_registry.h" #include "src/core/ext/client_config/subchannel.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/connected_channel.h" @@ -63,6 +64,8 @@ typedef struct client_channel_channel_data { grpc_resolver *resolver; /** have we started resolving this channel */ bool started_resolving; + /** client channel factory */ + grpc_client_channel_factory *client_channel_factory; /** mutex protecting client configuration, including all variables below in this data structure */ @@ -173,20 +176,24 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *state_error = GRPC_ERROR_CREATE("No load balancing policy"); if (chand->resolver_result != NULL) { - lb_policy = grpc_resolver_result_get_lb_policy(chand->resolver_result); + grpc_lb_policy_args lb_policy_args; + lb_policy_args.addresses = + grpc_resolver_result_get_addresses(chand->resolver_result); + lb_policy_args.client_channel_factory = chand->client_channel_factory; + lb_policy = grpc_lb_policy_create( + exec_ctx, + grpc_resolver_result_get_lb_policy_name(chand->resolver_result), + &lb_policy_args); if (lb_policy != NULL) { - GRPC_LB_POLICY_REF(lb_policy, "channel"); GRPC_LB_POLICY_REF(lb_policy, "config_change"); GRPC_ERROR_UNREF(state_error); state = grpc_lb_policy_check_connectivity(exec_ctx, lb_policy, &state_error); } - grpc_resolver_result_unref(exec_ctx, chand->resolver_result); + chand->resolver_result = NULL; } - chand->resolver_result = NULL; - if (lb_policy != NULL) { grpc_pollset_set_add_pollset_set(exec_ctx, lb_policy->interested_parties, chand->interested_parties); @@ -346,6 +353,9 @@ static void cc_destroy_channel_elem(grpc_exec_ctx *exec_ctx, grpc_resolver_shutdown(exec_ctx, chand->resolver); GRPC_RESOLVER_UNREF(exec_ctx, chand->resolver, "channel"); } + if (chand->client_channel_factory != NULL) { + grpc_client_channel_factory_unref(exec_ctx, chand->client_channel_factory); + } if (chand->lb_policy != NULL) { grpc_pollset_set_del_pollset_set(exec_ctx, chand->lb_policy->interested_parties, @@ -759,9 +769,10 @@ const grpc_channel_filter grpc_client_channel_filter = { "client-channel", }; -void grpc_client_channel_set_resolver(grpc_exec_ctx *exec_ctx, - grpc_channel_stack *channel_stack, - grpc_resolver *resolver) { +void grpc_client_channel_set_resolver_and_client_channel_factory( + 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 */ grpc_channel_element *elem = grpc_channel_stack_last_element(channel_stack); channel_data *chand = elem->channel_data; @@ -776,6 +787,8 @@ void grpc_client_channel_set_resolver(grpc_exec_ctx *exec_ctx, 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); } diff --git a/src/core/ext/client_config/client_channel.h b/src/core/ext/client_config/client_channel.h index 1e47ad34ad8..b3a9bc79951 100644 --- a/src/core/ext/client_config/client_channel.h +++ b/src/core/ext/client_config/client_channel.h @@ -34,6 +34,7 @@ #ifndef GRPC_CORE_EXT_CLIENT_CONFIG_CLIENT_CHANNEL_H #define GRPC_CORE_EXT_CLIENT_CONFIG_CLIENT_CHANNEL_H +#include "src/core/ext/client_config/client_channel_factory.h" #include "src/core/ext/client_config/resolver.h" #include "src/core/lib/channel/channel_stack.h" @@ -46,12 +47,12 @@ extern const grpc_channel_filter grpc_client_channel_filter; -/* post-construction initializer to let the client channel know which - transport setup it should cancel upon destruction, or initiate when it needs - a connection */ -void grpc_client_channel_set_resolver(grpc_exec_ctx *exec_ctx, - grpc_channel_stack *channel_stack, - grpc_resolver *resolver); +/* Post-construction initializer to give the client channel its resolver + and factory. */ +void grpc_client_channel_set_resolver_and_client_channel_factory( + 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/client_config/resolver_factory.h b/src/core/ext/client_config/resolver_factory.h index f69bf795649..9ec5b9a70e7 100644 --- a/src/core/ext/client_config/resolver_factory.h +++ b/src/core/ext/client_config/resolver_factory.h @@ -47,10 +47,7 @@ struct grpc_resolver_factory { const grpc_resolver_factory_vtable *vtable; }; -typedef struct grpc_resolver_args { - grpc_uri *uri; - grpc_client_channel_factory *client_channel_factory; -} grpc_resolver_args; +typedef struct grpc_resolver_args { grpc_uri *uri; } grpc_resolver_args; struct grpc_resolver_factory_vtable { void (*ref)(grpc_resolver_factory *factory); diff --git a/src/core/ext/client_config/resolver_registry.c b/src/core/ext/client_config/resolver_registry.c index e7a4abd568c..b5308a140c9 100644 --- a/src/core/ext/client_config/resolver_registry.c +++ b/src/core/ext/client_config/resolver_registry.c @@ -128,15 +128,13 @@ static grpc_resolver_factory *resolve_factory(const char *target, return factory; } -grpc_resolver *grpc_resolver_create( - const char *target, grpc_client_channel_factory *client_channel_factory) { +grpc_resolver *grpc_resolver_create(const char *target) { grpc_uri *uri = NULL; grpc_resolver_factory *factory = resolve_factory(target, &uri); grpc_resolver *resolver; grpc_resolver_args args; memset(&args, 0, sizeof(args)); args.uri = uri; - args.client_channel_factory = client_channel_factory; resolver = grpc_resolver_factory_create_resolver(factory, &args); grpc_uri_destroy(uri); return resolver; diff --git a/src/core/ext/client_config/resolver_registry.h b/src/core/ext/client_config/resolver_registry.h index 5ef1383cd35..92e248d5488 100644 --- a/src/core/ext/client_config/resolver_registry.h +++ b/src/core/ext/client_config/resolver_registry.h @@ -55,8 +55,7 @@ void grpc_register_resolver_type(grpc_resolver_factory *factory); If a resolver factory was found, use it to instantiate a resolver and return it. If a resolver factory was not found, return NULL. */ -grpc_resolver *grpc_resolver_create( - const char *target, grpc_client_channel_factory *client_channel_factory); +grpc_resolver *grpc_resolver_create(const char *target); /** Find a resolver factory given a name and return an (owned-by-the-caller) * reference to it */ diff --git a/src/core/ext/client_config/resolver_result.c b/src/core/ext/client_config/resolver_result.c index e14f761f05e..ba21349d2d5 100644 --- a/src/core/ext/client_config/resolver_result.c +++ b/src/core/ext/client_config/resolver_result.c @@ -36,6 +36,7 @@ #include #include +#include grpc_addresses *grpc_addresses_create(size_t num_addresses) { grpc_addresses *addresses = gpr_malloc(sizeof(grpc_addresses)); @@ -63,37 +64,39 @@ void grpc_addresses_destroy(grpc_addresses *addresses) { struct grpc_resolver_result { gpr_refcount refs; - grpc_lb_policy *lb_policy; + grpc_addresses *addresses; + char *lb_policy_name; }; -grpc_resolver_result *grpc_resolver_result_create() { - grpc_resolver_result *c = gpr_malloc(sizeof(*c)); - memset(c, 0, sizeof(*c)); - gpr_ref_init(&c->refs, 1); - return c; +grpc_resolver_result *grpc_resolver_result_create(grpc_addresses *addresses, + const char *lb_policy_name) { + grpc_resolver_result *result = gpr_malloc(sizeof(*result)); + memset(result, 0, sizeof(*result)); + gpr_ref_init(&result->refs, 1); + result->addresses = addresses; + result->lb_policy_name = gpr_strdup(lb_policy_name); + return result; } -void grpc_resolver_result_ref(grpc_resolver_result *c) { gpr_ref(&c->refs); } +void grpc_resolver_result_ref(grpc_resolver_result *result) { + gpr_ref(&result->refs); +} void grpc_resolver_result_unref(grpc_exec_ctx *exec_ctx, - grpc_resolver_result *c) { - if (gpr_unref(&c->refs)) { - if (c->lb_policy != NULL) { - GRPC_LB_POLICY_UNREF(exec_ctx, c->lb_policy, "resolver_result"); - } - gpr_free(c); + grpc_resolver_result *result) { + if (gpr_unref(&result->refs)) { + grpc_addresses_destroy(result->addresses); + gpr_free(result->lb_policy_name); + gpr_free(result); } } -void grpc_resolver_result_set_lb_policy(grpc_resolver_result *c, - grpc_lb_policy *lb_policy) { - GPR_ASSERT(c->lb_policy == NULL); - if (lb_policy) { - GRPC_LB_POLICY_REF(lb_policy, "resolver_result"); - } - c->lb_policy = lb_policy; +grpc_addresses *grpc_resolver_result_get_addresses( + grpc_resolver_result *result) { + return result->addresses; } -grpc_lb_policy *grpc_resolver_result_get_lb_policy(grpc_resolver_result *c) { - return c->lb_policy; +const char *grpc_resolver_result_get_lb_policy_name( + grpc_resolver_result *result) { + return result->lb_policy_name; } diff --git a/src/core/ext/client_config/resolver_result.h b/src/core/ext/client_config/resolver_result.h index 4199ef512ad..df92bec984e 100644 --- a/src/core/ext/client_config/resolver_result.h +++ b/src/core/ext/client_config/resolver_result.h @@ -63,14 +63,19 @@ void grpc_addresses_destroy(grpc_addresses *addresses); /** Results reported from a grpc_resolver. */ typedef struct grpc_resolver_result grpc_resolver_result; -grpc_resolver_result *grpc_resolver_result_create(); -void grpc_resolver_result_ref(grpc_resolver_result *client_config); +/** Takes ownership of \a addresses. */ +grpc_resolver_result *grpc_resolver_result_create(grpc_addresses *addresses, + const char *lb_policy_name); +void grpc_resolver_result_ref(grpc_resolver_result *result); void grpc_resolver_result_unref(grpc_exec_ctx *exec_ctx, - grpc_resolver_result *client_config); + grpc_resolver_result *result); -void grpc_resolver_result_set_lb_policy(grpc_resolver_result *client_config, - grpc_lb_policy *lb_policy); -grpc_lb_policy *grpc_resolver_result_get_lb_policy( - grpc_resolver_result *client_config); +/** Caller does NOT take ownership of result. */ +grpc_addresses *grpc_resolver_result_get_addresses( + grpc_resolver_result *result); + +/** Caller does NOT take ownership of result. */ +const char *grpc_resolver_result_get_lb_policy_name( + grpc_resolver_result *result); #endif /* GRPC_CORE_EXT_CLIENT_CONFIG_RESOLVER_RESULT_H */ diff --git a/src/core/ext/resolver/dns/native/dns_resolver.c b/src/core/ext/resolver/dns/native/dns_resolver.c index 8fc10d98a80..78a996788dc 100644 --- a/src/core/ext/resolver/dns/native/dns_resolver.c +++ b/src/core/ext/resolver/dns/native/dns_resolver.c @@ -37,7 +37,6 @@ #include #include -#include "src/core/ext/client_config/lb_policy_registry.h" #include "src/core/ext/client_config/resolver_registry.h" #include "src/core/lib/iomgr/resolve_address.h" #include "src/core/lib/iomgr/timer.h" @@ -58,8 +57,6 @@ typedef struct { char *name; /** default port to use */ char *default_port; - /** subchannel factory */ - grpc_client_channel_factory *client_channel_factory; /** load balancing policy name */ char *lb_policy_name; @@ -166,29 +163,18 @@ static void dns_on_resolved(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { dns_resolver *r = arg; grpc_resolver_result *result = NULL; - grpc_lb_policy *lb_policy; gpr_mu_lock(&r->mu); GPR_ASSERT(r->resolving); r->resolving = 0; if (r->addresses != NULL) { - grpc_lb_policy_args lb_policy_args; - memset(&lb_policy_args, 0, sizeof(lb_policy_args)); - lb_policy_args.addresses = grpc_addresses_create(r->addresses->naddrs); + grpc_addresses *addresses = grpc_addresses_create(r->addresses->naddrs); for (size_t i = 0; i < r->addresses->naddrs; ++i) { - grpc_addresses_set_address( - lb_policy_args.addresses, i, &r->addresses->addrs[i].addr, - r->addresses->addrs[i].len, false /* is_balancer */); + grpc_addresses_set_address(addresses, i, &r->addresses->addrs[i].addr, + r->addresses->addrs[i].len, + false /* is_balancer */); } grpc_resolved_addresses_destroy(r->addresses); - lb_policy_args.client_channel_factory = r->client_channel_factory; - lb_policy = - grpc_lb_policy_create(exec_ctx, r->lb_policy_name, &lb_policy_args); - grpc_addresses_destroy(lb_policy_args.addresses); - result = grpc_resolver_result_create(); - if (lb_policy != NULL) { - grpc_resolver_result_set_lb_policy(result, lb_policy); - GRPC_LB_POLICY_UNREF(exec_ctx, lb_policy, "construction"); - } + result = grpc_resolver_result_create(addresses, r->lb_policy_name); } else { gpr_timespec now = gpr_now(GPR_CLOCK_MONOTONIC); gpr_timespec next_try = gpr_backoff_step(&r->backoff_state, now); @@ -249,7 +235,6 @@ static void dns_destroy(grpc_exec_ctx *exec_ctx, grpc_resolver *gr) { if (r->resolved_result) { grpc_resolver_result_unref(exec_ctx, r->resolved_result); } - grpc_client_channel_factory_unref(exec_ctx, r->client_channel_factory); gpr_free(r->name); gpr_free(r->default_port); gpr_free(r->lb_policy_name); @@ -276,10 +261,8 @@ static grpc_resolver *dns_create(grpc_resolver_args *args, grpc_resolver_init(&r->base, &dns_resolver_vtable); r->name = gpr_strdup(path); r->default_port = gpr_strdup(default_port); - r->client_channel_factory = args->client_channel_factory; gpr_backoff_init(&r->backoff_state, BACKOFF_MULTIPLIER, BACKOFF_JITTER, BACKOFF_MIN_SECONDS * 1000, BACKOFF_MAX_SECONDS * 1000); - grpc_client_channel_factory_ref(r->client_channel_factory); r->lb_policy_name = gpr_strdup(lb_policy_name); return &r->base; } diff --git a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c index 94d2f892ebb..328c7cb6f93 100644 --- a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c +++ b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c @@ -40,7 +40,6 @@ #include #include -#include "src/core/ext/client_config/lb_policy_registry.h" #include "src/core/ext/client_config/parse_address.h" #include "src/core/ext/client_config/resolver_registry.h" #include "src/core/lib/iomgr/resolve_address.h" @@ -52,8 +51,6 @@ typedef struct { grpc_resolver base; /** refcount */ gpr_refcount refs; - /** subchannel factory */ - grpc_client_channel_factory *client_channel_factory; /** load balancing policy name */ char *lb_policy_name; @@ -122,17 +119,9 @@ static void sockaddr_next(grpc_exec_ctx *exec_ctx, grpc_resolver *resolver, static void sockaddr_maybe_finish_next_locked(grpc_exec_ctx *exec_ctx, sockaddr_resolver *r) { if (r->next_completion != NULL && !r->published) { - grpc_resolver_result *result = grpc_resolver_result_create(); - grpc_lb_policy_args lb_policy_args; - memset(&lb_policy_args, 0, sizeof(lb_policy_args)); - lb_policy_args.addresses = r->addresses; - lb_policy_args.client_channel_factory = r->client_channel_factory; - grpc_lb_policy *lb_policy = - grpc_lb_policy_create(exec_ctx, r->lb_policy_name, &lb_policy_args); - grpc_resolver_result_set_lb_policy(result, lb_policy); - GRPC_LB_POLICY_UNREF(exec_ctx, lb_policy, "sockaddr"); r->published = true; - *r->target_result = result; + *r->target_result = + grpc_resolver_result_create(r->addresses, r->lb_policy_name); grpc_exec_ctx_sched(exec_ctx, r->next_completion, GRPC_ERROR_NONE, NULL); r->next_completion = NULL; } @@ -141,7 +130,6 @@ static void sockaddr_maybe_finish_next_locked(grpc_exec_ctx *exec_ctx, static void sockaddr_destroy(grpc_exec_ctx *exec_ctx, grpc_resolver *gr) { sockaddr_resolver *r = (sockaddr_resolver *)gr; gpr_mu_destroy(&r->mu); - grpc_client_channel_factory_unref(exec_ctx, r->client_channel_factory); grpc_addresses_destroy(r->addresses); gpr_free(r->lb_policy_name); gpr_free(r); @@ -243,8 +231,6 @@ static grpc_resolver *sockaddr_create( gpr_ref_init(&r->refs, 1); gpr_mu_init(&r->mu); grpc_resolver_init(&r->base, &sockaddr_resolver_vtable); - r->client_channel_factory = args->client_channel_factory; - grpc_client_channel_factory_ref(r->client_channel_factory); return &r->base; } 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 cbaa75a90af..550080c7d5e 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.c @@ -210,15 +210,15 @@ static grpc_channel *client_channel_factory_create_channel( grpc_channel *channel = grpc_channel_create(exec_ctx, target, final_args, GRPC_CLIENT_CHANNEL, NULL); grpc_channel_args_destroy(final_args); - grpc_resolver *resolver = grpc_resolver_create(target, &f->base); + grpc_resolver *resolver = grpc_resolver_create(target); if (!resolver) { GRPC_CHANNEL_INTERNAL_UNREF(exec_ctx, channel, "client_channel_factory_create_channel"); return NULL; } - grpc_client_channel_set_resolver( - exec_ctx, grpc_channel_get_channel_stack(channel), resolver); + grpc_client_channel_set_resolver_and_client_channel_factory( + exec_ctx, grpc_channel_get_channel_stack(channel), resolver, &f->base); GRPC_RESOLVER_UNREF(exec_ctx, resolver, "create_channel"); return 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 9e2bdd758f9..3285dbf6125 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 @@ -276,10 +276,10 @@ static grpc_channel *client_channel_factory_create_channel( GRPC_CLIENT_CHANNEL, NULL); grpc_channel_args_destroy(final_args); - grpc_resolver *resolver = grpc_resolver_create(target, &f->base); + grpc_resolver *resolver = grpc_resolver_create(target); if (resolver != NULL) { - grpc_client_channel_set_resolver( - exec_ctx, grpc_channel_get_channel_stack(channel), resolver); + grpc_client_channel_set_resolver_and_client_channel_factory( + 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, From 7309664d032c82724c12e79d8cbb2e18d842598e Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 9 Sep 2016 08:33:35 -0700 Subject: [PATCH 2/9] Avoid circular refcounting. --- .../transport/chttp2/client/insecure/channel_create.c | 11 +---------- .../chttp2/client/secure/secure_channel_create.c | 9 --------- 2 files changed, 1 insertion(+), 19 deletions(-) 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 550080c7d5e..5b9101e018b 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.c @@ -160,7 +160,6 @@ typedef struct { grpc_client_channel_factory base; gpr_refcount refs; grpc_channel_args *merge_args; - grpc_channel *master; } client_channel_factory; static void client_channel_factory_ref( @@ -173,10 +172,6 @@ static void client_channel_factory_unref( grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *cc_factory) { client_channel_factory *f = (client_channel_factory *)cc_factory; if (gpr_unref(&f->refs)) { - if (f->master != NULL) { - GRPC_CHANNEL_INTERNAL_UNREF(exec_ctx, f->master, - "client_channel_factory"); - } grpc_channel_args_destroy(f->merge_args); gpr_free(f); } @@ -250,12 +245,8 @@ grpc_channel *grpc_insecure_channel_create(const char *target, grpc_channel *channel = client_channel_factory_create_channel( &exec_ctx, &f->base, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, NULL); - if (channel != NULL) { - f->master = channel; - GRPC_CHANNEL_INTERNAL_REF(f->master, "grpc_insecure_channel_create"); - } - grpc_client_channel_factory_unref(&exec_ctx, &f->base); + grpc_client_channel_factory_unref(&exec_ctx, &f->base); grpc_exec_ctx_finish(&exec_ctx); return channel != NULL ? channel : grpc_lame_client_channel_create( 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 3285dbf6125..da738138b51 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 @@ -220,7 +220,6 @@ typedef struct { gpr_refcount refs; grpc_channel_args *merge_args; grpc_channel_security_connector *security_connector; - grpc_channel *master; } client_channel_factory; static void client_channel_factory_ref( @@ -235,10 +234,6 @@ static void client_channel_factory_unref( if (gpr_unref(&f->refs)) { GRPC_SECURITY_CONNECTOR_UNREF(&f->security_connector->base, "client_channel_factory"); - if (f->master != NULL) { - GRPC_CHANNEL_INTERNAL_UNREF(exec_ctx, f->master, - "client_channel_factory"); - } grpc_channel_args_destroy(f->merge_args); gpr_free(f); } @@ -356,10 +351,6 @@ grpc_channel *grpc_secure_channel_create(grpc_channel_credentials *creds, grpc_channel *channel = client_channel_factory_create_channel( &exec_ctx, &f->base, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, NULL); - if (channel != NULL) { - f->master = channel; - GRPC_CHANNEL_INTERNAL_REF(f->master, "grpc_secure_channel_create"); - } grpc_client_channel_factory_unref(&exec_ctx, &f->base); grpc_exec_ctx_finish(&exec_ctx); From 90f49f58a555bb24fb13588b638fa4c795514358 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 9 Sep 2016 14:20:42 -0700 Subject: [PATCH 3/9] Fix resolver test build failures. --- .../dns_resolver_connectivity_test.c | 24 ------------------- .../resolvers/dns_resolver_test.c | 24 ------------------- .../resolvers/sockaddr_resolver_test.c | 24 ------------------- 3 files changed, 72 deletions(-) diff --git a/test/core/client_config/resolvers/dns_resolver_connectivity_test.c b/test/core/client_config/resolvers/dns_resolver_connectivity_test.c index 6a33525f629..d3b961959d2 100644 --- a/test/core/client_config/resolvers/dns_resolver_connectivity_test.c +++ b/test/core/client_config/resolvers/dns_resolver_connectivity_test.c @@ -41,29 +41,6 @@ #include "src/core/lib/iomgr/timer.h" #include "test/core/util/test_config.h" -static void client_channel_factory_ref(grpc_client_channel_factory *scv) {} -static void client_channel_factory_unref(grpc_exec_ctx *exec_ctx, - grpc_client_channel_factory *scv) {} -static grpc_subchannel *client_channel_factory_create_subchannel( - grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *factory, - grpc_subchannel_args *args) { - return NULL; -} - -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, - grpc_channel_args *args) { - GPR_UNREACHABLE_CODE(return NULL); -} - -static const grpc_client_channel_factory_vtable sc_vtable = { - client_channel_factory_ref, client_channel_factory_unref, - client_channel_factory_create_subchannel, - client_channel_factory_create_channel}; - -static grpc_client_channel_factory cc_factory = {&sc_vtable}; - static gpr_mu g_mu; static bool g_fail_resolution = true; @@ -92,7 +69,6 @@ static grpc_resolver *create_resolver(const char *name) { grpc_resolver_args args; memset(&args, 0, sizeof(args)); args.uri = uri; - args.client_channel_factory = &cc_factory; grpc_resolver *resolver = grpc_resolver_factory_create_resolver(factory, &args); grpc_resolver_factory_unref(factory); diff --git a/test/core/client_config/resolvers/dns_resolver_test.c b/test/core/client_config/resolvers/dns_resolver_test.c index 21dc99cadd8..c3f4cb12444 100644 --- a/test/core/client_config/resolvers/dns_resolver_test.c +++ b/test/core/client_config/resolvers/dns_resolver_test.c @@ -38,29 +38,6 @@ #include "src/core/ext/client_config/resolver_registry.h" #include "test/core/util/test_config.h" -static void client_channel_factory_ref(grpc_client_channel_factory *scv) {} -static void client_channel_factory_unref(grpc_exec_ctx *exec_ctx, - grpc_client_channel_factory *scv) {} -static grpc_subchannel *client_channel_factory_create_subchannel( - grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *factory, - grpc_subchannel_args *args) { - GPR_UNREACHABLE_CODE(return NULL); -} - -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, - grpc_channel_args *args) { - GPR_UNREACHABLE_CODE(return NULL); -} - -static const grpc_client_channel_factory_vtable sc_vtable = { - client_channel_factory_ref, client_channel_factory_unref, - client_channel_factory_create_subchannel, - client_channel_factory_create_channel}; - -static grpc_client_channel_factory cc_factory = {&sc_vtable}; - static void test_succeeds(grpc_resolver_factory *factory, const char *string) { grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; grpc_uri *uri = grpc_uri_parse(string, 0); @@ -71,7 +48,6 @@ static void test_succeeds(grpc_resolver_factory *factory, const char *string) { GPR_ASSERT(uri); memset(&args, 0, sizeof(args)); args.uri = uri; - args.client_channel_factory = &cc_factory; resolver = grpc_resolver_factory_create_resolver(factory, &args); GPR_ASSERT(resolver != NULL); GRPC_RESOLVER_UNREF(&exec_ctx, resolver, "test_succeeds"); diff --git a/test/core/client_config/resolvers/sockaddr_resolver_test.c b/test/core/client_config/resolvers/sockaddr_resolver_test.c index b11546b6b1c..d8430d39c4c 100644 --- a/test/core/client_config/resolvers/sockaddr_resolver_test.c +++ b/test/core/client_config/resolvers/sockaddr_resolver_test.c @@ -38,29 +38,6 @@ #include "src/core/ext/client_config/resolver_registry.h" #include "test/core/util/test_config.h" -static void client_channel_factory_ref(grpc_client_channel_factory *scv) {} -static void client_channel_factory_unref(grpc_exec_ctx *exec_ctx, - grpc_client_channel_factory *scv) {} -static grpc_subchannel *client_channel_factory_create_subchannel( - grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *factory, - grpc_subchannel_args *args) { - GPR_UNREACHABLE_CODE(return NULL); -} - -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, - grpc_channel_args *args) { - GPR_UNREACHABLE_CODE(return NULL); -} - -static const grpc_client_channel_factory_vtable sc_vtable = { - client_channel_factory_ref, client_channel_factory_unref, - client_channel_factory_create_subchannel, - client_channel_factory_create_channel}; - -static grpc_client_channel_factory cc_factory = {&sc_vtable}; - static void test_succeeds(grpc_resolver_factory *factory, const char *string) { grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; grpc_uri *uri = grpc_uri_parse(string, 0); @@ -71,7 +48,6 @@ static void test_succeeds(grpc_resolver_factory *factory, const char *string) { GPR_ASSERT(uri); memset(&args, 0, sizeof(args)); args.uri = uri; - args.client_channel_factory = &cc_factory; resolver = grpc_resolver_factory_create_resolver(factory, &args); GPR_ASSERT(resolver != NULL); GRPC_RESOLVER_UNREF(&exec_ctx, resolver, "test_succeeds"); From 38525a9a082df5c8a61efe046cde9cb14ae4170c Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 9 Sep 2016 14:45:00 -0700 Subject: [PATCH 4/9] Fix use-after-free bug. --- src/core/ext/client_config/resolver_result.c | 7 +++++++ src/core/ext/client_config/resolver_result.h | 2 ++ src/core/ext/resolver/sockaddr/sockaddr_resolver.c | 4 ++-- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/core/ext/client_config/resolver_result.c b/src/core/ext/client_config/resolver_result.c index ba21349d2d5..b0602d583d6 100644 --- a/src/core/ext/client_config/resolver_result.c +++ b/src/core/ext/client_config/resolver_result.c @@ -47,6 +47,13 @@ grpc_addresses *grpc_addresses_create(size_t num_addresses) { return addresses; } +grpc_addresses *grpc_addresses_copy(grpc_addresses* addresses) { + grpc_addresses *new = grpc_addresses_create(addresses->num_addresses); + memcpy(new->addresses, addresses->addresses, + sizeof(grpc_address) * addresses->num_addresses); + return new; +} + void grpc_addresses_set_address(grpc_addresses *addresses, size_t index, void *address, size_t address_len, bool is_balancer) { diff --git a/src/core/ext/client_config/resolver_result.h b/src/core/ext/client_config/resolver_result.h index df92bec984e..b1a34575654 100644 --- a/src/core/ext/client_config/resolver_result.h +++ b/src/core/ext/client_config/resolver_result.h @@ -54,6 +54,8 @@ typedef struct grpc_addresses { \a num_addresses addresses. */ grpc_addresses *grpc_addresses_create(size_t num_addresses); +grpc_addresses *grpc_addresses_copy(grpc_addresses* addresses); + void grpc_addresses_set_address(grpc_addresses *addresses, size_t index, void *address, size_t address_len, bool is_balancer); diff --git a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c index 328c7cb6f93..5c59e4397af 100644 --- a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c +++ b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c @@ -120,8 +120,8 @@ static void sockaddr_maybe_finish_next_locked(grpc_exec_ctx *exec_ctx, sockaddr_resolver *r) { if (r->next_completion != NULL && !r->published) { r->published = true; - *r->target_result = - grpc_resolver_result_create(r->addresses, r->lb_policy_name); + *r->target_result = grpc_resolver_result_create( + grpc_addresses_copy(r->addresses), r->lb_policy_name); grpc_exec_ctx_sched(exec_ctx, r->next_completion, GRPC_ERROR_NONE, NULL); r->next_completion = NULL; } From 1621c26c1ec49207e994e4b11671fb8fb38225ba Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 15 Sep 2016 12:51:29 -0700 Subject: [PATCH 5/9] Allow resolver to pass channel args to LB policy. --- src/core/ext/client_config/client_channel.c | 2 ++ .../ext/client_config/lb_policy_factory.h | 3 +++ src/core/ext/client_config/resolver_result.c | 15 +++++++++++-- src/core/ext/client_config/resolver_result.h | 21 ++++++++++++++++--- .../ext/resolver/dns/native/dns_resolver.c | 2 +- .../ext/resolver/sockaddr/sockaddr_resolver.c | 2 +- 6 files changed, 38 insertions(+), 7 deletions(-) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index 2e6f253d386..0343a5e2544 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -179,6 +179,8 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_lb_policy_args lb_policy_args; lb_policy_args.addresses = grpc_resolver_result_get_addresses(chand->resolver_result); + lb_policy_args.additional_args = + grpc_resolver_result_get_lb_policy_args(chand->resolver_result); lb_policy_args.client_channel_factory = chand->client_channel_factory; lb_policy = grpc_lb_policy_create( exec_ctx, diff --git a/src/core/ext/client_config/lb_policy_factory.h b/src/core/ext/client_config/lb_policy_factory.h index 6919b1eb984..e2364c31a82 100644 --- a/src/core/ext/client_config/lb_policy_factory.h +++ b/src/core/ext/client_config/lb_policy_factory.h @@ -47,9 +47,12 @@ struct grpc_lb_policy_factory { const grpc_lb_policy_factory_vtable *vtable; }; +// TODO(roth, ctiller): Consider replacing this struct with +// grpc_channel_args. See comment in resolver_result.h for details. typedef struct grpc_lb_policy_args { grpc_addresses *addresses; grpc_client_channel_factory *client_channel_factory; + grpc_channel_args *additional_args; } grpc_lb_policy_args; struct grpc_lb_policy_factory_vtable { diff --git a/src/core/ext/client_config/resolver_result.c b/src/core/ext/client_config/resolver_result.c index ac263b9a468..f97dc467e1d 100644 --- a/src/core/ext/client_config/resolver_result.c +++ b/src/core/ext/client_config/resolver_result.c @@ -36,6 +36,8 @@ #include #include +#include "src/core/lib/channel/channel_args.h" + grpc_addresses* grpc_addresses_create(size_t num_addresses) { grpc_addresses* addresses = gpr_malloc(sizeof(grpc_addresses)); addresses->num_addresses = num_addresses; @@ -71,15 +73,18 @@ struct grpc_resolver_result { gpr_refcount refs; grpc_addresses* addresses; char* lb_policy_name; + grpc_channel_args* lb_policy_args; }; -grpc_resolver_result* grpc_resolver_result_create(grpc_addresses* addresses, - const char* lb_policy_name) { +grpc_resolver_result* grpc_resolver_result_create( + grpc_addresses* addresses, const char* lb_policy_name, + grpc_channel_args* lb_policy_args) { grpc_resolver_result* result = gpr_malloc(sizeof(*result)); memset(result, 0, sizeof(*result)); gpr_ref_init(&result->refs, 1); result->addresses = addresses; result->lb_policy_name = gpr_strdup(lb_policy_name); + result->lb_policy_args = lb_policy_args; return result; } @@ -92,6 +97,7 @@ void grpc_resolver_result_unref(grpc_exec_ctx* exec_ctx, if (gpr_unref(&result->refs)) { grpc_addresses_destroy(result->addresses); gpr_free(result->lb_policy_name); + grpc_channel_args_destroy(result->lb_policy_args); gpr_free(result); } } @@ -105,3 +111,8 @@ const char* grpc_resolver_result_get_lb_policy_name( grpc_resolver_result* result) { return result->lb_policy_name; } + +grpc_channel_args* grpc_resolver_result_get_lb_policy_args( + grpc_resolver_result* result) { + return result->lb_policy_args; +} diff --git a/src/core/ext/client_config/resolver_result.h b/src/core/ext/client_config/resolver_result.h index fa60c8cc6d4..821208f7099 100644 --- a/src/core/ext/client_config/resolver_result.h +++ b/src/core/ext/client_config/resolver_result.h @@ -37,6 +37,16 @@ #include "src/core/ext/client_config/lb_policy.h" #include "src/core/lib/iomgr/resolve_address.h" +// TODO(roth, ctiller): In the long term, we are considering replacing +// the resolver_result data structure with grpc_channel_args. The idea is +// that the resolver will return a set of channel args that contains the +// information that is currently in the resolver_result struct. For +// example, there will be specific args indicating the set of addresses +// and the name of the LB policy to instantiate. Note that if we did +// this, we would probably want to change the data structure of +// grpc_channel_args such to a hash table or AVL or some other data +// structure that does not require linear search to find keys. + /// Used to represent addresses returned by the resolver. typedef struct grpc_address { grpc_resolved_address address; @@ -63,9 +73,10 @@ void grpc_addresses_destroy(grpc_addresses* addresses); /// Results reported from a grpc_resolver. typedef struct grpc_resolver_result grpc_resolver_result; -/// Takes ownership of \a addresses. -grpc_resolver_result* grpc_resolver_result_create(grpc_addresses* addresses, - const char* lb_policy_name); +/// Takes ownership of \a addresses and \a lb_policy_args. +grpc_resolver_result* grpc_resolver_result_create( + grpc_addresses* addresses, const char* lb_policy_name, + grpc_channel_args* lb_policy_args); void grpc_resolver_result_ref(grpc_resolver_result* result); void grpc_resolver_result_unref(grpc_exec_ctx* exec_ctx, grpc_resolver_result* result); @@ -78,4 +89,8 @@ grpc_addresses* grpc_resolver_result_get_addresses( const char* grpc_resolver_result_get_lb_policy_name( grpc_resolver_result* result); +/// Caller does NOT take ownership of result. +grpc_channel_args* grpc_resolver_result_get_lb_policy_args( + grpc_resolver_result* result); + #endif /* GRPC_CORE_EXT_CLIENT_CONFIG_RESOLVER_RESULT_H */ diff --git a/src/core/ext/resolver/dns/native/dns_resolver.c b/src/core/ext/resolver/dns/native/dns_resolver.c index 78a996788dc..103d3effbd0 100644 --- a/src/core/ext/resolver/dns/native/dns_resolver.c +++ b/src/core/ext/resolver/dns/native/dns_resolver.c @@ -174,7 +174,7 @@ static void dns_on_resolved(grpc_exec_ctx *exec_ctx, void *arg, false /* is_balancer */); } grpc_resolved_addresses_destroy(r->addresses); - result = grpc_resolver_result_create(addresses, r->lb_policy_name); + result = grpc_resolver_result_create(addresses, r->lb_policy_name, NULL); } else { gpr_timespec now = gpr_now(GPR_CLOCK_MONOTONIC); gpr_timespec next_try = gpr_backoff_step(&r->backoff_state, now); diff --git a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c index 5c59e4397af..fb67f1a227f 100644 --- a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c +++ b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c @@ -121,7 +121,7 @@ static void sockaddr_maybe_finish_next_locked(grpc_exec_ctx *exec_ctx, if (r->next_completion != NULL && !r->published) { r->published = true; *r->target_result = grpc_resolver_result_create( - grpc_addresses_copy(r->addresses), r->lb_policy_name); + grpc_addresses_copy(r->addresses), r->lb_policy_name, NULL); grpc_exec_ctx_sched(exec_ctx, r->next_completion, GRPC_ERROR_NONE, NULL); r->next_completion = NULL; } From 6d5715867f737442805338a139a1fd91bd37238d Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 16 Sep 2016 10:16:49 -0700 Subject: [PATCH 6/9] clang-format --- src/core/ext/client_config/lb_policy_factory.c | 7 +++---- src/core/ext/client_config/lb_policy_factory.h | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/core/ext/client_config/lb_policy_factory.c b/src/core/ext/client_config/lb_policy_factory.c index 2737f141c4e..88e8f15d997 100644 --- a/src/core/ext/client_config/lb_policy_factory.c +++ b/src/core/ext/client_config/lb_policy_factory.c @@ -46,8 +46,8 @@ grpc_lb_addresses* grpc_lb_addresses_create(size_t num_addresses) { return addresses; } -grpc_lb_addresses *grpc_lb_addresses_copy(grpc_lb_addresses* addresses, - void *(*user_data_copy)(void *)) { +grpc_lb_addresses* grpc_lb_addresses_copy(grpc_lb_addresses* addresses, + void* (*user_data_copy)(void*)) { grpc_lb_addresses* new = grpc_lb_addresses_create(addresses->num_addresses); memcpy(new->addresses, addresses->addresses, sizeof(grpc_address) * addresses->num_addresses); @@ -55,8 +55,7 @@ grpc_lb_addresses *grpc_lb_addresses_copy(grpc_lb_addresses* addresses, new->addresses[i].balancer_name = gpr_strdup(new->addresses[i].balancer_name); if (user_data_copy != NULL) { - new->addresses[i].user_data = - user_data_copy(new->addresses[i].user_data); + new->addresses[i].user_data = user_data_copy(new->addresses[i].user_data); } } return new; diff --git a/src/core/ext/client_config/lb_policy_factory.h b/src/core/ext/client_config/lb_policy_factory.h index 70dadff0ab9..ac62dd9bc06 100644 --- a/src/core/ext/client_config/lb_policy_factory.h +++ b/src/core/ext/client_config/lb_policy_factory.h @@ -70,7 +70,7 @@ grpc_lb_addresses *grpc_lb_addresses_create(size_t num_addresses); /** Creates a copy of \a addresses. If \a user_data_copy is not NULL, * it will be invoked to copy the \a user_data field of each address. */ -grpc_lb_addresses *grpc_lb_addresses_copy(grpc_lb_addresses* addresses, +grpc_lb_addresses *grpc_lb_addresses_copy(grpc_lb_addresses *addresses, void *(*user_data_copy)(void *)); /** Sets the value of the address at index \a index of \a addresses. From fef585f1be2dcc03ee813d1083a50e4ca0ba8486 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 16 Sep 2016 10:23:28 -0700 Subject: [PATCH 7/9] Fix bugs from merge. --- src/core/ext/client_config/lb_policy_factory.c | 9 ++++++--- src/core/ext/client_config/lb_policy_factory.h | 2 +- src/core/ext/client_config/resolver_result.c | 8 ++++---- src/core/ext/client_config/resolver_result.h | 2 +- 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/core/ext/client_config/lb_policy_factory.c b/src/core/ext/client_config/lb_policy_factory.c index 88e8f15d997..24895057919 100644 --- a/src/core/ext/client_config/lb_policy_factory.c +++ b/src/core/ext/client_config/lb_policy_factory.c @@ -34,6 +34,7 @@ #include #include +#include #include "src/core/ext/client_config/lb_policy_factory.h" @@ -50,10 +51,12 @@ grpc_lb_addresses* grpc_lb_addresses_copy(grpc_lb_addresses* addresses, void* (*user_data_copy)(void*)) { grpc_lb_addresses* new = grpc_lb_addresses_create(addresses->num_addresses); memcpy(new->addresses, addresses->addresses, - sizeof(grpc_address) * addresses->num_addresses); + sizeof(grpc_lb_address) * addresses->num_addresses); for (size_t i = 0; i < addresses->num_addresses; ++i) { - new->addresses[i].balancer_name = - gpr_strdup(new->addresses[i].balancer_name); + if (new->addresses[i].balancer_name != NULL) { + new->addresses[i].balancer_name = + gpr_strdup(new->addresses[i].balancer_name); + } if (user_data_copy != NULL) { new->addresses[i].user_data = user_data_copy(new->addresses[i].user_data); } diff --git a/src/core/ext/client_config/lb_policy_factory.h b/src/core/ext/client_config/lb_policy_factory.h index ac62dd9bc06..c9499d4f877 100644 --- a/src/core/ext/client_config/lb_policy_factory.h +++ b/src/core/ext/client_config/lb_policy_factory.h @@ -36,9 +36,9 @@ #include "src/core/ext/client_config/client_channel_factory.h" #include "src/core/ext/client_config/lb_policy.h" -#include "src/core/ext/client_config/resolver_result.h" #include "src/core/lib/iomgr/exec_ctx.h" +#include "src/core/lib/iomgr/resolve_address.h" typedef struct grpc_lb_policy_factory grpc_lb_policy_factory; typedef struct grpc_lb_policy_factory_vtable grpc_lb_policy_factory_vtable; diff --git a/src/core/ext/client_config/resolver_result.c b/src/core/ext/client_config/resolver_result.c index c07bd8b4da3..59c9e7dc255 100644 --- a/src/core/ext/client_config/resolver_result.c +++ b/src/core/ext/client_config/resolver_result.c @@ -40,13 +40,13 @@ struct grpc_resolver_result { gpr_refcount refs; - grpc_addresses* addresses; + grpc_lb_addresses* addresses; char* lb_policy_name; grpc_channel_args* lb_policy_args; }; grpc_resolver_result* grpc_resolver_result_create( - grpc_addresses* addresses, const char* lb_policy_name, + grpc_lb_addresses* addresses, const char* lb_policy_name, grpc_channel_args* lb_policy_args) { grpc_resolver_result* result = gpr_malloc(sizeof(*result)); memset(result, 0, sizeof(*result)); @@ -64,14 +64,14 @@ void grpc_resolver_result_ref(grpc_resolver_result* result) { void grpc_resolver_result_unref(grpc_exec_ctx* exec_ctx, grpc_resolver_result* result) { if (gpr_unref(&result->refs)) { - grpc_addresses_destroy(result->addresses); + grpc_lb_addresses_destroy(result->addresses, NULL /* user_data_destroy */); gpr_free(result->lb_policy_name); grpc_channel_args_destroy(result->lb_policy_args); gpr_free(result); } } -grpc_addresses* grpc_resolver_result_get_addresses( +grpc_lb_addresses* grpc_resolver_result_get_addresses( grpc_resolver_result* result) { return result->addresses; } diff --git a/src/core/ext/client_config/resolver_result.h b/src/core/ext/client_config/resolver_result.h index e28a2f5bf3b..d4118b90e8a 100644 --- a/src/core/ext/client_config/resolver_result.h +++ b/src/core/ext/client_config/resolver_result.h @@ -57,7 +57,7 @@ void grpc_resolver_result_unref(grpc_exec_ctx* exec_ctx, grpc_resolver_result* result); /// Caller does NOT take ownership of result. -grpc_addresses* grpc_resolver_result_get_addresses( +grpc_lb_addresses* grpc_resolver_result_get_addresses( grpc_resolver_result* result); /// Caller does NOT take ownership of result. From 6053497f21de429640758cebad6014aa16177417 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 20 Sep 2016 08:37:12 -0700 Subject: [PATCH 8/9] Code review changes. --- src/core/ext/client_config/client_channel.c | 2 +- src/core/ext/client_config/client_channel.h | 2 +- src/core/ext/transport/chttp2/client/insecure/channel_create.c | 2 +- .../ext/transport/chttp2/client/secure/secure_channel_create.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index e4500cc50da..946fff9cf21 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -779,7 +779,7 @@ const grpc_channel_filter grpc_client_channel_filter = { "client-channel", }; -void grpc_client_channel_set_resolver_and_client_channel_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) { diff --git a/src/core/ext/client_config/client_channel.h b/src/core/ext/client_config/client_channel.h index b3a9bc79951..abb5ac4d878 100644 --- a/src/core/ext/client_config/client_channel.h +++ b/src/core/ext/client_config/client_channel.h @@ -49,7 +49,7 @@ 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_set_resolver_and_client_channel_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); 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 5b9101e018b..ddc00bd79f8 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.c @@ -212,7 +212,7 @@ static grpc_channel *client_channel_factory_create_channel( return NULL; } - grpc_client_channel_set_resolver_and_client_channel_factory( + grpc_client_channel_finish_initialization( exec_ctx, grpc_channel_get_channel_stack(channel), resolver, &f->base); GRPC_RESOLVER_UNREF(exec_ctx, resolver, "create_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 da738138b51..f36fbbfc579 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 @@ -273,7 +273,7 @@ static grpc_channel *client_channel_factory_create_channel( grpc_resolver *resolver = grpc_resolver_create(target); if (resolver != NULL) { - grpc_client_channel_set_resolver_and_client_channel_factory( + grpc_client_channel_finish_initialization( exec_ctx, grpc_channel_get_channel_stack(channel), resolver, &f->base); GRPC_RESOLVER_UNREF(exec_ctx, resolver, "create"); } else { From 7f7d165fa8123ef0346f3d3314c21ee27519a188 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 20 Sep 2016 10:46:15 -0700 Subject: [PATCH 9/9] Code review changes. --- src/core/ext/client_config/client_channel.c | 1 + src/core/ext/client_config/lb_policy_factory.c | 16 +++++++++------- src/core/ext/client_config/lb_policy_factory.h | 1 + 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index 946fff9cf21..63797bfa1e3 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -784,6 +784,7 @@ void grpc_client_channel_finish_initialization( 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); diff --git a/src/core/ext/client_config/lb_policy_factory.c b/src/core/ext/client_config/lb_policy_factory.c index 8ce87455046..c17af91a09e 100644 --- a/src/core/ext/client_config/lb_policy_factory.c +++ b/src/core/ext/client_config/lb_policy_factory.c @@ -49,19 +49,21 @@ grpc_lb_addresses* grpc_lb_addresses_create(size_t num_addresses) { grpc_lb_addresses* grpc_lb_addresses_copy(grpc_lb_addresses* addresses, void* (*user_data_copy)(void*)) { - grpc_lb_addresses* new = grpc_lb_addresses_create(addresses->num_addresses); - memcpy(new->addresses, addresses->addresses, + grpc_lb_addresses* new_addresses = + grpc_lb_addresses_create(addresses->num_addresses); + memcpy(new_addresses->addresses, addresses->addresses, sizeof(grpc_lb_address) * addresses->num_addresses); for (size_t i = 0; i < addresses->num_addresses; ++i) { - if (new->addresses[i].balancer_name != NULL) { - new->addresses[i].balancer_name = - gpr_strdup(new->addresses[i].balancer_name); + if (new_addresses->addresses[i].balancer_name != NULL) { + new_addresses->addresses[i].balancer_name = + gpr_strdup(new_addresses->addresses[i].balancer_name); } if (user_data_copy != NULL) { - new->addresses[i].user_data = user_data_copy(new->addresses[i].user_data); + new_addresses->addresses[i].user_data = + user_data_copy(new_addresses->addresses[i].user_data); } } - return new; + return new_addresses; } void grpc_lb_addresses_set_address(grpc_lb_addresses* addresses, size_t index, diff --git a/src/core/ext/client_config/lb_policy_factory.h b/src/core/ext/client_config/lb_policy_factory.h index c9499d4f877..54408c63080 100644 --- a/src/core/ext/client_config/lb_policy_factory.h +++ b/src/core/ext/client_config/lb_policy_factory.h @@ -92,6 +92,7 @@ void grpc_lb_addresses_destroy(grpc_lb_addresses *addresses, typedef struct grpc_lb_policy_args { grpc_lb_addresses *addresses; grpc_client_channel_factory *client_channel_factory; + /* Can be used to pass implementation-specific parameters to the LB policy. */ grpc_channel_args *additional_args; } grpc_lb_policy_args;