From 1d0fce9d60494936aa0e79c6b262d113b5b1acd5 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 26 Oct 2016 08:26:22 -0700 Subject: Add some endpoint pair tests --- src/core/lib/surface/server.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/surface/server.c b/src/core/lib/surface/server.c index 3a90308058..648c23580a 100644 --- a/src/core/lib/surface/server.c +++ b/src/core/lib/surface/server.c @@ -195,6 +195,7 @@ struct grpc_server { grpc_completion_queue **cqs; grpc_pollset **pollsets; size_t cq_count; + size_t pollset_count; bool started; /* The two following mutexes control access to server-state @@ -1084,7 +1085,7 @@ void grpc_server_start(grpc_server *server) { GRPC_API_TRACE("grpc_server_start(server=%p)", 1, (server)); server->started = true; - size_t pollset_count = 0; + server->pollset_count = 0; server->pollsets = gpr_malloc(sizeof(grpc_pollset *) * server->cq_count); server->request_freelist_per_cq = gpr_malloc(sizeof(*server->request_freelist_per_cq) * server->cq_count); @@ -1092,7 +1093,8 @@ void grpc_server_start(grpc_server *server) { gpr_malloc(sizeof(*server->requested_calls_per_cq) * server->cq_count); for (i = 0; i < server->cq_count; i++) { if (!grpc_cq_is_non_listening_server_cq(server->cqs[i])) { - server->pollsets[pollset_count++] = grpc_cq_pollset(server->cqs[i]); + server->pollsets[server->pollset_count++] = + grpc_cq_pollset(server->cqs[i]); } server->request_freelist_per_cq[i] = gpr_stack_lockfree_create((size_t)server->max_requested_calls_per_cq); @@ -1111,7 +1113,8 @@ void grpc_server_start(grpc_server *server) { } for (l = server->listeners; l; l = l->next) { - l->start(&exec_ctx, server, l->arg, server->pollsets, pollset_count); + l->start(&exec_ctx, server, l->arg, server->pollsets, + server->pollset_count); } grpc_exec_ctx_finish(&exec_ctx); @@ -1119,7 +1122,7 @@ void grpc_server_start(grpc_server *server) { void grpc_server_get_pollsets(grpc_server *server, grpc_pollset ***pollsets, size_t *pollset_count) { - *pollset_count = server->cq_count; + *pollset_count = server->pollset_count; *pollsets = server->pollsets; } -- cgit v1.2.3 From c968e60e2ef60cd20353c34fb6260a63ce9db031 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 2 Nov 2016 14:07:36 -0700 Subject: Use JSON for service config channel arg. --- src/core/ext/client_channel/client_channel.c | 51 ++++++++--- src/core/lib/channel/message_size_filter.c | 34 +++++--- src/core/lib/support/string.c | 9 ++ src/core/lib/support/string.h | 3 + src/core/lib/transport/method_config.c | 122 +++++++++++++++++++++++++++ src/core/lib/transport/method_config.h | 7 ++ 6 files changed, 202 insertions(+), 24 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index ff773ac334..e569af68e9 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -94,17 +94,44 @@ static int method_parameters_cmp(void *value1, void *value2) { static const grpc_mdstr_hash_table_vtable method_parameters_vtable = { gpr_free, method_parameters_copy, method_parameters_cmp}; -static void *method_config_convert_value( - const grpc_method_config *method_config) { +static void *method_config_convert_value(const grpc_json *json) { + wait_for_ready_value wait_for_ready = WAIT_FOR_READY_UNSET; + gpr_timespec timeout = { 0, 0, GPR_TIMESPAN }; + for (grpc_json* field = json->child; field != NULL; field = field->next) { + if (field->key == NULL) continue; + if (strcmp(field->key, "wait_for_ready") == 0) { + if (wait_for_ready != WAIT_FOR_READY_UNSET) return NULL; // Duplicate. + if (field->type != GRPC_JSON_TRUE && field->type != GRPC_JSON_FALSE) { + return NULL; + } + wait_for_ready = field->type == GRPC_JSON_TRUE; + } else if (strcmp(field->key, "timeout") == 0) { + if (timeout.tv_sec > 0 || timeout.tv_nsec > 0) return NULL; // Duplicate. + if (field->type != GRPC_JSON_OBJECT) return NULL; + if (field->child == NULL) return NULL; + for (grpc_json* subfield = field->child; subfield != NULL; + subfield = subfield->next) { + if (subfield->key == NULL) return NULL; + if (strcmp(subfield->key, "seconds") == 0) { + if (timeout.tv_sec > 0) return NULL; // Duplicate. + if (subfield->type != GRPC_JSON_NUMBER) return NULL; + timeout.tv_sec = gpr_parse_nonnegative_number(subfield->value); + if (timeout.tv_sec == -1) return NULL; + } else if (strcmp(subfield->key, "nanos") == 0) { + if (timeout.tv_nsec > 0) return NULL; // Duplicate. + if (subfield->type != GRPC_JSON_NUMBER) return NULL; + timeout.tv_nsec = gpr_parse_nonnegative_number(subfield->value); + if (timeout.tv_nsec == -1) return NULL; + } else { + // Unknown key. + return NULL; + } + } + } + } method_parameters *value = gpr_malloc(sizeof(method_parameters)); - const gpr_timespec *timeout = grpc_method_config_get_timeout(method_config); - value->timeout = timeout != NULL ? *timeout : gpr_time_0(GPR_TIMESPAN); - const bool *wait_for_ready = - grpc_method_config_get_wait_for_ready(method_config); - value->wait_for_ready = - wait_for_ready == NULL - ? WAIT_FOR_READY_UNSET - : (wait_for_ready ? WAIT_FOR_READY_TRUE : WAIT_FOR_READY_FALSE); + value->timeout = timeout; + value->wait_for_ready = wait_for_ready; return value; } @@ -285,8 +312,8 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_channel_args_find(lb_policy_args.args, GRPC_ARG_SERVICE_CONFIG); if (channel_arg != NULL) { GPR_ASSERT(channel_arg->type == GRPC_ARG_POINTER); - method_params_table = grpc_method_config_table_convert( - (grpc_method_config_table *)channel_arg->value.pointer.p, + method_params_table = grpc_method_config_table_create_from_json( + (grpc_json *)channel_arg->value.pointer.p, method_config_convert_value, &method_parameters_vtable); } grpc_channel_args_destroy(chand->resolver_result); diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index 7dc5ae0df1..4723ab8098 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -39,6 +39,7 @@ #include #include "src/core/lib/channel/channel_args.h" +#include "src/core/lib/support/string.h" #include "src/core/lib/transport/method_config.h" #define DEFAULT_MAX_SEND_MESSAGE_LENGTH -1 // Unlimited. @@ -69,17 +70,26 @@ static int message_size_limits_cmp(void* value1, void* value2) { static const grpc_mdstr_hash_table_vtable message_size_limits_vtable = { gpr_free, message_size_limits_copy, message_size_limits_cmp}; -static void* method_config_convert_value( - const grpc_method_config* method_config) { +static void* method_config_convert_value(const grpc_json* json) { + int max_request_message_bytes = -1; + int max_response_message_bytes = -1; + for (grpc_json* field = json->child; field != NULL; field = field->next) { + if (field->key == NULL) continue; + if (strcmp(field->key, "max_request_message_bytes") == 0) { + if (max_request_message_bytes >= 0) return NULL; // Duplicate. + if (field->type != GRPC_JSON_NUMBER) return NULL; + max_request_message_bytes = gpr_parse_nonnegative_number(field->value); + if (max_request_message_bytes == -1) return NULL; + } else if (strcmp(field->key, "max_response_message_bytes") == 0) { + if (max_response_message_bytes >= 0) return NULL; // Duplicate. + if (field->type != GRPC_JSON_NUMBER) return NULL; + max_response_message_bytes = gpr_parse_nonnegative_number(field->value); + if (max_response_message_bytes == -1) return NULL; + } + } message_size_limits* value = gpr_malloc(sizeof(message_size_limits)); - const int32_t* max_request_message_bytes = - grpc_method_config_get_max_request_message_bytes(method_config); - value->max_send_size = - max_request_message_bytes != NULL ? *max_request_message_bytes : -1; - const int32_t* max_response_message_bytes = - grpc_method_config_get_max_response_message_bytes(method_config); - value->max_recv_size = - max_response_message_bytes != NULL ? *max_response_message_bytes : -1; + value->max_send_size = max_request_message_bytes; + value->max_recv_size = max_response_message_bytes; return value; } @@ -224,8 +234,8 @@ static void init_channel_elem(grpc_exec_ctx* exec_ctx, grpc_channel_args_find(args->channel_args, GRPC_ARG_SERVICE_CONFIG); if (channel_arg != NULL) { GPR_ASSERT(channel_arg->type == GRPC_ARG_POINTER); - chand->method_limit_table = grpc_method_config_table_convert( - (grpc_method_config_table*)channel_arg->value.pointer.p, + chand->method_limit_table = grpc_method_config_table_create_from_json( + (grpc_json*)channel_arg->value.pointer.p, method_config_convert_value, &message_size_limits_vtable); } } diff --git a/src/core/lib/support/string.c b/src/core/lib/support/string.c index d17fb9da4b..56f29492bf 100644 --- a/src/core/lib/support/string.c +++ b/src/core/lib/support/string.c @@ -34,7 +34,9 @@ #include "src/core/lib/support/string.h" #include +#include #include +#include #include #include @@ -194,6 +196,13 @@ int int64_ttoa(int64_t value, char *string) { return i; } +int gpr_parse_nonnegative_number(const char* value) { + char* end; + long result = strtol(value, &end, 0); + if (*end != '\0' || result < 0 || result > INT_MAX) return -1; + return (int)result; +} + char *gpr_leftpad(const char *str, char flag, size_t length) { const size_t str_length = strlen(str); const size_t out_length = str_length > length ? str_length : length; diff --git a/src/core/lib/support/string.h b/src/core/lib/support/string.h index 9a94e9471c..3a5a8ed826 100644 --- a/src/core/lib/support/string.h +++ b/src/core/lib/support/string.h @@ -80,6 +80,9 @@ NOTE: This function ensures sufficient bit width even on Win x64, where long is 32bit is size.*/ int int64_ttoa(int64_t value, char *output); +// Parses a non-negative number from a value string. Returns -1 on error. +int gpr_parse_nonnegative_number(const char* value); + /* Reverse a run of bytes */ void gpr_reverse_bytes(char *str, int len); diff --git a/src/core/lib/transport/method_config.c b/src/core/lib/transport/method_config.c index 57d97700bf..23cab2b96b 100644 --- a/src/core/lib/transport/method_config.c +++ b/src/core/lib/transport/method_config.c @@ -39,6 +39,8 @@ #include #include +#include "src/core/lib/json/json.h" +#include "src/core/lib/support/string.h" #include "src/core/lib/transport/mdstr_hash_table.h" #include "src/core/lib/transport/metadata.h" @@ -338,3 +340,123 @@ grpc_mdstr_hash_table* grpc_method_config_table_convert( // Return the new table. return new_table; } + +// Returns the number of names specified in the method config \a json. +static size_t count_names_in_method_config_json(grpc_json* json) { + size_t num_names = 0; + for (grpc_json* field = json->child; field != NULL; field = field->next) { + if (field->key != NULL && strcmp(field->key, "name") == 0) ++num_names; + } + return num_names; +} + +// Returns a path string for the name specified by \a json. +// Returns NULL on error. Caller takes ownership of result. +static char* parse_json_method_name(grpc_json* json) { + if (json->type != GRPC_JSON_OBJECT) return NULL; + const char* service_name = NULL; + const char* method_name = NULL; + for (grpc_json* child = json->child; child != NULL; child = child->next) { + if (child->key == NULL) return NULL; + if (child->type != GRPC_JSON_STRING) return NULL; + if (strcmp(child->key, "service") == 0) { + if (service_name != NULL) return NULL; // Duplicate. + if (child->value == NULL) return NULL; + service_name = child->value; + } else if (strcmp(child->key, "method") == 0) { + if (method_name != NULL) return NULL; // Duplicate. + if (child->value == NULL) return NULL; + method_name = child->value; + } + } + if (service_name == NULL) return NULL; // Required field. + char* path; + gpr_asprintf(&path, "/%s/%s", service_name, + method_name == NULL ? "*" : method_name); + return path; +} + +// Parses the method config from \a json. Adds an entry to \a entries for +// each name found, incrementing \a idx for each entry added. +static bool parse_json_method_config( + grpc_json* json, + void* (*create_value)(const grpc_json* method_config_json), + const grpc_mdstr_hash_table_vtable* vtable, + grpc_mdstr_hash_table_entry* entries, size_t *idx) { + // Construct value. + void* method_config = create_value(json); + if (method_config == NULL) return NULL; + // Construct list of paths. + bool retval = false; + gpr_strvec paths; + gpr_strvec_init(&paths); + for (grpc_json* child = json->child; child != NULL; child = child->next) { + if (child->key == NULL) continue; + if (strcmp(child->key, "name") == 0) { + if (child->type != GRPC_JSON_ARRAY) goto done; + for (grpc_json* name = child->child; name != NULL; name = name->next) { + char* path = parse_json_method_name(name); + gpr_strvec_add(&paths, path); + } + } + } + if (paths.count == 0) goto done; // No names specified. + // Add entry for each path. + for (size_t i = 0; i < paths.count; ++i) { + entries[*idx].key = grpc_mdstr_from_string(paths.strs[i]); + entries[*idx].value = vtable->copy_value(method_config); + entries[*idx].vtable = vtable; + ++*idx; + } + retval = true; +done: + vtable->destroy_value(method_config); + gpr_strvec_destroy(&paths); + return retval; +} + +grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( + const grpc_json* json, + void* (*create_value)(const grpc_json* method_config_json), + const grpc_mdstr_hash_table_vtable* vtable) { + // Traverse parsed JSON tree. + if (json->type != GRPC_JSON_OBJECT || json->key != NULL) return NULL; + size_t num_entries = 0; + grpc_mdstr_hash_table_entry* entries = NULL; + for (grpc_json* field = json->child; field != NULL; field = field->next) { + if (field->key == NULL) return NULL; + if (strcmp(field->key, "method_config") == 0) { + if (entries != NULL) return NULL; // Duplicate. + if (field->type != GRPC_JSON_ARRAY) return NULL; + // Find number of entries. + for (grpc_json* method = field->child; method != NULL; + method = method->next) { + num_entries += count_names_in_method_config_json(method); + } + // Populate method config table entries. + entries = + gpr_malloc(num_entries * sizeof(grpc_method_config_table_entry)); + size_t idx = 0; + for (grpc_json* method = field->child; method != NULL; + method = method->next) { + if (!parse_json_method_config(method, create_value, vtable, entries, + &idx)) { + return NULL; + } + } + GPR_ASSERT(idx == num_entries); + } + } + // Instantiate method config table. + grpc_mdstr_hash_table* method_config_table = NULL; + if (entries != NULL) { + method_config_table = grpc_mdstr_hash_table_create(num_entries, entries); + // Clean up. + for (size_t i = 0; i < num_entries; ++i) { + GRPC_MDSTR_UNREF(entries[i].key); + vtable->destroy_value(entries[i].value); + } + gpr_free(entries); + } + return method_config_table; +} diff --git a/src/core/lib/transport/method_config.h b/src/core/lib/transport/method_config.h index 58fedd9436..eac05f8173 100644 --- a/src/core/lib/transport/method_config.h +++ b/src/core/lib/transport/method_config.h @@ -37,6 +37,7 @@ #include #include +#include "src/core/lib/json/json.h" #include "src/core/lib/transport/mdstr_hash_table.h" #include "src/core/lib/transport/metadata.h" @@ -133,4 +134,10 @@ grpc_mdstr_hash_table* grpc_method_config_table_convert( void* (*convert_value)(const grpc_method_config* method_config), const grpc_mdstr_hash_table_vtable* vtable); +// FIXME: document +grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( + const grpc_json* json, + void* (*create_value)(const grpc_json* method_config_json), + const grpc_mdstr_hash_table_vtable* vtable); + #endif /* GRPC_CORE_LIB_TRANSPORT_METHOD_CONFIG_H */ -- cgit v1.2.3 From 896d92568b1b615a19dec26035596de244cbf535 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 3 Nov 2016 07:58:25 -0700 Subject: Add grpc_json_tree to handle refcounting for channel arg. --- src/core/ext/client_channel/client_channel.c | 8 ++-- src/core/lib/channel/message_size_filter.c | 5 ++- src/core/lib/json/json.c | 59 +++++++++++++++++++++++++++ src/core/lib/json/json.h | 19 +++++++++ src/core/lib/transport/method_config.c | 23 +++++++++++ src/core/lib/transport/method_config.h | 2 + test/core/end2end/connection_refused_test.c | 22 +++++----- test/core/end2end/tests/cancel_after_accept.c | 23 +++++------ test/core/end2end/tests/max_message_length.c | 47 ++++++++++----------- 9 files changed, 155 insertions(+), 53 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index e569af68e9..4a1c0ae1a2 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -104,7 +104,8 @@ static void *method_config_convert_value(const grpc_json *json) { if (field->type != GRPC_JSON_TRUE && field->type != GRPC_JSON_FALSE) { return NULL; } - wait_for_ready = field->type == GRPC_JSON_TRUE; + wait_for_ready = field->type == GRPC_JSON_TRUE + ? WAIT_FOR_READY_TRUE : WAIT_FOR_READY_FALSE; } else if (strcmp(field->key, "timeout") == 0) { if (timeout.tv_sec > 0 || timeout.tv_nsec > 0) return NULL; // Duplicate. if (field->type != GRPC_JSON_OBJECT) return NULL; @@ -312,9 +313,10 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_channel_args_find(lb_policy_args.args, GRPC_ARG_SERVICE_CONFIG); if (channel_arg != NULL) { GPR_ASSERT(channel_arg->type == GRPC_ARG_POINTER); + grpc_json_tree* json_tree = channel_arg->value.pointer.p; method_params_table = grpc_method_config_table_create_from_json( - (grpc_json *)channel_arg->value.pointer.p, - method_config_convert_value, &method_parameters_vtable); + json_tree->root, method_config_convert_value, + &method_parameters_vtable); } grpc_channel_args_destroy(chand->resolver_result); chand->resolver_result = NULL; diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index 4723ab8098..86fdf72684 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -234,9 +234,10 @@ static void init_channel_elem(grpc_exec_ctx* exec_ctx, grpc_channel_args_find(args->channel_args, GRPC_ARG_SERVICE_CONFIG); if (channel_arg != NULL) { GPR_ASSERT(channel_arg->type == GRPC_ARG_POINTER); + grpc_json_tree* json_tree = channel_arg->value.pointer.p; chand->method_limit_table = grpc_method_config_table_create_from_json( - (grpc_json*)channel_arg->value.pointer.p, - method_config_convert_value, &message_size_limits_vtable); + json_tree->root, method_config_convert_value, + &message_size_limits_vtable); } } diff --git a/src/core/lib/json/json.c b/src/core/lib/json/json.c index 5b583a1f2e..c1a99df6c3 100644 --- a/src/core/lib/json/json.c +++ b/src/core/lib/json/json.c @@ -34,6 +34,8 @@ #include #include +#include +#include #include "src/core/lib/json/json.h" @@ -62,3 +64,60 @@ void grpc_json_destroy(grpc_json *json) { gpr_free(json); } + +int grpc_json_cmp(const grpc_json* json1, const grpc_json* json2) { + if (json1 == NULL) { + if (json2 != NULL) return 1; + return 0; // Both NULL. + } else { + if (json2 == NULL) return -1; + } + // Compare type. + if (json1->type > json2->type) return 1; + if (json1->type < json2->type) return -1; + // Compare key. + if (json1->key == NULL) { + if (json2->key != NULL) return -1; + } else { + if (json2->key == NULL) return 1; + int retval = strcmp(json1->key, json2->key); + if (retval != 0) return retval; + } + // Compare value. + if (json1->value == NULL) { + if (json2->value != NULL) return -1; + } else { + if (json2->value == NULL) return 1; + int retval = strcmp(json1->value, json2->value); + if (retval != 0) return retval; + } + // Recursively compare the next pointer. + int retval = grpc_json_cmp(json1->next, json2->next); + if (retval != 0) return retval; + // Recursively compare the child pointer. + retval = grpc_json_cmp(json1->child, json2->child); + if (retval != 0) return retval; + // Both are the same. + return 0; +} + +grpc_json_tree* grpc_json_tree_create(const char* json_string) { + grpc_json_tree* tree = gpr_malloc(sizeof(*tree)); + tree->string = gpr_strdup(json_string); + tree->root = grpc_json_parse_string(tree->string); + gpr_ref_init(&tree->refs, 1); + return tree; +} + +grpc_json_tree* grpc_json_tree_ref(grpc_json_tree* tree) { + gpr_ref(&tree->refs); + return tree; +} + +void grpc_json_tree_unref(grpc_json_tree* tree) { + if (gpr_unref(&tree->refs)) { + grpc_json_destroy(tree->root); + gpr_free(tree->string); + gpr_free(tree); + } +} diff --git a/src/core/lib/json/json.h b/src/core/lib/json/json.h index 681df4bb77..e18ace7547 100644 --- a/src/core/lib/json/json.h +++ b/src/core/lib/json/json.h @@ -36,6 +36,8 @@ #include +#include + #include "src/core/lib/json/json_common.h" /* A tree-like structure to hold json values. The key and value pointers @@ -85,4 +87,21 @@ char *grpc_json_dump_to_string(grpc_json *json, int indent); grpc_json *grpc_json_create(grpc_json_type type); void grpc_json_destroy(grpc_json *json); +/* Compares two JSON trees. */ +int grpc_json_cmp(const grpc_json* json1, const grpc_json* json2); + +/* A wrapper that contains the string used for underlying allocation and + is refcounted. */ +typedef struct { + grpc_json* root; + char* string; + gpr_refcount refs; +} grpc_json_tree; + +/* Creates a copy of \a json_string. */ +grpc_json_tree* grpc_json_tree_create(const char* json_string); + +grpc_json_tree* grpc_json_tree_ref(grpc_json_tree* tree); +void grpc_json_tree_unref(grpc_json_tree* tree); + #endif /* GRPC_CORE_LIB_JSON_JSON_H */ diff --git a/src/core/lib/transport/method_config.c b/src/core/lib/transport/method_config.c index 23cab2b96b..c2bf88a250 100644 --- a/src/core/lib/transport/method_config.c +++ b/src/core/lib/transport/method_config.c @@ -460,3 +460,26 @@ grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( } return method_config_table; } + +static void* copy_json_tree(void* t) { return grpc_json_tree_ref(t); } + +static void destroy_json_tree(void* t) { grpc_json_tree_unref(t); } + +static int cmp_json_tree(void* t1, void* t2) { + grpc_json_tree* tree1 = t1; + grpc_json_tree* tree2 = t2; + return grpc_json_cmp(tree1->root, tree2->root); +} + +static grpc_arg_pointer_vtable service_config_arg_vtable = { + copy_json_tree, destroy_json_tree, cmp_json_tree}; + +grpc_arg grpc_service_config_create_channel_arg( + grpc_json_tree* service_config) { + grpc_arg arg; + arg.type = GRPC_ARG_POINTER; + arg.key = GRPC_ARG_SERVICE_CONFIG; + arg.value.pointer.p = service_config; + arg.value.pointer.vtable = &service_config_arg_vtable; + return arg; +} diff --git a/src/core/lib/transport/method_config.h b/src/core/lib/transport/method_config.h index eac05f8173..84c6329415 100644 --- a/src/core/lib/transport/method_config.h +++ b/src/core/lib/transport/method_config.h @@ -140,4 +140,6 @@ grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( void* (*create_value)(const grpc_json* method_config_json), const grpc_mdstr_hash_table_vtable* vtable); +grpc_arg grpc_service_config_create_channel_arg(grpc_json_tree* service_config); + #endif /* GRPC_CORE_LIB_TRANSPORT_METHOD_CONFIG_H */ diff --git a/test/core/end2end/connection_refused_test.c b/test/core/end2end/connection_refused_test.c index 13414c0378..ba9fb3d30a 100644 --- a/test/core/end2end/connection_refused_test.c +++ b/test/core/end2end/connection_refused_test.c @@ -76,18 +76,18 @@ static void run_test(bool wait_for_ready, bool use_service_config) { grpc_channel_args *args = NULL; if (use_service_config) { GPR_ASSERT(wait_for_ready); - grpc_method_config_table_entry entry = { - grpc_mdstr_from_string("/service/method"), - grpc_method_config_create(&wait_for_ready, NULL, NULL, NULL), - }; - grpc_method_config_table *method_config_table = - grpc_method_config_table_create(1, &entry); - GRPC_MDSTR_UNREF(entry.method_name); - grpc_method_config_unref(entry.method_config); - grpc_arg arg = - grpc_method_config_table_create_channel_arg(method_config_table); + grpc_json_tree* service_config_json = grpc_json_tree_create( + "{\n" + " \"method_config\": [ {\n" + " \"name\": [\n" + " { \"service\": \"service\", \"method\": \"method\" }\n" + " ],\n" + " \"wait_for_ready\": true\n" + " } ]\n" + "}"); + grpc_arg arg = grpc_service_config_create_channel_arg(service_config_json); args = grpc_channel_args_copy_and_add(args, &arg, 1); - grpc_method_config_table_unref(method_config_table); + grpc_json_tree_unref(service_config_json); } /* create a call, channel to a port which will refuse connection */ diff --git a/test/core/end2end/tests/cancel_after_accept.c b/test/core/end2end/tests/cancel_after_accept.c index 104d8fd54f..68a8256092 100644 --- a/test/core/end2end/tests/cancel_after_accept.c +++ b/test/core/end2end/tests/cancel_after_accept.c @@ -132,19 +132,18 @@ static void test_cancel_after_accept(grpc_end2end_test_config config, grpc_channel_args *args = NULL; if (use_service_config) { - gpr_timespec timeout = {5, 0, GPR_TIMESPAN}; - grpc_method_config_table_entry entry = { - grpc_mdstr_from_string("/service/method"), - grpc_method_config_create(NULL, &timeout, NULL, NULL), - }; - grpc_method_config_table *method_config_table = - grpc_method_config_table_create(1, &entry); - GRPC_MDSTR_UNREF(entry.method_name); - grpc_method_config_unref(entry.method_config); - grpc_arg arg = - grpc_method_config_table_create_channel_arg(method_config_table); + grpc_json_tree* service_config_json = grpc_json_tree_create( + "{\n" + " \"method_config\": [ {\n" + " \"name\": [\n" + " { \"service\": \"service\", \"method\": \"method\" }\n" + " ],\n" + " \"timeout\": { \"seconds\": 5 }\n" + " } ]\n" + "}"); + grpc_arg arg = grpc_service_config_create_channel_arg(service_config_json); args = grpc_channel_args_copy_and_add(args, &arg, 1); - grpc_method_config_table_unref(method_config_table); + grpc_json_tree_unref(service_config_json); } grpc_end2end_test_fixture f = diff --git a/test/core/end2end/tests/max_message_length.c b/test/core/end2end/tests/max_message_length.c index 3a927ddcbc..f0bc7acdff 100644 --- a/test/core/end2end/tests/max_message_length.c +++ b/test/core/end2end/tests/max_message_length.c @@ -137,19 +137,18 @@ static void test_max_message_length_on_request(grpc_end2end_test_config config, if (use_service_config) { // We don't currently support service configs on the server side. GPR_ASSERT(send_limit); - int32_t max_request_message_bytes = 5; - grpc_method_config_table_entry entry = { - grpc_mdstr_from_string("/service/method"), - grpc_method_config_create(NULL, NULL, &max_request_message_bytes, NULL), - }; - grpc_method_config_table *method_config_table = - grpc_method_config_table_create(1, &entry); - GRPC_MDSTR_UNREF(entry.method_name); - grpc_method_config_unref(entry.method_config); - grpc_arg arg = - grpc_method_config_table_create_channel_arg(method_config_table); + grpc_json_tree* service_config_json = grpc_json_tree_create( + "{\n" + " \"method_config\": [ {\n" + " \"name\": [\n" + " { \"service\": \"service\", \"method\": \"method\" }\n" + " ],\n" + " \"max_request_message_bytes\": 5\n" + " } ]\n" + "}"); + grpc_arg arg = grpc_service_config_create_channel_arg(service_config_json); client_args = grpc_channel_args_copy_and_add(NULL, &arg, 1); - grpc_method_config_table_unref(method_config_table); + grpc_json_tree_unref(service_config_json); } else { // Set limit via channel args. grpc_arg arg; @@ -309,20 +308,18 @@ static void test_max_message_length_on_response(grpc_end2end_test_config config, if (use_service_config) { // We don't currently support service configs on the server side. GPR_ASSERT(!send_limit); - int32_t max_response_message_bytes = 5; - grpc_method_config_table_entry entry = { - grpc_mdstr_from_string("/service/method"), - grpc_method_config_create(NULL, NULL, NULL, - &max_response_message_bytes), - }; - grpc_method_config_table *method_config_table = - grpc_method_config_table_create(1, &entry); - GRPC_MDSTR_UNREF(entry.method_name); - grpc_method_config_unref(entry.method_config); - grpc_arg arg = - grpc_method_config_table_create_channel_arg(method_config_table); + grpc_json_tree* service_config_json = grpc_json_tree_create( + "{\n" + " \"method_config\": [ {\n" + " \"name\": [\n" + " { \"service\": \"service\", \"method\": \"method\" }\n" + " ],\n" + " \"max_response_message_bytes\": 5\n" + " } ]\n" + "}"); + grpc_arg arg = grpc_service_config_create_channel_arg(service_config_json); client_args = grpc_channel_args_copy_and_add(NULL, &arg, 1); - grpc_method_config_table_unref(method_config_table); + grpc_json_tree_unref(service_config_json); } else { // Set limit via channel args. grpc_arg arg; -- cgit v1.2.3 From 638c38a79cf69577d2fa2ff4b546bc3ea97da860 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 3 Nov 2016 08:09:34 -0700 Subject: Remove now-unnecessary code. --- src/core/lib/transport/method_config.c | 322 +++------------------------------ src/core/lib/transport/method_config.h | 105 +---------- 2 files changed, 32 insertions(+), 395 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/transport/method_config.c b/src/core/lib/transport/method_config.c index c2bf88a250..a163af91d6 100644 --- a/src/core/lib/transport/method_config.c +++ b/src/core/lib/transport/method_config.c @@ -37,309 +37,10 @@ #include #include #include -#include #include "src/core/lib/json/json.h" #include "src/core/lib/support/string.h" #include "src/core/lib/transport/mdstr_hash_table.h" -#include "src/core/lib/transport/metadata.h" - -// -// grpc_method_config -// - -// bool vtable - -static void* bool_copy(void* valuep) { - bool value = *(bool*)valuep; - bool* new_value = gpr_malloc(sizeof(bool)); - *new_value = value; - return new_value; -} - -static int bool_cmp(void* v1, void* v2) { - bool b1 = *(bool*)v1; - bool b2 = *(bool*)v2; - if (!b1 && b2) return -1; - if (b1 && !b2) return 1; - return 0; -} - -static grpc_mdstr_hash_table_vtable bool_vtable = {gpr_free, bool_copy, - bool_cmp}; - -// timespec vtable - -static void* timespec_copy(void* valuep) { - gpr_timespec value = *(gpr_timespec*)valuep; - gpr_timespec* new_value = gpr_malloc(sizeof(gpr_timespec)); - *new_value = value; - return new_value; -} - -static int timespec_cmp(void* v1, void* v2) { - return gpr_time_cmp(*(gpr_timespec*)v1, *(gpr_timespec*)v2); -} - -static grpc_mdstr_hash_table_vtable timespec_vtable = {gpr_free, timespec_copy, - timespec_cmp}; - -// int32 vtable - -static void* int32_copy(void* valuep) { - int32_t value = *(int32_t*)valuep; - int32_t* new_value = gpr_malloc(sizeof(int32_t)); - *new_value = value; - return new_value; -} - -static int int32_cmp(void* v1, void* v2) { - int32_t i1 = *(int32_t*)v1; - int32_t i2 = *(int32_t*)v2; - if (i1 < i2) return -1; - if (i1 > i2) return 1; - return 0; -} - -static grpc_mdstr_hash_table_vtable int32_vtable = {gpr_free, int32_copy, - int32_cmp}; - -// Hash table keys. -#define GRPC_METHOD_CONFIG_WAIT_FOR_READY "grpc.wait_for_ready" // bool -#define GRPC_METHOD_CONFIG_TIMEOUT "grpc.timeout" // gpr_timespec -#define GRPC_METHOD_CONFIG_MAX_REQUEST_MESSAGE_BYTES \ - "grpc.max_request_message_bytes" // int32 -#define GRPC_METHOD_CONFIG_MAX_RESPONSE_MESSAGE_BYTES \ - "grpc.max_response_message_bytes" // int32 - -struct grpc_method_config { - grpc_mdstr_hash_table* table; - grpc_mdstr* wait_for_ready_key; - grpc_mdstr* timeout_key; - grpc_mdstr* max_request_message_bytes_key; - grpc_mdstr* max_response_message_bytes_key; -}; - -grpc_method_config* grpc_method_config_create( - bool* wait_for_ready, gpr_timespec* timeout, - int32_t* max_request_message_bytes, int32_t* max_response_message_bytes) { - grpc_method_config* method_config = gpr_malloc(sizeof(grpc_method_config)); - memset(method_config, 0, sizeof(grpc_method_config)); - method_config->wait_for_ready_key = - grpc_mdstr_from_string(GRPC_METHOD_CONFIG_WAIT_FOR_READY); - method_config->timeout_key = - grpc_mdstr_from_string(GRPC_METHOD_CONFIG_TIMEOUT); - method_config->max_request_message_bytes_key = - grpc_mdstr_from_string(GRPC_METHOD_CONFIG_MAX_REQUEST_MESSAGE_BYTES); - method_config->max_response_message_bytes_key = - grpc_mdstr_from_string(GRPC_METHOD_CONFIG_MAX_RESPONSE_MESSAGE_BYTES); - grpc_mdstr_hash_table_entry entries[4]; - size_t num_entries = 0; - if (wait_for_ready != NULL) { - entries[num_entries].key = method_config->wait_for_ready_key; - entries[num_entries].value = wait_for_ready; - entries[num_entries].vtable = &bool_vtable; - ++num_entries; - } - if (timeout != NULL) { - entries[num_entries].key = method_config->timeout_key; - entries[num_entries].value = timeout; - entries[num_entries].vtable = ×pec_vtable; - ++num_entries; - } - if (max_request_message_bytes != NULL) { - entries[num_entries].key = method_config->max_request_message_bytes_key; - entries[num_entries].value = max_request_message_bytes; - entries[num_entries].vtable = &int32_vtable; - ++num_entries; - } - if (max_response_message_bytes != NULL) { - entries[num_entries].key = method_config->max_response_message_bytes_key; - entries[num_entries].value = max_response_message_bytes; - entries[num_entries].vtable = &int32_vtable; - ++num_entries; - } - method_config->table = grpc_mdstr_hash_table_create(num_entries, entries); - return method_config; -} - -grpc_method_config* grpc_method_config_ref(grpc_method_config* method_config) { - grpc_mdstr_hash_table_ref(method_config->table); - return method_config; -} - -void grpc_method_config_unref(grpc_method_config* method_config) { - if (grpc_mdstr_hash_table_unref(method_config->table)) { - GRPC_MDSTR_UNREF(method_config->wait_for_ready_key); - GRPC_MDSTR_UNREF(method_config->timeout_key); - GRPC_MDSTR_UNREF(method_config->max_request_message_bytes_key); - GRPC_MDSTR_UNREF(method_config->max_response_message_bytes_key); - gpr_free(method_config); - } -} - -int grpc_method_config_cmp(const grpc_method_config* method_config1, - const grpc_method_config* method_config2) { - return grpc_mdstr_hash_table_cmp(method_config1->table, - method_config2->table); -} - -const bool* grpc_method_config_get_wait_for_ready( - const grpc_method_config* method_config) { - return grpc_mdstr_hash_table_get(method_config->table, - method_config->wait_for_ready_key); -} - -const gpr_timespec* grpc_method_config_get_timeout( - const grpc_method_config* method_config) { - return grpc_mdstr_hash_table_get(method_config->table, - method_config->timeout_key); -} - -const int32_t* grpc_method_config_get_max_request_message_bytes( - const grpc_method_config* method_config) { - return grpc_mdstr_hash_table_get( - method_config->table, method_config->max_request_message_bytes_key); -} - -const int32_t* grpc_method_config_get_max_response_message_bytes( - const grpc_method_config* method_config) { - return grpc_mdstr_hash_table_get( - method_config->table, method_config->max_response_message_bytes_key); -} - -// -// grpc_method_config_table -// - -static void method_config_unref(void* valuep) { - grpc_method_config_unref(valuep); -} - -static void* method_config_ref(void* valuep) { - return grpc_method_config_ref(valuep); -} - -static int method_config_cmp(void* valuep1, void* valuep2) { - return grpc_method_config_cmp(valuep1, valuep2); -} - -static const grpc_mdstr_hash_table_vtable method_config_table_vtable = { - method_config_unref, method_config_ref, method_config_cmp}; - -grpc_method_config_table* grpc_method_config_table_create( - size_t num_entries, grpc_method_config_table_entry* entries) { - grpc_mdstr_hash_table_entry* hash_table_entries = - gpr_malloc(sizeof(grpc_mdstr_hash_table_entry) * num_entries); - for (size_t i = 0; i < num_entries; ++i) { - hash_table_entries[i].key = entries[i].method_name; - hash_table_entries[i].value = entries[i].method_config; - hash_table_entries[i].vtable = &method_config_table_vtable; - } - grpc_method_config_table* method_config_table = - grpc_mdstr_hash_table_create(num_entries, hash_table_entries); - gpr_free(hash_table_entries); - return method_config_table; -} - -grpc_method_config_table* grpc_method_config_table_ref( - grpc_method_config_table* table) { - return grpc_mdstr_hash_table_ref(table); -} - -void grpc_method_config_table_unref(grpc_method_config_table* table) { - grpc_mdstr_hash_table_unref(table); -} - -int grpc_method_config_table_cmp(const grpc_method_config_table* table1, - const grpc_method_config_table* table2) { - return grpc_mdstr_hash_table_cmp(table1, table2); -} - -void* grpc_method_config_table_get(const grpc_mdstr_hash_table* table, - const grpc_mdstr* path) { - void* value = grpc_mdstr_hash_table_get(table, path); - // If we didn't find a match for the path, try looking for a wildcard - // entry (i.e., change "/service/method" to "/service/*"). - if (value == NULL) { - const char* path_str = grpc_mdstr_as_c_string(path); - const char* sep = strrchr(path_str, '/') + 1; - const size_t len = (size_t)(sep - path_str); - char* buf = gpr_malloc(len + 2); // '*' and NUL - memcpy(buf, path_str, len); - buf[len] = '*'; - buf[len + 1] = '\0'; - grpc_mdstr* wildcard_path = grpc_mdstr_from_string(buf); - gpr_free(buf); - value = grpc_mdstr_hash_table_get(table, wildcard_path); - GRPC_MDSTR_UNREF(wildcard_path); - } - return value; -} - -static void* copy_arg(void* p) { return grpc_method_config_table_ref(p); } - -static void destroy_arg(void* p) { grpc_method_config_table_unref(p); } - -static int cmp_arg(void* p1, void* p2) { - return grpc_method_config_table_cmp(p1, p2); -} - -static grpc_arg_pointer_vtable arg_vtable = {copy_arg, destroy_arg, cmp_arg}; - -grpc_arg grpc_method_config_table_create_channel_arg( - grpc_method_config_table* table) { - grpc_arg arg; - arg.type = GRPC_ARG_POINTER; - arg.key = GRPC_ARG_SERVICE_CONFIG; - arg.value.pointer.p = table; - arg.value.pointer.vtable = &arg_vtable; - return arg; -} - -// State used by convert_entry() below. -typedef struct conversion_state { - void* (*convert_value)(const grpc_method_config* method_config); - const grpc_mdstr_hash_table_vtable* vtable; - size_t num_entries; - grpc_mdstr_hash_table_entry* entries; -} conversion_state; - -// A function to be passed to grpc_mdstr_hash_table_iterate() to create -// a copy of the entries. -static void convert_entry(const grpc_mdstr_hash_table_entry* entry, - void* user_data) { - conversion_state* state = user_data; - state->entries[state->num_entries].key = GRPC_MDSTR_REF(entry->key); - state->entries[state->num_entries].value = state->convert_value(entry->value); - state->entries[state->num_entries].vtable = state->vtable; - ++state->num_entries; -} - -grpc_mdstr_hash_table* grpc_method_config_table_convert( - const grpc_method_config_table* table, - void* (*convert_value)(const grpc_method_config* method_config), - const grpc_mdstr_hash_table_vtable* vtable) { - // Create an array of the entries in the table with converted values. - conversion_state state; - state.convert_value = convert_value; - state.vtable = vtable; - state.num_entries = 0; - state.entries = gpr_malloc(sizeof(grpc_mdstr_hash_table_entry) * - grpc_mdstr_hash_table_num_entries(table)); - grpc_mdstr_hash_table_iterate(table, convert_entry, &state); - // Create a new table based on the array we just constructed. - grpc_mdstr_hash_table* new_table = - grpc_mdstr_hash_table_create(state.num_entries, state.entries); - // Clean up the array. - for (size_t i = 0; i < state.num_entries; ++i) { - GRPC_MDSTR_UNREF(state.entries[i].key); - vtable->destroy_value(state.entries[i].value); - } - gpr_free(state.entries); - // Return the new table. - return new_table; -} // Returns the number of names specified in the method config \a json. static size_t count_names_in_method_config_json(grpc_json* json) { @@ -435,7 +136,7 @@ grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( } // Populate method config table entries. entries = - gpr_malloc(num_entries * sizeof(grpc_method_config_table_entry)); + gpr_malloc(num_entries * sizeof(grpc_mdstr_hash_table_entry)); size_t idx = 0; for (grpc_json* method = field->child; method != NULL; method = method->next) { @@ -461,6 +162,27 @@ grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( return method_config_table; } +void* grpc_method_config_table_get(const grpc_mdstr_hash_table* table, + const grpc_mdstr* path) { + void* value = grpc_mdstr_hash_table_get(table, path); + // If we didn't find a match for the path, try looking for a wildcard + // entry (i.e., change "/service/method" to "/service/*"). + if (value == NULL) { + const char* path_str = grpc_mdstr_as_c_string(path); + const char* sep = strrchr(path_str, '/') + 1; + const size_t len = (size_t)(sep - path_str); + char* buf = gpr_malloc(len + 2); // '*' and NUL + memcpy(buf, path_str, len); + buf[len] = '*'; + buf[len + 1] = '\0'; + grpc_mdstr* wildcard_path = grpc_mdstr_from_string(buf); + gpr_free(buf); + value = grpc_mdstr_hash_table_get(table, wildcard_path); + GRPC_MDSTR_UNREF(wildcard_path); + } + return value; +} + static void* copy_json_tree(void* t) { return grpc_json_tree_ref(t); } static void destroy_json_tree(void* t) { grpc_json_tree_unref(t); } diff --git a/src/core/lib/transport/method_config.h b/src/core/lib/transport/method_config.h index 84c6329415..f33f4279d6 100644 --- a/src/core/lib/transport/method_config.h +++ b/src/core/lib/transport/method_config.h @@ -32,114 +32,29 @@ #ifndef GRPC_CORE_LIB_TRANSPORT_METHOD_CONFIG_H #define GRPC_CORE_LIB_TRANSPORT_METHOD_CONFIG_H -#include - -#include #include #include "src/core/lib/json/json.h" #include "src/core/lib/transport/mdstr_hash_table.h" -#include "src/core/lib/transport/metadata.h" - -/// Per-method configuration. -typedef struct grpc_method_config grpc_method_config; - -/// Creates a grpc_method_config with the specified parameters. -/// Any parameter may be NULL to indicate that the value is unset. -/// -/// \a wait_for_ready indicates whether the client should wait until the -/// request deadline for the channel to become ready, even if there is a -/// temporary failure before the deadline while attempting to connect. -/// -/// \a timeout indicates the timeout for calls. -/// -/// \a max_request_message_bytes and \a max_response_message_bytes -/// indicate the maximum sizes of the request (checked when sending) and -/// response (checked when receiving) messages. -grpc_method_config* grpc_method_config_create( - bool* wait_for_ready, gpr_timespec* timeout, - int32_t* max_request_message_bytes, int32_t* max_response_message_bytes); - -grpc_method_config* grpc_method_config_ref(grpc_method_config* method_config); -void grpc_method_config_unref(grpc_method_config* method_config); - -/// Compares two grpc_method_configs. -/// The sort order is stable but undefined. -int grpc_method_config_cmp(const grpc_method_config* method_config1, - const grpc_method_config* method_config2); - -/// These methods return NULL if the requested field is unset. -/// The caller does NOT take ownership of the result. -const bool* grpc_method_config_get_wait_for_ready( - const grpc_method_config* method_config); -const gpr_timespec* grpc_method_config_get_timeout( - const grpc_method_config* method_config); -const int32_t* grpc_method_config_get_max_request_message_bytes( - const grpc_method_config* method_config); -const int32_t* grpc_method_config_get_max_response_message_bytes( - const grpc_method_config* method_config); -/// A table of method configs. -typedef grpc_mdstr_hash_table grpc_method_config_table; - -typedef struct grpc_method_config_table_entry { - /// The name is of one of the following forms: - /// service/method -- specifies exact service and method name - /// service/* -- matches all methods for the specified service - grpc_mdstr* method_name; - grpc_method_config* method_config; -} grpc_method_config_table_entry; - -/// Takes new references to all keys and values in \a entries. -grpc_method_config_table* grpc_method_config_table_create( - size_t num_entries, grpc_method_config_table_entry* entries); - -grpc_method_config_table* grpc_method_config_table_ref( - grpc_method_config_table* table); -void grpc_method_config_table_unref(grpc_method_config_table* table); - -/// Compares two grpc_method_config_tables. -/// The sort order is stable but undefined. -int grpc_method_config_table_cmp(const grpc_method_config_table* table1, - const grpc_method_config_table* table2); +/// Creates a method config table based on the data in \a json. +/// The table's keys are request paths. The table's value type is +/// returned by \a create_value(), based on data parsed from the JSON tree. +/// \a vtable provides methods used to manage the values. +/// Returns NULL on error. +grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( + const grpc_json* json, + void* (*create_value)(const grpc_json* method_config_json), + const grpc_mdstr_hash_table_vtable* vtable); /// Gets the method config for the specified \a path, which should be of /// the form "/service/method". /// Returns NULL if the method has no config. /// Caller does NOT own a reference to the result. -/// -/// Note: This returns a void* instead of a grpc_method_config* so that -/// it can also be used for tables constructed via -/// grpc_method_config_table_convert(). void* grpc_method_config_table_get(const grpc_mdstr_hash_table* table, const grpc_mdstr* path); -/// Returns a channel arg containing \a table. -grpc_arg grpc_method_config_table_create_channel_arg( - grpc_method_config_table* table); - -/// Generates a new table from \a table whose values are converted to a -/// new form via the \a convert_value function. The new table will use -/// \a vtable for its values. -/// -/// This is generally used to convert the table's value type from -/// grpc_method_config to a simple struct containing only the parameters -/// relevant to a particular filter, thus avoiding the need for a hash -/// table lookup on the fast path. In that scenario, \a convert_value -/// will return a new instance of the struct containing the values from -/// the grpc_method_config, and \a vtable provides the methods for -/// operating on the struct type. -grpc_mdstr_hash_table* grpc_method_config_table_convert( - const grpc_method_config_table* table, - void* (*convert_value)(const grpc_method_config* method_config), - const grpc_mdstr_hash_table_vtable* vtable); - -// FIXME: document -grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( - const grpc_json* json, - void* (*create_value)(const grpc_json* method_config_json), - const grpc_mdstr_hash_table_vtable* vtable); - +/// Creates a channel arg containing \a service_config. grpc_arg grpc_service_config_create_channel_arg(grpc_json_tree* service_config); #endif /* GRPC_CORE_LIB_TRANSPORT_METHOD_CONFIG_H */ -- cgit v1.2.3 From e30baeb6a3de046ccd4d6120a2b2a2dba67aef82 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 3 Nov 2016 08:16:19 -0700 Subject: Minor cleanups. --- src/core/ext/client_channel/client_channel.c | 4 ++-- src/core/lib/channel/message_size_filter.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index 4a1c0ae1a2..1f27761b65 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -94,7 +94,7 @@ static int method_parameters_cmp(void *value1, void *value2) { static const grpc_mdstr_hash_table_vtable method_parameters_vtable = { gpr_free, method_parameters_copy, method_parameters_cmp}; -static void *method_config_convert_value(const grpc_json *json) { +static void *method_parameters_create_from_json(const grpc_json *json) { wait_for_ready_value wait_for_ready = WAIT_FOR_READY_UNSET; gpr_timespec timeout = { 0, 0, GPR_TIMESPAN }; for (grpc_json* field = json->child; field != NULL; field = field->next) { @@ -315,7 +315,7 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, GPR_ASSERT(channel_arg->type == GRPC_ARG_POINTER); grpc_json_tree* json_tree = channel_arg->value.pointer.p; method_params_table = grpc_method_config_table_create_from_json( - json_tree->root, method_config_convert_value, + json_tree->root, method_parameters_create_from_json, &method_parameters_vtable); } grpc_channel_args_destroy(chand->resolver_result); diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index 86fdf72684..726bc4f165 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -70,7 +70,7 @@ static int message_size_limits_cmp(void* value1, void* value2) { static const grpc_mdstr_hash_table_vtable message_size_limits_vtable = { gpr_free, message_size_limits_copy, message_size_limits_cmp}; -static void* method_config_convert_value(const grpc_json* json) { +static void* message_size_limits_create_from_json(const grpc_json* json) { int max_request_message_bytes = -1; int max_response_message_bytes = -1; for (grpc_json* field = json->child; field != NULL; field = field->next) { @@ -236,7 +236,7 @@ static void init_channel_elem(grpc_exec_ctx* exec_ctx, GPR_ASSERT(channel_arg->type == GRPC_ARG_POINTER); grpc_json_tree* json_tree = channel_arg->value.pointer.p; chand->method_limit_table = grpc_method_config_table_create_from_json( - json_tree->root, method_config_convert_value, + json_tree->root, message_size_limits_create_from_json, &message_size_limits_vtable); } } -- cgit v1.2.3 From 6ad80579de9f995ec210f40447bffe986cd4cb98 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 3 Nov 2016 08:33:17 -0700 Subject: Remove more now-unnecessary code. --- src/core/ext/client_channel/client_channel.c | 12 +------- src/core/lib/channel/message_size_filter.c | 12 +------- src/core/lib/transport/mdstr_hash_table.c | 42 +--------------------------- src/core/lib/transport/mdstr_hash_table.h | 18 +----------- 4 files changed, 4 insertions(+), 80 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index 1f27761b65..d53265bc9c 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -81,18 +81,8 @@ static void *method_parameters_copy(void *value) { return new_value; } -static int method_parameters_cmp(void *value1, void *value2) { - const method_parameters *v1 = value1; - const method_parameters *v2 = value2; - const int retval = gpr_time_cmp(v1->timeout, v2->timeout); - if (retval != 0) return retval; - if (v1->wait_for_ready > v2->wait_for_ready) return 1; - if (v1->wait_for_ready < v2->wait_for_ready) return -1; - return 0; -} - static const grpc_mdstr_hash_table_vtable method_parameters_vtable = { - gpr_free, method_parameters_copy, method_parameters_cmp}; + gpr_free, method_parameters_copy}; static void *method_parameters_create_from_json(const grpc_json *json) { wait_for_ready_value wait_for_ready = WAIT_FOR_READY_UNSET; diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index 726bc4f165..2950eef87e 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -57,18 +57,8 @@ static void* message_size_limits_copy(void* value) { return new_value; } -static int message_size_limits_cmp(void* value1, void* value2) { - const message_size_limits* v1 = value1; - const message_size_limits* v2 = value2; - if (v1->max_send_size > v2->max_send_size) return 1; - if (v1->max_send_size < v2->max_send_size) return -1; - if (v1->max_recv_size > v2->max_recv_size) return 1; - if (v1->max_recv_size < v2->max_recv_size) return -1; - return 0; -} - static const grpc_mdstr_hash_table_vtable message_size_limits_vtable = { - gpr_free, message_size_limits_copy, message_size_limits_cmp}; + gpr_free, message_size_limits_copy}; static void* message_size_limits_create_from_json(const grpc_json* json) { int max_request_message_bytes = -1; diff --git a/src/core/lib/transport/mdstr_hash_table.c b/src/core/lib/transport/mdstr_hash_table.c index 8e914c420b..3d01e56df7 100644 --- a/src/core/lib/transport/mdstr_hash_table.c +++ b/src/core/lib/transport/mdstr_hash_table.c @@ -41,7 +41,6 @@ struct grpc_mdstr_hash_table { gpr_refcount refs; - size_t num_entries; size_t size; grpc_mdstr_hash_table_entry* entries; }; @@ -77,7 +76,6 @@ grpc_mdstr_hash_table* grpc_mdstr_hash_table_create( grpc_mdstr_hash_table* table = gpr_malloc(sizeof(*table)); memset(table, 0, sizeof(*table)); gpr_ref_init(&table->refs, 1); - table->num_entries = num_entries; // Quadratic probing gets best performance when the table is no more // than half full. table->size = num_entries * 2; @@ -96,7 +94,7 @@ grpc_mdstr_hash_table* grpc_mdstr_hash_table_ref(grpc_mdstr_hash_table* table) { return table; } -int grpc_mdstr_hash_table_unref(grpc_mdstr_hash_table* table) { +void grpc_mdstr_hash_table_unref(grpc_mdstr_hash_table* table) { if (table != NULL && gpr_unref(&table->refs)) { for (size_t i = 0; i < table->size; ++i) { grpc_mdstr_hash_table_entry* entry = &table->entries[i]; @@ -107,13 +105,7 @@ int grpc_mdstr_hash_table_unref(grpc_mdstr_hash_table* table) { } gpr_free(table->entries); gpr_free(table); - return 1; } - return 0; -} - -size_t grpc_mdstr_hash_table_num_entries(const grpc_mdstr_hash_table* table) { - return table->num_entries; } void* grpc_mdstr_hash_table_get(const grpc_mdstr_hash_table* table, @@ -123,35 +115,3 @@ void* grpc_mdstr_hash_table_get(const grpc_mdstr_hash_table* table, if (idx == table->size) return NULL; // Not found. return table->entries[idx].value; } - -int grpc_mdstr_hash_table_cmp(const grpc_mdstr_hash_table* table1, - const grpc_mdstr_hash_table* table2) { - // Compare by num_entries. - if (table1->num_entries < table2->num_entries) return -1; - if (table1->num_entries > table2->num_entries) return 1; - for (size_t i = 0; i < table1->num_entries; ++i) { - grpc_mdstr_hash_table_entry* e1 = &table1->entries[i]; - grpc_mdstr_hash_table_entry* e2 = &table2->entries[i]; - // Compare keys by hash value. - if (e1->key->hash < e2->key->hash) return -1; - if (e1->key->hash > e2->key->hash) return 1; - // Compare by vtable (pointer equality). - if (e1->vtable < e2->vtable) return -1; - if (e1->vtable > e2->vtable) return 1; - // Compare values via vtable. - const int value_result = e1->vtable->compare_value(e1->value, e2->value); - if (value_result != 0) return value_result; - } - return 0; -} - -void grpc_mdstr_hash_table_iterate( - const grpc_mdstr_hash_table* table, - void (*func)(const grpc_mdstr_hash_table_entry* entry, void* user_data), - void* user_data) { - for (size_t i = 0; i < table->size; ++i) { - if (table->entries[i].key != NULL) { - func(&table->entries[i], user_data); - } - } -} diff --git a/src/core/lib/transport/mdstr_hash_table.h b/src/core/lib/transport/mdstr_hash_table.h index bceb4df93d..8982ec3a8d 100644 --- a/src/core/lib/transport/mdstr_hash_table.h +++ b/src/core/lib/transport/mdstr_hash_table.h @@ -51,7 +51,6 @@ typedef struct grpc_mdstr_hash_table grpc_mdstr_hash_table; typedef struct grpc_mdstr_hash_table_vtable { void (*destroy_value)(void* value); void* (*copy_value)(void* value); - int (*compare_value)(void* value1, void* value2); } grpc_mdstr_hash_table_vtable; typedef struct grpc_mdstr_hash_table_entry { @@ -67,26 +66,11 @@ grpc_mdstr_hash_table* grpc_mdstr_hash_table_create( size_t num_entries, grpc_mdstr_hash_table_entry* entries); grpc_mdstr_hash_table* grpc_mdstr_hash_table_ref(grpc_mdstr_hash_table* table); -/** Returns 1 when \a table is destroyed. */ -int grpc_mdstr_hash_table_unref(grpc_mdstr_hash_table* table); - -/** Returns the number of entries in \a table. */ -size_t grpc_mdstr_hash_table_num_entries(const grpc_mdstr_hash_table* table); +void grpc_mdstr_hash_table_unref(grpc_mdstr_hash_table* table); /** Returns the value from \a table associated with \a key. Returns NULL if \a key is not found. */ void* grpc_mdstr_hash_table_get(const grpc_mdstr_hash_table* table, const grpc_mdstr* key); -/** Compares two hash tables. - The sort order is stable but undefined. */ -int grpc_mdstr_hash_table_cmp(const grpc_mdstr_hash_table* table1, - const grpc_mdstr_hash_table* table2); - -/** Iterates over the entries in \a table, calling \a func for each entry. */ -void grpc_mdstr_hash_table_iterate( - const grpc_mdstr_hash_table* table, - void (*func)(const grpc_mdstr_hash_table_entry* entry, void* user_data), - void* user_data); - #endif /* GRPC_CORE_LIB_TRANSPORT_MDSTR_HASH_TABLE_H */ -- cgit v1.2.3 From 47f1084ce835d4288ddfa9f280e07d0a8d5719c5 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 3 Nov 2016 08:45:27 -0700 Subject: clang-format --- src/core/ext/client_channel/client_channel.c | 12 ++++++------ src/core/lib/json/json.c | 6 +++--- src/core/lib/json/json.h | 22 +++++++++++----------- src/core/lib/support/string.c | 4 ++-- src/core/lib/support/string.h | 2 +- src/core/lib/transport/method_config.c | 8 +++----- test/core/end2end/connection_refused_test.c | 2 +- test/core/end2end/tests/cancel_after_accept.c | 2 +- test/core/end2end/tests/max_message_length.c | 4 ++-- 9 files changed, 30 insertions(+), 32 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index d53265bc9c..4ca24c4dbc 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -86,21 +86,21 @@ static const grpc_mdstr_hash_table_vtable method_parameters_vtable = { static void *method_parameters_create_from_json(const grpc_json *json) { wait_for_ready_value wait_for_ready = WAIT_FOR_READY_UNSET; - gpr_timespec timeout = { 0, 0, GPR_TIMESPAN }; - for (grpc_json* field = json->child; field != NULL; field = field->next) { + gpr_timespec timeout = {0, 0, GPR_TIMESPAN}; + for (grpc_json *field = json->child; field != NULL; field = field->next) { if (field->key == NULL) continue; if (strcmp(field->key, "wait_for_ready") == 0) { if (wait_for_ready != WAIT_FOR_READY_UNSET) return NULL; // Duplicate. if (field->type != GRPC_JSON_TRUE && field->type != GRPC_JSON_FALSE) { return NULL; } - wait_for_ready = field->type == GRPC_JSON_TRUE - ? WAIT_FOR_READY_TRUE : WAIT_FOR_READY_FALSE; + wait_for_ready = field->type == GRPC_JSON_TRUE ? WAIT_FOR_READY_TRUE + : WAIT_FOR_READY_FALSE; } else if (strcmp(field->key, "timeout") == 0) { if (timeout.tv_sec > 0 || timeout.tv_nsec > 0) return NULL; // Duplicate. if (field->type != GRPC_JSON_OBJECT) return NULL; if (field->child == NULL) return NULL; - for (grpc_json* subfield = field->child; subfield != NULL; + for (grpc_json *subfield = field->child; subfield != NULL; subfield = subfield->next) { if (subfield->key == NULL) return NULL; if (strcmp(subfield->key, "seconds") == 0) { @@ -303,7 +303,7 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_channel_args_find(lb_policy_args.args, GRPC_ARG_SERVICE_CONFIG); if (channel_arg != NULL) { GPR_ASSERT(channel_arg->type == GRPC_ARG_POINTER); - grpc_json_tree* json_tree = channel_arg->value.pointer.p; + grpc_json_tree *json_tree = channel_arg->value.pointer.p; method_params_table = grpc_method_config_table_create_from_json( json_tree->root, method_parameters_create_from_json, &method_parameters_vtable); diff --git a/src/core/lib/json/json.c b/src/core/lib/json/json.c index c1a99df6c3..9acf3b0438 100644 --- a/src/core/lib/json/json.c +++ b/src/core/lib/json/json.c @@ -39,15 +39,15 @@ #include "src/core/lib/json/json.h" -grpc_json *grpc_json_create(grpc_json_type type) { - grpc_json *json = gpr_malloc(sizeof(*json)); +grpc_json* grpc_json_create(grpc_json_type type) { + grpc_json* json = gpr_malloc(sizeof(*json)); memset(json, 0, sizeof(*json)); json->type = type; return json; } -void grpc_json_destroy(grpc_json *json) { +void grpc_json_destroy(grpc_json* json) { while (json->child) { grpc_json_destroy(json->child); } diff --git a/src/core/lib/json/json.h b/src/core/lib/json/json.h index e18ace7547..1663c690e5 100644 --- a/src/core/lib/json/json.h +++ b/src/core/lib/json/json.h @@ -44,14 +44,14 @@ * are not owned by it. */ typedef struct grpc_json { - struct grpc_json *next; - struct grpc_json *prev; - struct grpc_json *child; - struct grpc_json *parent; + struct grpc_json* next; + struct grpc_json* prev; + struct grpc_json* child; + struct grpc_json* parent; grpc_json_type type; - const char *key; - const char *value; + const char* key; + const char* value; } grpc_json; /* The next two functions are going to parse the input string, and @@ -67,8 +67,8 @@ typedef struct grpc_json { * * Delete the allocated tree afterward using grpc_json_destroy(). */ -grpc_json *grpc_json_parse_string_with_len(char *input, size_t size); -grpc_json *grpc_json_parse_string(char *input); +grpc_json* grpc_json_parse_string_with_len(char* input, size_t size); +grpc_json* grpc_json_parse_string(char* input); /* This function will create a new string using gpr_realloc, and will * deserialize the grpc_json tree into it. It'll be zero-terminated, @@ -78,14 +78,14 @@ grpc_json *grpc_json_parse_string(char *input); * If indent is 0, then newlines will be suppressed as well, and the * output will be condensed at its maximum. */ -char *grpc_json_dump_to_string(grpc_json *json, int indent); +char* grpc_json_dump_to_string(grpc_json* json, int indent); /* Use these to create or delete a grpc_json object. * Deletion is recursive. We will not attempt to free any of the strings * in any of the objects of that tree. */ -grpc_json *grpc_json_create(grpc_json_type type); -void grpc_json_destroy(grpc_json *json); +grpc_json* grpc_json_create(grpc_json_type type); +void grpc_json_destroy(grpc_json* json); /* Compares two JSON trees. */ int grpc_json_cmp(const grpc_json* json1, const grpc_json* json2); diff --git a/src/core/lib/support/string.c b/src/core/lib/support/string.c index 56f29492bf..165b3189a5 100644 --- a/src/core/lib/support/string.c +++ b/src/core/lib/support/string.c @@ -196,8 +196,8 @@ int int64_ttoa(int64_t value, char *string) { return i; } -int gpr_parse_nonnegative_number(const char* value) { - char* end; +int gpr_parse_nonnegative_number(const char *value) { + char *end; long result = strtol(value, &end, 0); if (*end != '\0' || result < 0 || result > INT_MAX) return -1; return (int)result; diff --git a/src/core/lib/support/string.h b/src/core/lib/support/string.h index 3a5a8ed826..34fd154a83 100644 --- a/src/core/lib/support/string.h +++ b/src/core/lib/support/string.h @@ -81,7 +81,7 @@ where long is 32bit is size.*/ int int64_ttoa(int64_t value, char *output); // Parses a non-negative number from a value string. Returns -1 on error. -int gpr_parse_nonnegative_number(const char* value); +int gpr_parse_nonnegative_number(const char *value); /* Reverse a run of bytes */ void gpr_reverse_bytes(char *str, int len); diff --git a/src/core/lib/transport/method_config.c b/src/core/lib/transport/method_config.c index a163af91d6..1ce43ca13f 100644 --- a/src/core/lib/transport/method_config.c +++ b/src/core/lib/transport/method_config.c @@ -80,10 +80,9 @@ static char* parse_json_method_name(grpc_json* json) { // Parses the method config from \a json. Adds an entry to \a entries for // each name found, incrementing \a idx for each entry added. static bool parse_json_method_config( - grpc_json* json, - void* (*create_value)(const grpc_json* method_config_json), + grpc_json* json, void* (*create_value)(const grpc_json* method_config_json), const grpc_mdstr_hash_table_vtable* vtable, - grpc_mdstr_hash_table_entry* entries, size_t *idx) { + grpc_mdstr_hash_table_entry* entries, size_t* idx) { // Construct value. void* method_config = create_value(json); if (method_config == NULL) return NULL; @@ -135,8 +134,7 @@ grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( num_entries += count_names_in_method_config_json(method); } // Populate method config table entries. - entries = - gpr_malloc(num_entries * sizeof(grpc_mdstr_hash_table_entry)); + entries = gpr_malloc(num_entries * sizeof(grpc_mdstr_hash_table_entry)); size_t idx = 0; for (grpc_json* method = field->child; method != NULL; method = method->next) { diff --git a/test/core/end2end/connection_refused_test.c b/test/core/end2end/connection_refused_test.c index ba9fb3d30a..2857ca70e8 100644 --- a/test/core/end2end/connection_refused_test.c +++ b/test/core/end2end/connection_refused_test.c @@ -76,7 +76,7 @@ static void run_test(bool wait_for_ready, bool use_service_config) { grpc_channel_args *args = NULL; if (use_service_config) { GPR_ASSERT(wait_for_ready); - grpc_json_tree* service_config_json = grpc_json_tree_create( + grpc_json_tree *service_config_json = grpc_json_tree_create( "{\n" " \"method_config\": [ {\n" " \"name\": [\n" diff --git a/test/core/end2end/tests/cancel_after_accept.c b/test/core/end2end/tests/cancel_after_accept.c index 68a8256092..bef33f2a33 100644 --- a/test/core/end2end/tests/cancel_after_accept.c +++ b/test/core/end2end/tests/cancel_after_accept.c @@ -132,7 +132,7 @@ static void test_cancel_after_accept(grpc_end2end_test_config config, grpc_channel_args *args = NULL; if (use_service_config) { - grpc_json_tree* service_config_json = grpc_json_tree_create( + grpc_json_tree *service_config_json = grpc_json_tree_create( "{\n" " \"method_config\": [ {\n" " \"name\": [\n" diff --git a/test/core/end2end/tests/max_message_length.c b/test/core/end2end/tests/max_message_length.c index f0bc7acdff..4cffff915e 100644 --- a/test/core/end2end/tests/max_message_length.c +++ b/test/core/end2end/tests/max_message_length.c @@ -137,7 +137,7 @@ static void test_max_message_length_on_request(grpc_end2end_test_config config, if (use_service_config) { // We don't currently support service configs on the server side. GPR_ASSERT(send_limit); - grpc_json_tree* service_config_json = grpc_json_tree_create( + grpc_json_tree *service_config_json = grpc_json_tree_create( "{\n" " \"method_config\": [ {\n" " \"name\": [\n" @@ -308,7 +308,7 @@ static void test_max_message_length_on_response(grpc_end2end_test_config config, if (use_service_config) { // We don't currently support service configs on the server side. GPR_ASSERT(!send_limit); - grpc_json_tree* service_config_json = grpc_json_tree_create( + grpc_json_tree *service_config_json = grpc_json_tree_create( "{\n" " \"method_config\": [ {\n" " \"name\": [\n" -- cgit v1.2.3 From 4112499c2822d37efdb6c5dfb73438c8ad606247 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 3 Nov 2016 11:22:20 -0700 Subject: Minor cleanups. --- src/core/ext/client_channel/client_channel.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index 4ca24c4dbc..d96ec57f04 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -249,14 +249,10 @@ 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) { - grpc_lb_policy_args lb_policy_args; - lb_policy_args.args = chand->resolver_result; - lb_policy_args.client_channel_factory = chand->client_channel_factory; - // Find LB policy name. const char *lb_policy_name = NULL; const grpc_arg *channel_arg = - grpc_channel_args_find(lb_policy_args.args, GRPC_ARG_LB_POLICY_NAME); + grpc_channel_args_find(chand->resolver_result, GRPC_ARG_LB_POLICY_NAME); if (channel_arg != NULL) { GPR_ASSERT(channel_arg->type == GRPC_ARG_STRING); lb_policy_name = channel_arg->value.string; @@ -265,7 +261,7 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, // assume that we should use the grpclb policy, regardless of what the // resolver actually specified. channel_arg = - grpc_channel_args_find(lb_policy_args.args, GRPC_ARG_LB_ADDRESSES); + grpc_channel_args_find(chand->resolver_result, GRPC_ARG_LB_ADDRESSES); if (channel_arg != NULL) { GPR_ASSERT(channel_arg->type == GRPC_ARG_POINTER); grpc_lb_addresses *addresses = channel_arg->value.pointer.p; @@ -290,7 +286,10 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, // Use pick_first if nothing was specified and we didn't select grpclb // above. if (lb_policy_name == NULL) lb_policy_name = "pick_first"; - + // Instantiate LB policy. + grpc_lb_policy_args lb_policy_args; + lb_policy_args.args = chand->resolver_result; + lb_policy_args.client_channel_factory = chand->client_channel_factory; lb_policy = grpc_lb_policy_create(exec_ctx, lb_policy_name, &lb_policy_args); if (lb_policy != NULL) { @@ -299,15 +298,17 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, state = grpc_lb_policy_check_connectivity(exec_ctx, lb_policy, &state_error); } + // Find service config. channel_arg = - grpc_channel_args_find(lb_policy_args.args, GRPC_ARG_SERVICE_CONFIG); + grpc_channel_args_find(chand->resolver_result, GRPC_ARG_SERVICE_CONFIG); if (channel_arg != NULL) { GPR_ASSERT(channel_arg->type == GRPC_ARG_POINTER); - grpc_json_tree *json_tree = channel_arg->value.pointer.p; + grpc_json_tree *service_config_json = channel_arg->value.pointer.p; method_params_table = grpc_method_config_table_create_from_json( - json_tree->root, method_parameters_create_from_json, + service_config_json->root, method_parameters_create_from_json, &method_parameters_vtable); } + // Clean up. grpc_channel_args_destroy(chand->resolver_result); chand->resolver_result = NULL; } -- cgit v1.2.3 From f2e8a6a138d4d5988ffef3c79a7a64d586c041fc Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 3 Nov 2016 11:25:22 -0700 Subject: Add method to extract LB policy name from service config JSON. --- src/core/lib/transport/method_config.c | 16 ++++++++++++++++ src/core/lib/transport/method_config.h | 6 ++++++ 2 files changed, 22 insertions(+) (limited to 'src/core') diff --git a/src/core/lib/transport/method_config.c b/src/core/lib/transport/method_config.c index 1ce43ca13f..49e60a47bc 100644 --- a/src/core/lib/transport/method_config.c +++ b/src/core/lib/transport/method_config.c @@ -181,6 +181,22 @@ void* grpc_method_config_table_get(const grpc_mdstr_hash_table* table, return value; } +const char* grpc_service_config_get_lb_policy_name( + grpc_json_tree* service_config) { + grpc_json* json = service_config->root; + if (json->type != GRPC_JSON_OBJECT || json->key != NULL) return NULL; + const char* lb_policy_name = NULL; + for (grpc_json* field = json->child; field != NULL; field = field->next) { + if (field->key == NULL) return NULL; + if (strcmp(field->key, "lb_policy_name") == 0) { + if (lb_policy_name != NULL) return NULL; // Duplicate. + if (field->type != GRPC_JSON_STRING) return NULL; + lb_policy_name = field->value; + } + } + return lb_policy_name; +} + static void* copy_json_tree(void* t) { return grpc_json_tree_ref(t); } static void destroy_json_tree(void* t) { grpc_json_tree_unref(t); } diff --git a/src/core/lib/transport/method_config.h b/src/core/lib/transport/method_config.h index f33f4279d6..9922a7a657 100644 --- a/src/core/lib/transport/method_config.h +++ b/src/core/lib/transport/method_config.h @@ -54,6 +54,12 @@ grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( void* grpc_method_config_table_get(const grpc_mdstr_hash_table* table, const grpc_mdstr* path); +/// Gets the LB policy name from \a service_config. +/// Returns NULL if no LB policy name was specified. +/// Caller does NOT take ownership. +const char* grpc_service_config_get_lb_policy_name( + grpc_json_tree* service_config); + /// Creates a channel arg containing \a service_config. grpc_arg grpc_service_config_create_channel_arg(grpc_json_tree* service_config); -- cgit v1.2.3 From ea846a08004501fa4b63dbb5bd7bb12bd1017a33 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 3 Nov 2016 11:32:54 -0700 Subject: Rename method_config.[ch] to service_config.[ch]. --- BUILD | 16 +- CMakeLists.txt | 6 +- Makefile | 8 +- binding.gyp | 2 +- build.yaml | 4 +- config.m4 | 2 +- gRPC-Core.podspec | 6 +- grpc.gemspec | 4 +- package.xml | 4 +- src/core/ext/client_channel/client_channel.c | 2 +- src/core/lib/channel/message_size_filter.c | 2 +- src/core/lib/transport/method_config.c | 221 --------------------- src/core/lib/transport/method_config.h | 66 ------ src/core/lib/transport/service_config.c | 221 +++++++++++++++++++++ src/core/lib/transport/service_config.h | 66 ++++++ src/python/grpcio/grpc_core_dependencies.py | 2 +- test/core/end2end/connection_refused_test.c | 2 +- test/core/end2end/tests/cancel_after_accept.c | 2 +- test/core/end2end/tests/max_message_length.c | 2 +- tools/doxygen/Doxyfile.core.internal | 4 +- tools/run_tests/sources_and_headers.json | 6 +- vsprojects/vcxproj/grpc/grpc.vcxproj | 4 +- vsprojects/vcxproj/grpc/grpc.vcxproj.filters | 4 +- .../vcxproj/grpc_test_util/grpc_test_util.vcxproj | 4 +- .../grpc_test_util/grpc_test_util.vcxproj.filters | 4 +- .../vcxproj/grpc_unsecure/grpc_unsecure.vcxproj | 4 +- .../grpc_unsecure/grpc_unsecure.vcxproj.filters | 4 +- 27 files changed, 336 insertions(+), 336 deletions(-) delete mode 100644 src/core/lib/transport/method_config.c delete mode 100644 src/core/lib/transport/method_config.h create mode 100644 src/core/lib/transport/service_config.c create mode 100644 src/core/lib/transport/service_config.h (limited to 'src/core') diff --git a/BUILD b/BUILD index 97ddc56b41..d747e71dde 100644 --- a/BUILD +++ b/BUILD @@ -251,7 +251,7 @@ cc_library( "src/core/lib/transport/mdstr_hash_table.h", "src/core/lib/transport/metadata.h", "src/core/lib/transport/metadata_batch.h", - "src/core/lib/transport/method_config.h", + "src/core/lib/transport/service_config.h", "src/core/lib/transport/static_metadata.h", "src/core/lib/transport/timeout_encoding.h", "src/core/lib/transport/transport.h", @@ -435,7 +435,7 @@ cc_library( "src/core/lib/transport/mdstr_hash_table.c", "src/core/lib/transport/metadata.c", "src/core/lib/transport/metadata_batch.c", - "src/core/lib/transport/method_config.c", + "src/core/lib/transport/service_config.c", "src/core/lib/transport/static_metadata.c", "src/core/lib/transport/timeout_encoding.c", "src/core/lib/transport/transport.c", @@ -675,7 +675,7 @@ cc_library( "src/core/lib/transport/mdstr_hash_table.h", "src/core/lib/transport/metadata.h", "src/core/lib/transport/metadata_batch.h", - "src/core/lib/transport/method_config.h", + "src/core/lib/transport/service_config.h", "src/core/lib/transport/static_metadata.h", "src/core/lib/transport/timeout_encoding.h", "src/core/lib/transport/transport.h", @@ -844,7 +844,7 @@ cc_library( "src/core/lib/transport/mdstr_hash_table.c", "src/core/lib/transport/metadata.c", "src/core/lib/transport/metadata_batch.c", - "src/core/lib/transport/method_config.c", + "src/core/lib/transport/service_config.c", "src/core/lib/transport/static_metadata.c", "src/core/lib/transport/timeout_encoding.c", "src/core/lib/transport/transport.c", @@ -1054,7 +1054,7 @@ cc_library( "src/core/lib/transport/mdstr_hash_table.h", "src/core/lib/transport/metadata.h", "src/core/lib/transport/metadata_batch.h", - "src/core/lib/transport/method_config.h", + "src/core/lib/transport/service_config.h", "src/core/lib/transport/static_metadata.h", "src/core/lib/transport/timeout_encoding.h", "src/core/lib/transport/transport.h", @@ -1215,7 +1215,7 @@ cc_library( "src/core/lib/transport/mdstr_hash_table.c", "src/core/lib/transport/metadata.c", "src/core/lib/transport/metadata_batch.c", - "src/core/lib/transport/method_config.c", + "src/core/lib/transport/service_config.c", "src/core/lib/transport/static_metadata.c", "src/core/lib/transport/timeout_encoding.c", "src/core/lib/transport/transport.c", @@ -2072,7 +2072,7 @@ objc_library( "src/core/lib/transport/mdstr_hash_table.c", "src/core/lib/transport/metadata.c", "src/core/lib/transport/metadata_batch.c", - "src/core/lib/transport/method_config.c", + "src/core/lib/transport/service_config.c", "src/core/lib/transport/static_metadata.c", "src/core/lib/transport/timeout_encoding.c", "src/core/lib/transport/transport.c", @@ -2291,7 +2291,7 @@ objc_library( "src/core/lib/transport/mdstr_hash_table.h", "src/core/lib/transport/metadata.h", "src/core/lib/transport/metadata_batch.h", - "src/core/lib/transport/method_config.h", + "src/core/lib/transport/service_config.h", "src/core/lib/transport/static_metadata.h", "src/core/lib/transport/timeout_encoding.h", "src/core/lib/transport/transport.h", diff --git a/CMakeLists.txt b/CMakeLists.txt index 4b073b0d68..dec3c8a7f9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -392,7 +392,7 @@ add_library(grpc src/core/lib/transport/mdstr_hash_table.c src/core/lib/transport/metadata.c src/core/lib/transport/metadata_batch.c - src/core/lib/transport/method_config.c + src/core/lib/transport/service_config.c src/core/lib/transport/static_metadata.c src/core/lib/transport/timeout_encoding.c src/core/lib/transport/transport.c @@ -664,7 +664,7 @@ add_library(grpc_cronet src/core/lib/transport/mdstr_hash_table.c src/core/lib/transport/metadata.c src/core/lib/transport/metadata_batch.c - src/core/lib/transport/method_config.c + src/core/lib/transport/service_config.c src/core/lib/transport/static_metadata.c src/core/lib/transport/timeout_encoding.c src/core/lib/transport/transport.c @@ -908,7 +908,7 @@ add_library(grpc_unsecure src/core/lib/transport/mdstr_hash_table.c src/core/lib/transport/metadata.c src/core/lib/transport/metadata_batch.c - src/core/lib/transport/method_config.c + src/core/lib/transport/service_config.c src/core/lib/transport/static_metadata.c src/core/lib/transport/timeout_encoding.c src/core/lib/transport/transport.c diff --git a/Makefile b/Makefile index 4b713cef3b..2d4acb8757 100644 --- a/Makefile +++ b/Makefile @@ -2687,7 +2687,7 @@ LIBGRPC_SRC = \ src/core/lib/transport/mdstr_hash_table.c \ src/core/lib/transport/metadata.c \ src/core/lib/transport/metadata_batch.c \ - src/core/lib/transport/method_config.c \ + src/core/lib/transport/service_config.c \ src/core/lib/transport/static_metadata.c \ src/core/lib/transport/timeout_encoding.c \ src/core/lib/transport/transport.c \ @@ -2977,7 +2977,7 @@ LIBGRPC_CRONET_SRC = \ src/core/lib/transport/mdstr_hash_table.c \ src/core/lib/transport/metadata.c \ src/core/lib/transport/metadata_batch.c \ - src/core/lib/transport/method_config.c \ + src/core/lib/transport/service_config.c \ src/core/lib/transport/static_metadata.c \ src/core/lib/transport/timeout_encoding.c \ src/core/lib/transport/transport.c \ @@ -3258,7 +3258,7 @@ LIBGRPC_TEST_UTIL_SRC = \ src/core/lib/transport/mdstr_hash_table.c \ src/core/lib/transport/metadata.c \ src/core/lib/transport/metadata_batch.c \ - src/core/lib/transport/method_config.c \ + src/core/lib/transport/service_config.c \ src/core/lib/transport/static_metadata.c \ src/core/lib/transport/timeout_encoding.c \ src/core/lib/transport/transport.c \ @@ -3468,7 +3468,7 @@ LIBGRPC_UNSECURE_SRC = \ src/core/lib/transport/mdstr_hash_table.c \ src/core/lib/transport/metadata.c \ src/core/lib/transport/metadata_batch.c \ - src/core/lib/transport/method_config.c \ + src/core/lib/transport/service_config.c \ src/core/lib/transport/static_metadata.c \ src/core/lib/transport/timeout_encoding.c \ src/core/lib/transport/transport.c \ diff --git a/binding.gyp b/binding.gyp index 9f3b75aaf3..7f6e06b1c3 100644 --- a/binding.gyp +++ b/binding.gyp @@ -670,7 +670,7 @@ 'src/core/lib/transport/mdstr_hash_table.c', 'src/core/lib/transport/metadata.c', 'src/core/lib/transport/metadata_batch.c', - 'src/core/lib/transport/method_config.c', + 'src/core/lib/transport/service_config.c', 'src/core/lib/transport/static_metadata.c', 'src/core/lib/transport/timeout_encoding.c', 'src/core/lib/transport/transport.c', diff --git a/build.yaml b/build.yaml index 56c220d4ab..c7bfc58ea4 100644 --- a/build.yaml +++ b/build.yaml @@ -255,7 +255,7 @@ filegroups: - src/core/lib/transport/mdstr_hash_table.h - src/core/lib/transport/metadata.h - src/core/lib/transport/metadata_batch.h - - src/core/lib/transport/method_config.h + - src/core/lib/transport/service_config.h - src/core/lib/transport/static_metadata.h - src/core/lib/transport/timeout_encoding.h - src/core/lib/transport/transport.h @@ -363,7 +363,7 @@ filegroups: - src/core/lib/transport/mdstr_hash_table.c - src/core/lib/transport/metadata.c - src/core/lib/transport/metadata_batch.c - - src/core/lib/transport/method_config.c + - src/core/lib/transport/service_config.c - src/core/lib/transport/static_metadata.c - src/core/lib/transport/timeout_encoding.c - src/core/lib/transport/transport.c diff --git a/config.m4 b/config.m4 index 8f26e42678..ff94bbc79a 100644 --- a/config.m4 +++ b/config.m4 @@ -186,7 +186,7 @@ if test "$PHP_GRPC" != "no"; then src/core/lib/transport/mdstr_hash_table.c \ src/core/lib/transport/metadata.c \ src/core/lib/transport/metadata_batch.c \ - src/core/lib/transport/method_config.c \ + src/core/lib/transport/service_config.c \ src/core/lib/transport/static_metadata.c \ src/core/lib/transport/timeout_encoding.c \ src/core/lib/transport/transport.c \ diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index 43aedecd06..a6abebcac7 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -338,7 +338,7 @@ Pod::Spec.new do |s| 'src/core/lib/transport/mdstr_hash_table.h', 'src/core/lib/transport/metadata.h', 'src/core/lib/transport/metadata_batch.h', - 'src/core/lib/transport/method_config.h', + 'src/core/lib/transport/service_config.h', 'src/core/lib/transport/static_metadata.h', 'src/core/lib/transport/timeout_encoding.h', 'src/core/lib/transport/transport.h', @@ -526,7 +526,7 @@ Pod::Spec.new do |s| 'src/core/lib/transport/mdstr_hash_table.c', 'src/core/lib/transport/metadata.c', 'src/core/lib/transport/metadata_batch.c', - 'src/core/lib/transport/method_config.c', + 'src/core/lib/transport/service_config.c', 'src/core/lib/transport/static_metadata.c', 'src/core/lib/transport/timeout_encoding.c', 'src/core/lib/transport/transport.c', @@ -734,7 +734,7 @@ Pod::Spec.new do |s| 'src/core/lib/transport/mdstr_hash_table.h', 'src/core/lib/transport/metadata.h', 'src/core/lib/transport/metadata_batch.h', - 'src/core/lib/transport/method_config.h', + 'src/core/lib/transport/service_config.h', 'src/core/lib/transport/static_metadata.h', 'src/core/lib/transport/timeout_encoding.h', 'src/core/lib/transport/transport.h', diff --git a/grpc.gemspec b/grpc.gemspec index 615a962cdb..16f8381859 100755 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -258,7 +258,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/transport/mdstr_hash_table.h ) s.files += %w( src/core/lib/transport/metadata.h ) s.files += %w( src/core/lib/transport/metadata_batch.h ) - s.files += %w( src/core/lib/transport/method_config.h ) + s.files += %w( src/core/lib/transport/service_config.h ) s.files += %w( src/core/lib/transport/static_metadata.h ) s.files += %w( src/core/lib/transport/timeout_encoding.h ) s.files += %w( src/core/lib/transport/transport.h ) @@ -446,7 +446,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/transport/mdstr_hash_table.c ) s.files += %w( src/core/lib/transport/metadata.c ) s.files += %w( src/core/lib/transport/metadata_batch.c ) - s.files += %w( src/core/lib/transport/method_config.c ) + s.files += %w( src/core/lib/transport/service_config.c ) s.files += %w( src/core/lib/transport/static_metadata.c ) s.files += %w( src/core/lib/transport/timeout_encoding.c ) s.files += %w( src/core/lib/transport/transport.c ) diff --git a/package.xml b/package.xml index a60eac6820..d402154434 100644 --- a/package.xml +++ b/package.xml @@ -265,7 +265,7 @@ - + @@ -453,7 +453,7 @@ - + diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index d96ec57f04..d9161f47cd 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -55,7 +55,7 @@ #include "src/core/lib/transport/connectivity_state.h" #include "src/core/lib/transport/metadata.h" #include "src/core/lib/transport/metadata_batch.h" -#include "src/core/lib/transport/method_config.h" +#include "src/core/lib/transport/service_config.h" #include "src/core/lib/transport/static_metadata.h" /* Client channel implementation */ diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index 2950eef87e..f826c74f89 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -40,7 +40,7 @@ #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/support/string.h" -#include "src/core/lib/transport/method_config.h" +#include "src/core/lib/transport/service_config.h" #define DEFAULT_MAX_SEND_MESSAGE_LENGTH -1 // Unlimited. // The protobuf library will (by default) start warning at 100 megs. diff --git a/src/core/lib/transport/method_config.c b/src/core/lib/transport/method_config.c deleted file mode 100644 index 49e60a47bc..0000000000 --- a/src/core/lib/transport/method_config.c +++ /dev/null @@ -1,221 +0,0 @@ -// -// Copyright 2015, Google Inc. -// All rights reserved. -// -// Redistribution and use in source and binary forms, with or without -// modification, are permitted provided that the following conditions are -// met: -// -// * Redistributions of source code must retain the above copyright -// notice, this list of conditions and the following disclaimer. -// * Redistributions in binary form must reproduce the above -// copyright notice, this list of conditions and the following disclaimer -// in the documentation and/or other materials provided with the -// distribution. -// * Neither the name of Google Inc. nor the names of its -// contributors may be used to endorse or promote products derived from -// this software without specific prior written permission. -// -// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS -// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT -// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR -// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT -// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, -// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT -// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, -// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY -// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT -// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE -// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -// - -#include "src/core/lib/transport/method_config.h" - -#include - -#include -#include -#include -#include - -#include "src/core/lib/json/json.h" -#include "src/core/lib/support/string.h" -#include "src/core/lib/transport/mdstr_hash_table.h" - -// Returns the number of names specified in the method config \a json. -static size_t count_names_in_method_config_json(grpc_json* json) { - size_t num_names = 0; - for (grpc_json* field = json->child; field != NULL; field = field->next) { - if (field->key != NULL && strcmp(field->key, "name") == 0) ++num_names; - } - return num_names; -} - -// Returns a path string for the name specified by \a json. -// Returns NULL on error. Caller takes ownership of result. -static char* parse_json_method_name(grpc_json* json) { - if (json->type != GRPC_JSON_OBJECT) return NULL; - const char* service_name = NULL; - const char* method_name = NULL; - for (grpc_json* child = json->child; child != NULL; child = child->next) { - if (child->key == NULL) return NULL; - if (child->type != GRPC_JSON_STRING) return NULL; - if (strcmp(child->key, "service") == 0) { - if (service_name != NULL) return NULL; // Duplicate. - if (child->value == NULL) return NULL; - service_name = child->value; - } else if (strcmp(child->key, "method") == 0) { - if (method_name != NULL) return NULL; // Duplicate. - if (child->value == NULL) return NULL; - method_name = child->value; - } - } - if (service_name == NULL) return NULL; // Required field. - char* path; - gpr_asprintf(&path, "/%s/%s", service_name, - method_name == NULL ? "*" : method_name); - return path; -} - -// Parses the method config from \a json. Adds an entry to \a entries for -// each name found, incrementing \a idx for each entry added. -static bool parse_json_method_config( - grpc_json* json, void* (*create_value)(const grpc_json* method_config_json), - const grpc_mdstr_hash_table_vtable* vtable, - grpc_mdstr_hash_table_entry* entries, size_t* idx) { - // Construct value. - void* method_config = create_value(json); - if (method_config == NULL) return NULL; - // Construct list of paths. - bool retval = false; - gpr_strvec paths; - gpr_strvec_init(&paths); - for (grpc_json* child = json->child; child != NULL; child = child->next) { - if (child->key == NULL) continue; - if (strcmp(child->key, "name") == 0) { - if (child->type != GRPC_JSON_ARRAY) goto done; - for (grpc_json* name = child->child; name != NULL; name = name->next) { - char* path = parse_json_method_name(name); - gpr_strvec_add(&paths, path); - } - } - } - if (paths.count == 0) goto done; // No names specified. - // Add entry for each path. - for (size_t i = 0; i < paths.count; ++i) { - entries[*idx].key = grpc_mdstr_from_string(paths.strs[i]); - entries[*idx].value = vtable->copy_value(method_config); - entries[*idx].vtable = vtable; - ++*idx; - } - retval = true; -done: - vtable->destroy_value(method_config); - gpr_strvec_destroy(&paths); - return retval; -} - -grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( - const grpc_json* json, - void* (*create_value)(const grpc_json* method_config_json), - const grpc_mdstr_hash_table_vtable* vtable) { - // Traverse parsed JSON tree. - if (json->type != GRPC_JSON_OBJECT || json->key != NULL) return NULL; - size_t num_entries = 0; - grpc_mdstr_hash_table_entry* entries = NULL; - for (grpc_json* field = json->child; field != NULL; field = field->next) { - if (field->key == NULL) return NULL; - if (strcmp(field->key, "method_config") == 0) { - if (entries != NULL) return NULL; // Duplicate. - if (field->type != GRPC_JSON_ARRAY) return NULL; - // Find number of entries. - for (grpc_json* method = field->child; method != NULL; - method = method->next) { - num_entries += count_names_in_method_config_json(method); - } - // Populate method config table entries. - entries = gpr_malloc(num_entries * sizeof(grpc_mdstr_hash_table_entry)); - size_t idx = 0; - for (grpc_json* method = field->child; method != NULL; - method = method->next) { - if (!parse_json_method_config(method, create_value, vtable, entries, - &idx)) { - return NULL; - } - } - GPR_ASSERT(idx == num_entries); - } - } - // Instantiate method config table. - grpc_mdstr_hash_table* method_config_table = NULL; - if (entries != NULL) { - method_config_table = grpc_mdstr_hash_table_create(num_entries, entries); - // Clean up. - for (size_t i = 0; i < num_entries; ++i) { - GRPC_MDSTR_UNREF(entries[i].key); - vtable->destroy_value(entries[i].value); - } - gpr_free(entries); - } - return method_config_table; -} - -void* grpc_method_config_table_get(const grpc_mdstr_hash_table* table, - const grpc_mdstr* path) { - void* value = grpc_mdstr_hash_table_get(table, path); - // If we didn't find a match for the path, try looking for a wildcard - // entry (i.e., change "/service/method" to "/service/*"). - if (value == NULL) { - const char* path_str = grpc_mdstr_as_c_string(path); - const char* sep = strrchr(path_str, '/') + 1; - const size_t len = (size_t)(sep - path_str); - char* buf = gpr_malloc(len + 2); // '*' and NUL - memcpy(buf, path_str, len); - buf[len] = '*'; - buf[len + 1] = '\0'; - grpc_mdstr* wildcard_path = grpc_mdstr_from_string(buf); - gpr_free(buf); - value = grpc_mdstr_hash_table_get(table, wildcard_path); - GRPC_MDSTR_UNREF(wildcard_path); - } - return value; -} - -const char* grpc_service_config_get_lb_policy_name( - grpc_json_tree* service_config) { - grpc_json* json = service_config->root; - if (json->type != GRPC_JSON_OBJECT || json->key != NULL) return NULL; - const char* lb_policy_name = NULL; - for (grpc_json* field = json->child; field != NULL; field = field->next) { - if (field->key == NULL) return NULL; - if (strcmp(field->key, "lb_policy_name") == 0) { - if (lb_policy_name != NULL) return NULL; // Duplicate. - if (field->type != GRPC_JSON_STRING) return NULL; - lb_policy_name = field->value; - } - } - return lb_policy_name; -} - -static void* copy_json_tree(void* t) { return grpc_json_tree_ref(t); } - -static void destroy_json_tree(void* t) { grpc_json_tree_unref(t); } - -static int cmp_json_tree(void* t1, void* t2) { - grpc_json_tree* tree1 = t1; - grpc_json_tree* tree2 = t2; - return grpc_json_cmp(tree1->root, tree2->root); -} - -static grpc_arg_pointer_vtable service_config_arg_vtable = { - copy_json_tree, destroy_json_tree, cmp_json_tree}; - -grpc_arg grpc_service_config_create_channel_arg( - grpc_json_tree* service_config) { - grpc_arg arg; - arg.type = GRPC_ARG_POINTER; - arg.key = GRPC_ARG_SERVICE_CONFIG; - arg.value.pointer.p = service_config; - arg.value.pointer.vtable = &service_config_arg_vtable; - return arg; -} diff --git a/src/core/lib/transport/method_config.h b/src/core/lib/transport/method_config.h deleted file mode 100644 index 9922a7a657..0000000000 --- a/src/core/lib/transport/method_config.h +++ /dev/null @@ -1,66 +0,0 @@ -// -// Copyright 2016, Google Inc. -// All rights reserved. -// -// Redistribution and use in source and binary forms, with or without -// modification, are permitted provided that the following conditions are -// met: -// -// * Redistributions of source code must retain the above copyright -// notice, this list of conditions and the following disclaimer. -// * Redistributions in binary form must reproduce the above -// copyright notice, this list of conditions and the following disclaimer -// in the documentation and/or other materials provided with the -// distribution. -// * Neither the name of Google Inc. nor the names of its -// contributors may be used to endorse or promote products derived from -// this software without specific prior written permission. -// -// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS -// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT -// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR -// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT -// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, -// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT -// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, -// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY -// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT -// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE -// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -// - -#ifndef GRPC_CORE_LIB_TRANSPORT_METHOD_CONFIG_H -#define GRPC_CORE_LIB_TRANSPORT_METHOD_CONFIG_H - -#include - -#include "src/core/lib/json/json.h" -#include "src/core/lib/transport/mdstr_hash_table.h" - -/// Creates a method config table based on the data in \a json. -/// The table's keys are request paths. The table's value type is -/// returned by \a create_value(), based on data parsed from the JSON tree. -/// \a vtable provides methods used to manage the values. -/// Returns NULL on error. -grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( - const grpc_json* json, - void* (*create_value)(const grpc_json* method_config_json), - const grpc_mdstr_hash_table_vtable* vtable); - -/// Gets the method config for the specified \a path, which should be of -/// the form "/service/method". -/// Returns NULL if the method has no config. -/// Caller does NOT own a reference to the result. -void* grpc_method_config_table_get(const grpc_mdstr_hash_table* table, - const grpc_mdstr* path); - -/// Gets the LB policy name from \a service_config. -/// Returns NULL if no LB policy name was specified. -/// Caller does NOT take ownership. -const char* grpc_service_config_get_lb_policy_name( - grpc_json_tree* service_config); - -/// Creates a channel arg containing \a service_config. -grpc_arg grpc_service_config_create_channel_arg(grpc_json_tree* service_config); - -#endif /* GRPC_CORE_LIB_TRANSPORT_METHOD_CONFIG_H */ diff --git a/src/core/lib/transport/service_config.c b/src/core/lib/transport/service_config.c new file mode 100644 index 0000000000..2a717e7b9b --- /dev/null +++ b/src/core/lib/transport/service_config.c @@ -0,0 +1,221 @@ +// +// Copyright 2015, Google Inc. +// All rights reserved. +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +// + +#include "src/core/lib/transport/service_config.h" + +#include + +#include +#include +#include +#include + +#include "src/core/lib/json/json.h" +#include "src/core/lib/support/string.h" +#include "src/core/lib/transport/mdstr_hash_table.h" + +// Returns the number of names specified in the method config \a json. +static size_t count_names_in_method_config_json(grpc_json* json) { + size_t num_names = 0; + for (grpc_json* field = json->child; field != NULL; field = field->next) { + if (field->key != NULL && strcmp(field->key, "name") == 0) ++num_names; + } + return num_names; +} + +// Returns a path string for the name specified by \a json. +// Returns NULL on error. Caller takes ownership of result. +static char* parse_json_method_name(grpc_json* json) { + if (json->type != GRPC_JSON_OBJECT) return NULL; + const char* service_name = NULL; + const char* method_name = NULL; + for (grpc_json* child = json->child; child != NULL; child = child->next) { + if (child->key == NULL) return NULL; + if (child->type != GRPC_JSON_STRING) return NULL; + if (strcmp(child->key, "service") == 0) { + if (service_name != NULL) return NULL; // Duplicate. + if (child->value == NULL) return NULL; + service_name = child->value; + } else if (strcmp(child->key, "method") == 0) { + if (method_name != NULL) return NULL; // Duplicate. + if (child->value == NULL) return NULL; + method_name = child->value; + } + } + if (service_name == NULL) return NULL; // Required field. + char* path; + gpr_asprintf(&path, "/%s/%s", service_name, + method_name == NULL ? "*" : method_name); + return path; +} + +// Parses the method config from \a json. Adds an entry to \a entries for +// each name found, incrementing \a idx for each entry added. +static bool parse_json_method_config( + grpc_json* json, void* (*create_value)(const grpc_json* method_config_json), + const grpc_mdstr_hash_table_vtable* vtable, + grpc_mdstr_hash_table_entry* entries, size_t* idx) { + // Construct value. + void* method_config = create_value(json); + if (method_config == NULL) return NULL; + // Construct list of paths. + bool retval = false; + gpr_strvec paths; + gpr_strvec_init(&paths); + for (grpc_json* child = json->child; child != NULL; child = child->next) { + if (child->key == NULL) continue; + if (strcmp(child->key, "name") == 0) { + if (child->type != GRPC_JSON_ARRAY) goto done; + for (grpc_json* name = child->child; name != NULL; name = name->next) { + char* path = parse_json_method_name(name); + gpr_strvec_add(&paths, path); + } + } + } + if (paths.count == 0) goto done; // No names specified. + // Add entry for each path. + for (size_t i = 0; i < paths.count; ++i) { + entries[*idx].key = grpc_mdstr_from_string(paths.strs[i]); + entries[*idx].value = vtable->copy_value(method_config); + entries[*idx].vtable = vtable; + ++*idx; + } + retval = true; +done: + vtable->destroy_value(method_config); + gpr_strvec_destroy(&paths); + return retval; +} + +grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( + const grpc_json* json, + void* (*create_value)(const grpc_json* method_config_json), + const grpc_mdstr_hash_table_vtable* vtable) { + // Traverse parsed JSON tree. + if (json->type != GRPC_JSON_OBJECT || json->key != NULL) return NULL; + size_t num_entries = 0; + grpc_mdstr_hash_table_entry* entries = NULL; + for (grpc_json* field = json->child; field != NULL; field = field->next) { + if (field->key == NULL) return NULL; + if (strcmp(field->key, "method_config") == 0) { + if (entries != NULL) return NULL; // Duplicate. + if (field->type != GRPC_JSON_ARRAY) return NULL; + // Find number of entries. + for (grpc_json* method = field->child; method != NULL; + method = method->next) { + num_entries += count_names_in_method_config_json(method); + } + // Populate method config table entries. + entries = gpr_malloc(num_entries * sizeof(grpc_mdstr_hash_table_entry)); + size_t idx = 0; + for (grpc_json* method = field->child; method != NULL; + method = method->next) { + if (!parse_json_method_config(method, create_value, vtable, entries, + &idx)) { + return NULL; + } + } + GPR_ASSERT(idx == num_entries); + } + } + // Instantiate method config table. + grpc_mdstr_hash_table* method_config_table = NULL; + if (entries != NULL) { + method_config_table = grpc_mdstr_hash_table_create(num_entries, entries); + // Clean up. + for (size_t i = 0; i < num_entries; ++i) { + GRPC_MDSTR_UNREF(entries[i].key); + vtable->destroy_value(entries[i].value); + } + gpr_free(entries); + } + return method_config_table; +} + +void* grpc_method_config_table_get(const grpc_mdstr_hash_table* table, + const grpc_mdstr* path) { + void* value = grpc_mdstr_hash_table_get(table, path); + // If we didn't find a match for the path, try looking for a wildcard + // entry (i.e., change "/service/method" to "/service/*"). + if (value == NULL) { + const char* path_str = grpc_mdstr_as_c_string(path); + const char* sep = strrchr(path_str, '/') + 1; + const size_t len = (size_t)(sep - path_str); + char* buf = gpr_malloc(len + 2); // '*' and NUL + memcpy(buf, path_str, len); + buf[len] = '*'; + buf[len + 1] = '\0'; + grpc_mdstr* wildcard_path = grpc_mdstr_from_string(buf); + gpr_free(buf); + value = grpc_mdstr_hash_table_get(table, wildcard_path); + GRPC_MDSTR_UNREF(wildcard_path); + } + return value; +} + +const char* grpc_service_config_get_lb_policy_name( + grpc_json_tree* service_config) { + grpc_json* json = service_config->root; + if (json->type != GRPC_JSON_OBJECT || json->key != NULL) return NULL; + const char* lb_policy_name = NULL; + for (grpc_json* field = json->child; field != NULL; field = field->next) { + if (field->key == NULL) return NULL; + if (strcmp(field->key, "lb_policy_name") == 0) { + if (lb_policy_name != NULL) return NULL; // Duplicate. + if (field->type != GRPC_JSON_STRING) return NULL; + lb_policy_name = field->value; + } + } + return lb_policy_name; +} + +static void* copy_json_tree(void* t) { return grpc_json_tree_ref(t); } + +static void destroy_json_tree(void* t) { grpc_json_tree_unref(t); } + +static int cmp_json_tree(void* t1, void* t2) { + grpc_json_tree* tree1 = t1; + grpc_json_tree* tree2 = t2; + return grpc_json_cmp(tree1->root, tree2->root); +} + +static grpc_arg_pointer_vtable service_config_arg_vtable = { + copy_json_tree, destroy_json_tree, cmp_json_tree}; + +grpc_arg grpc_service_config_create_channel_arg( + grpc_json_tree* service_config) { + grpc_arg arg; + arg.type = GRPC_ARG_POINTER; + arg.key = GRPC_ARG_SERVICE_CONFIG; + arg.value.pointer.p = service_config; + arg.value.pointer.vtable = &service_config_arg_vtable; + return arg; +} diff --git a/src/core/lib/transport/service_config.h b/src/core/lib/transport/service_config.h new file mode 100644 index 0000000000..c6c8e6be7f --- /dev/null +++ b/src/core/lib/transport/service_config.h @@ -0,0 +1,66 @@ +// +// Copyright 2016, Google Inc. +// All rights reserved. +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +// + +#ifndef GRPC_CORE_LIB_TRANSPORT_SERVICE_CONFIG_H +#define GRPC_CORE_LIB_TRANSPORT_SERVICE_CONFIG_H + +#include + +#include "src/core/lib/json/json.h" +#include "src/core/lib/transport/mdstr_hash_table.h" + +/// Creates a method config table based on the data in \a json. +/// The table's keys are request paths. The table's value type is +/// returned by \a create_value(), based on data parsed from the JSON tree. +/// \a vtable provides methods used to manage the values. +/// Returns NULL on error. +grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( + const grpc_json* json, + void* (*create_value)(const grpc_json* method_config_json), + const grpc_mdstr_hash_table_vtable* vtable); + +/// Gets the method config for the specified \a path, which should be of +/// the form "/service/method". +/// Returns NULL if the method has no config. +/// Caller does NOT own a reference to the result. +void* grpc_method_config_table_get(const grpc_mdstr_hash_table* table, + const grpc_mdstr* path); + +/// Gets the LB policy name from \a service_config. +/// Returns NULL if no LB policy name was specified. +/// Caller does NOT take ownership. +const char* grpc_service_config_get_lb_policy_name( + grpc_json_tree* service_config); + +/// Creates a channel arg containing \a service_config. +grpc_arg grpc_service_config_create_channel_arg(grpc_json_tree* service_config); + +#endif /* GRPC_CORE_LIB_TRANSPORT_SERVICE_CONFIG_H */ diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index 1ad423909f..6ec2984721 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -180,7 +180,7 @@ CORE_SOURCE_FILES = [ 'src/core/lib/transport/mdstr_hash_table.c', 'src/core/lib/transport/metadata.c', 'src/core/lib/transport/metadata_batch.c', - 'src/core/lib/transport/method_config.c', + 'src/core/lib/transport/service_config.c', 'src/core/lib/transport/static_metadata.c', 'src/core/lib/transport/timeout_encoding.c', 'src/core/lib/transport/transport.c', diff --git a/test/core/end2end/connection_refused_test.c b/test/core/end2end/connection_refused_test.c index 2857ca70e8..2814c94e2e 100644 --- a/test/core/end2end/connection_refused_test.c +++ b/test/core/end2end/connection_refused_test.c @@ -41,7 +41,7 @@ #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/transport/metadata.h" -#include "src/core/lib/transport/method_config.h" +#include "src/core/lib/transport/service_config.h" #include "test/core/end2end/cq_verifier.h" #include "test/core/util/port.h" diff --git a/test/core/end2end/tests/cancel_after_accept.c b/test/core/end2end/tests/cancel_after_accept.c index bef33f2a33..a187311b79 100644 --- a/test/core/end2end/tests/cancel_after_accept.c +++ b/test/core/end2end/tests/cancel_after_accept.c @@ -44,7 +44,7 @@ #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/transport/metadata.h" -#include "src/core/lib/transport/method_config.h" +#include "src/core/lib/transport/service_config.h" #include "test/core/end2end/cq_verifier.h" #include "test/core/end2end/tests/cancel_test_helpers.h" diff --git a/test/core/end2end/tests/max_message_length.c b/test/core/end2end/tests/max_message_length.c index 4cffff915e..05ac087c13 100644 --- a/test/core/end2end/tests/max_message_length.c +++ b/test/core/end2end/tests/max_message_length.c @@ -44,7 +44,7 @@ #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/transport/metadata.h" -#include "src/core/lib/transport/method_config.h" +#include "src/core/lib/transport/service_config.h" #include "test/core/end2end/cq_verifier.h" diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index e83298766f..c886c88402 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -875,7 +875,7 @@ src/core/lib/transport/connectivity_state.h \ src/core/lib/transport/mdstr_hash_table.h \ src/core/lib/transport/metadata.h \ src/core/lib/transport/metadata_batch.h \ -src/core/lib/transport/method_config.h \ +src/core/lib/transport/service_config.h \ src/core/lib/transport/static_metadata.h \ src/core/lib/transport/timeout_encoding.h \ src/core/lib/transport/transport.h \ @@ -1063,7 +1063,7 @@ src/core/lib/transport/connectivity_state.c \ src/core/lib/transport/mdstr_hash_table.c \ src/core/lib/transport/metadata.c \ src/core/lib/transport/metadata_batch.c \ -src/core/lib/transport/method_config.c \ +src/core/lib/transport/service_config.c \ src/core/lib/transport/static_metadata.c \ src/core/lib/transport/timeout_encoding.c \ src/core/lib/transport/transport.c \ diff --git a/tools/run_tests/sources_and_headers.json b/tools/run_tests/sources_and_headers.json index b6c9e0ae94..bf5a0094cc 100644 --- a/tools/run_tests/sources_and_headers.json +++ b/tools/run_tests/sources_and_headers.json @@ -6604,7 +6604,7 @@ "src/core/lib/transport/mdstr_hash_table.h", "src/core/lib/transport/metadata.h", "src/core/lib/transport/metadata_batch.h", - "src/core/lib/transport/method_config.h", + "src/core/lib/transport/service_config.h", "src/core/lib/transport/static_metadata.h", "src/core/lib/transport/timeout_encoding.h", "src/core/lib/transport/transport.h", @@ -6812,8 +6812,8 @@ "src/core/lib/transport/metadata.h", "src/core/lib/transport/metadata_batch.c", "src/core/lib/transport/metadata_batch.h", - "src/core/lib/transport/method_config.c", - "src/core/lib/transport/method_config.h", + "src/core/lib/transport/service_config.c", + "src/core/lib/transport/service_config.h", "src/core/lib/transport/static_metadata.c", "src/core/lib/transport/static_metadata.h", "src/core/lib/transport/timeout_encoding.c", diff --git a/vsprojects/vcxproj/grpc/grpc.vcxproj b/vsprojects/vcxproj/grpc/grpc.vcxproj index d9d0d42d67..a7aecb1c98 100644 --- a/vsprojects/vcxproj/grpc/grpc.vcxproj +++ b/vsprojects/vcxproj/grpc/grpc.vcxproj @@ -384,7 +384,7 @@ - + @@ -677,7 +677,7 @@ - + diff --git a/vsprojects/vcxproj/grpc/grpc.vcxproj.filters b/vsprojects/vcxproj/grpc/grpc.vcxproj.filters index ab9b76073c..9574f9784a 100644 --- a/vsprojects/vcxproj/grpc/grpc.vcxproj.filters +++ b/vsprojects/vcxproj/grpc/grpc.vcxproj.filters @@ -310,7 +310,7 @@ src\core\lib\transport - + src\core\lib\transport @@ -974,7 +974,7 @@ src\core\lib\transport - + src\core\lib\transport diff --git a/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj b/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj index 39c144d992..9d3bfa808f 100644 --- a/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj +++ b/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj @@ -277,7 +277,7 @@ - + @@ -528,7 +528,7 @@ - + diff --git a/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj.filters b/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj.filters index 0fbfc3acd3..8257c731af 100644 --- a/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj.filters +++ b/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj.filters @@ -367,7 +367,7 @@ src\core\lib\transport - + src\core\lib\transport @@ -770,7 +770,7 @@ src\core\lib\transport - + src\core\lib\transport diff --git a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj index 87e3921ff9..1d334bdeee 100644 --- a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj +++ b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj @@ -374,7 +374,7 @@ - + @@ -645,7 +645,7 @@ - + diff --git a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters index 1e719226fa..daed8db5ae 100644 --- a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters +++ b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters @@ -313,7 +313,7 @@ src\core\lib\transport - + src\core\lib\transport @@ -887,7 +887,7 @@ src\core\lib\transport - + src\core\lib\transport -- cgit v1.2.3 From 9ec28af03aaad085902c65b7a59fca5abbedcbad Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 3 Nov 2016 12:32:39 -0700 Subject: Change value of channel arg to string form, and reparse in each place we use it. --- src/core/ext/client_channel/client_channel.c | 10 ++-- src/core/lib/channel/message_size_filter.c | 10 ++-- src/core/lib/json/json.c | 59 -------------------- src/core/lib/json/json.h | 19 ------- src/core/lib/transport/service_config.c | 78 +++++++++++++-------------- src/core/lib/transport/service_config.h | 26 +++++---- test/core/end2end/connection_refused_test.c | 9 ++-- test/core/end2end/tests/cancel_after_accept.c | 9 ++-- test/core/end2end/tests/max_message_length.c | 18 ++++--- 9 files changed, 84 insertions(+), 154 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index d9161f47cd..023bd24c19 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -302,11 +302,13 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, channel_arg = grpc_channel_args_find(chand->resolver_result, GRPC_ARG_SERVICE_CONFIG); if (channel_arg != NULL) { - GPR_ASSERT(channel_arg->type == GRPC_ARG_POINTER); - grpc_json_tree *service_config_json = channel_arg->value.pointer.p; - method_params_table = grpc_method_config_table_create_from_json( - service_config_json->root, method_parameters_create_from_json, + GPR_ASSERT(channel_arg->type == GRPC_ARG_STRING); + grpc_service_config* service_config = + grpc_service_config_create(channel_arg->value.string); + method_params_table = grpc_service_config_create_method_config_table( + service_config, method_parameters_create_from_json, &method_parameters_vtable); + grpc_service_config_destroy(service_config); } // Clean up. grpc_channel_args_destroy(chand->resolver_result); diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index f826c74f89..b579fa367a 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -223,11 +223,13 @@ static void init_channel_elem(grpc_exec_ctx* exec_ctx, const grpc_arg* channel_arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_SERVICE_CONFIG); if (channel_arg != NULL) { - GPR_ASSERT(channel_arg->type == GRPC_ARG_POINTER); - grpc_json_tree* json_tree = channel_arg->value.pointer.p; - chand->method_limit_table = grpc_method_config_table_create_from_json( - json_tree->root, message_size_limits_create_from_json, + GPR_ASSERT(channel_arg->type == GRPC_ARG_STRING); + grpc_service_config* service_config = + grpc_service_config_create(channel_arg->value.string); + chand->method_limit_table = grpc_service_config_create_method_config_table( + service_config, message_size_limits_create_from_json, &message_size_limits_vtable); + grpc_service_config_destroy(service_config); } } diff --git a/src/core/lib/json/json.c b/src/core/lib/json/json.c index 9acf3b0438..48b13686d7 100644 --- a/src/core/lib/json/json.c +++ b/src/core/lib/json/json.c @@ -34,8 +34,6 @@ #include #include -#include -#include #include "src/core/lib/json/json.h" @@ -64,60 +62,3 @@ void grpc_json_destroy(grpc_json* json) { gpr_free(json); } - -int grpc_json_cmp(const grpc_json* json1, const grpc_json* json2) { - if (json1 == NULL) { - if (json2 != NULL) return 1; - return 0; // Both NULL. - } else { - if (json2 == NULL) return -1; - } - // Compare type. - if (json1->type > json2->type) return 1; - if (json1->type < json2->type) return -1; - // Compare key. - if (json1->key == NULL) { - if (json2->key != NULL) return -1; - } else { - if (json2->key == NULL) return 1; - int retval = strcmp(json1->key, json2->key); - if (retval != 0) return retval; - } - // Compare value. - if (json1->value == NULL) { - if (json2->value != NULL) return -1; - } else { - if (json2->value == NULL) return 1; - int retval = strcmp(json1->value, json2->value); - if (retval != 0) return retval; - } - // Recursively compare the next pointer. - int retval = grpc_json_cmp(json1->next, json2->next); - if (retval != 0) return retval; - // Recursively compare the child pointer. - retval = grpc_json_cmp(json1->child, json2->child); - if (retval != 0) return retval; - // Both are the same. - return 0; -} - -grpc_json_tree* grpc_json_tree_create(const char* json_string) { - grpc_json_tree* tree = gpr_malloc(sizeof(*tree)); - tree->string = gpr_strdup(json_string); - tree->root = grpc_json_parse_string(tree->string); - gpr_ref_init(&tree->refs, 1); - return tree; -} - -grpc_json_tree* grpc_json_tree_ref(grpc_json_tree* tree) { - gpr_ref(&tree->refs); - return tree; -} - -void grpc_json_tree_unref(grpc_json_tree* tree) { - if (gpr_unref(&tree->refs)) { - grpc_json_destroy(tree->root); - gpr_free(tree->string); - gpr_free(tree); - } -} diff --git a/src/core/lib/json/json.h b/src/core/lib/json/json.h index 1663c690e5..7111db0b52 100644 --- a/src/core/lib/json/json.h +++ b/src/core/lib/json/json.h @@ -36,8 +36,6 @@ #include -#include - #include "src/core/lib/json/json_common.h" /* A tree-like structure to hold json values. The key and value pointers @@ -87,21 +85,4 @@ char* grpc_json_dump_to_string(grpc_json* json, int indent); grpc_json* grpc_json_create(grpc_json_type type); void grpc_json_destroy(grpc_json* json); -/* Compares two JSON trees. */ -int grpc_json_cmp(const grpc_json* json1, const grpc_json* json2); - -/* A wrapper that contains the string used for underlying allocation and - is refcounted. */ -typedef struct { - grpc_json* root; - char* string; - gpr_refcount refs; -} grpc_json_tree; - -/* Creates a copy of \a json_string. */ -grpc_json_tree* grpc_json_tree_create(const char* json_string); - -grpc_json_tree* grpc_json_tree_ref(grpc_json_tree* tree); -void grpc_json_tree_unref(grpc_json_tree* tree); - #endif /* GRPC_CORE_LIB_JSON_JSON_H */ diff --git a/src/core/lib/transport/service_config.c b/src/core/lib/transport/service_config.c index 2a717e7b9b..a467b8c6af 100644 --- a/src/core/lib/transport/service_config.c +++ b/src/core/lib/transport/service_config.c @@ -42,6 +42,40 @@ #include "src/core/lib/support/string.h" #include "src/core/lib/transport/mdstr_hash_table.h" +struct grpc_service_config { + char* string; + grpc_json* json; +}; + +grpc_service_config* grpc_service_config_create(const char* json_string) { + grpc_service_config* service_config = gpr_malloc(sizeof(*service_config)); + service_config->string = gpr_strdup(json_string); + service_config->json = grpc_json_parse_string(service_config->string); + return service_config; +} + +void grpc_service_config_destroy(grpc_service_config* service_config) { + grpc_json_destroy(service_config->json); + gpr_free(service_config->string); + gpr_free(service_config); +} + +const char* grpc_service_config_get_lb_policy_name( + const grpc_service_config* service_config) { + const grpc_json* json = service_config->json; + if (json->type != GRPC_JSON_OBJECT || json->key != NULL) return NULL; + const char* lb_policy_name = NULL; + for (grpc_json* field = json->child; field != NULL; field = field->next) { + if (field->key == NULL) return NULL; + if (strcmp(field->key, "lb_policy_name") == 0) { + if (lb_policy_name != NULL) return NULL; // Duplicate. + if (field->type != GRPC_JSON_STRING) return NULL; + lb_policy_name = field->value; + } + } + return lb_policy_name; +} + // Returns the number of names specified in the method config \a json. static size_t count_names_in_method_config_json(grpc_json* json) { size_t num_names = 0; @@ -115,10 +149,11 @@ done: return retval; } -grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( - const grpc_json* json, +grpc_mdstr_hash_table* grpc_service_config_create_method_config_table( + const grpc_service_config* service_config, void* (*create_value)(const grpc_json* method_config_json), const grpc_mdstr_hash_table_vtable* vtable) { + const grpc_json* json = service_config->json; // Traverse parsed JSON tree. if (json->type != GRPC_JSON_OBJECT || json->key != NULL) return NULL; size_t num_entries = 0; @@ -180,42 +215,3 @@ void* grpc_method_config_table_get(const grpc_mdstr_hash_table* table, } return value; } - -const char* grpc_service_config_get_lb_policy_name( - grpc_json_tree* service_config) { - grpc_json* json = service_config->root; - if (json->type != GRPC_JSON_OBJECT || json->key != NULL) return NULL; - const char* lb_policy_name = NULL; - for (grpc_json* field = json->child; field != NULL; field = field->next) { - if (field->key == NULL) return NULL; - if (strcmp(field->key, "lb_policy_name") == 0) { - if (lb_policy_name != NULL) return NULL; // Duplicate. - if (field->type != GRPC_JSON_STRING) return NULL; - lb_policy_name = field->value; - } - } - return lb_policy_name; -} - -static void* copy_json_tree(void* t) { return grpc_json_tree_ref(t); } - -static void destroy_json_tree(void* t) { grpc_json_tree_unref(t); } - -static int cmp_json_tree(void* t1, void* t2) { - grpc_json_tree* tree1 = t1; - grpc_json_tree* tree2 = t2; - return grpc_json_cmp(tree1->root, tree2->root); -} - -static grpc_arg_pointer_vtable service_config_arg_vtable = { - copy_json_tree, destroy_json_tree, cmp_json_tree}; - -grpc_arg grpc_service_config_create_channel_arg( - grpc_json_tree* service_config) { - grpc_arg arg; - arg.type = GRPC_ARG_POINTER; - arg.key = GRPC_ARG_SERVICE_CONFIG; - arg.value.pointer.p = service_config; - arg.value.pointer.vtable = &service_config_arg_vtable; - return arg; -} diff --git a/src/core/lib/transport/service_config.h b/src/core/lib/transport/service_config.h index c6c8e6be7f..bf50226f96 100644 --- a/src/core/lib/transport/service_config.h +++ b/src/core/lib/transport/service_config.h @@ -37,16 +37,29 @@ #include "src/core/lib/json/json.h" #include "src/core/lib/transport/mdstr_hash_table.h" +typedef struct grpc_service_config grpc_service_config; + +grpc_service_config* grpc_service_config_create(const char* json_string); +void grpc_service_config_destroy(grpc_service_config* service_config); + +/// Gets the LB policy name from \a service_config. +/// Returns NULL if no LB policy name was specified. +/// Caller does NOT take ownership. +const char* grpc_service_config_get_lb_policy_name( + const grpc_service_config* service_config); + /// Creates a method config table based on the data in \a json. /// The table's keys are request paths. The table's value type is /// returned by \a create_value(), based on data parsed from the JSON tree. /// \a vtable provides methods used to manage the values. /// Returns NULL on error. -grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( - const grpc_json* json, +grpc_mdstr_hash_table* grpc_service_config_create_method_config_table( + const grpc_service_config* service_config, void* (*create_value)(const grpc_json* method_config_json), const grpc_mdstr_hash_table_vtable* vtable); +/// A helper function for looking up values in the table returned by +/// grpc_service_config_create_method_config_table(). /// Gets the method config for the specified \a path, which should be of /// the form "/service/method". /// Returns NULL if the method has no config. @@ -54,13 +67,4 @@ grpc_mdstr_hash_table* grpc_method_config_table_create_from_json( void* grpc_method_config_table_get(const grpc_mdstr_hash_table* table, const grpc_mdstr* path); -/// Gets the LB policy name from \a service_config. -/// Returns NULL if no LB policy name was specified. -/// Caller does NOT take ownership. -const char* grpc_service_config_get_lb_policy_name( - grpc_json_tree* service_config); - -/// Creates a channel arg containing \a service_config. -grpc_arg grpc_service_config_create_channel_arg(grpc_json_tree* service_config); - #endif /* GRPC_CORE_LIB_TRANSPORT_SERVICE_CONFIG_H */ diff --git a/test/core/end2end/connection_refused_test.c b/test/core/end2end/connection_refused_test.c index 2814c94e2e..c14d72f49e 100644 --- a/test/core/end2end/connection_refused_test.c +++ b/test/core/end2end/connection_refused_test.c @@ -76,7 +76,10 @@ static void run_test(bool wait_for_ready, bool use_service_config) { grpc_channel_args *args = NULL; if (use_service_config) { GPR_ASSERT(wait_for_ready); - grpc_json_tree *service_config_json = grpc_json_tree_create( + grpc_arg arg; + arg.type = GRPC_ARG_STRING; + arg.key = GRPC_ARG_SERVICE_CONFIG; + arg.value.string = "{\n" " \"method_config\": [ {\n" " \"name\": [\n" @@ -84,10 +87,8 @@ static void run_test(bool wait_for_ready, bool use_service_config) { " ],\n" " \"wait_for_ready\": true\n" " } ]\n" - "}"); - grpc_arg arg = grpc_service_config_create_channel_arg(service_config_json); + "}"; args = grpc_channel_args_copy_and_add(args, &arg, 1); - grpc_json_tree_unref(service_config_json); } /* create a call, channel to a port which will refuse connection */ diff --git a/test/core/end2end/tests/cancel_after_accept.c b/test/core/end2end/tests/cancel_after_accept.c index a187311b79..d3155f1902 100644 --- a/test/core/end2end/tests/cancel_after_accept.c +++ b/test/core/end2end/tests/cancel_after_accept.c @@ -132,7 +132,10 @@ static void test_cancel_after_accept(grpc_end2end_test_config config, grpc_channel_args *args = NULL; if (use_service_config) { - grpc_json_tree *service_config_json = grpc_json_tree_create( + grpc_arg arg; + arg.type = GRPC_ARG_STRING; + arg.key = GRPC_ARG_SERVICE_CONFIG; + arg.value.string = "{\n" " \"method_config\": [ {\n" " \"name\": [\n" @@ -140,10 +143,8 @@ static void test_cancel_after_accept(grpc_end2end_test_config config, " ],\n" " \"timeout\": { \"seconds\": 5 }\n" " } ]\n" - "}"); - grpc_arg arg = grpc_service_config_create_channel_arg(service_config_json); + "}"; args = grpc_channel_args_copy_and_add(args, &arg, 1); - grpc_json_tree_unref(service_config_json); } grpc_end2end_test_fixture f = diff --git a/test/core/end2end/tests/max_message_length.c b/test/core/end2end/tests/max_message_length.c index 05ac087c13..f1dbc214dd 100644 --- a/test/core/end2end/tests/max_message_length.c +++ b/test/core/end2end/tests/max_message_length.c @@ -137,7 +137,10 @@ static void test_max_message_length_on_request(grpc_end2end_test_config config, if (use_service_config) { // We don't currently support service configs on the server side. GPR_ASSERT(send_limit); - grpc_json_tree *service_config_json = grpc_json_tree_create( + grpc_arg arg; + arg.type = GRPC_ARG_STRING; + arg.key = GRPC_ARG_SERVICE_CONFIG; + arg.value.string = "{\n" " \"method_config\": [ {\n" " \"name\": [\n" @@ -145,10 +148,8 @@ static void test_max_message_length_on_request(grpc_end2end_test_config config, " ],\n" " \"max_request_message_bytes\": 5\n" " } ]\n" - "}"); - grpc_arg arg = grpc_service_config_create_channel_arg(service_config_json); + "}"; client_args = grpc_channel_args_copy_and_add(NULL, &arg, 1); - grpc_json_tree_unref(service_config_json); } else { // Set limit via channel args. grpc_arg arg; @@ -308,7 +309,10 @@ static void test_max_message_length_on_response(grpc_end2end_test_config config, if (use_service_config) { // We don't currently support service configs on the server side. GPR_ASSERT(!send_limit); - grpc_json_tree *service_config_json = grpc_json_tree_create( + grpc_arg arg; + arg.type = GRPC_ARG_STRING; + arg.key = GRPC_ARG_SERVICE_CONFIG; + arg.value.string = "{\n" " \"method_config\": [ {\n" " \"name\": [\n" @@ -316,10 +320,8 @@ static void test_max_message_length_on_response(grpc_end2end_test_config config, " ],\n" " \"max_response_message_bytes\": 5\n" " } ]\n" - "}"); - grpc_arg arg = grpc_service_config_create_channel_arg(service_config_json); + "}"; client_args = grpc_channel_args_copy_and_add(NULL, &arg, 1); - grpc_json_tree_unref(service_config_json); } else { // Set limit via channel args. grpc_arg arg; -- cgit v1.2.3 From 4aa3687b91a9e69b414fe2a5f1a17973f897309a Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 3 Nov 2016 13:23:49 -0700 Subject: Fix build problem. --- src/core/lib/transport/service_config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/core') diff --git a/src/core/lib/transport/service_config.c b/src/core/lib/transport/service_config.c index a467b8c6af..fcb5c1f761 100644 --- a/src/core/lib/transport/service_config.c +++ b/src/core/lib/transport/service_config.c @@ -119,7 +119,7 @@ static bool parse_json_method_config( grpc_mdstr_hash_table_entry* entries, size_t* idx) { // Construct value. void* method_config = create_value(json); - if (method_config == NULL) return NULL; + if (method_config == NULL) return false; // Construct list of paths. bool retval = false; gpr_strvec paths; -- cgit v1.2.3 From b121fc7c3f4e766da11b80b939d5d161a831ba84 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Thu, 3 Nov 2016 15:22:59 -0700 Subject: Support percent-encoding status messages --- include/grpc/support/slice.h | 4 +++ src/core/lib/channel/http_client_filter.c | 57 ++++++++++++++++++++++++++----- src/core/lib/channel/http_server_filter.c | 28 +++++++++++++-- src/core/lib/support/slice.c | 8 +++++ src/core/lib/surface/call.c | 5 ++- src/core/lib/transport/metadata_batch.c | 1 + test/core/end2end/tests/binary_metadata.c | 37 ++++++++++++++++++-- 7 files changed, 124 insertions(+), 16 deletions(-) (limited to 'src/core') diff --git a/include/grpc/support/slice.h b/include/grpc/support/slice.h index b31fe6c0c5..ce7737e529 100644 --- a/include/grpc/support/slice.h +++ b/include/grpc/support/slice.h @@ -121,6 +121,10 @@ GPRAPI gpr_slice gpr_empty_slice(void); GPRAPI int gpr_slice_cmp(gpr_slice a, gpr_slice b); GPRAPI int gpr_slice_str_cmp(gpr_slice a, const char *b); +/* Do two slices point at the same memory, with the same length + If a or b is inlined, actually compares data */ +GPRAPI int gpr_slice_is_equivalent(gpr_slice a, gpr_slice b); + #ifdef __cplusplus } #endif diff --git a/src/core/lib/channel/http_client_filter.c b/src/core/lib/channel/http_client_filter.c index 1dc05fb20d..0875cc18c4 100644 --- a/src/core/lib/channel/http_client_filter.c +++ b/src/core/lib/channel/http_client_filter.c @@ -36,6 +36,7 @@ #include #include #include "src/core/lib/profiling/timers.h" +#include "src/core/lib/support/percent_encoding.h" #include "src/core/lib/support/string.h" #include "src/core/lib/transport/static_metadata.h" #include "src/core/lib/transport/transport_impl.h" @@ -56,6 +57,7 @@ typedef struct call_data { grpc_linked_mdelem payload_bin; grpc_metadata_batch *recv_initial_metadata; + grpc_metadata_batch *recv_trailing_metadata; uint8_t *payload_bytes; /* Vars to read data off of send_message */ @@ -69,14 +71,16 @@ typedef struct call_data { bool send_message_blocked; /** Closure to call when finished with the hc_on_recv hook */ - grpc_closure *on_done_recv; + grpc_closure *on_done_recv_initial_metadata; + grpc_closure *on_done_recv_trailing_metadata; grpc_closure *on_complete; grpc_closure *post_send; /** Receive closures are chained: we inject this closure as the on_done_recv up-call on transport_op, and remember to call our on_done_recv member after handling it. */ - grpc_closure hc_on_recv; + grpc_closure hc_on_recv_initial_metadata; + grpc_closure hc_on_recv_trailing_metadata; grpc_closure hc_on_complete; grpc_closure got_slice; grpc_closure send_done; @@ -106,6 +110,16 @@ static grpc_mdelem *client_recv_filter(void *user_data, grpc_mdelem *md) { grpc_call_element_send_close_with_message(a->exec_ctx, a->elem, GRPC_STATUS_CANCELLED, &message); return NULL; + } else if (md->key == GRPC_MDSTR_GRPC_MESSAGE) { + gpr_slice pct_decoded_msg = + gpr_permissive_percent_decode_slice(md->value->slice); + if (gpr_slice_is_equivalent(pct_decoded_msg, md->value->slice)) { + gpr_slice_unref(pct_decoded_msg); + return md; + } else { + return grpc_mdelem_from_metadata_strings( + GRPC_MDSTR_GRPC_MESSAGE, grpc_mdstr_from_slice(pct_decoded_msg)); + } } else if (md == GRPC_MDELEM_CONTENT_TYPE_APPLICATION_SLASH_GRPC) { return NULL; } else if (md->key == GRPC_MDSTR_CONTENT_TYPE) { @@ -129,8 +143,8 @@ static grpc_mdelem *client_recv_filter(void *user_data, grpc_mdelem *md) { return md; } -static void hc_on_recv(grpc_exec_ctx *exec_ctx, void *user_data, - grpc_error *error) { +static void hc_on_recv_initial_metadata(grpc_exec_ctx *exec_ctx, + void *user_data, grpc_error *error) { grpc_call_element *elem = user_data; call_data *calld = elem->call_data; client_recv_filter_args a; @@ -138,7 +152,21 @@ static void hc_on_recv(grpc_exec_ctx *exec_ctx, void *user_data, a.exec_ctx = exec_ctx; grpc_metadata_batch_filter(calld->recv_initial_metadata, client_recv_filter, &a); - calld->on_done_recv->cb(exec_ctx, calld->on_done_recv->cb_arg, error); + grpc_closure_run(exec_ctx, calld->on_done_recv_initial_metadata, + GRPC_ERROR_REF(error)); +} + +static void hc_on_recv_trailing_metadata(grpc_exec_ctx *exec_ctx, + void *user_data, grpc_error *error) { + grpc_call_element *elem = user_data; + call_data *calld = elem->call_data; + client_recv_filter_args a; + a.elem = elem; + a.exec_ctx = exec_ctx; + grpc_metadata_batch_filter(calld->recv_trailing_metadata, client_recv_filter, + &a); + grpc_closure_run(exec_ctx, calld->on_done_recv_trailing_metadata, + GRPC_ERROR_REF(error)); } static void hc_on_complete(grpc_exec_ctx *exec_ctx, void *user_data, @@ -281,8 +309,15 @@ static void hc_mutate_op(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, if (op->recv_initial_metadata != NULL) { /* substitute our callback for the higher callback */ calld->recv_initial_metadata = op->recv_initial_metadata; - calld->on_done_recv = op->recv_initial_metadata_ready; - op->recv_initial_metadata_ready = &calld->hc_on_recv; + calld->on_done_recv_initial_metadata = op->recv_initial_metadata_ready; + op->recv_initial_metadata_ready = &calld->hc_on_recv_initial_metadata; + } + + if (op->recv_trailing_metadata != NULL) { + /* substitute our callback for the higher callback */ + calld->recv_trailing_metadata = op->recv_trailing_metadata; + calld->on_done_recv_trailing_metadata = op->on_complete; + op->on_complete = &calld->hc_on_recv_trailing_metadata; } } @@ -308,11 +343,15 @@ static grpc_error *init_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, grpc_call_element_args *args) { call_data *calld = elem->call_data; - calld->on_done_recv = NULL; + calld->on_done_recv_initial_metadata = NULL; + calld->on_done_recv_trailing_metadata = NULL; calld->on_complete = NULL; calld->payload_bytes = NULL; gpr_slice_buffer_init(&calld->slices); - grpc_closure_init(&calld->hc_on_recv, hc_on_recv, elem); + grpc_closure_init(&calld->hc_on_recv_initial_metadata, + hc_on_recv_initial_metadata, elem); + grpc_closure_init(&calld->hc_on_recv_trailing_metadata, + hc_on_recv_trailing_metadata, elem); grpc_closure_init(&calld->hc_on_complete, hc_on_complete, elem); grpc_closure_init(&calld->got_slice, got_slice, elem); grpc_closure_init(&calld->send_done, send_done, elem); diff --git a/src/core/lib/channel/http_server_filter.c b/src/core/lib/channel/http_server_filter.c index f2221fb0fb..eb335b6b5c 100644 --- a/src/core/lib/channel/http_server_filter.c +++ b/src/core/lib/channel/http_server_filter.c @@ -37,6 +37,7 @@ #include #include #include "src/core/lib/profiling/timers.h" +#include "src/core/lib/support/percent_encoding.h" #include "src/core/lib/transport/static_metadata.h" #define EXPECTED_CONTENT_TYPE "application/grpc" @@ -86,6 +87,23 @@ typedef struct { grpc_exec_ctx *exec_ctx; } server_filter_args; +static grpc_mdelem *server_filter_outgoing_metadata(void *user_data, + grpc_mdelem *md) { + if (md->key == GRPC_MDSTR_GRPC_MESSAGE) { + gpr_slice pct_encoded_msg = gpr_percent_encode_slice( + md->value->slice, gpr_compatible_percent_encoding_unreserved_bytes); + if (gpr_slice_is_equivalent(pct_encoded_msg, md->value->slice)) { + gpr_slice_unref(pct_encoded_msg); + return md; + } else { + return grpc_mdelem_from_metadata_strings( + GRPC_MDSTR_GRPC_MESSAGE, grpc_mdstr_from_slice(pct_encoded_msg)); + } + } else { + return md; + } +} + static grpc_mdelem *server_filter(void *user_data, grpc_mdelem *md) { server_filter_args *a = user_data; grpc_call_element *elem = a->elem; @@ -255,7 +273,7 @@ static void hs_recv_message_ready(grpc_exec_ctx *exec_ctx, void *user_data, } } -static void hs_mutate_op(grpc_call_element *elem, +static void hs_mutate_op(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, grpc_transport_stream_op *op) { /* grab pointers to our data from the call element */ call_data *calld = elem->call_data; @@ -291,6 +309,12 @@ static void hs_mutate_op(grpc_call_element *elem, op->on_complete = &calld->hs_on_complete; } } + + if (op->send_trailing_metadata) { + server_filter_args a = {elem, exec_ctx}; + grpc_metadata_batch_filter(op->send_trailing_metadata, + server_filter_outgoing_metadata, &a); + } } static void hs_start_transport_op(grpc_exec_ctx *exec_ctx, @@ -298,7 +322,7 @@ static void hs_start_transport_op(grpc_exec_ctx *exec_ctx, grpc_transport_stream_op *op) { GRPC_CALL_LOG_OP(GPR_INFO, elem, op); GPR_TIMER_BEGIN("hs_start_transport_op", 0); - hs_mutate_op(elem, op); + hs_mutate_op(exec_ctx, elem, op); grpc_call_next_op(exec_ctx, elem, op); GPR_TIMER_END("hs_start_transport_op", 0); } diff --git a/src/core/lib/support/slice.c b/src/core/lib/support/slice.c index 8a2c0a9086..f15a9a1e21 100644 --- a/src/core/lib/support/slice.c +++ b/src/core/lib/support/slice.c @@ -348,3 +348,11 @@ int gpr_slice_str_cmp(gpr_slice a, const char *b) { if (d != 0) return d; return memcmp(GPR_SLICE_START_PTR(a), b, b_length); } + +int gpr_slice_is_equivalent(gpr_slice a, gpr_slice b) { + if (a.refcount == NULL || b.refcount == NULL) { + return gpr_slice_cmp(a, b); + } + return a.data.refcounted.length == b.data.refcounted.length && + a.data.refcounted.bytes == b.data.refcounted.bytes; +} diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c index 6c25952c0a..3f4afd9a29 100644 --- a/src/core/lib/surface/call.c +++ b/src/core/lib/surface/call.c @@ -632,9 +632,6 @@ static int prepare_application_metadata(grpc_call *call, int count, if (call->send_extra_metadata_count == 0) { prepend_extra_metadata = 0; } else { - for (i = 0; i < call->send_extra_metadata_count; i++) { - GRPC_MDELEM_REF(call->send_extra_metadata[i].md); - } for (i = 1; i < call->send_extra_metadata_count; i++) { call->send_extra_metadata[i].prev = &call->send_extra_metadata[i - 1]; } @@ -680,6 +677,7 @@ static int prepare_application_metadata(grpc_call *call, int count, &call->send_extra_metadata[call->send_extra_metadata_count - 1]; batch->list.head->prev = NULL; batch->list.tail->next = NULL; + call->send_extra_metadata_count = 0; break; case 3: { /* prepend AND md */ @@ -695,6 +693,7 @@ static int prepare_application_metadata(grpc_call *call, int count, batch->list.tail = linked_from_md(last_md); batch->list.head->prev = NULL; batch->list.tail->next = NULL; + call->send_extra_metadata_count = 0; break; } default: diff --git a/src/core/lib/transport/metadata_batch.c b/src/core/lib/transport/metadata_batch.c index 84b5a74d51..f2d4595d3a 100644 --- a/src/core/lib/transport/metadata_batch.c +++ b/src/core/lib/transport/metadata_batch.c @@ -154,6 +154,7 @@ void grpc_metadata_batch_filter(grpc_metadata_batch *batch, grpc_mdelem *orig = l->md; grpc_mdelem *filt = filter(user_data, orig); next = l->next; + gpr_log(GPR_DEBUG, "FILT: %p %p->%p", l, orig, filt); if (filt == NULL) { if (l->prev) { l->prev->next = l->next; diff --git a/test/core/end2end/tests/binary_metadata.c b/test/core/end2end/tests/binary_metadata.c index 73b0f17c24..b94bb84e54 100644 --- a/test/core/end2end/tests/binary_metadata.c +++ b/test/core/end2end/tests/binary_metadata.c @@ -234,7 +234,22 @@ static void test_request_response_with_metadata_and_payload( op->op = GRPC_OP_SEND_STATUS_FROM_SERVER; op->data.send_status_from_server.trailing_metadata_count = 0; op->data.send_status_from_server.status = GRPC_STATUS_OK; - op->data.send_status_from_server.status_details = "xyz"; + op->data.send_status_from_server.status_details = + "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x10\x11\x12" + "\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f\x20\x21\x22\x23\x24" + "\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f\x30\x31\x32\x33\x34\x35\x36" + "\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f\x40\x41\x42\x43\x44\x45\x46\x47\x48" + "\x49\x4a\x4b\x4c\x4d\x4e\x4f\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a" + "\x5b\x5c\x5d\x5e\x5f\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c" + "\x6d\x6e\x6f\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e" + "\x7f\x80\x81\x82\x83\x84\x85\x86\x87\x88\x89\x8a\x8b\x8c\x8d\x8e\x8f\x90" + "\x91\x92\x93\x94\x95\x96\x97\x98\x99\x9a\x9b\x9c\x9d\x9e\x9f\xa0\xa1\xa2" + "\xa3\xa4\xa5\xa6\xa7\xa8\xa9\xaa\xab\xac\xad\xae\xaf\xb0\xb1\xb2\xb3\xb4" + "\xb5\xb6\xb7\xb8\xb9\xba\xbb\xbc\xbd\xbe\xbf\xc0\xc1\xc2\xc3\xc4\xc5\xc6" + "\xc7\xc8\xc9\xca\xcb\xcc\xcd\xce\xcf\xd0\xd1\xd2\xd3\xd4\xd5\xd6\xd7\xd8" + "\xd9\xda\xdb\xdc\xdd\xde\xdf\xe0\xe1\xe2\xe3\xe4\xe5\xe6\xe7\xe8\xe9\xea" + "\xeb\xec\xed\xee\xef\xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc" + "\xfd\xfe\xff"; op->flags = 0; op->reserved = NULL; op++; @@ -246,7 +261,25 @@ static void test_request_response_with_metadata_and_payload( cq_verify(cqv); GPR_ASSERT(status == GRPC_STATUS_OK); - GPR_ASSERT(0 == strcmp(details, "xyz")); + GPR_ASSERT( + 0 == + strcmp(details, + "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x10" + "\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f\x20" + "\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f\x30" + "\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f\x40" + "\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f\x50" + "\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f\x60" + "\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f\x70" + "\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f\x80" + "\x81\x82\x83\x84\x85\x86\x87\x88\x89\x8a\x8b\x8c\x8d\x8e\x8f\x90" + "\x91\x92\x93\x94\x95\x96\x97\x98\x99\x9a\x9b\x9c\x9d\x9e\x9f\xa0" + "\xa1\xa2\xa3\xa4\xa5\xa6\xa7\xa8\xa9\xaa\xab\xac\xad\xae\xaf\xb0" + "\xb1\xb2\xb3\xb4\xb5\xb6\xb7\xb8\xb9\xba\xbb\xbc\xbd\xbe\xbf\xc0" + "\xc1\xc2\xc3\xc4\xc5\xc6\xc7\xc8\xc9\xca\xcb\xcc\xcd\xce\xcf\xd0" + "\xd1\xd2\xd3\xd4\xd5\xd6\xd7\xd8\xd9\xda\xdb\xdc\xdd\xde\xdf\xe0" + "\xe1\xe2\xe3\xe4\xe5\xe6\xe7\xe8\xe9\xea\xeb\xec\xed\xee\xef\xf0" + "\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc\xfd\xfe\xff")); GPR_ASSERT(0 == strcmp(call_details.method, "/foo")); GPR_ASSERT(0 == strcmp(call_details.host, "foo.test.google.fr")); GPR_ASSERT(was_cancelled == 0); -- cgit v1.2.3 From b044445174f0afd04c44b86813b49a222d6e25d0 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 4 Nov 2016 08:24:34 -0700 Subject: Remove spam --- src/core/lib/transport/metadata_batch.c | 1 - 1 file changed, 1 deletion(-) (limited to 'src/core') diff --git a/src/core/lib/transport/metadata_batch.c b/src/core/lib/transport/metadata_batch.c index f2d4595d3a..84b5a74d51 100644 --- a/src/core/lib/transport/metadata_batch.c +++ b/src/core/lib/transport/metadata_batch.c @@ -154,7 +154,6 @@ void grpc_metadata_batch_filter(grpc_metadata_batch *batch, grpc_mdelem *orig = l->md; grpc_mdelem *filt = filter(user_data, orig); next = l->next; - gpr_log(GPR_DEBUG, "FILT: %p %p->%p", l, orig, filt); if (filt == NULL) { if (l->prev) { l->prev->next = l->next; -- cgit v1.2.3 From bdc58b23566c91b6a911c7daaf122c74e8356cd7 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 4 Nov 2016 09:25:57 -0700 Subject: Code review changes. --- src/core/ext/client_channel/client_channel.c | 10 +++-- src/core/lib/channel/message_size_filter.c | 11 ++++-- src/core/lib/transport/service_config.c | 58 ++++++++++++++++++++++------ src/core/lib/transport/service_config.h | 2 +- 4 files changed, 60 insertions(+), 21 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index 023bd24c19..e97587b0bd 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -305,10 +305,12 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, GPR_ASSERT(channel_arg->type == GRPC_ARG_STRING); grpc_service_config* service_config = grpc_service_config_create(channel_arg->value.string); - method_params_table = grpc_service_config_create_method_config_table( - service_config, method_parameters_create_from_json, - &method_parameters_vtable); - grpc_service_config_destroy(service_config); + if (service_config != NULL) { + method_params_table = grpc_service_config_create_method_config_table( + service_config, method_parameters_create_from_json, + &method_parameters_vtable); + grpc_service_config_destroy(service_config); + } } // Clean up. grpc_channel_args_destroy(chand->resolver_result); diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index b579fa367a..c1b8deed5d 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -226,10 +226,13 @@ static void init_channel_elem(grpc_exec_ctx* exec_ctx, GPR_ASSERT(channel_arg->type == GRPC_ARG_STRING); grpc_service_config* service_config = grpc_service_config_create(channel_arg->value.string); - chand->method_limit_table = grpc_service_config_create_method_config_table( - service_config, message_size_limits_create_from_json, - &message_size_limits_vtable); - grpc_service_config_destroy(service_config); + if (service_config != NULL) { + chand->method_limit_table = + grpc_service_config_create_method_config_table( + service_config, message_size_limits_create_from_json, + &message_size_limits_vtable); + grpc_service_config_destroy(service_config); + } } } diff --git a/src/core/lib/transport/service_config.c b/src/core/lib/transport/service_config.c index fcb5c1f761..a8494cbe8b 100644 --- a/src/core/lib/transport/service_config.c +++ b/src/core/lib/transport/service_config.c @@ -42,27 +42,60 @@ #include "src/core/lib/support/string.h" #include "src/core/lib/transport/mdstr_hash_table.h" +// The main purpose of the code here is to parse the service config in +// JSON form, which will look like this: +// +// { +// "lb_policy_name": "string", // optional +// "method_config": [ // array of one or more method_config objects +// { +// "name": [ // array of one or more name objects +// { +// "service": "string", // required +// "method": "string", // optional +// } +// ], +// // remaining fields are optional +// "wait_for_ready": bool, +// "timeout": { +// // one or both of these fields may be specified +// "seconds": number, +// "nanos": number, +// }, +// "max_request_message_bytes": number, +// "max_response_message_bytes": number +// } +// ] +// } + struct grpc_service_config { - char* string; - grpc_json* json; + char* json_string; // Underlying storage for json_tree. + grpc_json* json_tree; }; grpc_service_config* grpc_service_config_create(const char* json_string) { grpc_service_config* service_config = gpr_malloc(sizeof(*service_config)); - service_config->string = gpr_strdup(json_string); - service_config->json = grpc_json_parse_string(service_config->string); + service_config->json_string = gpr_strdup(json_string); + service_config->json_tree = + grpc_json_parse_string(service_config->json_string); + if (service_config->json_tree == NULL) { + gpr_log(GPR_INFO, "failed to parse JSON for service config"); + gpr_free(service_config->json_string); + gpr_free(service_config); + return NULL; + } return service_config; } void grpc_service_config_destroy(grpc_service_config* service_config) { - grpc_json_destroy(service_config->json); - gpr_free(service_config->string); + grpc_json_destroy(service_config->json_tree); + gpr_free(service_config->json_string); gpr_free(service_config); } const char* grpc_service_config_get_lb_policy_name( const grpc_service_config* service_config) { - const grpc_json* json = service_config->json; + const grpc_json* json = service_config->json_tree; if (json->type != GRPC_JSON_OBJECT || json->key != NULL) return NULL; const char* lb_policy_name = NULL; for (grpc_json* field = json->child; field != NULL; field = field->next) { @@ -85,7 +118,7 @@ static size_t count_names_in_method_config_json(grpc_json* json) { return num_names; } -// Returns a path string for the name specified by \a json. +// Returns a path string for the JSON name object specified by \a json. // Returns NULL on error. Caller takes ownership of result. static char* parse_json_method_name(grpc_json* json) { if (json->type != GRPC_JSON_OBJECT) return NULL; @@ -113,6 +146,7 @@ static char* parse_json_method_name(grpc_json* json) { // Parses the method config from \a json. Adds an entry to \a entries for // each name found, incrementing \a idx for each entry added. +// Returns false on error. static bool parse_json_method_config( grpc_json* json, void* (*create_value)(const grpc_json* method_config_json), const grpc_mdstr_hash_table_vtable* vtable, @@ -121,7 +155,7 @@ static bool parse_json_method_config( void* method_config = create_value(json); if (method_config == NULL) return false; // Construct list of paths. - bool retval = false; + bool success = false; gpr_strvec paths; gpr_strvec_init(&paths); for (grpc_json* child = json->child; child != NULL; child = child->next) { @@ -142,18 +176,18 @@ static bool parse_json_method_config( entries[*idx].vtable = vtable; ++*idx; } - retval = true; + success = true; done: vtable->destroy_value(method_config); gpr_strvec_destroy(&paths); - return retval; + return success; } grpc_mdstr_hash_table* grpc_service_config_create_method_config_table( const grpc_service_config* service_config, void* (*create_value)(const grpc_json* method_config_json), const grpc_mdstr_hash_table_vtable* vtable) { - const grpc_json* json = service_config->json; + const grpc_json* json = service_config->json_tree; // Traverse parsed JSON tree. if (json->type != GRPC_JSON_OBJECT || json->key != NULL) return NULL; size_t num_entries = 0; diff --git a/src/core/lib/transport/service_config.h b/src/core/lib/transport/service_config.h index bf50226f96..2ffe475193 100644 --- a/src/core/lib/transport/service_config.h +++ b/src/core/lib/transport/service_config.h @@ -59,7 +59,7 @@ grpc_mdstr_hash_table* grpc_service_config_create_method_config_table( const grpc_mdstr_hash_table_vtable* vtable); /// A helper function for looking up values in the table returned by -/// grpc_service_config_create_method_config_table(). +/// \a grpc_service_config_create_method_config_table(). /// Gets the method config for the specified \a path, which should be of /// the form "/service/method". /// Returns NULL if the method has no config. -- cgit v1.2.3 From 70a1abdb45d74ce46480d74d58a8b01887f96cac Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 4 Nov 2016 09:26:37 -0700 Subject: clang-format --- src/core/ext/client_channel/client_channel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/core') diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index e97587b0bd..2675063f86 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -303,7 +303,7 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_channel_args_find(chand->resolver_result, GRPC_ARG_SERVICE_CONFIG); if (channel_arg != NULL) { GPR_ASSERT(channel_arg->type == GRPC_ARG_STRING); - grpc_service_config* service_config = + grpc_service_config *service_config = grpc_service_config_create(channel_arg->value.string); if (service_config != NULL) { method_params_table = grpc_service_config_create_method_config_table( -- cgit v1.2.3 From 268557790603150f3519b451b089b98d281d9b80 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 4 Nov 2016 10:54:20 -0700 Subject: Fix memory leak --- src/core/lib/support/slice.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/core') diff --git a/src/core/lib/support/slice.c b/src/core/lib/support/slice.c index f15a9a1e21..2959a6716f 100644 --- a/src/core/lib/support/slice.c +++ b/src/core/lib/support/slice.c @@ -351,7 +351,7 @@ int gpr_slice_str_cmp(gpr_slice a, const char *b) { int gpr_slice_is_equivalent(gpr_slice a, gpr_slice b) { if (a.refcount == NULL || b.refcount == NULL) { - return gpr_slice_cmp(a, b); + return gpr_slice_cmp(a, b) == 0; } return a.data.refcounted.length == b.data.refcounted.length && a.data.refcounted.bytes == b.data.refcounted.bytes; -- cgit v1.2.3 From 42909c58fb1337a1020186fa9c4e7dca7f400959 Mon Sep 17 00:00:00 2001 From: Ken Payson Date: Sun, 6 Nov 2016 20:06:12 -0800 Subject: Bypass poll thread if wakeup fd is set --- src/core/lib/iomgr/ev_poll_posix.c | 15 +++++++-------- test/core/iomgr/wakeup_fd_cv_test.c | 7 +++---- 2 files changed, 10 insertions(+), 12 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_poll_posix.c b/src/core/lib/iomgr/ev_poll_posix.c index e1d620cfff..27083ca6d0 100644 --- a/src/core/lib/iomgr/ev_poll_posix.c +++ b/src/core/lib/iomgr/ev_poll_posix.c @@ -1343,6 +1343,7 @@ static int cvfd_poll(struct pollfd *fds, nfds_t nfds, int timeout) { int res, idx; gpr_cv *pollcv; cv_node *cvn, *prev; + int skip_poll = 0; nfds_t nsockfds = 0; gpr_thd_id t_id; gpr_thd_options opt; @@ -1358,17 +1359,17 @@ static int cvfd_poll(struct pollfd *fds, nfds_t nfds, int timeout) { cvn->cv = pollcv; cvn->next = g_cvfds.cvfds[idx].cvs; g_cvfds.cvfds[idx].cvs = cvn; - // We should return immediately if there are pending events, - // but we still need to call poll() to check for socket events + // Don't bother polling if a wakeup fd is ready if (g_cvfds.cvfds[idx].is_set) { - timeout = 0; + skip_poll=1; } } else if (fds[i].fd >= 0) { nsockfds++; } } - if (nsockfds > 0) { + res = 0; + if (!skip_poll && nsockfds > 0) { pargs = gpr_malloc(sizeof(struct poll_args)); // Both the main thread and calling thread get a reference gpr_ref_init(&pargs->refcount, 2); @@ -1398,16 +1399,14 @@ static int cvfd_poll(struct pollfd *fds, nfds_t nfds, int timeout) { res = pargs->retval; errno = pargs->err; } else { - res = 0; errno = 0; gpr_atm_no_barrier_store(&pargs->status, CANCELLED); } - } else { + } else if (!skip_poll) { gpr_timespec deadline = gpr_now(GPR_CLOCK_REALTIME); deadline = gpr_time_add(deadline, gpr_time_from_millis(timeout, GPR_TIMESPAN)); gpr_cv_wait(pollcv, &g_cvfds.mu, deadline); - res = 0; } idx = 0; @@ -1431,7 +1430,7 @@ static int cvfd_poll(struct pollfd *fds, nfds_t nfds, int timeout) { fds[i].revents = POLLIN; if (res >= 0) res++; } - } else if (fds[i].fd >= 0 && + } else if (!skip_poll && fds[i].fd >= 0 && gpr_atm_no_barrier_load(&pargs->status) == COMPLETED) { fds[i].revents = pargs->fds[idx].revents; idx++; diff --git a/test/core/iomgr/wakeup_fd_cv_test.c b/test/core/iomgr/wakeup_fd_cv_test.c index 82452d2157..04ae9376dd 100644 --- a/test/core/iomgr/wakeup_fd_cv_test.c +++ b/test/core/iomgr/wakeup_fd_cv_test.c @@ -195,16 +195,15 @@ void test_poll_cv_trigger(void) { GPR_ASSERT(pfds[4].revents == 0); GPR_ASSERT(pfds[5].revents == 0); - // Pollin on wakeup fd + socket fd - trigger_socket_event(); + // Pollin on wakeupfd before poll() pargs.result = -2; gpr_thd_new(&t_id, &background_poll, &pargs, &opt); gpr_thd_join(t_id); - GPR_ASSERT(pargs.result == 2); + GPR_ASSERT(pargs.result == 1); GPR_ASSERT(pfds[0].revents == 0); GPR_ASSERT(pfds[1].revents == POLLIN); - GPR_ASSERT(pfds[2].revents == POLLIN); + GPR_ASSERT(pfds[2].revents == 0); GPR_ASSERT(pfds[3].revents == 0); GPR_ASSERT(pfds[4].revents == 0); GPR_ASSERT(pfds[5].revents == 0); -- cgit v1.2.3 From c625c7a023cb688a135be96cef64543f0e08a851 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 9 Nov 2016 14:12:37 -0800 Subject: Allow fetching service config via grpc_channel_get_info(). --- include/grpc/impl/codegen/grpc_types.h | 3 +++ src/core/ext/client_channel/client_channel.c | 16 +++++++++++- test/core/client_channel/lb_policies_test.c | 37 ++++++++++++++++++++++------ 3 files changed, 48 insertions(+), 8 deletions(-) (limited to 'src/core') diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index 1bf78e4e69..9cb89e9841 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -473,6 +473,9 @@ typedef struct { /* If non-NULL, will be set to point to a string indicating the LB * policy name. Caller takes ownership. */ char **lb_policy_name; + /* If non-NULL, will be set to point to a string containing the + * service config used by the channel in JSON form. */ + char **service_config_json; } grpc_channel_info; typedef struct grpc_resource_quota grpc_resource_quota; diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index 55cb660311..139912e4ba 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -144,6 +144,8 @@ typedef struct client_channel_channel_data { /** currently active load balancer */ char *lb_policy_name; grpc_lb_policy *lb_policy; + /** service config in JSON form */ + char *service_config_json; /** maps method names to method_parameters structs */ grpc_mdstr_hash_table *method_params_table; /** incoming resolver result - set by resolver.next() */ @@ -250,6 +252,7 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_connectivity_state state = GRPC_CHANNEL_TRANSIENT_FAILURE; bool exit_idle = false; grpc_error *state_error = GRPC_ERROR_CREATE("No load balancing policy"); + char *service_config_json = NULL; if (chand->resolver_result != NULL) { // Find LB policy name. @@ -305,8 +308,9 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_channel_args_find(chand->resolver_result, GRPC_ARG_SERVICE_CONFIG); if (channel_arg != NULL) { GPR_ASSERT(channel_arg->type == GRPC_ARG_STRING); + service_config_json = gpr_strdup(channel_arg->value.string); grpc_service_config *service_config = - grpc_service_config_create(channel_arg->value.string); + grpc_service_config_create(service_config_json); if (service_config != NULL) { method_params_table = grpc_service_config_create_method_config_table( service_config, method_parameters_create_from_json, @@ -334,6 +338,10 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, } old_lb_policy = chand->lb_policy; chand->lb_policy = lb_policy; + if (service_config_json != NULL) { + gpr_free(chand->service_config_json); + chand->service_config_json = service_config_json; + } if (chand->method_params_table != NULL) { grpc_mdstr_hash_table_unref(chand->method_params_table); } @@ -469,6 +477,11 @@ static void cc_get_channel_info(grpc_exec_ctx *exec_ctx, ? NULL : gpr_strdup(chand->lb_policy_name); } + if (info->service_config_json != NULL) { + *info->service_config_json = chand->service_config_json == NULL + ? NULL + : gpr_strdup(chand->service_config_json); + } gpr_mu_unlock(&chand->mu); } @@ -512,6 +525,7 @@ static void cc_destroy_channel_elem(grpc_exec_ctx *exec_ctx, GRPC_LB_POLICY_UNREF(exec_ctx, chand->lb_policy, "channel"); } gpr_free(chand->lb_policy_name); + gpr_free(chand->service_config_json); if (chand->method_params_table != NULL) { grpc_mdstr_hash_table_unref(chand->method_params_table); } diff --git a/test/core/client_channel/lb_policies_test.c b/test/core/client_channel/lb_policies_test.c index 198aafb91f..3aba906094 100644 --- a/test/core/client_channel/lb_policies_test.c +++ b/test/core/client_channel/lb_policies_test.c @@ -43,6 +43,7 @@ #include "src/core/ext/client_channel/client_channel.h" #include "src/core/ext/client_channel/lb_policy_registry.h" +#include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/channel_stack.h" #include "src/core/lib/support/string.h" #include "src/core/lib/surface/channel.h" @@ -642,21 +643,43 @@ static void test_pending_calls(size_t concurrent_calls) { } static void test_get_channel_info() { - grpc_channel *channel = grpc_insecure_channel_create( - "test:127.0.0.1:1234?lb_policy=round_robin", NULL, NULL); + grpc_channel *channel = + grpc_insecure_channel_create("ipv4:127.0.0.1:1234", NULL, NULL); // Ensures that resolver returns. grpc_channel_check_connectivity_state(channel, true /* try_to_connect */); - // Use grpc_channel_get_info() to get LB policy name. - char *lb_policy_name = NULL; + // First, request no fields. This is a no-op. grpc_channel_info channel_info; + memset(&channel_info, 0, sizeof(channel_info)); + grpc_channel_get_info(channel, &channel_info); + // Request LB policy name. + char *lb_policy_name = NULL; channel_info.lb_policy_name = &lb_policy_name; grpc_channel_get_info(channel, &channel_info); GPR_ASSERT(lb_policy_name != NULL); - GPR_ASSERT(strcmp(lb_policy_name, "round_robin") == 0); + GPR_ASSERT(strcmp(lb_policy_name, "pick_first") == 0); gpr_free(lb_policy_name); - // Try again without requesting anything. This is a no-op. - channel_info.lb_policy_name = NULL; + // Request service config, which does not exist, so we'll get nothing back. + memset(&channel_info, 0, sizeof(channel_info)); + char *service_config_json = "dummy_string"; + channel_info.service_config_json = &service_config_json; + grpc_channel_get_info(channel, &channel_info); + GPR_ASSERT(service_config_json == NULL); + // Recreate the channel such that it has a service config. + grpc_channel_destroy(channel); + grpc_arg arg; + arg.type = GRPC_ARG_STRING; + arg.key = GRPC_ARG_SERVICE_CONFIG; + arg.value.string = "{\"lb_policy_name\": \"round_robin\"}"; + grpc_channel_args *args = grpc_channel_args_copy_and_add(NULL, &arg, 1); + channel = grpc_insecure_channel_create("ipv4:127.0.0.1:1234", args, NULL); + grpc_channel_args_destroy(args); + // Ensures that resolver returns. + grpc_channel_check_connectivity_state(channel, true /* try_to_connect */); + // Now request the service config again. grpc_channel_get_info(channel, &channel_info); + GPR_ASSERT(service_config_json != NULL); + GPR_ASSERT(strcmp(service_config_json, arg.value.string) == 0); + gpr_free(service_config_json); // Clean up. grpc_channel_destroy(channel); } -- cgit v1.2.3 From 84c8a027d3b9427982bc31be52e587ecd3462b9d Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 10 Nov 2016 09:39:34 -0800 Subject: Change JSON format to use the standard proto3 mapping for each type. --- src/core/ext/client_channel/client_channel.c | 49 +++++++++++++++++---------- src/core/lib/channel/message_size_filter.c | 12 +++---- src/core/lib/support/string.c | 2 +- src/core/lib/support/string.h | 2 +- src/core/lib/transport/service_config.c | 24 ++++++------- test/core/end2end/connection_refused_test.c | 4 +-- test/core/end2end/tests/cancel_after_accept.c | 4 +-- test/core/end2end/tests/max_message_length.c | 8 ++--- 8 files changed, 58 insertions(+), 47 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index 139912e4ba..a92a220c74 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -90,7 +90,7 @@ static void *method_parameters_create_from_json(const grpc_json *json) { gpr_timespec timeout = {0, 0, GPR_TIMESPAN}; for (grpc_json *field = json->child; field != NULL; field = field->next) { if (field->key == NULL) continue; - if (strcmp(field->key, "wait_for_ready") == 0) { + if (strcmp(field->key, "waitForReady") == 0) { if (wait_for_ready != WAIT_FOR_READY_UNSET) return NULL; // Duplicate. if (field->type != GRPC_JSON_TRUE && field->type != GRPC_JSON_FALSE) { return NULL; @@ -99,26 +99,39 @@ static void *method_parameters_create_from_json(const grpc_json *json) { : WAIT_FOR_READY_FALSE; } else if (strcmp(field->key, "timeout") == 0) { if (timeout.tv_sec > 0 || timeout.tv_nsec > 0) return NULL; // Duplicate. - if (field->type != GRPC_JSON_OBJECT) return NULL; - if (field->child == NULL) return NULL; - for (grpc_json *subfield = field->child; subfield != NULL; - subfield = subfield->next) { - if (subfield->key == NULL) return NULL; - if (strcmp(subfield->key, "seconds") == 0) { - if (timeout.tv_sec > 0) return NULL; // Duplicate. - if (subfield->type != GRPC_JSON_NUMBER) return NULL; - timeout.tv_sec = gpr_parse_nonnegative_number(subfield->value); - if (timeout.tv_sec == -1) return NULL; - } else if (strcmp(subfield->key, "nanos") == 0) { - if (timeout.tv_nsec > 0) return NULL; // Duplicate. - if (subfield->type != GRPC_JSON_NUMBER) return NULL; - timeout.tv_nsec = gpr_parse_nonnegative_number(subfield->value); - if (timeout.tv_nsec == -1) return NULL; - } else { - // Unknown key. + if (field->type != GRPC_JSON_STRING) return NULL; + size_t len = strlen(field->value); + if (field->value[len - 1] != 's') return NULL; + char* buf = gpr_strdup(field->value); + buf[len - 1] = '\0'; // Remove trailing 's'. + char* decimal_point = strchr(buf, '.'); + if (decimal_point != NULL) { + *decimal_point = '\0'; + timeout.tv_nsec = gpr_parse_nonnegative_int(decimal_point + 1); + if (timeout.tv_nsec == -1) { + gpr_free(buf); return NULL; } + // There should always be exactly 3, 6, or 9 fractional digits. + int multiplier = 1; + switch (strlen(decimal_point + 1)) { + case 9: + break; + case 6: + multiplier *= 1000; + break; + case 3: + multiplier *= 1000000; + break; + default: // Unsupported number of digits. + gpr_free(buf); + return NULL; + } + timeout.tv_nsec *= multiplier; } + timeout.tv_sec = gpr_parse_nonnegative_int(buf); + if (timeout.tv_sec == -1) return NULL; + gpr_free(buf); } } method_parameters *value = gpr_malloc(sizeof(method_parameters)); diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index 507ea26c05..28ad587c0e 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -65,15 +65,15 @@ static void* message_size_limits_create_from_json(const grpc_json* json) { int max_response_message_bytes = -1; for (grpc_json* field = json->child; field != NULL; field = field->next) { if (field->key == NULL) continue; - if (strcmp(field->key, "max_request_message_bytes") == 0) { + if (strcmp(field->key, "maxRequestMessageBytes") == 0) { if (max_request_message_bytes >= 0) return NULL; // Duplicate. - if (field->type != GRPC_JSON_NUMBER) return NULL; - max_request_message_bytes = gpr_parse_nonnegative_number(field->value); + if (field->type != GRPC_JSON_STRING) return NULL; + max_request_message_bytes = gpr_parse_nonnegative_int(field->value); if (max_request_message_bytes == -1) return NULL; - } else if (strcmp(field->key, "max_response_message_bytes") == 0) { + } else if (strcmp(field->key, "maxResponseMessageBytes") == 0) { if (max_response_message_bytes >= 0) return NULL; // Duplicate. - if (field->type != GRPC_JSON_NUMBER) return NULL; - max_response_message_bytes = gpr_parse_nonnegative_number(field->value); + if (field->type != GRPC_JSON_STRING) return NULL; + max_response_message_bytes = gpr_parse_nonnegative_int(field->value); if (max_response_message_bytes == -1) return NULL; } } diff --git a/src/core/lib/support/string.c b/src/core/lib/support/string.c index aa58dd15e3..f10a30f0fd 100644 --- a/src/core/lib/support/string.c +++ b/src/core/lib/support/string.c @@ -191,7 +191,7 @@ int int64_ttoa(int64_t value, char *string) { return i; } -int gpr_parse_nonnegative_number(const char *value) { +int gpr_parse_nonnegative_int(const char *value) { char *end; long result = strtol(value, &end, 0); if (*end != '\0' || result < 0 || result > INT_MAX) return -1; diff --git a/src/core/lib/support/string.h b/src/core/lib/support/string.h index a691ebb2a6..e933e2eb46 100644 --- a/src/core/lib/support/string.h +++ b/src/core/lib/support/string.h @@ -78,7 +78,7 @@ where long is 32bit is size.*/ int int64_ttoa(int64_t value, char *output); // Parses a non-negative number from a value string. Returns -1 on error. -int gpr_parse_nonnegative_number(const char *value); +int gpr_parse_nonnegative_int(const char *value); /* Reverse a run of bytes */ void gpr_reverse_bytes(char *str, int len); diff --git a/src/core/lib/transport/service_config.c b/src/core/lib/transport/service_config.c index a8494cbe8b..2e2b59e3f7 100644 --- a/src/core/lib/transport/service_config.c +++ b/src/core/lib/transport/service_config.c @@ -46,8 +46,8 @@ // JSON form, which will look like this: // // { -// "lb_policy_name": "string", // optional -// "method_config": [ // array of one or more method_config objects +// "loadBalancingPolicy": "string", // optional +// "methodConfig": [ // array of one or more method_config objects // { // "name": [ // array of one or more name objects // { @@ -55,15 +55,13 @@ // "method": "string", // optional // } // ], -// // remaining fields are optional -// "wait_for_ready": bool, -// "timeout": { -// // one or both of these fields may be specified -// "seconds": number, -// "nanos": number, -// }, -// "max_request_message_bytes": number, -// "max_response_message_bytes": number +// // remaining fields are optional. +// // see https://developers.google.com/protocol-buffers/docs/proto3#json +// // for format details. +// "waitForReady": bool, +// "timeout": "duration_string", +// "maxRequestMessageBytes": "int64_string", +// "maxResponseMessageBytes": "int64_string", // } // ] // } @@ -100,7 +98,7 @@ const char* grpc_service_config_get_lb_policy_name( const char* lb_policy_name = NULL; for (grpc_json* field = json->child; field != NULL; field = field->next) { if (field->key == NULL) return NULL; - if (strcmp(field->key, "lb_policy_name") == 0) { + if (strcmp(field->key, "loadBalancingPolicy") == 0) { if (lb_policy_name != NULL) return NULL; // Duplicate. if (field->type != GRPC_JSON_STRING) return NULL; lb_policy_name = field->value; @@ -194,7 +192,7 @@ grpc_mdstr_hash_table* grpc_service_config_create_method_config_table( grpc_mdstr_hash_table_entry* entries = NULL; for (grpc_json* field = json->child; field != NULL; field = field->next) { if (field->key == NULL) return NULL; - if (strcmp(field->key, "method_config") == 0) { + if (strcmp(field->key, "methodConfig") == 0) { if (entries != NULL) return NULL; // Duplicate. if (field->type != GRPC_JSON_ARRAY) return NULL; // Find number of entries. diff --git a/test/core/end2end/connection_refused_test.c b/test/core/end2end/connection_refused_test.c index c14d72f49e..728d6dca59 100644 --- a/test/core/end2end/connection_refused_test.c +++ b/test/core/end2end/connection_refused_test.c @@ -81,11 +81,11 @@ static void run_test(bool wait_for_ready, bool use_service_config) { arg.key = GRPC_ARG_SERVICE_CONFIG; arg.value.string = "{\n" - " \"method_config\": [ {\n" + " \"methodConfig\": [ {\n" " \"name\": [\n" " { \"service\": \"service\", \"method\": \"method\" }\n" " ],\n" - " \"wait_for_ready\": true\n" + " \"waitForReady\": true\n" " } ]\n" "}"; args = grpc_channel_args_copy_and_add(args, &arg, 1); diff --git a/test/core/end2end/tests/cancel_after_accept.c b/test/core/end2end/tests/cancel_after_accept.c index 28d24aa306..e582c59f2d 100644 --- a/test/core/end2end/tests/cancel_after_accept.c +++ b/test/core/end2end/tests/cancel_after_accept.c @@ -139,11 +139,11 @@ static void test_cancel_after_accept(grpc_end2end_test_config config, arg.key = GRPC_ARG_SERVICE_CONFIG; arg.value.string = "{\n" - " \"method_config\": [ {\n" + " \"methodConfig\": [ {\n" " \"name\": [\n" " { \"service\": \"service\", \"method\": \"method\" }\n" " ],\n" - " \"timeout\": { \"seconds\": 5 }\n" + " \"timeout\": \"5s\"\n" " } ]\n" "}"; args = grpc_channel_args_copy_and_add(args, &arg, 1); diff --git a/test/core/end2end/tests/max_message_length.c b/test/core/end2end/tests/max_message_length.c index 90dae50b75..c222d9dfae 100644 --- a/test/core/end2end/tests/max_message_length.c +++ b/test/core/end2end/tests/max_message_length.c @@ -143,11 +143,11 @@ static void test_max_message_length_on_request(grpc_end2end_test_config config, arg.key = GRPC_ARG_SERVICE_CONFIG; arg.value.string = "{\n" - " \"method_config\": [ {\n" + " \"methodConfig\": [ {\n" " \"name\": [\n" " { \"service\": \"service\", \"method\": \"method\" }\n" " ],\n" - " \"max_request_message_bytes\": 5\n" + " \"maxRequestMessageBytes\": \"5\"\n" " } ]\n" "}"; client_args = grpc_channel_args_copy_and_add(NULL, &arg, 1); @@ -317,11 +317,11 @@ static void test_max_message_length_on_response(grpc_end2end_test_config config, arg.key = GRPC_ARG_SERVICE_CONFIG; arg.value.string = "{\n" - " \"method_config\": [ {\n" + " \"methodConfig\": [ {\n" " \"name\": [\n" " { \"service\": \"service\", \"method\": \"method\" }\n" " ],\n" - " \"max_response_message_bytes\": 5\n" + " \"maxResponseMessageBytes\": \"5\"\n" " } ]\n" "}"; client_args = grpc_channel_args_copy_and_add(NULL, &arg, 1); -- cgit v1.2.3 From c19049c39d151150d1947256a8d4a477bea16927 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 10 Nov 2016 09:43:06 -0800 Subject: clang-format --- src/core/ext/client_channel/client_channel.c | 4 ++-- src/core/ext/lb_policy/grpclb/grpclb.c | 4 ++-- src/core/lib/iomgr/tcp_uv.c | 14 ++++++++------ 3 files changed, 12 insertions(+), 10 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index a92a220c74..bbc698abda 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -102,9 +102,9 @@ static void *method_parameters_create_from_json(const grpc_json *json) { if (field->type != GRPC_JSON_STRING) return NULL; size_t len = strlen(field->value); if (field->value[len - 1] != 's') return NULL; - char* buf = gpr_strdup(field->value); + char *buf = gpr_strdup(field->value); buf[len - 1] = '\0'; // Remove trailing 's'. - char* decimal_point = strchr(buf, '.'); + char *decimal_point = strchr(buf, '.'); if (decimal_point != NULL) { *decimal_point = '\0'; timeout.tv_nsec = gpr_parse_nonnegative_int(decimal_point + 1); diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index d68fbb3dfb..44ac9a017e 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -1155,8 +1155,8 @@ static void lb_on_response_received(grpc_exec_ctx *exec_ctx, void *arg, } gpr_mu_unlock(&glb_policy->mu); } else { /* empty payload: call cancelled. */ - /* dispose of the "lb_on_response_received" weak ref taken in - * query_for_backends_locked() and reused in every reception loop */ + /* dispose of the "lb_on_response_received" weak ref taken in + * query_for_backends_locked() and reused in every reception loop */ gpr_mu_unlock(&glb_policy->mu); GRPC_LB_POLICY_WEAK_UNREF(exec_ctx, &glb_policy->base, "lb_on_response_received_empty_payload"); diff --git a/src/core/lib/iomgr/tcp_uv.c b/src/core/lib/iomgr/tcp_uv.c index e690b18f20..068af5187c 100644 --- a/src/core/lib/iomgr/tcp_uv.c +++ b/src/core/lib/iomgr/tcp_uv.c @@ -87,10 +87,12 @@ static void tcp_free(grpc_exec_ctx *exec_ctx, grpc_tcp *tcp) { /*#define GRPC_TCP_REFCOUNT_DEBUG*/ #ifdef GRPC_TCP_REFCOUNT_DEBUG -#define TCP_UNREF(exec_ctx, tcp, reason) tcp_unref((exec_ctx), (tcp), (reason), __FILE__, __LINE__) -#define TCP_REF(tcp, reason) tcp_ref((exec_ctx), (tcp), (reason), __FILE__, __LINE__) -static void tcp_unref(grpc_exec_ctx *exec_ctx, grpc_tcp *tcp, const char *reason, const char *file, - int line) { +#define TCP_UNREF(exec_ctx, tcp, reason) \ + tcp_unref((exec_ctx), (tcp), (reason), __FILE__, __LINE__) +#define TCP_REF(tcp, reason) \ + tcp_ref((exec_ctx), (tcp), (reason), __FILE__, __LINE__) +static void tcp_unref(grpc_exec_ctx *exec_ctx, grpc_tcp *tcp, + const char *reason, const char *file, int line) { gpr_log(file, line, GPR_LOG_SEVERITY_DEBUG, "TCP unref %p : %s %d -> %d", tcp, reason, tcp->refcount.count, tcp->refcount.count - 1); if (gpr_unref(&tcp->refcount)) { @@ -157,7 +159,7 @@ static void read_callback(uv_stream_t *stream, ssize_t nread, grpc_error_free_string(str); for (i = 0; i < tcp->read_slices->count; i++) { char *dump = grpc_dump_slice(tcp->read_slices->slices[i], - GPR_DUMP_HEX | GPR_DUMP_ASCII); + GPR_DUMP_HEX | GPR_DUMP_ASCII); gpr_log(GPR_DEBUG, "READ %p (peer=%s): %s", tcp, tcp->peer_string, dump); gpr_free(dump); @@ -234,7 +236,7 @@ static void uv_endpoint_write(grpc_exec_ctx *exec_ctx, grpc_endpoint *ep, for (j = 0; j < write_slices->count; j++) { char *data = grpc_dump_slice(write_slices->slices[j], - GPR_DUMP_HEX | GPR_DUMP_ASCII); + GPR_DUMP_HEX | GPR_DUMP_ASCII); gpr_log(GPR_DEBUG, "WRITE %p (peer=%s): %s", tcp, tcp->peer_string, data); gpr_free(data); } -- cgit v1.2.3 From 69f783dffcb94625f503a7af35fd460b83c2fd40 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 10 Nov 2016 10:10:56 -0800 Subject: Allow LB policy name to be matched in a case-insensitive manner. --- src/core/ext/client_channel/lb_policy_registry.c | 8 +++++--- test/core/client_channel/lb_policies_test.c | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/client_channel/lb_policy_registry.c b/src/core/ext/client_channel/lb_policy_registry.c index f46a721f9d..90c149d947 100644 --- a/src/core/ext/client_channel/lb_policy_registry.c +++ b/src/core/ext/client_channel/lb_policy_registry.c @@ -35,6 +35,8 @@ #include +#include "src/core/lib/support/string.h" + #define MAX_POLICIES 10 static grpc_lb_policy_factory *g_all_of_the_lb_policies[MAX_POLICIES]; @@ -52,8 +54,8 @@ void grpc_lb_policy_registry_shutdown(void) { void grpc_register_lb_policy(grpc_lb_policy_factory *factory) { int i; for (i = 0; i < g_number_of_lb_policies; i++) { - GPR_ASSERT(0 != strcmp(factory->vtable->name, - g_all_of_the_lb_policies[i]->vtable->name)); + GPR_ASSERT(0 != gpr_stricmp(factory->vtable->name, + g_all_of_the_lb_policies[i]->vtable->name)); } GPR_ASSERT(g_number_of_lb_policies != MAX_POLICIES); grpc_lb_policy_factory_ref(factory); @@ -66,7 +68,7 @@ static grpc_lb_policy_factory *lookup_factory(const char *name) { if (name == NULL) return NULL; for (i = 0; i < g_number_of_lb_policies; i++) { - if (0 == strcmp(name, g_all_of_the_lb_policies[i]->vtable->name)) { + if (0 == gpr_stricmp(name, g_all_of_the_lb_policies[i]->vtable->name)) { return g_all_of_the_lb_policies[i]; } } diff --git a/test/core/client_channel/lb_policies_test.c b/test/core/client_channel/lb_policies_test.c index 3aba906094..b9bd637d35 100644 --- a/test/core/client_channel/lb_policies_test.c +++ b/test/core/client_channel/lb_policies_test.c @@ -554,7 +554,7 @@ static grpc_channel *create_client(const servers_fixture *f) { arg_array[0].value.integer = RETRY_TIMEOUT; arg_array[1].type = GRPC_ARG_STRING; arg_array[1].key = GRPC_ARG_LB_POLICY_NAME; - arg_array[1].value.string = "round_robin"; + arg_array[1].value.string = "ROUND_ROBIN"; args.num_args = 2; args.args = arg_array; @@ -669,7 +669,7 @@ static void test_get_channel_info() { grpc_arg arg; arg.type = GRPC_ARG_STRING; arg.key = GRPC_ARG_SERVICE_CONFIG; - arg.value.string = "{\"lb_policy_name\": \"round_robin\"}"; + arg.value.string = "{\"loadBalancingPolicy\": \"ROUND_ROBIN\"}"; grpc_channel_args *args = grpc_channel_args_copy_and_add(NULL, &arg, 1); channel = grpc_insecure_channel_create("ipv4:127.0.0.1:1234", args, NULL); grpc_channel_args_destroy(args); -- cgit v1.2.3 From 850cbaaac4075d56d76a1d67dafd9f90e5284942 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 15 Nov 2016 15:13:35 -0800 Subject: Added more testing for grpclb tokens --- src/core/ext/lb_policy/grpclb/grpclb.c | 28 +++++++++++++-------- test/cpp/grpclb/grpclb_test.cc | 45 +++++++++++++++++++++++----------- 2 files changed, 49 insertions(+), 24 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index d8ef0c8098..be05be9abe 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -186,14 +186,20 @@ static void wrapped_rr_closure(grpc_exec_ctx *exec_ctx, void *arg, * addresses failed to connect). There won't be any user_data/token * available */ if (wc_arg->target != NULL) { - GPR_ASSERT(wc_arg->lb_token != NULL); - initial_metadata_add_lb_token(wc_arg->initial_metadata, - wc_arg->lb_token_mdelem_storage, - GRPC_MDELEM_REF(wc_arg->lb_token)); + if (wc_arg->lb_token != NULL) { + initial_metadata_add_lb_token(wc_arg->initial_metadata, + wc_arg->lb_token_mdelem_storage, + GRPC_MDELEM_REF(wc_arg->lb_token)); + } else { + gpr_log(GPR_ERROR, + "No LB token for connected subchannel pick %p (from RR " + "instance %p).", + (void *)*wc_arg->target, (void *)wc_arg->rr_policy); + abort(); + } } if (grpc_lb_glb_trace) { - gpr_log(GPR_INFO, "Unreffing RR (0x%" PRIxPTR ")", - (intptr_t)wc_arg->rr_policy); + gpr_log(GPR_INFO, "Unreffing RR %p", (void *)wc_arg->rr_policy); } GRPC_LB_POLICY_UNREF(exec_ctx, wc_arg->rr_policy, "wrapped_rr_closure"); } @@ -411,7 +417,7 @@ static void parse_server(const grpc_grpclb_server *server, } /* Returns addresses extracted from \a serverlist. */ -static grpc_lb_addresses *process_serverlist( +static grpc_lb_addresses *process_serverlist_locked( const grpc_grpclb_serverlist *serverlist) { size_t num_valid = 0; /* first pass: count how many are valid in order to allocate the necessary @@ -451,10 +457,12 @@ static grpc_lb_addresses *process_serverlist( user_data = grpc_mdelem_from_metadata_strings(GRPC_MDSTR_LB_TOKEN, lb_token_mdstr); } else { - gpr_log(GPR_ERROR, + char *uri = grpc_sockaddr_to_uri(&addr); + gpr_log(GPR_INFO, "Missing LB token for backend address '%s'. The empty token will " "be used instead", - grpc_sockaddr_to_uri(&addr)); + uri); + gpr_free(uri); user_data = GRPC_MDELEM_LB_TOKEN_EMPTY; } @@ -508,7 +516,7 @@ static grpc_lb_policy *create_rr_locked( grpc_lb_policy_args args; memset(&args, 0, sizeof(args)); args.client_channel_factory = glb_policy->cc_factory; - grpc_lb_addresses *addresses = process_serverlist(serverlist); + grpc_lb_addresses *addresses = process_serverlist_locked(serverlist); // Replace the LB addresses in the channel args that we pass down to // the subchannel. diff --git a/test/cpp/grpclb/grpclb_test.cc b/test/cpp/grpclb/grpclb_test.cc index 175786332b..217e3375d5 100644 --- a/test/cpp/grpclb/grpclb_test.cc +++ b/test/cpp/grpclb/grpclb_test.cc @@ -108,6 +108,7 @@ typedef struct server_fixture { grpc_completion_queue *cq; char *servers_hostport; int port; + const char *lb_token_prefix; gpr_thd_id tid; int num_calls_serviced; } server_fixture; @@ -123,7 +124,8 @@ static void *tag(intptr_t t) { return (void *)t; } static grpc_slice build_response_payload_slice( const char *host, int *ports, size_t nports, - int64_t expiration_interval_secs, int32_t expiration_interval_nanos) { + int64_t expiration_interval_secs, int32_t expiration_interval_nanos, + const char *token_prefix) { // server_list { // servers { // ip_address: @@ -150,15 +152,19 @@ static grpc_slice build_response_payload_slice( struct in_addr ip4; GPR_ASSERT(inet_pton(AF_INET, host, &ip4) == 1); server->set_ip_address( - grpc::string(reinterpret_cast(&ip4), sizeof(ip4))); + string(reinterpret_cast(&ip4), sizeof(ip4))); server->set_port(ports[i]); - // The following long long int cast is meant to work around the - // disfunctional implementation of std::to_string in gcc 4.4, which doesn't - // have a version for int but does have one for long long int. - string token_data = "token" + std::to_string((long long int)ports[i]); - server->set_load_balance_token(token_data); + // Missing tokens are acceptable. Test that path. + if (strlen(token_prefix) > 0) { + // The following long long int cast is meant to work around the + // disfunctional implementation of std::to_string in gcc 4.4, which + // doesn't have a version for int but does have one for long long int. + string token_data = + token_prefix + std::to_string((long long int)ports[i]); + server->set_load_balance_token(token_data); + } } - const grpc::string &enc_resp = response.SerializeAsString(); + const string &enc_resp = response.SerializeAsString(); return grpc_slice_from_copied_buffer(enc_resp.data(), enc_resp.size()); } @@ -250,14 +256,14 @@ static void start_lb_server(server_fixture *sf, int *ports, size_t nports, for (int i = 0; i < 2; i++) { if (i == 0) { // First half of the ports. - response_payload_slice = - build_response_payload_slice("127.0.0.1", ports, nports / 2, -1, -1); + response_payload_slice = build_response_payload_slice( + "127.0.0.1", ports, nports / 2, -1, -1, sf->lb_token_prefix); } else { // Second half of the ports. sleep_ms(update_delay_ms); - response_payload_slice = - build_response_payload_slice("127.0.0.1", ports + (nports / 2), - (nports + 1) / 2 /* ceil */, -1, -1); + response_payload_slice = build_response_payload_slice( + "127.0.0.1", ports + (nports / 2), (nports + 1) / 2 /* ceil */, -1, + -1, "" /* this half doesn't get to receive an LB token */); } response_payload = grpc_raw_byte_buffer_create(&response_payload_slice, 1); @@ -343,7 +349,10 @@ static void start_backend_server(server_fixture *sf) { // The following long long int cast is meant to work around the // disfunctional implementation of std::to_string in gcc 4.4, which doesn't // have a version for int but does have one for long long int. - string expected_token = "token" + std::to_string((long long int)sf->port); + const string expected_token = + strlen(sf->lb_token_prefix) == 0 + ? "" + : sf->lb_token_prefix + std::to_string((long long int)sf->port); GPR_ASSERT(contains_metadata(&request_metadata_recv, "lb-token", expected_token.c_str())); @@ -626,6 +635,7 @@ static void fork_lb_server(void *arg) { tf->lb_server_update_delay_ms); } +#define LB_TOKEN_PREFIX "token" static test_fixture setup_test_fixture(int lb_server_update_delay_ms) { test_fixture tf; memset(&tf, 0, sizeof(tf)); @@ -635,11 +645,18 @@ static test_fixture setup_test_fixture(int lb_server_update_delay_ms) { gpr_thd_options_set_joinable(&options); for (int i = 0; i < NUM_BACKENDS; ++i) { + // Only the first half of the servers expect an LB token. + if (i < NUM_BACKENDS / 2) { + tf.lb_backends[i].lb_token_prefix = LB_TOKEN_PREFIX; + } else { + tf.lb_backends[i].lb_token_prefix = ""; + } setup_server("127.0.0.1", &tf.lb_backends[i]); gpr_thd_new(&tf.lb_backends[i].tid, fork_backend_server, &tf.lb_backends[i], &options); } + tf.lb_server.lb_token_prefix = LB_TOKEN_PREFIX; setup_server("127.0.0.1", &tf.lb_server); gpr_thd_new(&tf.lb_server.tid, fork_lb_server, &tf.lb_server, &options); -- cgit v1.2.3 From 077da7fa318df97db196621740629e5b910bb325 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 16 Nov 2016 14:55:44 -0800 Subject: Fix merge error --- src/core/lib/channel/http_server_filter.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/channel/http_server_filter.c b/src/core/lib/channel/http_server_filter.c index 80f3fc3cc3..b42ff06039 100644 --- a/src/core/lib/channel/http_server_filter.c +++ b/src/core/lib/channel/http_server_filter.c @@ -37,7 +37,7 @@ #include #include #include "src/core/lib/profiling/timers.h" -#include "src/core/lib/support/percent_encoding.h" +#include "src/core/lib/slice/percent_encoding.h" #include "src/core/lib/transport/static_metadata.h" #define EXPECTED_CONTENT_TYPE "application/grpc" @@ -90,10 +90,10 @@ typedef struct { static grpc_mdelem *server_filter_outgoing_metadata(void *user_data, grpc_mdelem *md) { if (md->key == GRPC_MDSTR_GRPC_MESSAGE) { - gpr_slice pct_encoded_msg = gpr_percent_encode_slice( - md->value->slice, gpr_compatible_percent_encoding_unreserved_bytes); - if (gpr_slice_is_equivalent(pct_encoded_msg, md->value->slice)) { - gpr_slice_unref(pct_encoded_msg); + grpc_slice pct_encoded_msg = grpc_percent_encode_slice( + md->value->slice, grpc_compatible_percent_encoding_unreserved_bytes); + if (grpc_slice_is_equivalent(pct_encoded_msg, md->value->slice)) { + grpc_slice_unref(pct_encoded_msg); return md; } else { return grpc_mdelem_from_metadata_strings( -- cgit v1.2.3 From 4a0e584d50ce735b9583f657feeed2bd24d694d5 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Wed, 16 Nov 2016 23:57:16 -0800 Subject: Undo wrong NULLing in grpclb --- src/core/ext/lb_policy/grpclb/grpclb.c | 1 - 1 file changed, 1 deletion(-) (limited to 'src/core') diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index d8ef0c8098..b7f4698400 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -768,7 +768,6 @@ static void glb_shutdown(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { * while holding glb_policy->mu: lb_on_server_status_received, invoked due to * the cancel, needs to acquire that same lock */ grpc_call *lb_call = glb_policy->lb_call; - glb_policy->lb_call = NULL; gpr_mu_unlock(&glb_policy->mu); /* glb_policy->lb_call and this local lb_call must be consistent at this point -- cgit v1.2.3 From e46de3d416efd6d2f6f69bb3c3803ebaac20bc01 Mon Sep 17 00:00:00 2001 From: yang-g Date: Thu, 17 Nov 2016 15:20:02 -0800 Subject: Expose message limit constants so that users can reference them --- include/grpc/impl/codegen/grpc_types.h | 4 ++++ src/core/lib/channel/message_size_filter.c | 17 +++++++---------- 2 files changed, 11 insertions(+), 10 deletions(-) (limited to 'src/core') diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index 9a86224795..750d9b806e 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -261,6 +261,10 @@ typedef enum grpc_call_error { GRPC_CALL_ERROR_PAYLOAD_TYPE_MISMATCH } grpc_call_error; +/* Default send/receive message size limits in bytes. -1 for unlimited. */ +#define GRPC_DEFAULT_MAX_SEND_MESSAGE_LENGTH -1 +#define GRPC_DEFAULT_MAX_RECV_MESSAGE_LENGTH (4 * 1024 * 1024) + /* Write Flags: */ /** Hint that the write may be buffered and need not go out on the wire immediately. GRPC is free to buffer the message until the next non-buffered diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index 28ad587c0e..1cf68d790d 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -34,6 +34,7 @@ #include #include +#include #include #include #include @@ -42,10 +43,6 @@ #include "src/core/lib/support/string.h" #include "src/core/lib/transport/service_config.h" -#define DEFAULT_MAX_SEND_MESSAGE_LENGTH -1 // Unlimited. -// The protobuf library will (by default) start warning at 100 megs. -#define DEFAULT_MAX_RECV_MESSAGE_LENGTH (4 * 1024 * 1024) - typedef struct message_size_limits { int max_send_size; int max_recv_size; @@ -201,20 +198,20 @@ static void init_channel_elem(grpc_exec_ctx* exec_ctx, GPR_ASSERT(!args->is_last); channel_data* chand = elem->channel_data; memset(chand, 0, sizeof(*chand)); - chand->max_send_size = DEFAULT_MAX_SEND_MESSAGE_LENGTH; - chand->max_recv_size = DEFAULT_MAX_RECV_MESSAGE_LENGTH; + chand->max_send_size = GRPC_DEFAULT_MAX_SEND_MESSAGE_LENGTH; + chand->max_recv_size = GRPC_DEFAULT_MAX_RECV_MESSAGE_LENGTH; for (size_t i = 0; i < args->channel_args->num_args; ++i) { if (strcmp(args->channel_args->args[i].key, GRPC_ARG_MAX_SEND_MESSAGE_LENGTH) == 0) { - const grpc_integer_options options = {DEFAULT_MAX_SEND_MESSAGE_LENGTH, 0, - INT_MAX}; + const grpc_integer_options options = { + GRPC_DEFAULT_MAX_SEND_MESSAGE_LENGTH, 0, INT_MAX}; chand->max_send_size = grpc_channel_arg_get_integer(&args->channel_args->args[i], options); } if (strcmp(args->channel_args->args[i].key, GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH) == 0) { - const grpc_integer_options options = {DEFAULT_MAX_RECV_MESSAGE_LENGTH, 0, - INT_MAX}; + const grpc_integer_options options = { + GRPC_DEFAULT_MAX_RECV_MESSAGE_LENGTH, 0, INT_MAX}; chand->max_recv_size = grpc_channel_arg_get_integer(&args->channel_args->args[i], options); } -- cgit v1.2.3 From 149f09da97393cb95ab715bbd823eb0ddc948c83 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Thu, 17 Nov 2016 20:43:10 -0800 Subject: Rewrote connectivity status logic for gRPC LB --- src/core/ext/lb_policy/grpclb/grpclb.c | 168 ++++++++++++++++++++++++++------- test/cpp/grpclb/grpclb_test.cc | 5 + 2 files changed, 138 insertions(+), 35 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index b7f4698400..e094fdc799 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -467,6 +467,65 @@ static grpc_lb_addresses *process_serverlist( return lb_addresses; } +/* returns true if the new RR policy should replace the current one, if any */ +static bool update_lb_connectivity_status_locked( + grpc_exec_ctx *exec_ctx, glb_lb_policy *glb_policy, + grpc_connectivity_state new_rr_state, grpc_error *new_rr_state_error) { + grpc_error *curr_state_error; + const grpc_connectivity_state curr_glb_state = grpc_connectivity_state_check( + &glb_policy->state_tracker, &curr_state_error); + + /* The new connectivity status is a function of the previous one and the new + * input coming from the status of the RR policy. + * + * old state (grpclb's) + * | + * v || I | C | R | TF | SD | <- new state (RR's) + * ===++====+=====+=====+======+======+ + * I || I | C | R | I | I | + * ---++----+-----+-----+------+------+ + * C || I | C | R | C | C | + * ---++----+-----+-----+------+------+ + * R || I | C | R | R | R | + * ---++----+-----+-----+------+------+ + * TF || I | C | R | TF | TF | + * ---++----+-----+-----+------+------+ + * SD || NA | NA | NA | NA | NA | (*) + * ---++----+-----+-----+------+------+ + * + * In summary, if the new state is TRANSIENT_FAILURE or SHUTDOWN, stick to + * the previous RR instance. + * + * Note that the status is never updated to SHUTDOWN as a result of calling + * this function. Only glb_shutdown() has the power to set that state. + * + * (*) This function mustn't be called during shutting down. */ + GPR_ASSERT(curr_glb_state != GRPC_CHANNEL_SHUTDOWN); + + switch (new_rr_state) { + case GRPC_CHANNEL_TRANSIENT_FAILURE: + case GRPC_CHANNEL_SHUTDOWN: + GPR_ASSERT(new_rr_state_error != GRPC_ERROR_NONE); + return false; /* don't replace the RR policy */ + case GRPC_CHANNEL_INIT: + case GRPC_CHANNEL_IDLE: + case GRPC_CHANNEL_CONNECTING: + case GRPC_CHANNEL_READY: + GPR_ASSERT(new_rr_state_error == GRPC_ERROR_NONE); + } + + if (grpc_lb_glb_trace) { + gpr_log(GPR_INFO, + "Setting grpclb's state to %s from new RR policy %p state.", + grpc_connectivity_state_name(new_rr_state), + (void *)glb_policy->rr_policy); + } + grpc_connectivity_state_set(exec_ctx, &glb_policy->state_tracker, + new_rr_state, GRPC_ERROR_REF(new_rr_state_error), + "update_lb_connectivity_status_locked"); + return true; +} + /* perform a pick over \a rr_policy. Given that a pick can return immediately * (ignoring its completion callback) we need to perform the cleanups this * callback would be otherwise resposible for */ @@ -529,49 +588,81 @@ static void glb_rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error); /* glb_policy->rr_policy may be NULL (initial handover) */ static void rr_handover_locked(grpc_exec_ctx *exec_ctx, - glb_lb_policy *glb_policy, grpc_error *error) { + glb_lb_policy *glb_policy) { GPR_ASSERT(glb_policy->serverlist != NULL && glb_policy->serverlist->num_servers > 0); - if (grpc_lb_glb_trace) { - gpr_log(GPR_INFO, "RR handover. Old RR: %p", (void *)glb_policy->rr_policy); - } - if (glb_policy->rr_policy != NULL) { - /* if we are phasing out an existing RR instance, unref it. */ - GRPC_LB_POLICY_UNREF(exec_ctx, glb_policy->rr_policy, "rr_handover"); - } + if (glb_policy->shutting_down) return; + + grpc_lb_policy *old_rr_policy = glb_policy->rr_policy; glb_policy->rr_policy = create_rr_locked(exec_ctx, glb_policy->serverlist, glb_policy); + if (glb_policy->rr_policy == NULL) { + gpr_log(GPR_ERROR, + "Failure creating a RoundRobin policy for serverlist update with " + "%lu entries. The previous RR instance (%p), if any, will continue " + "to be used. Future updates from the LB will attempt to create new " + "instances.", + (unsigned long)glb_policy->serverlist->num_servers, + (void *)old_rr_policy); + /* restore the old policy */ + glb_policy->rr_policy = old_rr_policy; + return; + } + + grpc_error *new_rr_state_error = NULL; + const grpc_connectivity_state new_rr_state = + grpc_lb_policy_check_connectivity(exec_ctx, glb_policy->rr_policy, + &new_rr_state_error); + /* Connectivity state is a function of the new RR policy just created */ + const bool replace_old_rr = update_lb_connectivity_status_locked( + exec_ctx, glb_policy, new_rr_state, new_rr_state_error); + + if (!replace_old_rr) { + /* dispose of the new RR policy that won't be used after all */ + GRPC_LB_POLICY_UNREF(exec_ctx, glb_policy->rr_policy, + "rr_handover_no_replace"); + /* restore the old policy */ + glb_policy->rr_policy = old_rr_policy; + return; + } + if (grpc_lb_glb_trace) { gpr_log(GPR_INFO, "Created RR policy (%p)", (void *)glb_policy->rr_policy); + gpr_log(GPR_INFO, "RR handover. Old RR: %p", (void *)glb_policy->rr_policy); } - GPR_ASSERT(glb_policy->rr_policy != NULL); + if (old_rr_policy != NULL) { + /* if we are phasing out an existing RR instance, unref it. */ + GRPC_LB_POLICY_UNREF(exec_ctx, old_rr_policy, "rr_handover"); + } + + /* Add the gRPC LB's interested_parties pollset_set to that of the newly + * created RR policy. This will make the RR policy progress upon activity on + * gRPC LB, which in turn is tied to the application's call */ grpc_pollset_set_add_pollset_set(exec_ctx, glb_policy->rr_policy->interested_parties, glb_policy->base.interested_parties); + /* Allocate the data for the tracking of the new RR policy's connectivity. + * It'll be deallocated in glb_rr_connectivity_changed() */ rr_connectivity_data *rr_connectivity = gpr_malloc(sizeof(rr_connectivity_data)); memset(rr_connectivity, 0, sizeof(rr_connectivity_data)); grpc_closure_init(&rr_connectivity->on_change, glb_rr_connectivity_changed, rr_connectivity); rr_connectivity->glb_policy = glb_policy; - rr_connectivity->state = grpc_lb_policy_check_connectivity( - exec_ctx, glb_policy->rr_policy, &error); + rr_connectivity->state = new_rr_state; - grpc_connectivity_state_set(exec_ctx, &glb_policy->state_tracker, - rr_connectivity->state, GRPC_ERROR_REF(error), - "rr_handover"); - /* subscribe */ + /* Subscribe to changes to the connectivity of the new RR */ GRPC_LB_POLICY_WEAK_REF(&glb_policy->base, "rr_connectivity_cb"); grpc_lb_policy_notify_on_state_change(exec_ctx, glb_policy->rr_policy, &rr_connectivity->state, &rr_connectivity->on_change); grpc_lb_policy_exit_idle(exec_ctx, glb_policy->rr_policy); - /* flush pending ops */ + /* Update picks and pings in wait */ pending_pick *pp; while ((pp = glb_policy->pending_picks)) { glb_policy->pending_picks = pp->next; @@ -602,28 +693,35 @@ static void rr_handover_locked(grpc_exec_ctx *exec_ctx, static void glb_rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { - /* If shutdown or error free the arg. Rely on the rest of the code to set the - * right grpclb status. */ - rr_connectivity_data *rr_conn_data = arg; - glb_lb_policy *glb_policy = rr_conn_data->glb_policy; - gpr_mu_lock(&glb_policy->mu); + rr_connectivity_data *rr_connectivity = arg; + glb_lb_policy *glb_policy = rr_connectivity->glb_policy; - if (rr_conn_data->state != GRPC_CHANNEL_SHUTDOWN && - !glb_policy->shutting_down) { - /* RR not shutting down. Mimic the RR's policy state */ - grpc_connectivity_state_set(exec_ctx, &glb_policy->state_tracker, - rr_conn_data->state, GRPC_ERROR_REF(error), - "rr_connectivity_cb"); - /* resubscribe. Reuse the "rr_connectivity_cb" weak ref. */ + gpr_mu_lock(&glb_policy->mu); + const bool shutting_down = glb_policy->shutting_down; + grpc_lb_policy *maybe_unref = NULL; + GRPC_ERROR_REF(error); + + if (rr_connectivity->state == GRPC_CHANNEL_SHUTDOWN || shutting_down) { + /* RR policy shutting down. Don't renew subscription and free the arg of + * this callback. In addition we need to stash away the current policy to + * be UNREF'd after releasing the lock. Otherwise, if the UNREF is the last + * one, the policy would be destroyed, alongside the lock, which would + * result in a use-after-free */ + maybe_unref = &glb_policy->base; + gpr_free(rr_connectivity); + } else { /* rr state != SHUTDOWN && !shutting down: biz as usual */ + update_lb_connectivity_status_locked(exec_ctx, glb_policy, + rr_connectivity->state, error); + /* Resubscribe. Reuse the "rr_connectivity_cb" weak ref. */ grpc_lb_policy_notify_on_state_change(exec_ctx, glb_policy->rr_policy, - &rr_conn_data->state, - &rr_conn_data->on_change); - } else { - GRPC_LB_POLICY_WEAK_UNREF(exec_ctx, &glb_policy->base, - "rr_connectivity_cb"); - gpr_free(rr_conn_data); + &rr_connectivity->state, + &rr_connectivity->on_change); } gpr_mu_unlock(&glb_policy->mu); + if (maybe_unref != NULL) { + GRPC_LB_POLICY_WEAK_UNREF(exec_ctx, maybe_unref, "rr_connectivity_cb"); + } + GRPC_ERROR_UNREF(error); } static grpc_lb_policy *glb_create(grpc_exec_ctx *exec_ctx, @@ -1133,7 +1231,7 @@ static void lb_on_response_received(grpc_exec_ctx *exec_ctx, void *arg, /* and update the copy in the glb_lb_policy instance */ glb_policy->serverlist = serverlist; - rr_handover_locked(exec_ctx, glb_policy, error); + rr_handover_locked(exec_ctx, glb_policy); } } else { if (grpc_lb_glb_trace) { diff --git a/test/cpp/grpclb/grpclb_test.cc b/test/cpp/grpclb/grpclb_test.cc index 175786332b..61309805db 100644 --- a/test/cpp/grpclb/grpclb_test.cc +++ b/test/cpp/grpclb/grpclb_test.cc @@ -79,6 +79,9 @@ extern "C" { // - Test against a non-LB server. // - Random LB server closing the stream unexpectedly. // - Test using DNS-resolvable names (localhost?) +// - Test handling of creation of faulty RR instance by having the LB return a +// serverlist with non-existent backends after having initially returned a +// valid one. // // Findings from end to end testing to be covered here: // - Handling of LB servers restart, including reconnection after backing-off @@ -521,6 +524,8 @@ static void perform_request(client_fixture *cf) { CQ_EXPECT_COMPLETION(cqv, tag(2), 1); cq_verify(cqv); GPR_ASSERT(byte_buffer_eq_string(response_payload_recv, PAYLOAD)); + GPR_ASSERT(grpc_channel_check_connectivity_state( + cf->client, 0 /* try to connect */) == GRPC_CHANNEL_READY); grpc_byte_buffer_destroy(request_payload); grpc_byte_buffer_destroy(response_payload_recv); -- cgit v1.2.3 From e7d2f21d6b2670b7e7275b9842760aec582ad02a Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Thu, 17 Nov 2016 22:04:22 -0800 Subject: improved logging --- src/core/ext/lb_policy/grpclb/grpclb.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index 5a93aa6b57..d16489fbc9 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -633,12 +633,19 @@ static void rr_handover_locked(grpc_exec_ctx *exec_ctx, "rr_handover_no_replace"); /* restore the old policy */ glb_policy->rr_policy = old_rr_policy; + if (grpc_lb_glb_trace) { + gpr_log(GPR_INFO, + "Keeping old RR policy (%p) despite new serverlist: new RR " + "policy was in %s connectivity state.", + (void *)old_rr_policy, + grpc_connectivity_state_name(new_rr_state)); + } return; } if (grpc_lb_glb_trace) { - gpr_log(GPR_INFO, "Created RR policy (%p)", (void *)glb_policy->rr_policy); - gpr_log(GPR_INFO, "RR handover. Old RR: %p", (void *)glb_policy->rr_policy); + gpr_log(GPR_INFO, "Created RR policy (%p) to replace old RR (%p)", + (void *)glb_policy->rr_policy, (void *)old_rr_policy); } if (old_rr_policy != NULL) { -- cgit v1.2.3 From 4283a2648427ed3a095d9d36172eefd858781c4d Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Fri, 18 Nov 2016 10:43:56 -0800 Subject: pr comments --- src/core/ext/lb_policy/grpclb/grpclb.c | 50 +++++++++++++++++----------------- 1 file changed, 25 insertions(+), 25 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index d16489fbc9..0829e05b43 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -486,21 +486,24 @@ static bool update_lb_connectivity_status_locked( /* The new connectivity status is a function of the previous one and the new * input coming from the status of the RR policy. * - * old state (grpclb's) + * current state (grpclb's) * | * v || I | C | R | TF | SD | <- new state (RR's) * ===++====+=====+=====+======+======+ - * I || I | C | R | I | I | + * I || I | C | R | [I] | [I] | * ---++----+-----+-----+------+------+ - * C || I | C | R | C | C | + * C || I | C | R | [C] | [C] | * ---++----+-----+-----+------+------+ - * R || I | C | R | R | R | + * R || I | C | R | [R] | [R] | * ---++----+-----+-----+------+------+ - * TF || I | C | R | TF | TF | + * TF || I | C | R | [TF] | [TF] | * ---++----+-----+-----+------+------+ * SD || NA | NA | NA | NA | NA | (*) * ---++----+-----+-----+------+------+ * + * A [STATE] indicates that the old RR policy is kept. In those cases, STATE + * is the current state of grpclb, which is left untouched. + * * In summary, if the new state is TRANSIENT_FAILURE or SHUTDOWN, stick to * the previous RR instance. * @@ -602,26 +605,22 @@ static void rr_handover_locked(grpc_exec_ctx *exec_ctx, if (glb_policy->shutting_down) return; - grpc_lb_policy *old_rr_policy = glb_policy->rr_policy; - - glb_policy->rr_policy = + grpc_lb_policy *new_rr_policy = create_rr_locked(exec_ctx, glb_policy->serverlist, glb_policy); - if (glb_policy->rr_policy == NULL) { + if (new_rr_policy == NULL) { gpr_log(GPR_ERROR, "Failure creating a RoundRobin policy for serverlist update with " "%lu entries. The previous RR instance (%p), if any, will continue " "to be used. Future updates from the LB will attempt to create new " "instances.", (unsigned long)glb_policy->serverlist->num_servers, - (void *)old_rr_policy); - /* restore the old policy */ - glb_policy->rr_policy = old_rr_policy; + (void *)glb_policy->rr_policy); return; } grpc_error *new_rr_state_error = NULL; const grpc_connectivity_state new_rr_state = - grpc_lb_policy_check_connectivity(exec_ctx, glb_policy->rr_policy, + grpc_lb_policy_check_connectivity(exec_ctx, new_rr_policy, &new_rr_state_error); /* Connectivity state is a function of the new RR policy just created */ const bool replace_old_rr = update_lb_connectivity_status_locked( @@ -629,15 +628,12 @@ static void rr_handover_locked(grpc_exec_ctx *exec_ctx, if (!replace_old_rr) { /* dispose of the new RR policy that won't be used after all */ - GRPC_LB_POLICY_UNREF(exec_ctx, glb_policy->rr_policy, - "rr_handover_no_replace"); - /* restore the old policy */ - glb_policy->rr_policy = old_rr_policy; + GRPC_LB_POLICY_UNREF(exec_ctx, new_rr_policy, "rr_handover_no_replace"); if (grpc_lb_glb_trace) { gpr_log(GPR_INFO, "Keeping old RR policy (%p) despite new serverlist: new RR " "policy was in %s connectivity state.", - (void *)old_rr_policy, + (void *)glb_policy->rr_policy, grpc_connectivity_state_name(new_rr_state)); } return; @@ -645,14 +641,17 @@ static void rr_handover_locked(grpc_exec_ctx *exec_ctx, if (grpc_lb_glb_trace) { gpr_log(GPR_INFO, "Created RR policy (%p) to replace old RR (%p)", - (void *)glb_policy->rr_policy, (void *)old_rr_policy); + (void *)new_rr_policy, (void *)glb_policy->rr_policy); } - if (old_rr_policy != NULL) { + if (glb_policy->rr_policy != NULL) { /* if we are phasing out an existing RR instance, unref it. */ - GRPC_LB_POLICY_UNREF(exec_ctx, old_rr_policy, "rr_handover"); + GRPC_LB_POLICY_UNREF(exec_ctx, glb_policy->rr_policy, "rr_handover"); } + /* Finally update the RR policy to the newly created one */ + glb_policy->rr_policy = new_rr_policy; + /* Add the gRPC LB's interested_parties pollset_set to that of the newly * created RR policy. This will make the RR policy progress upon activity on * gRPC LB, which in turn is tied to the application's call */ @@ -713,7 +712,7 @@ static void glb_rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, gpr_mu_lock(&glb_policy->mu); const bool shutting_down = glb_policy->shutting_down; - grpc_lb_policy *maybe_unref = NULL; + bool unref_needed = false; GRPC_ERROR_REF(error); if (rr_connectivity->state == GRPC_CHANNEL_SHUTDOWN || shutting_down) { @@ -722,7 +721,7 @@ static void glb_rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, * be UNREF'd after releasing the lock. Otherwise, if the UNREF is the last * one, the policy would be destroyed, alongside the lock, which would * result in a use-after-free */ - maybe_unref = &glb_policy->base; + unref_needed = true; gpr_free(rr_connectivity); } else { /* rr state != SHUTDOWN && !shutting down: biz as usual */ update_lb_connectivity_status_locked(exec_ctx, glb_policy, @@ -733,8 +732,9 @@ static void glb_rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, &rr_connectivity->on_change); } gpr_mu_unlock(&glb_policy->mu); - if (maybe_unref != NULL) { - GRPC_LB_POLICY_WEAK_UNREF(exec_ctx, maybe_unref, "rr_connectivity_cb"); + if (unref_needed) { + GRPC_LB_POLICY_WEAK_UNREF(exec_ctx, &glb_policy->base, + "rr_connectivity_cb"); } GRPC_ERROR_UNREF(error); } -- cgit v1.2.3 From 19badff717ef3a7858486737af6265d86e9c1bcb Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Sun, 20 Nov 2016 20:17:46 -0800 Subject: tcp_client_posix: Don't overwrite error descr. --- src/core/lib/iomgr/tcp_client_posix.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/tcp_client_posix.c b/src/core/lib/iomgr/tcp_client_posix.c index 13347735df..a3a70a8ed7 100644 --- a/src/core/lib/iomgr/tcp_client_posix.c +++ b/src/core/lib/iomgr/tcp_client_posix.c @@ -251,8 +251,11 @@ finish: done = (--ac->refs == 0); gpr_mu_unlock(&ac->mu); if (error != GRPC_ERROR_NONE) { - error = grpc_error_set_str(error, GRPC_ERROR_STR_DESCRIPTION, - "Failed to connect to remote host"); + char *error_descr; + gpr_asprintf(&error_descr, "Failed to connect to remote host: %s", + grpc_error_get_str(error, GRPC_ERROR_STR_DESCRIPTION)); + error = grpc_error_set_str(error, GRPC_ERROR_STR_DESCRIPTION, error_descr); + gpr_free(error_descr); error = grpc_error_set_str(error, GRPC_ERROR_STR_TARGET_ADDRESS, ac->addr_str); } -- cgit v1.2.3 From 8fb73d22f157bc6c95442ff7407423f8cd5fa882 Mon Sep 17 00:00:00 2001 From: yang-g Date: Mon, 21 Nov 2016 16:29:58 -0800 Subject: Pretty print outgoing headers --- .../ext/transport/chttp2/transport/chttp2_transport.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) (limited to 'src/core') diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index 127e1cdc13..4e3c7ff681 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -954,6 +954,16 @@ static void complete_fetch(grpc_exec_ctx *exec_ctx, void *gs, static void do_nothing(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) {} +static void log_metadata(const grpc_metadata_batch *md_batch, uint32_t id, + bool is_client, bool is_initial) { + for (grpc_linked_mdelem *md = md_batch->list.head; md != md_batch->list.tail; + md = md->next) { + gpr_log(GPR_INFO, "HTTP:%d:%s:%s: %s: %s", id, is_initial ? "HDR" : "TRL", + is_client ? "CLI" : "SVR", grpc_mdstr_as_c_string(md->md->key), + grpc_mdstr_as_c_string(md->md->value)); + } +} + static void perform_stream_op_locked(grpc_exec_ctx *exec_ctx, void *stream_op, grpc_error *error_ignored) { GPR_TIMER_BEGIN("perform_stream_op_locked", 0); @@ -967,6 +977,12 @@ static void perform_stream_op_locked(grpc_exec_ctx *exec_ctx, void *stream_op, gpr_log(GPR_DEBUG, "perform_stream_op_locked: %s; on_complete = %p", str, op->on_complete); gpr_free(str); + if (op->send_initial_metadata) { + log_metadata(op->send_initial_metadata, s->id, t->is_client, true); + } + if (op->send_trailing_metadata) { + log_metadata(op->send_trailing_metadata, s->id, t->is_client, false); + } } grpc_closure *on_complete = op->on_complete; -- cgit v1.2.3 From 1ebcaa2f547d4f38319c4125380dc4ee93a3f3f2 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Mon, 21 Nov 2016 21:52:47 -0800 Subject: Fixed leak upon duped serverlist updates --- src/core/ext/lb_policy/grpclb/grpclb.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'src/core') diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index 0829e05b43..6132ccaf78 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -1238,12 +1238,15 @@ static void lb_on_response_received(grpc_exec_ctx *exec_ctx, void *arg, gpr_log(GPR_INFO, "Incoming server list identical to current, ignoring."); } + grpc_grpclb_destroy_serverlist(serverlist); } else { /* new serverlist */ if (glb_policy->serverlist != NULL) { /* dispose of the old serverlist */ grpc_grpclb_destroy_serverlist(glb_policy->serverlist); } - /* and update the copy in the glb_lb_policy instance */ + /* and update the copy in the glb_lb_policy instance. This serverlist + * instance will be destroyed either upon the next update or in + * glb_destroy() */ glb_policy->serverlist = serverlist; rr_handover_locked(exec_ctx, glb_policy); -- cgit v1.2.3 From 6f8507f2b07e0da7d8ffb30d4150a1f2e84ca581 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Tue, 22 Nov 2016 15:13:34 +0100 Subject: UserResource does not reference tcp endpoint on windows --- src/core/lib/iomgr/tcp_windows.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/tcp_windows.c b/src/core/lib/iomgr/tcp_windows.c index 62afbcef51..d4613b674e 100644 --- a/src/core/lib/iomgr/tcp_windows.c +++ b/src/core/lib/iomgr/tcp_windows.c @@ -423,7 +423,7 @@ grpc_endpoint *grpc_tcp_create(grpc_winsocket *socket, tcp->base.vtable = &vtable; tcp->socket = socket; gpr_mu_init(&tcp->mu); - gpr_ref_init(&tcp->refcount, 2); + gpr_ref_init(&tcp->refcount, 1); grpc_closure_init(&tcp->on_read, on_read, tcp); grpc_closure_init(&tcp->on_write, on_write, tcp); tcp->peer_string = gpr_strdup(peer_string); -- cgit v1.2.3 From 6493a7388838e7674ff13ddade2595e780bb3102 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 22 Nov 2016 10:25:52 -0800 Subject: Check on *target, not target --- src/core/ext/lb_policy/grpclb/grpclb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index 0829e05b43..658b55551f 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -182,10 +182,10 @@ static void wrapped_rr_closure(grpc_exec_ctx *exec_ctx, void *arg, NULL); if (wc_arg->rr_policy != NULL) { - /* if target is NULL, no pick has been made by the RR policy (eg, all + /* if *target is NULL, no pick has been made by the RR policy (eg, all * addresses failed to connect). There won't be any user_data/token * available */ - if (wc_arg->target != NULL) { + if (*wc_arg->target != NULL) { if (wc_arg->lb_token != NULL) { initial_metadata_add_lb_token(wc_arg->initial_metadata, wc_arg->lb_token_mdelem_storage, -- cgit v1.2.3 From d255a727f9638ce97e3b97f70e7ec54a6e9ce74f Mon Sep 17 00:00:00 2001 From: yang-g Date: Wed, 23 Nov 2016 13:10:44 -0800 Subject: Clang-format --- src/core/lib/iomgr/ev_poll_posix.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_poll_posix.c b/src/core/lib/iomgr/ev_poll_posix.c index 27083ca6d0..8f5d9d43fb 100644 --- a/src/core/lib/iomgr/ev_poll_posix.c +++ b/src/core/lib/iomgr/ev_poll_posix.c @@ -1361,7 +1361,7 @@ static int cvfd_poll(struct pollfd *fds, nfds_t nfds, int timeout) { g_cvfds.cvfds[idx].cvs = cvn; // Don't bother polling if a wakeup fd is ready if (g_cvfds.cvfds[idx].is_set) { - skip_poll=1; + skip_poll = 1; } } else if (fds[i].fd >= 0) { nsockfds++; -- cgit v1.2.3 From b4b8e1efc62c473c18fdf30b647fe850f97fec74 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 28 Nov 2016 07:33:13 -0800 Subject: Fix mac build Localize global_wakeup_fd declarations, instead of trying to share them, so that this bug is less likely to occur in the future. --- src/core/lib/iomgr/ev_epoll_linux.c | 26 +++++++++++++------------- src/core/lib/iomgr/ev_poll_posix.c | 12 +++++++----- src/core/lib/iomgr/ev_posix.h | 1 - 3 files changed, 20 insertions(+), 19 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index 91041a7c28..48ab5bf826 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -72,6 +72,11 @@ static int grpc_polling_trace = 0; /* Disabled by default */ static int grpc_wakeup_signal = -1; static bool is_grpc_wakeup_signal_initialized = false; +/* TODO: sreek: Right now, this wakes up all pollers. In future we should make + * sure to wake up one polling thread (which can wake up other threads if + * needed) */ +static grpc_wakeup_fd global_wakeup_fd; + /* Implements the function defined in grpc_posix.h. This function might be * called before even calling grpc_init() to set either a different signal to * use. If signum == -1, then the use of signals is disabled */ @@ -438,7 +443,7 @@ static void polling_island_add_wakeup_fd_locked(polling_island *pi, "epoll_ctl (epoll_fd: %d) add wakeup fd: %d failed with " "error: %d (%s)", pi->epoll_fd, - GRPC_WAKEUP_FD_GET_READ_FD(&grpc_global_wakeup_fd), errno, + GRPC_WAKEUP_FD_GET_READ_FD(&global_wakeup_fd), errno, strerror(errno)); append_error(error, GRPC_OS_ERROR(errno, err_msg), err_desc); gpr_free(err_msg); @@ -541,7 +546,7 @@ static polling_island *polling_island_create(grpc_exec_ctx *exec_ctx, goto done; } - polling_island_add_wakeup_fd_locked(pi, &grpc_global_wakeup_fd, error); + polling_island_add_wakeup_fd_locked(pi, &global_wakeup_fd, error); polling_island_add_wakeup_fd_locked(pi, &pi->workqueue_wakeup_fd, error); if (initial_fd != NULL) { @@ -843,11 +848,6 @@ static void polling_island_global_shutdown() { * alarm 'epoch'). This wakeup_fd gives us something to alert on when such a * case occurs. */ -/* TODO: sreek: Right now, this wakes up all pollers. In future we should make - * sure to wake up one polling thread (which can wake up other threads if - * needed) */ -grpc_wakeup_fd grpc_global_wakeup_fd; - static grpc_fd *fd_freelist = NULL; static gpr_mu fd_freelist_mu; @@ -1163,11 +1163,11 @@ static grpc_error *pollset_global_init(void) { gpr_tls_init(&g_current_thread_pollset); gpr_tls_init(&g_current_thread_worker); poller_kick_init(); - return grpc_wakeup_fd_init(&grpc_global_wakeup_fd); + return grpc_wakeup_fd_init(&global_wakeup_fd); } static void pollset_global_shutdown(void) { - grpc_wakeup_fd_destroy(&grpc_global_wakeup_fd); + grpc_wakeup_fd_destroy(&global_wakeup_fd); gpr_tls_destroy(&g_current_thread_pollset); gpr_tls_destroy(&g_current_thread_worker); } @@ -1274,7 +1274,7 @@ static grpc_error *pollset_kick(grpc_pollset *p, } static grpc_error *kick_poller(void) { - return grpc_wakeup_fd_wakeup(&grpc_global_wakeup_fd); + return grpc_wakeup_fd_wakeup(&global_wakeup_fd); } static void pollset_init(grpc_pollset *pollset, gpr_mu **mu) { @@ -1501,13 +1501,13 @@ static void pollset_work_and_unlock(grpc_exec_ctx *exec_ctx, for (int i = 0; i < ep_rv; ++i) { void *data_ptr = ep_ev[i].data.ptr; - if (data_ptr == &grpc_global_wakeup_fd) { + if (data_ptr == &global_wakeup_fd) { append_error(error, - grpc_wakeup_fd_consume_wakeup(&grpc_global_wakeup_fd), + grpc_wakeup_fd_consume_wakeup(&global_wakeup_fd), err_desc); } else if (data_ptr == &pi->workqueue_wakeup_fd) { append_error(error, - grpc_wakeup_fd_consume_wakeup(&grpc_global_wakeup_fd), + grpc_wakeup_fd_consume_wakeup(&global_wakeup_fd), err_desc); maybe_do_workqueue_work(exec_ctx, pi); } else if (data_ptr == &polling_island_wakeup_fd) { diff --git a/src/core/lib/iomgr/ev_poll_posix.c b/src/core/lib/iomgr/ev_poll_posix.c index 8f5d9d43fb..f9d8332686 100644 --- a/src/core/lib/iomgr/ev_poll_posix.c +++ b/src/core/lib/iomgr/ev_poll_posix.c @@ -120,6 +120,8 @@ struct grpc_fd { grpc_pollset *read_notifier_pollset; }; +static grpc_wakeup_fd global_wakeup_fd; + /* Begin polling on an fd. Registers that the given pollset is interested in this fd - so that if read or writability interest changes, the pollset can be kicked to pick up that @@ -769,17 +771,17 @@ static grpc_error *pollset_kick(grpc_pollset *p, static grpc_error *pollset_global_init(void) { gpr_tls_init(&g_current_thread_poller); gpr_tls_init(&g_current_thread_worker); - return grpc_wakeup_fd_init(&grpc_global_wakeup_fd); + return grpc_wakeup_fd_init(&global_wakeup_fd); } static void pollset_global_shutdown(void) { - grpc_wakeup_fd_destroy(&grpc_global_wakeup_fd); + grpc_wakeup_fd_destroy(&global_wakeup_fd); gpr_tls_destroy(&g_current_thread_poller); gpr_tls_destroy(&g_current_thread_worker); } static grpc_error *kick_poller(void) { - return grpc_wakeup_fd_wakeup(&grpc_global_wakeup_fd); + return grpc_wakeup_fd_wakeup(&global_wakeup_fd); } /* main interface */ @@ -947,7 +949,7 @@ static grpc_error *pollset_work(grpc_exec_ctx *exec_ctx, grpc_pollset *pollset, fd_count = 0; pfd_count = 2; - pfds[0].fd = GRPC_WAKEUP_FD_GET_READ_FD(&grpc_global_wakeup_fd); + pfds[0].fd = GRPC_WAKEUP_FD_GET_READ_FD(&global_wakeup_fd); pfds[0].events = POLLIN; pfds[0].revents = 0; pfds[1].fd = GRPC_WAKEUP_FD_GET_READ_FD(&worker.wakeup_fd->fd); @@ -1002,7 +1004,7 @@ static grpc_error *pollset_work(grpc_exec_ctx *exec_ctx, grpc_pollset *pollset, } else { if (pfds[0].revents & POLLIN_CHECK) { work_combine_error( - &error, grpc_wakeup_fd_consume_wakeup(&grpc_global_wakeup_fd)); + &error, grpc_wakeup_fd_consume_wakeup(&global_wakeup_fd)); } if (pfds[1].revents & POLLIN_CHECK) { work_combine_error( diff --git a/src/core/lib/iomgr/ev_posix.h b/src/core/lib/iomgr/ev_posix.h index 2fdef06838..cb5832539d 100644 --- a/src/core/lib/iomgr/ev_posix.h +++ b/src/core/lib/iomgr/ev_posix.h @@ -183,6 +183,5 @@ void grpc_pollset_set_del_fd(grpc_exec_ctx *exec_ctx, /* override to allow tests to hook poll() usage */ typedef int (*grpc_poll_function_type)(struct pollfd *, nfds_t, int); extern grpc_poll_function_type grpc_poll_function; -extern grpc_wakeup_fd grpc_global_wakeup_fd; #endif /* GRPC_CORE_LIB_IOMGR_EV_POSIX_H */ -- cgit v1.2.3 From 1fa9ddb12ec9aafd37bb51910e1b9c6f27094abf Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 28 Nov 2016 08:19:37 -0800 Subject: Fix clang-format --- src/core/lib/iomgr/ev_epoll_linux.c | 11 ++++------- src/core/lib/iomgr/ev_poll_posix.c | 4 ++-- 2 files changed, 6 insertions(+), 9 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index 48ab5bf826..07fbfd849e 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -442,9 +442,8 @@ static void polling_island_add_wakeup_fd_locked(polling_island *pi, gpr_asprintf(&err_msg, "epoll_ctl (epoll_fd: %d) add wakeup fd: %d failed with " "error: %d (%s)", - pi->epoll_fd, - GRPC_WAKEUP_FD_GET_READ_FD(&global_wakeup_fd), errno, - strerror(errno)); + pi->epoll_fd, GRPC_WAKEUP_FD_GET_READ_FD(&global_wakeup_fd), + errno, strerror(errno)); append_error(error, GRPC_OS_ERROR(errno, err_msg), err_desc); gpr_free(err_msg); } @@ -1502,12 +1501,10 @@ static void pollset_work_and_unlock(grpc_exec_ctx *exec_ctx, for (int i = 0; i < ep_rv; ++i) { void *data_ptr = ep_ev[i].data.ptr; if (data_ptr == &global_wakeup_fd) { - append_error(error, - grpc_wakeup_fd_consume_wakeup(&global_wakeup_fd), + append_error(error, grpc_wakeup_fd_consume_wakeup(&global_wakeup_fd), err_desc); } else if (data_ptr == &pi->workqueue_wakeup_fd) { - append_error(error, - grpc_wakeup_fd_consume_wakeup(&global_wakeup_fd), + append_error(error, grpc_wakeup_fd_consume_wakeup(&global_wakeup_fd), err_desc); maybe_do_workqueue_work(exec_ctx, pi); } else if (data_ptr == &polling_island_wakeup_fd) { diff --git a/src/core/lib/iomgr/ev_poll_posix.c b/src/core/lib/iomgr/ev_poll_posix.c index f9d8332686..21b28e5554 100644 --- a/src/core/lib/iomgr/ev_poll_posix.c +++ b/src/core/lib/iomgr/ev_poll_posix.c @@ -1003,8 +1003,8 @@ static grpc_error *pollset_work(grpc_exec_ctx *exec_ctx, grpc_pollset *pollset, } } else { if (pfds[0].revents & POLLIN_CHECK) { - work_combine_error( - &error, grpc_wakeup_fd_consume_wakeup(&global_wakeup_fd)); + work_combine_error(&error, + grpc_wakeup_fd_consume_wakeup(&global_wakeup_fd)); } if (pfds[1].revents & POLLIN_CHECK) { work_combine_error( -- cgit v1.2.3 From 491d97546c54ad1192968bbf8505d0db6e3b983e Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 28 Nov 2016 10:39:00 -0800 Subject: Fix subprocess code to avoid redundant calls to waitpid(). --- src/core/lib/support/subprocess_posix.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'src/core') diff --git a/src/core/lib/support/subprocess_posix.c b/src/core/lib/support/subprocess_posix.c index 4f4de9298e..daf371d03e 100644 --- a/src/core/lib/support/subprocess_posix.c +++ b/src/core/lib/support/subprocess_posix.c @@ -40,6 +40,7 @@ #include #include #include +#include #include #include #include @@ -52,7 +53,7 @@ struct gpr_subprocess { int pid; - int joined; + bool joined; }; const char *gpr_subprocess_binary_extension() { return ""; } @@ -100,6 +101,7 @@ retry: gpr_log(GPR_ERROR, "waitpid failed: %s", strerror(errno)); return -1; } + p->joined = true; return status; } -- cgit v1.2.3 From a540848c20b556f6827fd8c9b23f5313df6b1d18 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Mon, 28 Nov 2016 13:46:19 -0800 Subject: Fixed http_client race --- src/core/lib/channel/http_client_filter.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/channel/http_client_filter.c b/src/core/lib/channel/http_client_filter.c index dbe0d25211..fd8b46afcb 100644 --- a/src/core/lib/channel/http_client_filter.c +++ b/src/core/lib/channel/http_client_filter.c @@ -245,12 +245,15 @@ static void hc_mutate_op(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, message, and the payload is below the size threshold, and all the data for this request is immediately available. */ grpc_mdelem *method = GRPC_MDELEM_METHOD_POST; - calld->send_message_blocked = false; if ((op->send_initial_metadata_flags & GRPC_INITIAL_METADATA_CACHEABLE_REQUEST) && op->send_message != NULL && op->send_message->length < channeld->max_payload_size_for_get) { method = GRPC_MDELEM_METHOD_GET; + /* The following write to calld->send_message_blocked isn't racy with + reads in hc_start_transport_op (which deals with SEND_MESSAGE ops) because + being here means ops->send_message is not NULL, which is primarily + guarding the read there. */ calld->send_message_blocked = true; } else if (op->send_initial_metadata_flags & GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST) { @@ -331,8 +334,7 @@ static void hc_start_transport_op(grpc_exec_ctx *exec_ctx, call_data *calld = elem->call_data; if (op->send_message != NULL && calld->send_message_blocked) { /* Don't forward the op. send_message contains slices that aren't ready - yet. The call will be forwarded by the op_complete of slice read call. - */ + yet. The call will be forwarded by the op_complete of slice read call. */ } else { grpc_call_next_op(exec_ctx, elem, op); } @@ -347,6 +349,7 @@ static grpc_error *init_call_elem(grpc_exec_ctx *exec_ctx, calld->on_done_recv_trailing_metadata = NULL; calld->on_complete = NULL; calld->payload_bytes = NULL; + calld->send_message_blocked = false; grpc_slice_buffer_init(&calld->slices); grpc_closure_init(&calld->hc_on_recv_initial_metadata, hc_on_recv_initial_metadata, elem); -- cgit v1.2.3 From da81d1a43deecd815b7d2520a2425f0afd1a858d Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 28 Nov 2016 15:00:53 -0800 Subject: Flush platform stuff after timer events It can happen that a timer event causes something to be queued to an IOCP, which means that on Windows we need to flush that queue each time a timer event fires during shutdown. --- src/core/lib/iomgr/iomgr.c | 1 + 1 file changed, 1 insertion(+) (limited to 'src/core') diff --git a/src/core/lib/iomgr/iomgr.c b/src/core/lib/iomgr/iomgr.c index 4fd83e0b22..3470b5ac81 100644 --- a/src/core/lib/iomgr/iomgr.c +++ b/src/core/lib/iomgr/iomgr.c @@ -108,6 +108,7 @@ void grpc_iomgr_shutdown(void) { NULL)) { gpr_mu_unlock(&g_mu); grpc_exec_ctx_flush(&exec_ctx); + grpc_iomgr_platform_flush(); gpr_mu_lock(&g_mu); continue; } -- cgit v1.2.3 From c0c0dbc01ce092a9f8bbf3f0dd8ea12d5486b026 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 29 Nov 2016 10:10:21 -0800 Subject: Fix TSAN race on adding a reclaimer --- src/core/lib/iomgr/resource_quota.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/resource_quota.c b/src/core/lib/iomgr/resource_quota.c index 051a30baa3..379bf9bd23 100644 --- a/src/core/lib/iomgr/resource_quota.c +++ b/src/core/lib/iomgr/resource_quota.c @@ -104,6 +104,9 @@ struct grpc_resource_user { /* Reclaimers: index 0 is the benign reclaimer, 1 is the destructive reclaimer */ grpc_closure *reclaimers[2]; + /* Reclaimers just posted: once we're in the combiner lock, we'll move them + to the array above */ + grpc_closure *new_reclaimers[2]; /* Trampoline closures to finish reclamation and re-enter the quota combiner lock */ grpc_closure post_reclaimer_closure[2]; @@ -418,9 +421,25 @@ static void ru_add_to_free_pool(grpc_exec_ctx *exec_ctx, void *ru, rulist_add_tail(resource_user, GRPC_RULIST_NON_EMPTY_FREE_POOL); } +static bool ru_post_reclaimer(grpc_exec_ctx *exec_ctx, + grpc_resource_user *resource_user, + bool destructive) { + grpc_closure *closure = resource_user->new_reclaimers[destructive]; + GPR_ASSERT(closure != NULL); + resource_user->new_reclaimers[destructive] = NULL; + GPR_ASSERT(resource_user->reclaimers[destructive] == NULL); + if (gpr_atm_acq_load(&resource_user->shutdown) > 0) { + grpc_exec_ctx_sched(exec_ctx, closure, GRPC_ERROR_CANCELLED, NULL); + return false; + } + resource_user->reclaimers[destructive] = closure; + return true; +} + static void ru_post_benign_reclaimer(grpc_exec_ctx *exec_ctx, void *ru, grpc_error *error) { grpc_resource_user *resource_user = ru; + if (!ru_post_reclaimer(exec_ctx, resource_user, false)) return; if (!rulist_empty(resource_user->resource_quota, GRPC_RULIST_AWAITING_ALLOCATION) && rulist_empty(resource_user->resource_quota, @@ -435,6 +454,7 @@ static void ru_post_benign_reclaimer(grpc_exec_ctx *exec_ctx, void *ru, static void ru_post_destructive_reclaimer(grpc_exec_ctx *exec_ctx, void *ru, grpc_error *error) { grpc_resource_user *resource_user = ru; + if (!ru_post_reclaimer(exec_ctx, resource_user, true)) return; if (!rulist_empty(resource_user->resource_quota, GRPC_RULIST_AWAITING_ALLOCATION) && rulist_empty(resource_user->resource_quota, @@ -649,6 +669,8 @@ grpc_resource_user *grpc_resource_user_create( resource_user->added_to_free_pool = false; resource_user->reclaimers[0] = NULL; resource_user->reclaimers[1] = NULL; + resource_user->new_reclaimers[0] = NULL; + resource_user->new_reclaimers[1] = NULL; for (int i = 0; i < GRPC_RULIST_COUNT; i++) { resource_user->links[i].next = resource_user->links[i].prev = NULL; } @@ -748,12 +770,8 @@ void grpc_resource_user_post_reclaimer(grpc_exec_ctx *exec_ctx, grpc_resource_user *resource_user, bool destructive, grpc_closure *closure) { - GPR_ASSERT(resource_user->reclaimers[destructive] == NULL); - if (gpr_atm_acq_load(&resource_user->shutdown) > 0) { - grpc_exec_ctx_sched(exec_ctx, closure, GRPC_ERROR_CANCELLED, NULL); - return; - } - resource_user->reclaimers[destructive] = closure; + GPR_ASSERT(resource_user->new_reclaimers[destructive] == NULL); + resource_user->new_reclaimers[destructive] = closure; grpc_combiner_execute(exec_ctx, resource_user->resource_quota->combiner, &resource_user->post_reclaimer_closure[destructive], GRPC_ERROR_NONE, false); -- cgit v1.2.3 From 4cdcd12f755e98709e2b470715aefa1005601a33 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 29 Nov 2016 12:39:54 -0800 Subject: Fix locking bug in HTTP CONNECT handshaker. --- src/core/ext/client_channel/http_connect_handshaker.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/client_channel/http_connect_handshaker.c b/src/core/ext/client_channel/http_connect_handshaker.c index 971bbe8944..c9861a5aed 100644 --- a/src/core/ext/client_channel/http_connect_handshaker.c +++ b/src/core/ext/client_channel/http_connect_handshaker.c @@ -86,9 +86,9 @@ static void http_connect_handshaker_unref(http_connect_handshaker* handshaker) { static void on_write_done(grpc_exec_ctx* exec_ctx, void* arg, grpc_error* error) { http_connect_handshaker* handshaker = arg; + gpr_mu_lock(&handshaker->mu); if (error != GRPC_ERROR_NONE || handshaker->args == NULL) { // If the write failed, invoke the callback immediately with the error. - gpr_mu_lock(&handshaker->mu); grpc_exec_ctx_sched(exec_ctx, handshaker->on_handshake_done, GRPC_ERROR_REF(error), NULL); handshaker->args = NULL; @@ -97,7 +97,6 @@ static void on_write_done(grpc_exec_ctx* exec_ctx, void* arg, } else { // Otherwise, read the response. // The read callback inherits our ref to the handshaker. - gpr_mu_lock(&handshaker->mu); grpc_endpoint_read(exec_ctx, handshaker->args->endpoint, handshaker->args->read_buffer, &handshaker->response_read_closure); -- cgit v1.2.3 From 447569490d05f95b8caa79a1e9f35f2ac1f7a2bd Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 29 Nov 2016 12:43:55 -0800 Subject: Eliminate the user_data overloading hack in handshake_manager. --- src/core/lib/channel/handshaker.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/channel/handshaker.c b/src/core/lib/channel/handshaker.c index 905db118be..3c125a22f3 100644 --- a/src/core/lib/channel/handshaker.c +++ b/src/core/lib/channel/handshaker.c @@ -154,7 +154,6 @@ static void call_next_handshaker(grpc_exec_ctx* exec_ctx, void* arg, // on_handshake_done callback. static void call_next_handshaker_locked(grpc_exec_ctx* exec_ctx, grpc_handshake_manager* mgr, - grpc_handshaker_args* args, grpc_error* error) { GPR_ASSERT(mgr->index <= mgr->count); // If we got an error, skip all remaining handshakers and invoke the @@ -165,9 +164,7 @@ static void call_next_handshaker_locked(grpc_exec_ctx* exec_ctx, // Cancel deadline timer, since we're invoking the on_handshake_done // callback now. grpc_timer_cancel(exec_ctx, &mgr->deadline_timer); - args->user_data = mgr->user_data; - grpc_exec_ctx_sched(exec_ctx, &mgr->on_handshake_done, - GRPC_ERROR_REF(error), NULL); + grpc_exec_ctx_sched(exec_ctx, &mgr->on_handshake_done, error, NULL); // Since we're invoking the final callback, we won't be coming back // to this function, so we can release our reference to the // handshake manager. @@ -176,7 +173,8 @@ static void call_next_handshaker_locked(grpc_exec_ctx* exec_ctx, } // Call the next handshaker. grpc_handshaker_do_handshake(exec_ctx, mgr->handshakers[mgr->index], - mgr->acceptor, &mgr->call_next_handshaker, args); + mgr->acceptor, &mgr->call_next_handshaker, + &mgr->args); ++mgr->index; } @@ -184,10 +182,9 @@ static void call_next_handshaker_locked(grpc_exec_ctx* exec_ctx, // handshakers together. static void call_next_handshaker(grpc_exec_ctx* exec_ctx, void* arg, grpc_error* error) { - grpc_handshaker_args* args = arg; - grpc_handshake_manager* mgr = args->user_data; + grpc_handshake_manager* mgr = arg; gpr_mu_lock(&mgr->mu); - call_next_handshaker_locked(exec_ctx, mgr, args, error); + call_next_handshaker_locked(exec_ctx, mgr, GRPC_ERROR_REF(error)); gpr_mu_unlock(&mgr->mu); } @@ -209,21 +206,15 @@ void grpc_handshake_manager_do_handshake( // handshakers and eventually be freed by the on_handshake_done callback. mgr->args.endpoint = endpoint; mgr->args.args = grpc_channel_args_copy(channel_args); + mgr->args.user_data = user_data; mgr->args.read_buffer = gpr_malloc(sizeof(*mgr->args.read_buffer)); grpc_slice_buffer_init(mgr->args.read_buffer); // Initialize state needed for calling handshakers. gpr_mu_lock(&mgr->mu); GPR_ASSERT(mgr->index == 0); mgr->acceptor = acceptor; - grpc_closure_init(&mgr->call_next_handshaker, call_next_handshaker, - &mgr->args); + grpc_closure_init(&mgr->call_next_handshaker, call_next_handshaker, mgr); grpc_closure_init(&mgr->on_handshake_done, on_handshake_done, &mgr->args); - // While chaining between handshakers, we use args->user_data to - // store a pointer to the handshake manager. This will be - // changed to point to the caller-supplied user_data before calling - // the on_handshake_done callback. - mgr->args.user_data = mgr; - mgr->user_data = user_data; // Start deadline timer, which owns a ref. gpr_ref(&mgr->refs); grpc_timer_init(exec_ctx, &mgr->deadline_timer, @@ -231,6 +222,6 @@ void grpc_handshake_manager_do_handshake( on_timeout, mgr, gpr_now(GPR_CLOCK_MONOTONIC)); // Start first handshaker, which also owns a ref. gpr_ref(&mgr->refs); - call_next_handshaker_locked(exec_ctx, mgr, &mgr->args, GRPC_ERROR_NONE); + call_next_handshaker_locked(exec_ctx, mgr, GRPC_ERROR_NONE); gpr_mu_unlock(&mgr->mu); } -- cgit v1.2.3 From 30f698f1bcb8956d49b093391997b8d01dc2524f Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 29 Nov 2016 14:02:32 -0800 Subject: Make handshaker responsible for destroying endpoint on shutdown or failure. --- .../ext/client_channel/http_connect_handshaker.c | 81 +++++++++++++++++----- .../chttp2/client/insecure/channel_create.c | 6 +- .../chttp2/client/secure/secure_channel_create.c | 2 - .../chttp2/server/insecure/server_chttp2.c | 4 +- .../chttp2/server/secure/server_secure_chttp2.c | 3 - src/core/lib/channel/handshaker.c | 17 ++--- src/core/lib/channel/handshaker.h | 21 ++++-- 7 files changed, 87 insertions(+), 47 deletions(-) (limited to 'src/core') diff --git a/src/core/ext/client_channel/http_connect_handshaker.c b/src/core/ext/client_channel/http_connect_handshaker.c index c9861a5aed..48990f9dac 100644 --- a/src/core/ext/client_channel/http_connect_handshaker.c +++ b/src/core/ext/client_channel/http_connect_handshaker.c @@ -41,6 +41,7 @@ #include #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" #include "src/core/lib/http/parser.h" #include "src/core/lib/support/env.h" @@ -55,9 +56,12 @@ typedef struct http_connect_handshaker { gpr_refcount refcount; gpr_mu mu; + bool shutdown; + // Endpoint and read buffer to destroy after a shutdown. + grpc_endpoint* endpoint_to_destroy; + grpc_slice_buffer* read_buffer_to_destroy; + // State saved while performing the handshake. - // args will be NULL when either there is no handshake in progress or - // when the handshaker is shutting down. grpc_handshaker_args* args; grpc_closure* on_handshake_done; @@ -70,9 +74,17 @@ typedef struct http_connect_handshaker { } http_connect_handshaker; // Unref and clean up handshaker. -static void http_connect_handshaker_unref(http_connect_handshaker* handshaker) { +static void http_connect_handshaker_unref(grpc_exec_ctx* exec_ctx, + http_connect_handshaker* handshaker) { if (gpr_unref(&handshaker->refcount)) { gpr_mu_destroy(&handshaker->mu); + if (handshaker->endpoint_to_destroy != NULL) { + grpc_endpoint_destroy(exec_ctx, handshaker->endpoint_to_destroy); + } + if (handshaker->read_buffer_to_destroy != NULL) { + grpc_slice_buffer_destroy(handshaker->read_buffer_to_destroy); + gpr_free(handshaker->read_buffer_to_destroy); + } gpr_free(handshaker->proxy_server); gpr_free(handshaker->server_name); grpc_slice_buffer_destroy(&handshaker->write_buffer); @@ -82,18 +94,42 @@ static void http_connect_handshaker_unref(http_connect_handshaker* handshaker) { } } +// Set args fields to NULL, saving the endpoint and read buffer for +// later destruction. +static void cleanup_args_for_failure_locked( + http_connect_handshaker* handshaker) { + handshaker->endpoint_to_destroy = handshaker->args->endpoint; + handshaker->args->endpoint = NULL; + handshaker->read_buffer_to_destroy = handshaker->args->read_buffer; + handshaker->args->read_buffer = NULL; + grpc_channel_args_destroy(handshaker->args->args); + handshaker->args->args = NULL; +} + // Callback invoked when finished writing HTTP CONNECT request. static void on_write_done(grpc_exec_ctx* exec_ctx, void* arg, grpc_error* error) { http_connect_handshaker* handshaker = arg; gpr_mu_lock(&handshaker->mu); - if (error != GRPC_ERROR_NONE || handshaker->args == NULL) { - // If the write failed, invoke the callback immediately with the error. - grpc_exec_ctx_sched(exec_ctx, handshaker->on_handshake_done, - GRPC_ERROR_REF(error), NULL); - handshaker->args = NULL; + if (error != GRPC_ERROR_NONE || handshaker->shutdown) { + // If the write failed or we're shutting down, clean up and invoke the + // callback with the error. + if (error == GRPC_ERROR_NONE) { + // If we were shut down after the write succeeded but before this + // callback was invoked, we need to generate our own error. + error = GRPC_ERROR_CREATE("Handshaker shutdown"); + } else { + GRPC_ERROR_REF(error); // Take ref for the handshake-done callback. + } + if (!handshaker->shutdown) { + // Not shutting down, so the write failed. Clean up before + // invoking the callback. + cleanup_args_for_failure_locked(handshaker); + } + // Invoke callback. + grpc_exec_ctx_sched(exec_ctx, handshaker->on_handshake_done, error, NULL); gpr_mu_unlock(&handshaker->mu); - http_connect_handshaker_unref(handshaker); + http_connect_handshaker_unref(exec_ctx, handshaker); } else { // Otherwise, read the response. // The read callback inherits our ref to the handshaker. @@ -109,8 +145,21 @@ static void on_read_done(grpc_exec_ctx* exec_ctx, void* arg, grpc_error* error) { http_connect_handshaker* handshaker = arg; gpr_mu_lock(&handshaker->mu); - if (error != GRPC_ERROR_NONE || handshaker->args == NULL) { - GRPC_ERROR_REF(error); // Take ref to pass to the handshake-done callback. + if (error != GRPC_ERROR_NONE || handshaker->shutdown) { + // If the write failed or we're shutting down, clean up and invoke the + // callback with the error. + if (error == GRPC_ERROR_NONE) { + // If we were shut down after the write succeeded but before this + // callback was invoked, we need to generate our own error. + error = GRPC_ERROR_CREATE("Handshaker shutdown"); + } else { + GRPC_ERROR_REF(error); // Take ref for the handshake-done callback. + } + if (!handshaker->shutdown) { + // Not shutting down, so the write failed. Clean up before + // invoking the callback. + cleanup_args_for_failure_locked(handshaker); + } goto done; } // Add buffer to parser. @@ -172,10 +221,9 @@ static void on_read_done(grpc_exec_ctx* exec_ctx, void* arg, } done: // Invoke handshake-done callback. - handshaker->args = NULL; grpc_exec_ctx_sched(exec_ctx, handshaker->on_handshake_done, error, NULL); gpr_mu_unlock(&handshaker->mu); - http_connect_handshaker_unref(handshaker); + http_connect_handshaker_unref(exec_ctx, handshaker); } // @@ -185,16 +233,17 @@ done: static void http_connect_handshaker_destroy(grpc_exec_ctx* exec_ctx, grpc_handshaker* handshaker_in) { http_connect_handshaker* handshaker = (http_connect_handshaker*)handshaker_in; - http_connect_handshaker_unref(handshaker); + http_connect_handshaker_unref(exec_ctx, handshaker); } static void http_connect_handshaker_shutdown(grpc_exec_ctx* exec_ctx, grpc_handshaker* handshaker_in) { http_connect_handshaker* handshaker = (http_connect_handshaker*)handshaker_in; gpr_mu_lock(&handshaker->mu); - if (handshaker->args != NULL) { + if (!handshaker->shutdown) { + handshaker->shutdown = true; grpc_endpoint_shutdown(exec_ctx, handshaker->args->endpoint); - handshaker->args = NULL; + cleanup_args_for_failure_locked(handshaker); } gpr_mu_unlock(&handshaker->mu); } 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 e0bce57fc2..00b272de27 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.c @@ -96,11 +96,7 @@ static void on_handshake_done(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { grpc_handshaker_args *args = arg; connector *c = args->user_data; - if (error != GRPC_ERROR_NONE) { - grpc_endpoint_destroy(exec_ctx, args->endpoint); - grpc_channel_args_destroy(args->args); - gpr_free(args->read_buffer); - } else { + if (error == GRPC_ERROR_NONE) { c->result->transport = grpc_create_chttp2_transport(exec_ctx, args->args, args->endpoint, 1); GPR_ASSERT(c->result->transport); 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 4182aa730f..b4a30f94fc 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 @@ -135,8 +135,6 @@ static void on_handshake_done(grpc_exec_ctx *exec_ctx, void *arg, connector *c = args->user_data; c->tmp_args = args->args; if (error != GRPC_ERROR_NONE) { - grpc_endpoint_destroy(exec_ctx, args->endpoint); - gpr_free(args->read_buffer); grpc_closure *notify = c->notify; c->notify = NULL; grpc_exec_ctx_sched(exec_ctx, notify, GRPC_ERROR_REF(error), NULL); 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 5a9d4f8928..1b38d4decd 100644 --- a/src/core/ext/transport/chttp2/server/insecure/server_chttp2.c +++ b/src/core/ext/transport/chttp2/server/insecure/server_chttp2.c @@ -62,8 +62,6 @@ static void on_handshake_done(grpc_exec_ctx *exec_ctx, void *arg, const char *error_str = grpc_error_string(error); gpr_log(GPR_ERROR, "Handshaking failed: %s", error_str); grpc_error_free_string(error_str); - grpc_endpoint_destroy(exec_ctx, args->endpoint); - gpr_free(args->read_buffer); } else { // Beware that the call to grpc_create_chttp2_transport() has to happen // before grpc_tcp_server_destroy(). This is fine here, but similar code @@ -76,9 +74,9 @@ static void on_handshake_done(grpc_exec_ctx *exec_ctx, void *arg, state->accepting_pollset, grpc_server_get_channel_args(state->server)); grpc_chttp2_transport_start_reading(exec_ctx, transport, args->read_buffer); + grpc_channel_args_destroy(args->args); } // Clean up. - grpc_channel_args_destroy(args->args); grpc_handshake_manager_destroy(exec_ctx, state->handshake_mgr); gpr_free(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 1d1973be8b..22af94199f 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 @@ -123,9 +123,6 @@ static void on_handshake_done(grpc_exec_ctx *exec_ctx, void *arg, const char *error_str = grpc_error_string(error); gpr_log(GPR_ERROR, "Handshaking failed: %s", error_str); grpc_error_free_string(error_str); - grpc_endpoint_destroy(exec_ctx, args->endpoint); - grpc_channel_args_destroy(args->args); - gpr_free(args->read_buffer); grpc_handshake_manager_destroy(exec_ctx, connection_state->handshake_mgr); grpc_tcp_server_unref(exec_ctx, connection_state->server_state->tcp); gpr_free(connection_state); diff --git a/src/core/lib/channel/handshaker.c b/src/core/lib/channel/handshaker.c index 3c125a22f3..f3bd91284e 100644 --- a/src/core/lib/channel/handshaker.c +++ b/src/core/lib/channel/handshaker.c @@ -141,25 +141,20 @@ void grpc_handshake_manager_destroy(grpc_exec_ctx* exec_ctx, void grpc_handshake_manager_shutdown(grpc_exec_ctx* exec_ctx, grpc_handshake_manager* mgr) { gpr_mu_lock(&mgr->mu); - for (size_t i = 0; i < mgr->count; ++i) { - grpc_handshaker_shutdown(exec_ctx, mgr->handshakers[i]); + if (mgr->index > 0) { + grpc_handshaker_shutdown(exec_ctx, mgr->handshakers[mgr->index - 1]); } gpr_mu_unlock(&mgr->mu); } -static void call_next_handshaker(grpc_exec_ctx* exec_ctx, void* arg, - grpc_error* error); - // Helper function to call either the next handshaker or the // on_handshake_done callback. static void call_next_handshaker_locked(grpc_exec_ctx* exec_ctx, grpc_handshake_manager* mgr, grpc_error* error) { GPR_ASSERT(mgr->index <= mgr->count); - // If we got an error, skip all remaining handshakers and invoke the - // caller-supplied callback immediately. - // Otherwise, if this is the last handshaker, then call the on_handshake_done - // callback instead of chaining back to this function again. + // If we got an error or we've finished the last handshaker, invoke + // the on_handshake_done callback. Otherwise, call the next handshaker. if (error != GRPC_ERROR_NONE || mgr->index == mgr->count) { // Cancel deadline timer, since we're invoking the on_handshake_done // callback now. @@ -202,6 +197,8 @@ void grpc_handshake_manager_do_handshake( grpc_endpoint* endpoint, const grpc_channel_args* channel_args, gpr_timespec deadline, grpc_tcp_server_acceptor* acceptor, grpc_iomgr_cb_func on_handshake_done, void* user_data) { + gpr_mu_lock(&mgr->mu); + GPR_ASSERT(mgr->index == 0); // Construct handshaker args. These will be passed through all // handshakers and eventually be freed by the on_handshake_done callback. mgr->args.endpoint = endpoint; @@ -210,8 +207,6 @@ void grpc_handshake_manager_do_handshake( mgr->args.read_buffer = gpr_malloc(sizeof(*mgr->args.read_buffer)); grpc_slice_buffer_init(mgr->args.read_buffer); // Initialize state needed for calling handshakers. - gpr_mu_lock(&mgr->mu); - GPR_ASSERT(mgr->index == 0); mgr->acceptor = acceptor; grpc_closure_init(&mgr->call_next_handshaker, call_next_handshaker, mgr); grpc_closure_init(&mgr->on_handshake_done, on_handshake_done, &mgr->args); diff --git a/src/core/lib/channel/handshaker.h b/src/core/lib/channel/handshaker.h index f0614c354b..2e1f543512 100644 --- a/src/core/lib/channel/handshaker.h +++ b/src/core/lib/channel/handshaker.h @@ -57,17 +57,24 @@ typedef struct grpc_handshaker grpc_handshaker; /// Arguments passed through handshakers and to the on_handshake_done callback. /// /// For handshakers, all members are input/output parameters; for -/// example, a handshaker may read from \a endpoint and then later -/// replace it with a wrapped endpoint. Similarly, a handshaker may -/// modify \a args. +/// example, a handshaker may read from or write to \a endpoint and +/// then later replace it with a wrapped endpoint. Similarly, a +/// handshaker may modify \a args. +/// +/// A handshaker takes ownership of the members while a handshake is in +/// progress. Upon failure or shutdown of an in-progress handshaker, +/// the handshaker is responsible for destroying the members and setting +/// them to NULL before invoking the on_handshake_done callback. /// /// For the on_handshake_done callback, all members are input arguments, /// which the callback takes ownership of. typedef struct { grpc_endpoint* endpoint; grpc_channel_args* args; - void* user_data; grpc_slice_buffer* read_buffer; + // User data passed through the handshake manager. Not used by + // individual handshakers. + void* user_data; } grpc_handshaker_args; typedef struct { @@ -132,9 +139,9 @@ void grpc_handshake_manager_shutdown(grpc_exec_ctx* exec_ctx, /// /// When done, invokes \a on_handshake_done with a grpc_handshaker_args /// object as its argument. If the callback is invoked with error != -/// GRPC_ERROR_NONE, then handshaking failed and the resulting endpoint -/// will have already been shut down (although the caller will still be -/// responsible for destroying it). +/// GRPC_ERROR_NONE, then handshaking failed and the handshaker has done +/// the necessary clean-up. Otherwise, the callback takes ownership of +/// the arguments. void grpc_handshake_manager_do_handshake( grpc_exec_ctx* exec_ctx, grpc_handshake_manager* mgr, grpc_endpoint* endpoint, const grpc_channel_args* channel_args, -- cgit v1.2.3 From 0610434185cd528b72e5be936075cae4c4a07a8e Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 29 Nov 2016 14:06:45 -0800 Subject: Always shut down endpoints before destroying them. --- src/core/ext/client_channel/http_connect_handshaker.c | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'src/core') diff --git a/src/core/ext/client_channel/http_connect_handshaker.c b/src/core/ext/client_channel/http_connect_handshaker.c index 48990f9dac..61fec5cba9 100644 --- a/src/core/ext/client_channel/http_connect_handshaker.c +++ b/src/core/ext/client_channel/http_connect_handshaker.c @@ -122,6 +122,11 @@ static void on_write_done(grpc_exec_ctx* exec_ctx, void* arg, GRPC_ERROR_REF(error); // Take ref for the handshake-done callback. } if (!handshaker->shutdown) { + // 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, handshaker->args->endpoint); // Not shutting down, so the write failed. Clean up before // invoking the callback. cleanup_args_for_failure_locked(handshaker); @@ -156,6 +161,11 @@ static void on_read_done(grpc_exec_ctx* exec_ctx, void* arg, GRPC_ERROR_REF(error); // Take ref for the handshake-done callback. } if (!handshaker->shutdown) { + // 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, handshaker->args->endpoint); // Not shutting down, so the write failed. Clean up before // invoking the callback. cleanup_args_for_failure_locked(handshaker); -- cgit v1.2.3