Skip to content

Commit

Permalink
Eliminate redundant places where server name was stored.
Browse files Browse the repository at this point in the history
  • Loading branch information
markdroth committed Dec 12, 2016
1 parent f75d269 commit 201db7d
Show file tree
Hide file tree
Showing 17 changed files with 74 additions and 95 deletions.
2 changes: 0 additions & 2 deletions include/grpc/impl/codegen/grpc_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,6 @@ typedef struct {
#define GRPC_ARG_LB_POLICY_NAME "grpc.lb_policy_name"
/** Server URI. Not intended for external use. */
#define GRPC_ARG_SERVER_URI "grpc.server_uri"
/** Server name. Not intended for external use. */
#define GRPC_ARG_SERVER_NAME "grpc.server_name"
/** Resolved addresses in a form used by the LB policy.
Not intended for external use. */
#define GRPC_ARG_LB_ADDRESSES "grpc.lb_addresses"
Expand Down
30 changes: 19 additions & 11 deletions src/core/ext/client_channel/http_connect_handshaker.c
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#include <grpc/support/log.h>
#include <grpc/support/string_util.h>

#include "src/core/ext/client_channel/resolver_registry.h"
#include "src/core/ext/client_channel/uri_parser.h"
#include "src/core/lib/channel/channel_args.h"
#include "src/core/lib/http/format_request.h"
Expand All @@ -51,7 +52,6 @@ typedef struct http_connect_handshaker {
grpc_handshaker base;

char* proxy_server;
char* server_name;

gpr_refcount refcount;
gpr_mu mu;
Expand Down Expand Up @@ -86,7 +86,6 @@ static void http_connect_handshaker_unref(grpc_exec_ctx* exec_ctx,
gpr_free(handshaker->read_buffer_to_destroy);
}
gpr_free(handshaker->proxy_server);
gpr_free(handshaker->server_name);
grpc_slice_buffer_destroy(&handshaker->write_buffer);
grpc_http_parser_destroy(&handshaker->http_parser);
grpc_http_response_destroy(&handshaker->http_response);
Expand Down Expand Up @@ -265,18 +264,27 @@ static void http_connect_handshaker_do_handshake(
grpc_tcp_server_acceptor* acceptor, grpc_closure* on_handshake_done,
grpc_handshaker_args* args) {
http_connect_handshaker* handshaker = (http_connect_handshaker*)handshaker_in;
gpr_mu_lock(&handshaker->mu);
// Get server name from channel args.
const grpc_arg* arg = grpc_channel_args_find(args->args, GRPC_ARG_SERVER_URI);
GPR_ASSERT(arg != NULL);
GPR_ASSERT(arg->type == GRPC_ARG_STRING);
char *canonical_uri =
grpc_resolver_factory_add_default_prefix_if_needed(arg->value.string);
grpc_uri* uri = grpc_uri_parse(canonical_uri, 1);
char* server_name = uri->path;
if (server_name[0] == '/') ++server_name;
// Save state in the handshaker object.
gpr_mu_lock(&handshaker->mu);
handshaker->args = args;
handshaker->on_handshake_done = on_handshake_done;
// Send HTTP CONNECT request.
gpr_log(GPR_INFO, "Connecting to server %s via HTTP proxy %s",
handshaker->server_name, handshaker->proxy_server);
gpr_log(GPR_INFO, "Connecting to server %s via HTTP proxy %s", server_name,
handshaker->proxy_server);
grpc_httpcli_request request;
memset(&request, 0, sizeof(request));
request.host = handshaker->proxy_server;
request.host = server_name;
request.http.method = "CONNECT";
request.http.path = handshaker->server_name;
request.http.path = server_name;
request.handshaker = &grpc_httpcli_plaintext;
grpc_slice request_slice = grpc_httpcli_format_connect_request(&request);
grpc_slice_buffer_add(&handshaker->write_buffer, request_slice);
Expand All @@ -285,23 +293,23 @@ static void http_connect_handshaker_do_handshake(
grpc_endpoint_write(exec_ctx, args->endpoint, &handshaker->write_buffer,
&handshaker->request_done_closure);
gpr_mu_unlock(&handshaker->mu);
// Clean up.
gpr_free(canonical_uri);
grpc_uri_destroy(uri);
}

static const grpc_handshaker_vtable http_connect_handshaker_vtable = {
http_connect_handshaker_destroy, http_connect_handshaker_shutdown,
http_connect_handshaker_do_handshake};

grpc_handshaker* grpc_http_connect_handshaker_create(const char* proxy_server,
const char* server_name) {
grpc_handshaker* grpc_http_connect_handshaker_create(const char* proxy_server) {
GPR_ASSERT(proxy_server != NULL);
GPR_ASSERT(server_name != NULL);
http_connect_handshaker* handshaker = gpr_malloc(sizeof(*handshaker));
memset(handshaker, 0, sizeof(*handshaker));
grpc_handshaker_init(&http_connect_handshaker_vtable, &handshaker->base);
gpr_mu_init(&handshaker->mu);
gpr_ref_init(&handshaker->refcount, 1);
handshaker->proxy_server = gpr_strdup(proxy_server);
handshaker->server_name = gpr_strdup(server_name);
grpc_slice_buffer_init(&handshaker->write_buffer);
grpc_closure_init(&handshaker->request_done_closure, on_write_done,
handshaker);
Expand Down
5 changes: 2 additions & 3 deletions src/core/ext/client_channel/http_connect_handshaker.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,8 @@

#include "src/core/lib/channel/handshaker.h"

/// Does NOT take ownership of \a proxy_server or \a server_name.
grpc_handshaker* grpc_http_connect_handshaker_create(const char* proxy_server,
const char* server_name);
/// Does NOT take ownership of \a proxy_server.
grpc_handshaker* grpc_http_connect_handshaker_create(const char* proxy_server);

/// Returns the name of the proxy to use, or NULL if no proxy is configured.
/// Caller takes ownership of result.
Expand Down
32 changes: 23 additions & 9 deletions src/core/ext/client_channel/resolver_registry.c
Original file line number Diff line number Diff line change
Expand Up @@ -109,46 +109,60 @@ static grpc_resolver_factory *lookup_factory_by_uri(grpc_uri *uri) {
}

static grpc_resolver_factory *resolve_factory(const char *target,
grpc_uri **uri) {
char *tmp;
grpc_uri **uri,
char **canonical_target) {
grpc_resolver_factory *factory = NULL;

GPR_ASSERT(uri != NULL);
*uri = grpc_uri_parse(target, 1);
factory = lookup_factory_by_uri(*uri);
if (factory == NULL) {
grpc_uri_destroy(*uri);
gpr_asprintf(&tmp, "%s%s", g_default_resolver_prefix, target);
*uri = grpc_uri_parse(tmp, 1);
gpr_asprintf(canonical_target, "%s%s", g_default_resolver_prefix, target);
*uri = grpc_uri_parse(*canonical_target, 1);
factory = lookup_factory_by_uri(*uri);
if (factory == NULL) {
grpc_uri_destroy(grpc_uri_parse(target, 0));
grpc_uri_destroy(grpc_uri_parse(tmp, 0));
gpr_log(GPR_ERROR, "don't know how to resolve '%s' or '%s'", target, tmp);
grpc_uri_destroy(grpc_uri_parse(*canonical_target, 0));
gpr_log(GPR_ERROR, "don't know how to resolve '%s' or '%s'", target,
*canonical_target);
}
gpr_free(tmp);
}
return factory;
}

grpc_resolver *grpc_resolver_create(const char *target,
const grpc_channel_args *args) {
grpc_uri *uri = NULL;
grpc_resolver_factory *factory = resolve_factory(target, &uri);
char *canonical_target = NULL;
grpc_resolver_factory *factory =
resolve_factory(target, &uri, &canonical_target);
grpc_resolver *resolver;
grpc_resolver_args resolver_args;
memset(&resolver_args, 0, sizeof(resolver_args));
resolver_args.uri = uri;
resolver_args.args = args;
resolver = grpc_resolver_factory_create_resolver(factory, &resolver_args);
grpc_uri_destroy(uri);
gpr_free(canonical_target);
return resolver;
}

char *grpc_get_default_authority(const char *target) {
grpc_uri *uri = NULL;
grpc_resolver_factory *factory = resolve_factory(target, &uri);
char *canonical_target = NULL;
grpc_resolver_factory *factory =
resolve_factory(target, &uri, &canonical_target);
char *authority = grpc_resolver_factory_get_default_authority(factory, uri);
grpc_uri_destroy(uri);
gpr_free(canonical_target);
return authority;
}

char *grpc_resolver_factory_add_default_prefix_if_needed(const char *target) {
grpc_uri *uri = NULL;
char *canonical_target = NULL;
resolve_factory(target, &uri, &canonical_target);
grpc_uri_destroy(uri);
return canonical_target == NULL ? gpr_strdup(target) : canonical_target;
}
4 changes: 4 additions & 0 deletions src/core/ext/client_channel/resolver_registry.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,8 @@ grpc_resolver_factory *grpc_resolver_factory_lookup(const char *name);
representing the default authority to pass from a client. */
char *grpc_get_default_authority(const char *target);

/** Returns a newly allocated string containing \a target, adding the
default prefix if needed. */
char *grpc_resolver_factory_add_default_prefix_if_needed(const char *target);

#endif /* GRPC_CORE_EXT_CLIENT_CHANNEL_RESOLVER_REGISTRY_H */
2 changes: 0 additions & 2 deletions src/core/ext/client_channel/subchannel.h
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,6 @@ struct grpc_subchannel_args {
size_t filter_count;
/** Channel arguments to be supplied to the newly created channel */
const grpc_channel_args *args;
/** Server name */
const char *server_name;
/** Address to connect to */
grpc_resolved_address *addr;
};
Expand Down
4 changes: 0 additions & 4 deletions src/core/ext/client_channel/subchannel_index.c
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ static grpc_subchannel_key *create_key(
} else {
k->args.filters = NULL;
}
k->args.server_name = gpr_strdup(args->server_name);
k->args.addr = gpr_malloc(sizeof(grpc_resolved_address));
k->args.addr->len = args->addr->len;
if (k->args.addr->len > 0) {
Expand All @@ -113,8 +112,6 @@ static int subchannel_key_compare(grpc_subchannel_key *a,
if (c != 0) return c;
c = GPR_ICMP(a->args.filter_count, b->args.filter_count);
if (c != 0) return c;
c = strcmp(a->args.server_name, b->args.server_name);
if (c != 0) return c;
if (a->args.addr->len) {
c = memcmp(a->args.addr->addr, b->args.addr->addr, a->args.addr->len);
if (c != 0) return c;
Expand All @@ -132,7 +129,6 @@ void grpc_subchannel_key_destroy(grpc_exec_ctx *exec_ctx,
grpc_connector_unref(exec_ctx, k->connector);
gpr_free((grpc_channel_args *)k->args.filters);
grpc_channel_args_destroy((grpc_channel_args *)k->args.args);
gpr_free((void *)k->args.server_name);
gpr_free(k->args.addr);
gpr_free(k);
}
Expand Down
26 changes: 13 additions & 13 deletions src/core/ext/lb_policy/grpclb/grpclb.c
Original file line number Diff line number Diff line change
Expand Up @@ -743,20 +743,15 @@ static void glb_rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg,
static grpc_lb_policy *glb_create(grpc_exec_ctx *exec_ctx,
grpc_lb_policy_factory *factory,
grpc_lb_policy_args *args) {
/* Get server name. */
const grpc_arg *arg =
grpc_channel_args_find(args->args, GRPC_ARG_SERVER_NAME);
const char *server_name =
arg != NULL && arg->type == GRPC_ARG_STRING ? arg->value.string : NULL;

/* Count the number of gRPC-LB addresses. There must be at least one.
* TODO(roth): For now, we ignore non-balancer addresses, but in the
* future, we may change the behavior such that we fall back to using
* the non-balancer addresses if we cannot reach any balancers. At that
* time, this should be changed to allow a list with no balancer addresses,
* since the resolver might fail to return a balancer address even when
* this is the right LB policy to use. */
arg = grpc_channel_args_find(args->args, GRPC_ARG_LB_ADDRESSES);
const grpc_arg *arg =
grpc_channel_args_find(args->args, GRPC_ARG_LB_ADDRESSES);
GPR_ASSERT(arg != NULL && arg->type == GRPC_ARG_POINTER);
grpc_lb_addresses *addresses = arg->value.pointer.p;
size_t num_grpclb_addrs = 0;
Expand All @@ -768,13 +763,19 @@ static grpc_lb_policy *glb_create(grpc_exec_ctx *exec_ctx,
glb_lb_policy *glb_policy = gpr_malloc(sizeof(*glb_policy));
memset(glb_policy, 0, sizeof(*glb_policy));

/* Get server name. */
arg = grpc_channel_args_find(args->args, GRPC_ARG_SERVER_URI);
GPR_ASSERT(arg != NULL);
GPR_ASSERT(arg->type == GRPC_ARG_STRING);
grpc_uri *uri = grpc_uri_parse(arg->value.string, 1);
glb_policy->server_name = gpr_strdup(uri->path);
grpc_uri_destroy(uri);

/* All input addresses in addresses come from a resolver that claims
* they are LB services. It's the resolver's responsibility to make sure
* this
* policy is only instantiated and used in that case.
* this policy is only instantiated and used in that case.
*
* Create a client channel over them to communicate with a LB service */
glb_policy->server_name = gpr_strdup(server_name);
glb_policy->cc_factory = args->client_channel_factory;
glb_policy->args = grpc_channel_args_copy(args->args);
GPR_ASSERT(glb_policy->cc_factory != NULL);
Expand Down Expand Up @@ -824,9 +825,8 @@ static grpc_lb_policy *glb_create(grpc_exec_ctx *exec_ctx,
* channel. (The client channel factory will re-add this arg with
* the right value.)
*/
static const char *keys_to_remove[] = {GRPC_ARG_LB_POLICY_NAME,
GRPC_ARG_LB_ADDRESSES,
GRPC_ARG_SERVER_URI};
static const char *keys_to_remove[] = {
GRPC_ARG_LB_POLICY_NAME, GRPC_ARG_LB_ADDRESSES, GRPC_ARG_SERVER_URI};
grpc_channel_args *new_args = grpc_channel_args_copy_and_remove(
args->args, keys_to_remove, GPR_ARRAY_SIZE(keys_to_remove));
glb_policy->lb_channel = grpc_client_channel_factory_create_channel(
Expand Down
12 changes: 2 additions & 10 deletions src/core/ext/lb_policy/pick_first/pick_first.c
Original file line number Diff line number Diff line change
Expand Up @@ -438,15 +438,10 @@ static grpc_lb_policy *create_pick_first(grpc_exec_ctx *exec_ctx,
grpc_lb_policy_args *args) {
GPR_ASSERT(args->client_channel_factory != NULL);

/* Get server name. */
const grpc_arg *arg =
grpc_channel_args_find(args->args, GRPC_ARG_SERVER_NAME);
const char *server_name =
arg != NULL && arg->type == GRPC_ARG_STRING ? arg->value.string : NULL;

/* Find the number of backend addresses. We ignore balancer
* addresses, since we don't know how to handle them. */
arg = grpc_channel_args_find(args->args, GRPC_ARG_LB_ADDRESSES);
const grpc_arg *arg =
grpc_channel_args_find(args->args, GRPC_ARG_LB_ADDRESSES);
GPR_ASSERT(arg != NULL && arg->type == GRPC_ARG_POINTER);
grpc_lb_addresses *addresses = arg->value.pointer.p;
size_t num_addrs = 0;
Expand All @@ -472,9 +467,6 @@ static grpc_lb_policy *create_pick_first(grpc_exec_ctx *exec_ctx,
}

memset(&sc_args, 0, sizeof(grpc_subchannel_args));
/* server_name will be copied as part of the subchannel creation. This makes
* the copying of server_name (a borrowed pointer) OK. */
sc_args.server_name = server_name;
sc_args.addr = &addresses->addresses[i].address;
sc_args.args = args->args;

Expand Down
12 changes: 2 additions & 10 deletions src/core/ext/lb_policy/round_robin/round_robin.c
Original file line number Diff line number Diff line change
Expand Up @@ -703,15 +703,10 @@ static grpc_lb_policy *round_robin_create(grpc_exec_ctx *exec_ctx,
grpc_lb_policy_args *args) {
GPR_ASSERT(args->client_channel_factory != NULL);

/* Get server name. */
const grpc_arg *arg =
grpc_channel_args_find(args->args, GRPC_ARG_SERVER_NAME);
const char *server_name =
arg != NULL && arg->type == GRPC_ARG_STRING ? arg->value.string : NULL;

/* Find the number of backend addresses. We ignore balancer
* addresses, since we don't know how to handle them. */
arg = grpc_channel_args_find(args->args, GRPC_ARG_LB_ADDRESSES);
const grpc_arg *arg =
grpc_channel_args_find(args->args, GRPC_ARG_LB_ADDRESSES);
GPR_ASSERT(arg != NULL && arg->type == GRPC_ARG_POINTER);
grpc_lb_addresses *addresses = arg->value.pointer.p;
size_t num_addrs = 0;
Expand All @@ -734,9 +729,6 @@ static grpc_lb_policy *round_robin_create(grpc_exec_ctx *exec_ctx,
if (addresses->addresses[i].is_balancer) continue;

memset(&sc_args, 0, sizeof(grpc_subchannel_args));
/* server_name will be copied as part of the subchannel creation. This makes
* the copying of server_name (a borrowed pointer) OK. */
sc_args.server_name = server_name;
sc_args.addr = &addresses->addresses[i].address;
sc_args.args = args->args;

Expand Down
7 changes: 1 addition & 6 deletions src/core/ext/resolver/dns/native/dns_resolver.c
Original file line number Diff line number Diff line change
Expand Up @@ -264,12 +264,7 @@ static grpc_resolver *dns_create(grpc_resolver_args *args,
grpc_resolver_init(&r->base, &dns_resolver_vtable);
r->name_to_resolve = proxy_name == NULL ? gpr_strdup(path) : proxy_name;
r->default_port = gpr_strdup(default_port);
grpc_arg server_name_arg;
server_name_arg.type = GRPC_ARG_STRING;
server_name_arg.key = GRPC_ARG_SERVER_NAME;
server_name_arg.value.string = (char *)path;
r->channel_args =
grpc_channel_args_copy_and_add(args->args, &server_name_arg, 1);
r->channel_args = grpc_channel_args_copy(args->args);
gpr_backoff_init(&r->backoff_state, GRPC_DNS_INITIAL_CONNECT_BACKOFF_SECONDS,
GRPC_DNS_RECONNECT_BACKOFF_MULTIPLIER,
GRPC_DNS_RECONNECT_JITTER,
Expand Down
7 changes: 1 addition & 6 deletions src/core/ext/resolver/sockaddr/sockaddr_resolver.c
Original file line number Diff line number Diff line change
Expand Up @@ -198,12 +198,7 @@ static grpc_resolver *sockaddr_create(grpc_resolver_args *args,
sockaddr_resolver *r = gpr_malloc(sizeof(sockaddr_resolver));
memset(r, 0, sizeof(*r));
r->addresses = addresses;
grpc_arg server_name_arg;
server_name_arg.type = GRPC_ARG_STRING;
server_name_arg.key = GRPC_ARG_SERVER_NAME;
server_name_arg.value.string = args->uri->path;
r->channel_args =
grpc_channel_args_copy_and_add(args->args, &server_name_arg, 1);
r->channel_args = grpc_channel_args_copy(args->args);
gpr_mu_init(&r->mu);
grpc_resolver_init(&r->base, &sockaddr_resolver_vtable);
return &r->base;
Expand Down
Loading

0 comments on commit 201db7d

Please sign in to comment.