From af00d8bfb24f6debdc2a0ac78d2dc5bf67fc01e4 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 23 Aug 2016 12:48:16 -0700 Subject: [PATCH 01/40] Add channel arg for max send message size and add message size filter. --- BUILD | 8 + CMakeLists.txt | 3 + Makefile | 4 + binding.gyp | 1 + build.yaml | 2 + config.m4 | 1 + gRPC-Core.podspec | 3 + grpc.gemspec | 2 + include/grpc/impl/codegen/grpc_types.h | 6 +- package.xml | 2 + src/core/lib/channel/message_size_filter.c | 175 ++++++++++++++++++ src/core/lib/channel/message_size_filter.h | 40 ++++ src/core/lib/surface/call.c | 11 -- src/core/lib/surface/channel.c | 21 +-- src/core/lib/surface/channel.h | 1 - src/core/lib/surface/init.c | 10 + src/python/grpcio/grpc_core_dependencies.py | 1 + test/core/end2end/tests/max_message_length.c | 45 +++-- tools/doxygen/Doxyfile.core.internal | 2 + tools/run_tests/sources_and_headers.json | 3 + vsprojects/vcxproj/grpc/grpc.vcxproj | 3 + vsprojects/vcxproj/grpc/grpc.vcxproj.filters | 6 + .../grpc_test_util/grpc_test_util.vcxproj | 3 + .../grpc_test_util.vcxproj.filters | 6 + .../grpc_unsecure/grpc_unsecure.vcxproj | 3 + .../grpc_unsecure.vcxproj.filters | 6 + 26 files changed, 320 insertions(+), 48 deletions(-) create mode 100644 src/core/lib/channel/message_size_filter.c create mode 100644 src/core/lib/channel/message_size_filter.h diff --git a/BUILD b/BUILD index 12f0fd316ea..7f661e9714b 100644 --- a/BUILD +++ b/BUILD @@ -168,6 +168,7 @@ cc_library( "src/core/lib/channel/handshaker.h", "src/core/lib/channel/http_client_filter.h", "src/core/lib/channel/http_server_filter.h", + "src/core/lib/channel/message_size_filter.h", "src/core/lib/compression/algorithm_metadata.h", "src/core/lib/compression/message_compress.h", "src/core/lib/debug/trace.h", @@ -323,6 +324,7 @@ cc_library( "src/core/lib/channel/handshaker.c", "src/core/lib/channel/http_client_filter.c", "src/core/lib/channel/http_server_filter.c", + "src/core/lib/channel/message_size_filter.c", "src/core/lib/compression/compression.c", "src/core/lib/compression/message_compress.c", "src/core/lib/debug/trace.c", @@ -563,6 +565,7 @@ cc_library( "src/core/lib/channel/handshaker.h", "src/core/lib/channel/http_client_filter.h", "src/core/lib/channel/http_server_filter.h", + "src/core/lib/channel/message_size_filter.h", "src/core/lib/compression/algorithm_metadata.h", "src/core/lib/compression/message_compress.h", "src/core/lib/debug/trace.h", @@ -705,6 +708,7 @@ cc_library( "src/core/lib/channel/handshaker.c", "src/core/lib/channel/http_client_filter.c", "src/core/lib/channel/http_server_filter.c", + "src/core/lib/channel/message_size_filter.c", "src/core/lib/compression/compression.c", "src/core/lib/compression/message_compress.c", "src/core/lib/debug/trace.c", @@ -917,6 +921,7 @@ cc_library( "src/core/lib/channel/handshaker.h", "src/core/lib/channel/http_client_filter.h", "src/core/lib/channel/http_server_filter.h", + "src/core/lib/channel/message_size_filter.h", "src/core/lib/compression/algorithm_metadata.h", "src/core/lib/compression/message_compress.h", "src/core/lib/debug/trace.h", @@ -1049,6 +1054,7 @@ cc_library( "src/core/lib/channel/handshaker.c", "src/core/lib/channel/http_client_filter.c", "src/core/lib/channel/http_server_filter.c", + "src/core/lib/channel/message_size_filter.c", "src/core/lib/compression/compression.c", "src/core/lib/compression/message_compress.c", "src/core/lib/debug/trace.c", @@ -1816,6 +1822,7 @@ objc_library( "src/core/lib/channel/handshaker.c", "src/core/lib/channel/http_client_filter.c", "src/core/lib/channel/http_server_filter.c", + "src/core/lib/channel/message_size_filter.c", "src/core/lib/compression/compression.c", "src/core/lib/compression/message_compress.c", "src/core/lib/debug/trace.c", @@ -2035,6 +2042,7 @@ objc_library( "src/core/lib/channel/handshaker.h", "src/core/lib/channel/http_client_filter.h", "src/core/lib/channel/http_server_filter.h", + "src/core/lib/channel/message_size_filter.h", "src/core/lib/compression/algorithm_metadata.h", "src/core/lib/compression/message_compress.h", "src/core/lib/debug/trace.h", diff --git a/CMakeLists.txt b/CMakeLists.txt index f847c9b0e68..567ec1fcb3d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -299,6 +299,7 @@ add_library(grpc src/core/lib/channel/handshaker.c src/core/lib/channel/http_client_filter.c src/core/lib/channel/http_server_filter.c + src/core/lib/channel/message_size_filter.c src/core/lib/compression/compression.c src/core/lib/compression/message_compress.c src/core/lib/debug/trace.c @@ -557,6 +558,7 @@ add_library(grpc_cronet src/core/lib/channel/handshaker.c src/core/lib/channel/http_client_filter.c src/core/lib/channel/http_server_filter.c + src/core/lib/channel/message_size_filter.c src/core/lib/compression/compression.c src/core/lib/compression/message_compress.c src/core/lib/debug/trace.c @@ -789,6 +791,7 @@ add_library(grpc_unsecure src/core/lib/channel/handshaker.c src/core/lib/channel/http_client_filter.c src/core/lib/channel/http_server_filter.c + src/core/lib/channel/message_size_filter.c src/core/lib/compression/compression.c src/core/lib/compression/message_compress.c src/core/lib/debug/trace.c diff --git a/Makefile b/Makefile index 6b619e310b9..fdd1bfdfd18 100644 --- a/Makefile +++ b/Makefile @@ -2515,6 +2515,7 @@ LIBGRPC_SRC = \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ + src/core/lib/channel/message_size_filter.c \ src/core/lib/compression/compression.c \ src/core/lib/compression/message_compress.c \ src/core/lib/debug/trace.c \ @@ -2791,6 +2792,7 @@ LIBGRPC_CRONET_SRC = \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ + src/core/lib/channel/message_size_filter.c \ src/core/lib/compression/compression.c \ src/core/lib/compression/message_compress.c \ src/core/lib/debug/trace.c \ @@ -3057,6 +3059,7 @@ LIBGRPC_TEST_UTIL_SRC = \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ + src/core/lib/channel/message_size_filter.c \ src/core/lib/compression/compression.c \ src/core/lib/compression/message_compress.c \ src/core/lib/debug/trace.c \ @@ -3249,6 +3252,7 @@ LIBGRPC_UNSECURE_SRC = \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ + src/core/lib/channel/message_size_filter.c \ src/core/lib/compression/compression.c \ src/core/lib/compression/message_compress.c \ src/core/lib/debug/trace.c \ diff --git a/binding.gyp b/binding.gyp index a29cfda6fc5..2cb43b0eb5a 100644 --- a/binding.gyp +++ b/binding.gyp @@ -571,6 +571,7 @@ 'src/core/lib/channel/handshaker.c', 'src/core/lib/channel/http_client_filter.c', 'src/core/lib/channel/http_server_filter.c', + 'src/core/lib/channel/message_size_filter.c', 'src/core/lib/compression/compression.c', 'src/core/lib/compression/message_compress.c', 'src/core/lib/debug/trace.c', diff --git a/build.yaml b/build.yaml index 73c626351da..a0ff4ec8598 100644 --- a/build.yaml +++ b/build.yaml @@ -163,6 +163,7 @@ filegroups: - src/core/lib/channel/handshaker.h - src/core/lib/channel/http_client_filter.h - src/core/lib/channel/http_server_filter.h + - src/core/lib/channel/message_size_filter.h - src/core/lib/compression/algorithm_metadata.h - src/core/lib/compression/message_compress.h - src/core/lib/debug/trace.h @@ -243,6 +244,7 @@ filegroups: - src/core/lib/channel/handshaker.c - src/core/lib/channel/http_client_filter.c - src/core/lib/channel/http_server_filter.c + - src/core/lib/channel/message_size_filter.c - src/core/lib/compression/compression.c - src/core/lib/compression/message_compress.c - src/core/lib/debug/trace.c diff --git a/config.m4 b/config.m4 index 5b47074bcd7..c1b2fd9c9bf 100644 --- a/config.m4 +++ b/config.m4 @@ -90,6 +90,7 @@ if test "$PHP_GRPC" != "no"; then src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ + src/core/lib/channel/message_size_filter.c \ src/core/lib/compression/compression.c \ src/core/lib/compression/message_compress.c \ src/core/lib/debug/trace.c \ diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index 02a500f626e..1046f503dba 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -261,6 +261,7 @@ Pod::Spec.new do |s| 'src/core/lib/channel/handshaker.h', 'src/core/lib/channel/http_client_filter.h', 'src/core/lib/channel/http_server_filter.h', + 'src/core/lib/channel/message_size_filter.h', 'src/core/lib/compression/algorithm_metadata.h', 'src/core/lib/compression/message_compress.h', 'src/core/lib/debug/trace.h', @@ -420,6 +421,7 @@ Pod::Spec.new do |s| 'src/core/lib/channel/handshaker.c', 'src/core/lib/channel/http_client_filter.c', 'src/core/lib/channel/http_server_filter.c', + 'src/core/lib/channel/message_size_filter.c', 'src/core/lib/compression/compression.c', 'src/core/lib/compression/message_compress.c', 'src/core/lib/debug/trace.c', @@ -622,6 +624,7 @@ Pod::Spec.new do |s| 'src/core/lib/channel/handshaker.h', 'src/core/lib/channel/http_client_filter.h', 'src/core/lib/channel/http_server_filter.h', + 'src/core/lib/channel/message_size_filter.h', 'src/core/lib/compression/algorithm_metadata.h', 'src/core/lib/compression/message_compress.h', 'src/core/lib/debug/trace.h', diff --git a/grpc.gemspec b/grpc.gemspec index 9c82db8ee30..c69988f669d 100755 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -180,6 +180,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/channel/handshaker.h ) s.files += %w( src/core/lib/channel/http_client_filter.h ) s.files += %w( src/core/lib/channel/http_server_filter.h ) + s.files += %w( src/core/lib/channel/message_size_filter.h ) s.files += %w( src/core/lib/compression/algorithm_metadata.h ) s.files += %w( src/core/lib/compression/message_compress.h ) s.files += %w( src/core/lib/debug/trace.h ) @@ -339,6 +340,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/channel/handshaker.c ) s.files += %w( src/core/lib/channel/http_client_filter.c ) s.files += %w( src/core/lib/channel/http_server_filter.c ) + s.files += %w( src/core/lib/channel/message_size_filter.c ) s.files += %w( src/core/lib/compression/compression.c ) s.files += %w( src/core/lib/compression/message_compress.c ) s.files += %w( src/core/lib/debug/trace.c ) diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index e5a82883be5..8b1ba63494a 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -127,7 +127,11 @@ typedef struct { connection. Int valued. */ #define GRPC_ARG_MAX_CONCURRENT_STREAMS "grpc.max_concurrent_streams" /** Maximum message length that the channel can receive. Int valued, bytes. */ -#define GRPC_ARG_MAX_MESSAGE_LENGTH "grpc.max_message_length" +#define GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH "grpc.max_receive_message_length" +/** Backward compatibility. */ +#define GRPC_ARG_MAX_MESSAGE_LENGTH GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH +/** Maximum message length that the channel can send. Int valued, bytes. */ +#define GRPC_ARG_MAX_SEND_MESSAGE_LENGTH "grpc.max_send_message_length" /** Initial sequence number for http2 transports. Int valued. */ #define GRPC_ARG_HTTP2_INITIAL_SEQUENCE_NUMBER \ "grpc.http2.initial_sequence_number" diff --git a/package.xml b/package.xml index be8c08d766c..c2c8e0cdb5d 100644 --- a/package.xml +++ b/package.xml @@ -188,6 +188,7 @@ + @@ -347,6 +348,7 @@ + diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c new file mode 100644 index 00000000000..264c07ca8e8 --- /dev/null +++ b/src/core/lib/channel/message_size_filter.c @@ -0,0 +1,175 @@ +/* + * 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. + * + */ + +#include "src/core/lib/channel/message_size_filter.h" + +#include + +#include +#include +#include + +/* the protobuf library will (by default) start warning at 100megs */ +#define DEFAULT_MAX_MESSAGE_LENGTH (4 * 1024 * 1024) + +typedef struct call_data { + // Receive closures are chained: we inject this closure as the + // recv_message_ready up-call on transport_stream_op, and remember to + // call our next_recv_message_ready member after handling it. + grpc_closure recv_message_ready; + // Used by recv_message_ready. + grpc_byte_stream** recv_message; + // Original recv_message_ready callback, invoked after our own. + grpc_closure* next_recv_message_ready; +} call_data; + +typedef struct channel_data { + size_t max_send_size; + size_t max_recv_size; +} channel_data; + +// Callback invoked when we receive a message. Here we check the max +// receive message size. +static void recv_message_ready(grpc_exec_ctx* exec_ctx, void* user_data, + grpc_error* error) { + grpc_call_element* elem = user_data; + call_data* calld = elem->call_data; + channel_data* chand = elem->channel_data; + if ((*calld->recv_message)->length > chand->max_recv_size) { + char* message_string; + gpr_asprintf(&message_string, "Received message larger than max (%lu)", + chand->max_recv_size); + gpr_slice message = gpr_slice_from_copied_string(message_string); + gpr_free(message_string); + grpc_call_element_send_cancel_with_message( + exec_ctx, elem, GRPC_STATUS_INVALID_ARGUMENT, &message); + } + // Invoke the next callback. + calld->next_recv_message_ready->cb( + exec_ctx, calld->next_recv_message_ready->cb_arg, error); +} + +// Start transport op. +static void start_transport_stream_op(grpc_exec_ctx* exec_ctx, + grpc_call_element* elem, + grpc_transport_stream_op* op) { + call_data* calld = elem->call_data; + channel_data* chand = elem->channel_data; + // Check max send message size. + if (op->send_message != NULL && chand->max_send_size > 0 && + op->send_message->length > chand->max_send_size) { + char* message_string; + gpr_asprintf(&message_string, "Sent message larger than max (%lu)", + chand->max_send_size); + gpr_slice message = gpr_slice_from_copied_string(message_string); + gpr_free(message_string); + grpc_call_element_send_cancel_with_message( + exec_ctx, elem, GRPC_STATUS_INVALID_ARGUMENT, &message); + } + // Inject callback for receiving a message. + if (op->recv_message_ready != NULL && chand->max_recv_size > 0) { + calld->next_recv_message_ready = op->recv_message_ready; + calld->recv_message = op->recv_message; + op->recv_message_ready = &calld->recv_message_ready; + } + // Chain to the next filter. + grpc_call_next_op(exec_ctx, elem, op); +} + +// Constructor for call_data. +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->next_recv_message_ready = NULL; + grpc_closure_init(&calld->recv_message_ready, recv_message_ready, elem); + return GRPC_ERROR_NONE; +} + +// Destructor for call_data. +static void destroy_call_elem(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, + const grpc_call_final_info* final_info, + void* ignored) {} + +// Constructor for channel_data. +static void init_channel_elem(grpc_exec_ctx* exec_ctx, + grpc_channel_element* elem, + grpc_channel_element_args* args) { + GPR_ASSERT(!args->is_last); + channel_data* chand = elem->channel_data; + memset(chand, 0, sizeof(*chand)); + 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) { + if (args->channel_args->args[i].type != GRPC_ARG_INTEGER) { + gpr_log(GPR_ERROR, "%s ignored: it must be an integer", + GRPC_ARG_MAX_SEND_MESSAGE_LENGTH); + } else if (args->channel_args->args[i].value.integer < 0) { + gpr_log(GPR_ERROR, "%s ignored: it must be >= 0", + GRPC_ARG_MAX_SEND_MESSAGE_LENGTH); + } else { + chand->max_send_size = + (size_t)args->channel_args->args[i].value.integer; + } + } + if (strcmp(args->channel_args->args[i].key, + GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH) == 0) { + if (args->channel_args->args[i].type != GRPC_ARG_INTEGER) { + gpr_log(GPR_ERROR, "%s ignored: it must be an integer", + GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH); + } else if (args->channel_args->args[i].value.integer < 0) { + gpr_log(GPR_ERROR, "%s ignored: it must be >= 0", + GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH); + } else { + chand->max_recv_size = + (size_t)args->channel_args->args[i].value.integer; + } + } + } +} + +// Destructor for channel_data. +static void destroy_channel_elem(grpc_exec_ctx* exec_ctx, + grpc_channel_element* elem) {} + +const grpc_channel_filter grpc_message_size_filter = { + start_transport_stream_op, + grpc_channel_next_op, + sizeof(call_data), + init_call_elem, + grpc_call_stack_ignore_set_pollset_or_pollset_set, + destroy_call_elem, + sizeof(channel_data), + init_channel_elem, + destroy_channel_elem, + grpc_call_next_get_peer, + "message_size"}; diff --git a/src/core/lib/channel/message_size_filter.h b/src/core/lib/channel/message_size_filter.h new file mode 100644 index 00000000000..367a322b370 --- /dev/null +++ b/src/core/lib/channel/message_size_filter.h @@ -0,0 +1,40 @@ +/* + * 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_CHANNEL_MESSAGE_SIZE_FILTER_H +#define GRPC_CORE_LIB_CHANNEL_MESSAGE_SIZE_FILTER_H + +#include "src/core/lib/channel/channel_stack.h" + +extern const grpc_channel_filter grpc_message_size_filter; + +#endif /* GRPC_CORE_LIB_CHANNEL_MESSAGE_SIZE_FILTER_H */ diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c index 772681109a0..6717024ab2e 100644 --- a/src/core/lib/surface/call.c +++ b/src/core/lib/surface/call.c @@ -1160,17 +1160,6 @@ static void process_data_after_md(grpc_exec_ctx *exec_ctx, batch_control *bctl, if (gpr_unref(&bctl->steps_to_complete)) { post_batch_completion(exec_ctx, bctl); } - } else if (call->receiving_stream->length > - grpc_channel_get_max_message_length(call->channel)) { - cancel_with_status(exec_ctx, call, GRPC_STATUS_INTERNAL, - "Max message size exceeded"); - grpc_byte_stream_destroy(exec_ctx, call->receiving_stream); - call->receiving_stream = NULL; - *call->receiving_buffer = NULL; - call->receiving_message = 0; - if (gpr_unref(&bctl->steps_to_complete)) { - post_batch_completion(exec_ctx, bctl); - } } else { call->test_only_last_message_flags = call->receiving_stream->flags; if ((call->receiving_stream->flags & GRPC_WRITE_INTERNAL_COMPRESS) && diff --git a/src/core/lib/surface/channel.c b/src/core/lib/surface/channel.c index 6d2b1c49352..0e8e5bbfcd7 100644 --- a/src/core/lib/surface/channel.c +++ b/src/core/lib/surface/channel.c @@ -64,7 +64,6 @@ typedef struct registered_call { struct grpc_channel { int is_client; - uint32_t max_message_length; grpc_compression_options compression_options; grpc_mdelem *default_authority; @@ -80,9 +79,6 @@ struct grpc_channel { #define CHANNEL_FROM_TOP_ELEM(top_elem) \ CHANNEL_FROM_CHANNEL_STACK(grpc_channel_stack_from_top_element(top_elem)) -/* the protobuf library will (by default) start warning at 100megs */ -#define DEFAULT_MAX_MESSAGE_LENGTH (4 * 1024 * 1024) - static void destroy_channel(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error); @@ -114,21 +110,10 @@ grpc_channel *grpc_channel_create(grpc_exec_ctx *exec_ctx, const char *target, gpr_mu_init(&channel->registered_call_mu); channel->registered_calls = NULL; - channel->max_message_length = DEFAULT_MAX_MESSAGE_LENGTH; grpc_compression_options_init(&channel->compression_options); if (args) { for (size_t i = 0; i < args->num_args; i++) { - if (0 == strcmp(args->args[i].key, GRPC_ARG_MAX_MESSAGE_LENGTH)) { - if (args->args[i].type != GRPC_ARG_INTEGER) { - gpr_log(GPR_ERROR, "%s ignored: it must be an integer", - GRPC_ARG_MAX_MESSAGE_LENGTH); - } else if (args->args[i].value.integer < 0) { - gpr_log(GPR_ERROR, "%s ignored: it must be >= 0", - GRPC_ARG_MAX_MESSAGE_LENGTH); - } else { - channel->max_message_length = (uint32_t)args->args[i].value.integer; - } - } else if (0 == strcmp(args->args[i].key, GRPC_ARG_DEFAULT_AUTHORITY)) { + if (0 == strcmp(args->args[i].key, GRPC_ARG_DEFAULT_AUTHORITY)) { if (args->args[i].type != GRPC_ARG_STRING) { gpr_log(GPR_ERROR, "%s ignored: it must be a string", GRPC_ARG_DEFAULT_AUTHORITY); @@ -371,7 +356,3 @@ grpc_mdelem *grpc_channel_get_reffed_status_elem(grpc_channel *channel, int i) { return grpc_mdelem_from_metadata_strings(GRPC_MDSTR_GRPC_STATUS, grpc_mdstr_from_string(tmp)); } - -uint32_t grpc_channel_get_max_message_length(grpc_channel *channel) { - return channel->max_message_length; -} diff --git a/src/core/lib/surface/channel.h b/src/core/lib/surface/channel.h index 4c629743462..23cc8656cab 100644 --- a/src/core/lib/surface/channel.h +++ b/src/core/lib/surface/channel.h @@ -64,7 +64,6 @@ grpc_channel_stack *grpc_channel_get_channel_stack(grpc_channel *channel); The returned elem is owned by the caller. */ grpc_mdelem *grpc_channel_get_reffed_status_elem(grpc_channel *channel, int status_code); -uint32_t grpc_channel_get_max_message_length(grpc_channel *channel); #ifdef GRPC_STREAM_REFCOUNT_DEBUG void grpc_channel_internal_ref(grpc_channel *channel, const char *reason); diff --git a/src/core/lib/surface/init.c b/src/core/lib/surface/init.c index 5397913a21f..1f2769f871d 100644 --- a/src/core/lib/surface/init.c +++ b/src/core/lib/surface/init.c @@ -45,6 +45,7 @@ #include "src/core/lib/channel/connected_channel.h" #include "src/core/lib/channel/http_client_filter.h" #include "src/core/lib/channel/http_server_filter.h" +#include "src/core/lib/channel/message_size_filter.h" #include "src/core/lib/debug/trace.h" #include "src/core/lib/http/parser.h" #include "src/core/lib/iomgr/executor.h" @@ -97,6 +98,15 @@ static bool maybe_add_http_filter(grpc_channel_stack_builder *builder, } static void register_builtin_channel_init() { + grpc_channel_init_register_stage( + GRPC_CLIENT_CHANNEL, GRPC_CHANNEL_INIT_BUILTIN_PRIORITY, prepend_filter, + (void *)&grpc_message_size_filter); + grpc_channel_init_register_stage( + GRPC_CLIENT_DIRECT_CHANNEL, GRPC_CHANNEL_INIT_BUILTIN_PRIORITY, + prepend_filter, (void *)&grpc_message_size_filter); + grpc_channel_init_register_stage( + GRPC_SERVER_CHANNEL, GRPC_CHANNEL_INIT_BUILTIN_PRIORITY, prepend_filter, + (void *)&grpc_message_size_filter); grpc_channel_init_register_stage( GRPC_CLIENT_CHANNEL, GRPC_CHANNEL_INIT_BUILTIN_PRIORITY, prepend_filter, (void *)&grpc_compress_filter); diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index 660e34d7420..491e38c4cfd 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -84,6 +84,7 @@ CORE_SOURCE_FILES = [ 'src/core/lib/channel/handshaker.c', 'src/core/lib/channel/http_client_filter.c', 'src/core/lib/channel/http_server_filter.c', + 'src/core/lib/channel/message_size_filter.c', 'src/core/lib/compression/compression.c', 'src/core/lib/compression/message_compress.c', 'src/core/lib/debug/trace.c', diff --git a/test/core/end2end/tests/max_message_length.c b/test/core/end2end/tests/max_message_length.c index 08d326ab4d6..c8e0748f005 100644 --- a/test/core/end2end/tests/max_message_length.c +++ b/test/core/end2end/tests/max_message_length.c @@ -100,19 +100,22 @@ static void end_test(grpc_end2end_test_fixture *f) { grpc_completion_queue_destroy(f->cq); } -static void test_max_message_length(grpc_end2end_test_config config) { +static void test_max_message_length(grpc_end2end_test_config config, + bool send_limit) { + gpr_log(GPR_INFO, "testing with send_limit=%d", send_limit); + grpc_end2end_test_fixture f; - grpc_arg server_arg; - grpc_channel_args server_args; + grpc_arg channel_arg; + grpc_channel_args channel_args; grpc_call *c; - grpc_call *s; + grpc_call *s = NULL; cq_verifier *cqv; grpc_op ops[6]; grpc_op *op; gpr_slice request_payload_slice = gpr_slice_from_copied_string("hello world"); grpc_byte_buffer *request_payload = grpc_raw_byte_buffer_create(&request_payload_slice, 1); - grpc_byte_buffer *recv_payload; + grpc_byte_buffer *recv_payload = NULL; grpc_metadata_array initial_metadata_recv; grpc_metadata_array trailing_metadata_recv; grpc_metadata_array request_metadata_recv; @@ -123,14 +126,17 @@ static void test_max_message_length(grpc_end2end_test_config config) { size_t details_capacity = 0; int was_cancelled = 2; - server_arg.key = GRPC_ARG_MAX_MESSAGE_LENGTH; - server_arg.type = GRPC_ARG_INTEGER; - server_arg.value.integer = 5; + channel_arg.key = send_limit ? GRPC_ARG_MAX_SEND_MESSAGE_LENGTH + : GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH; + channel_arg.type = GRPC_ARG_INTEGER; + channel_arg.value.integer = 5; - server_args.num_args = 1; - server_args.args = &server_arg; + channel_args.num_args = 1; + channel_args.args = &channel_arg; - f = begin_test(config, "test_max_message_length", NULL, &server_args); + f = begin_test(config, "test_max_message_length", + send_limit ? &channel_args : NULL, + send_limit ? NULL : &channel_args); cqv = cq_verifier_create(f.cq); c = grpc_channel_create_call(f.client, NULL, GRPC_PROPAGATE_DEFAULTS, f.cq, @@ -175,6 +181,12 @@ static void test_max_message_length(grpc_end2end_test_config config) { error = grpc_call_start_batch(c, ops, (size_t)(op - ops), tag(1), NULL); GPR_ASSERT(GRPC_CALL_OK == error); + if (send_limit) { + cq_expect_completion(cqv, tag(1), 1); + cq_verify(cqv); + goto done; + } + error = grpc_server_request_call(f.server, &s, &call_details, &request_metadata_recv, f.cq, f.cq, tag(101)); @@ -201,11 +213,12 @@ static void test_max_message_length(grpc_end2end_test_config config) { cq_expect_completion(cqv, tag(1), 1); cq_verify(cqv); - GPR_ASSERT(status != GRPC_STATUS_OK); GPR_ASSERT(0 == strcmp(call_details.method, "/foo")); GPR_ASSERT(0 == strcmp(call_details.host, "foo.test.google.fr:1234")); GPR_ASSERT(was_cancelled == 1); - GPR_ASSERT(recv_payload == NULL); + +done: + GPR_ASSERT(status == GRPC_STATUS_INVALID_ARGUMENT); gpr_free(details); grpc_metadata_array_destroy(&initial_metadata_recv); @@ -213,9 +226,10 @@ static void test_max_message_length(grpc_end2end_test_config config) { grpc_metadata_array_destroy(&request_metadata_recv); grpc_call_details_destroy(&call_details); grpc_byte_buffer_destroy(request_payload); + grpc_byte_buffer_destroy(recv_payload); grpc_call_destroy(c); - grpc_call_destroy(s); + if (s != NULL) grpc_call_destroy(s); cq_verifier_destroy(cqv); @@ -224,7 +238,8 @@ static void test_max_message_length(grpc_end2end_test_config config) { } void max_message_length(grpc_end2end_test_config config) { - test_max_message_length(config); + test_max_message_length(config, true); + test_max_message_length(config, false); } void max_message_length_pre_init(void) {} diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 50cebfa84e4..d8e1dbc613c 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -799,6 +799,7 @@ src/core/lib/channel/context.h \ src/core/lib/channel/handshaker.h \ src/core/lib/channel/http_client_filter.h \ src/core/lib/channel/http_server_filter.h \ +src/core/lib/channel/message_size_filter.h \ src/core/lib/compression/algorithm_metadata.h \ src/core/lib/compression/message_compress.h \ src/core/lib/debug/trace.h \ @@ -958,6 +959,7 @@ src/core/lib/channel/connected_channel.c \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ +src/core/lib/channel/message_size_filter.c \ src/core/lib/compression/compression.c \ src/core/lib/compression/message_compress.c \ src/core/lib/debug/trace.c \ diff --git a/tools/run_tests/sources_and_headers.json b/tools/run_tests/sources_and_headers.json index 84f7da8cb9a..e3f43da8c7c 100644 --- a/tools/run_tests/sources_and_headers.json +++ b/tools/run_tests/sources_and_headers.json @@ -5825,6 +5825,7 @@ "src/core/lib/channel/handshaker.h", "src/core/lib/channel/http_client_filter.h", "src/core/lib/channel/http_server_filter.h", + "src/core/lib/channel/message_size_filter.h", "src/core/lib/compression/algorithm_metadata.h", "src/core/lib/compression/message_compress.h", "src/core/lib/debug/trace.h", @@ -5923,6 +5924,8 @@ "src/core/lib/channel/http_client_filter.h", "src/core/lib/channel/http_server_filter.c", "src/core/lib/channel/http_server_filter.h", + "src/core/lib/channel/message_size_filter.c", + "src/core/lib/channel/message_size_filter.h", "src/core/lib/compression/algorithm_metadata.h", "src/core/lib/compression/compression.c", "src/core/lib/compression/message_compress.c", diff --git a/vsprojects/vcxproj/grpc/grpc.vcxproj b/vsprojects/vcxproj/grpc/grpc.vcxproj index 298887e9157..e8e30818970 100644 --- a/vsprojects/vcxproj/grpc/grpc.vcxproj +++ b/vsprojects/vcxproj/grpc/grpc.vcxproj @@ -308,6 +308,7 @@ + @@ -478,6 +479,8 @@ + + diff --git a/vsprojects/vcxproj/grpc/grpc.vcxproj.filters b/vsprojects/vcxproj/grpc/grpc.vcxproj.filters index 539dbad3604..f8d97e9ab05 100644 --- a/vsprojects/vcxproj/grpc/grpc.vcxproj.filters +++ b/vsprojects/vcxproj/grpc/grpc.vcxproj.filters @@ -28,6 +28,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\compression @@ -692,6 +695,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\compression diff --git a/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj b/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj index 84a71d217d9..2e5fecc490a 100644 --- a/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj +++ b/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj @@ -198,6 +198,7 @@ + @@ -321,6 +322,8 @@ + + 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 18fe9264057..bfa0cdadcf5 100644 --- a/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj.filters +++ b/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj.filters @@ -76,6 +76,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\compression @@ -470,6 +473,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\compression diff --git a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj index 6a890656548..f2bcac82acc 100644 --- a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj +++ b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj @@ -297,6 +297,7 @@ + @@ -445,6 +446,8 @@ + + diff --git a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters index 8325d910ec2..679832388b4 100644 --- a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters +++ b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters @@ -31,6 +31,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\compression @@ -599,6 +602,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\compression From 370ef8941e261a19c53446cc5d337f59dca657d2 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 23 Aug 2016 15:08:37 -0700 Subject: [PATCH 02/40] Fix portability problem. --- src/core/lib/channel/message_size_filter.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index 264c07ca8e8..153f578a141 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -67,7 +67,7 @@ static void recv_message_ready(grpc_exec_ctx* exec_ctx, void* user_data, if ((*calld->recv_message)->length > chand->max_recv_size) { char* message_string; gpr_asprintf(&message_string, "Received message larger than max (%lu)", - chand->max_recv_size); + (unsigned long)chand->max_recv_size); gpr_slice message = gpr_slice_from_copied_string(message_string); gpr_free(message_string); grpc_call_element_send_cancel_with_message( @@ -89,7 +89,7 @@ static void start_transport_stream_op(grpc_exec_ctx* exec_ctx, op->send_message->length > chand->max_send_size) { char* message_string; gpr_asprintf(&message_string, "Sent message larger than max (%lu)", - chand->max_send_size); + (unsigned long)chand->max_send_size); gpr_slice message = gpr_slice_from_copied_string(message_string); gpr_free(message_string); grpc_call_element_send_cancel_with_message( From a6f1b98f2c125ed3df7397f89e6e5f9defc5c39a Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 24 Aug 2016 08:16:16 -0700 Subject: [PATCH 03/40] Set the right default max message sizes. Also use single-line comment style throughout the new files. --- src/core/lib/channel/message_size_filter.c | 69 +++++++++++----------- src/core/lib/channel/message_size_filter.h | 63 ++++++++++---------- 2 files changed, 66 insertions(+), 66 deletions(-) diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index 153f578a141..78aafc7d3d5 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -1,34 +1,33 @@ -/* - * 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. - * - */ +// +// 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. +// #include "src/core/lib/channel/message_size_filter.h" @@ -38,7 +37,7 @@ #include #include -/* the protobuf library will (by default) start warning at 100megs */ +// The protobuf library will (by default) start warning at 100 megs. #define DEFAULT_MAX_MESSAGE_LENGTH (4 * 1024 * 1024) typedef struct call_data { @@ -85,7 +84,7 @@ static void start_transport_stream_op(grpc_exec_ctx* exec_ctx, call_data* calld = elem->call_data; channel_data* chand = elem->channel_data; // Check max send message size. - if (op->send_message != NULL && chand->max_send_size > 0 && + if (op->send_message != NULL && op->send_message->length > chand->max_send_size) { char* message_string; gpr_asprintf(&message_string, "Sent message larger than max (%lu)", @@ -96,7 +95,7 @@ static void start_transport_stream_op(grpc_exec_ctx* exec_ctx, exec_ctx, elem, GRPC_STATUS_INVALID_ARGUMENT, &message); } // Inject callback for receiving a message. - if (op->recv_message_ready != NULL && chand->max_recv_size > 0) { + if (op->recv_message_ready != NULL) { calld->next_recv_message_ready = op->recv_message_ready; calld->recv_message = op->recv_message; op->recv_message_ready = &calld->recv_message_ready; @@ -127,6 +126,8 @@ 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_MESSAGE_LENGTH; + chand->max_recv_size = DEFAULT_MAX_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) { diff --git a/src/core/lib/channel/message_size_filter.h b/src/core/lib/channel/message_size_filter.h index 367a322b370..9460360b957 100644 --- a/src/core/lib/channel/message_size_filter.h +++ b/src/core/lib/channel/message_size_filter.h @@ -1,34 +1,33 @@ -/* - * 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. - * - */ +// +// 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_CHANNEL_MESSAGE_SIZE_FILTER_H #define GRPC_CORE_LIB_CHANNEL_MESSAGE_SIZE_FILTER_H @@ -37,4 +36,4 @@ extern const grpc_channel_filter grpc_message_size_filter; -#endif /* GRPC_CORE_LIB_CHANNEL_MESSAGE_SIZE_FILTER_H */ +#endif // GRPC_CORE_LIB_CHANNEL_MESSAGE_SIZE_FILTER_H From a30678039f41e70f176078ab2538c9d0b1201d2a Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 24 Aug 2016 08:19:50 -0700 Subject: [PATCH 04/40] Remove check for max message size from invalid_channel_args test, since this parameter is no longer checked at channel creation time. --- test/core/surface/invalid_channel_args_test.c | 34 ------------------- 1 file changed, 34 deletions(-) diff --git a/test/core/surface/invalid_channel_args_test.c b/test/core/surface/invalid_channel_args_test.c index 1b1b8b8f920..0640879866f 100644 --- a/test/core/surface/invalid_channel_args_test.c +++ b/test/core/surface/invalid_channel_args_test.c @@ -84,38 +84,6 @@ static void one_test(grpc_channel_args *args, char *expected_error_message) { static void test_no_error_message(void) { one_test(NULL, NULL); } -static void test_max_message_length_type(void) { - grpc_arg client_arg; - grpc_channel_args client_args; - char *expected_error_message; - - client_arg.type = GRPC_ARG_STRING; - client_arg.key = GRPC_ARG_MAX_MESSAGE_LENGTH; - client_arg.value.string = NULL; - - client_args.num_args = 1; - client_args.args = &client_arg; - expected_error_message = compose_error_string( - GRPC_ARG_MAX_MESSAGE_LENGTH, " ignored: it must be an integer"); - one_test(&client_args, expected_error_message); -} - -static void test_max_message_length_negative(void) { - grpc_arg client_arg; - grpc_channel_args client_args; - char *expected_error_message; - - client_arg.type = GRPC_ARG_INTEGER; - client_arg.key = GRPC_ARG_MAX_MESSAGE_LENGTH; - client_arg.value.integer = -1; - - client_args.num_args = 1; - client_args.args = &client_arg; - expected_error_message = compose_error_string(GRPC_ARG_MAX_MESSAGE_LENGTH, - " ignored: it must be >= 0"); - one_test(&client_args, expected_error_message); -} - static void test_default_authority_type(void) { grpc_arg client_arg; grpc_channel_args client_args; @@ -174,8 +142,6 @@ int main(int argc, char **argv) { gpr_set_log_function(log_error_sink); test_no_error_message(); - test_max_message_length_type(); - test_max_message_length_negative(); test_default_authority_type(); test_ssl_name_override_type(); test_ssl_name_override_failed(); From 500a466217760749aebf4a1204781aa36e8ded62 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 24 Aug 2016 14:28:08 -0700 Subject: [PATCH 05/40] Add filter to subchannel instead of main client channel. --- src/core/lib/surface/init.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/lib/surface/init.c b/src/core/lib/surface/init.c index 1f2769f871d..57cf6f53140 100644 --- a/src/core/lib/surface/init.c +++ b/src/core/lib/surface/init.c @@ -99,8 +99,8 @@ static bool maybe_add_http_filter(grpc_channel_stack_builder *builder, static void register_builtin_channel_init() { grpc_channel_init_register_stage( - GRPC_CLIENT_CHANNEL, GRPC_CHANNEL_INIT_BUILTIN_PRIORITY, prepend_filter, - (void *)&grpc_message_size_filter); + GRPC_CLIENT_SUBCHANNEL, GRPC_CHANNEL_INIT_BUILTIN_PRIORITY, + prepend_filter, (void *)&grpc_message_size_filter); grpc_channel_init_register_stage( GRPC_CLIENT_DIRECT_CHANNEL, GRPC_CHANNEL_INIT_BUILTIN_PRIORITY, prepend_filter, (void *)&grpc_message_size_filter); From 0002d8322260ad0ae4c7f4266ca7494194f12d45 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 25 Aug 2016 07:57:24 -0700 Subject: [PATCH 06/40] Fix crash caused by invoking receive callback with recv_message=NULL. --- src/core/lib/channel/message_size_filter.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index 78aafc7d3d5..10cb1998d13 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -63,7 +63,8 @@ static void recv_message_ready(grpc_exec_ctx* exec_ctx, void* user_data, grpc_call_element* elem = user_data; call_data* calld = elem->call_data; channel_data* chand = elem->channel_data; - if ((*calld->recv_message)->length > chand->max_recv_size) { + if (*calld->recv_message != NULL && + (*calld->recv_message)->length > chand->max_recv_size) { char* message_string; gpr_asprintf(&message_string, "Received message larger than max (%lu)", (unsigned long)chand->max_recv_size); From c3c6fafef8485dd5357d4771b53e25621f0d8080 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 26 Aug 2016 09:20:49 -0700 Subject: [PATCH 07/40] Add grpc_channel_arg_get_integer() utility function. --- src/core/ext/client_config/subchannel.c | 26 +++--- .../chttp2/transport/chttp2_transport.c | 92 +++++++++---------- src/core/lib/channel/channel_args.c | 18 ++++ src/core/lib/channel/channel_args.h | 8 ++ src/core/lib/channel/message_size_filter.c | 28 ++---- 5 files changed, 87 insertions(+), 85 deletions(-) diff --git a/src/core/ext/client_config/subchannel.c b/src/core/ext/client_config/subchannel.c index df35904b85d..3854e757c25 100644 --- a/src/core/ext/client_config/subchannel.c +++ b/src/core/ext/client_config/subchannel.c @@ -33,6 +33,7 @@ #include "src/core/ext/client_config/subchannel.h" +#include #include #include @@ -347,21 +348,16 @@ grpc_subchannel *grpc_subchannel_create(grpc_exec_ctx *exec_ctx, } if (0 == strcmp(c->args->args[i].key, GRPC_ARG_MAX_RECONNECT_BACKOFF_MS)) { - if (c->args->args[i].type == GRPC_ARG_INTEGER) { - if (c->args->args[i].value.integer >= 0) { - gpr_backoff_init( - &c->backoff_state, GRPC_SUBCHANNEL_RECONNECT_BACKOFF_MULTIPLIER, - GRPC_SUBCHANNEL_RECONNECT_JITTER, - GPR_MIN(c->args->args[i].value.integer, - GRPC_SUBCHANNEL_INITIAL_CONNECT_BACKOFF_SECONDS * 1000), - c->args->args[i].value.integer); - } else { - gpr_log(GPR_ERROR, GRPC_ARG_MAX_RECONNECT_BACKOFF_MS - " : must be non-negative"); - } - } else { - gpr_log(GPR_ERROR, - GRPC_ARG_MAX_RECONNECT_BACKOFF_MS " : must be an integer"); + const grpc_integer_options options = {-1, 0, INT_MAX}; + const int value = grpc_channel_arg_get_integer(&c->args->args[i], + options); + if (value >= 0) { + gpr_backoff_init( + &c->backoff_state, GRPC_SUBCHANNEL_RECONNECT_BACKOFF_MULTIPLIER, + GRPC_SUBCHANNEL_RECONNECT_JITTER, + GPR_MIN(value, + GRPC_SUBCHANNEL_INITIAL_CONNECT_BACKOFF_SECONDS * 1000), + value); } } } diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index 0e8dfb7d968..21759dd9afc 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -33,6 +33,7 @@ #include "src/core/ext/transport/chttp2/transport/chttp2_transport.h" +#include #include #include #include @@ -46,6 +47,7 @@ #include "src/core/ext/transport/chttp2/transport/http2_errors.h" #include "src/core/ext/transport/chttp2/transport/internal.h" #include "src/core/ext/transport/chttp2/transport/status_conversion.h" +#include "src/core/lib/channel/channel_args.h" #include "src/core/lib/http/parser.h" #include "src/core/lib/iomgr/workqueue.h" #include "src/core/lib/profiling/timers.h" @@ -337,76 +339,66 @@ static void init_transport(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, if (is_client) { gpr_log(GPR_ERROR, "%s: is ignored on the client", GRPC_ARG_MAX_CONCURRENT_STREAMS); - } else if (channel_args->args[i].type != GRPC_ARG_INTEGER) { - gpr_log(GPR_ERROR, "%s: must be an integer", - GRPC_ARG_MAX_CONCURRENT_STREAMS); } else { - push_setting(exec_ctx, t, GRPC_CHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, - (uint32_t)channel_args->args[i].value.integer); + const grpc_integer_options options = {-1, 0, INT_MAX}; + const int value = grpc_channel_arg_get_integer(&channel_args->args[i], + options); + if (value >= 0) { + push_setting(exec_ctx, t, + GRPC_CHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, + (uint32_t)value); + } } } else if (0 == strcmp(channel_args->args[i].key, GRPC_ARG_HTTP2_INITIAL_SEQUENCE_NUMBER)) { - if (channel_args->args[i].type != GRPC_ARG_INTEGER) { - gpr_log(GPR_ERROR, "%s: must be an integer", - GRPC_ARG_HTTP2_INITIAL_SEQUENCE_NUMBER); - } else if ((t->global.next_stream_id & 1) != - (channel_args->args[i].value.integer & 1)) { - gpr_log(GPR_ERROR, "%s: low bit must be %d on %s", - GRPC_ARG_HTTP2_INITIAL_SEQUENCE_NUMBER, - t->global.next_stream_id & 1, - is_client ? "client" : "server"); - } else { - t->global.next_stream_id = - (uint32_t)channel_args->args[i].value.integer; + const grpc_integer_options options = {-1, 0, INT_MAX}; + const int value = grpc_channel_arg_get_integer(&channel_args->args[i], + options); + if (value >= 0) { + if ((t->global.next_stream_id & 1) != (value & 1)) { + gpr_log(GPR_ERROR, "%s: low bit must be %d on %s", + GRPC_ARG_HTTP2_INITIAL_SEQUENCE_NUMBER, + t->global.next_stream_id & 1, + is_client ? "client" : "server"); + } else { + t->global.next_stream_id = (uint32_t)value; + } } } else if (0 == strcmp(channel_args->args[i].key, GRPC_ARG_HTTP2_STREAM_LOOKAHEAD_BYTES)) { - if (channel_args->args[i].type != GRPC_ARG_INTEGER) { - gpr_log(GPR_ERROR, "%s: must be an integer", - GRPC_ARG_HTTP2_STREAM_LOOKAHEAD_BYTES); - } else if (channel_args->args[i].value.integer <= 5) { - gpr_log(GPR_ERROR, "%s: must be at least 5", - GRPC_ARG_HTTP2_STREAM_LOOKAHEAD_BYTES); - } else { - t->global.stream_lookahead = - (uint32_t)channel_args->args[i].value.integer; + const grpc_integer_options options = {-1, 5, INT_MAX}; + const int value = grpc_channel_arg_get_integer(&channel_args->args[i], + options); + if (value >= 0) { + t->global.stream_lookahead = (uint32_t)value; } } else if (0 == strcmp(channel_args->args[i].key, GRPC_ARG_HTTP2_HPACK_TABLE_SIZE_DECODER)) { - if (channel_args->args[i].type != GRPC_ARG_INTEGER) { - gpr_log(GPR_ERROR, "%s: must be an integer", - GRPC_ARG_HTTP2_HPACK_TABLE_SIZE_DECODER); - } else if (channel_args->args[i].value.integer < 0) { - gpr_log(GPR_ERROR, "%s: must be non-negative", - GRPC_ARG_HTTP2_HPACK_TABLE_SIZE_DECODER); - } else { + const grpc_integer_options options = {-1, 0, INT_MAX}; + const int value = grpc_channel_arg_get_integer(&channel_args->args[i], + options); + if (value >= 0) { push_setting(exec_ctx, t, GRPC_CHTTP2_SETTINGS_HEADER_TABLE_SIZE, - (uint32_t)channel_args->args[i].value.integer); + (uint32_t)value); } } else if (0 == strcmp(channel_args->args[i].key, GRPC_ARG_HTTP2_HPACK_TABLE_SIZE_ENCODER)) { - if (channel_args->args[i].type != GRPC_ARG_INTEGER) { - gpr_log(GPR_ERROR, "%s: must be an integer", - GRPC_ARG_HTTP2_HPACK_TABLE_SIZE_ENCODER); - } else if (channel_args->args[i].value.integer < 0) { - gpr_log(GPR_ERROR, "%s: must be non-negative", - GRPC_ARG_HTTP2_HPACK_TABLE_SIZE_ENCODER); - } else { + const grpc_integer_options options = {-1, 0, INT_MAX}; + const int value = grpc_channel_arg_get_integer(&channel_args->args[i], + options); + if (value >= 0) { grpc_chttp2_hpack_compressor_set_max_usable_size( &t->writing.hpack_compressor, - (uint32_t)channel_args->args[i].value.integer); + (uint32_t)value); } } else if (0 == strcmp(channel_args->args[i].key, GRPC_ARG_MAX_METADATA_SIZE)) { - if (channel_args->args[i].type != GRPC_ARG_INTEGER) { - gpr_log(GPR_ERROR, "%s: must be an integer", - GRPC_ARG_MAX_METADATA_SIZE); - } else if (channel_args->args[i].value.integer < 0) { - gpr_log(GPR_ERROR, "%s: must be non-negative", - GRPC_ARG_MAX_METADATA_SIZE); - } else { + const grpc_integer_options options = {-1, 0, INT_MAX}; + const int value = grpc_channel_arg_get_integer(&channel_args->args[i], + options); + if (value >= 0) { push_setting(exec_ctx, t, GRPC_CHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE, - (uint32_t)channel_args->args[i].value.integer); + (uint32_t)value); } } } diff --git a/src/core/lib/channel/channel_args.c b/src/core/lib/channel/channel_args.c index 79ceeb66b38..3a56b1ff205 100644 --- a/src/core/lib/channel/channel_args.c +++ b/src/core/lib/channel/channel_args.c @@ -271,3 +271,21 @@ int grpc_channel_args_compare(const grpc_channel_args *a, } return 0; } + +int grpc_channel_arg_get_integer(grpc_arg *arg, grpc_integer_options options) { + if (arg->type != GRPC_ARG_INTEGER) { + gpr_log(GPR_ERROR, "%s ignored: it must be an integer", arg->key); + return options.default_value; + } + if (arg->value.integer < options.min_value) { + gpr_log(GPR_ERROR, "%s ignored: it must be >= %d", arg->key, + options.min_value); + return options.default_value; + } + if (arg->value.integer > options.max_value) { + gpr_log(GPR_ERROR, "%s ignored: it must be <= %d", arg->key, + options.max_value); + return options.default_value; + } + return arg->value.integer; +} diff --git a/src/core/lib/channel/channel_args.h b/src/core/lib/channel/channel_args.h index aec61ee7c62..71fde5ad7ed 100644 --- a/src/core/lib/channel/channel_args.h +++ b/src/core/lib/channel/channel_args.h @@ -89,4 +89,12 @@ uint32_t grpc_channel_args_compression_algorithm_get_states( int grpc_channel_args_compare(const grpc_channel_args *a, const grpc_channel_args *b); +/** Returns the value of \a arg, subject to the contraints in \a options. */ +typedef struct grpc_integer_options { + int default_value; // Return this if value is outside of expected bounds. + int min_value; + int max_value; +} grpc_integer_options; +int grpc_channel_arg_get_integer(grpc_arg *arg, grpc_integer_options options); + #endif /* GRPC_CORE_LIB_CHANNEL_CHANNEL_ARGS_H */ diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index 10cb1998d13..ee9e3ee7346 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -31,12 +31,15 @@ #include "src/core/lib/channel/message_size_filter.h" +#include #include #include #include #include +#include "src/core/lib/channel/channel_args.h" + // The protobuf library will (by default) start warning at 100 megs. #define DEFAULT_MAX_MESSAGE_LENGTH (4 * 1024 * 1024) @@ -129,32 +132,17 @@ static void init_channel_elem(grpc_exec_ctx* exec_ctx, memset(chand, 0, sizeof(*chand)); chand->max_send_size = DEFAULT_MAX_MESSAGE_LENGTH; chand->max_recv_size = DEFAULT_MAX_MESSAGE_LENGTH; + const grpc_integer_options options = {DEFAULT_MAX_MESSAGE_LENGTH, 0, INT_MAX}; 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) { - if (args->channel_args->args[i].type != GRPC_ARG_INTEGER) { - gpr_log(GPR_ERROR, "%s ignored: it must be an integer", - GRPC_ARG_MAX_SEND_MESSAGE_LENGTH); - } else if (args->channel_args->args[i].value.integer < 0) { - gpr_log(GPR_ERROR, "%s ignored: it must be >= 0", - GRPC_ARG_MAX_SEND_MESSAGE_LENGTH); - } else { - chand->max_send_size = - (size_t)args->channel_args->args[i].value.integer; - } + chand->max_send_size = (size_t)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) { - if (args->channel_args->args[i].type != GRPC_ARG_INTEGER) { - gpr_log(GPR_ERROR, "%s ignored: it must be an integer", - GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH); - } else if (args->channel_args->args[i].value.integer < 0) { - gpr_log(GPR_ERROR, "%s ignored: it must be >= 0", - GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH); - } else { - chand->max_recv_size = - (size_t)args->channel_args->args[i].value.integer; - } + chand->max_recv_size = (size_t)grpc_channel_arg_get_integer( + &args->channel_args->args[i], options); } } } From c09e21f546c87dea94cf1fedac41d0dafbc71f7b Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 26 Aug 2016 13:34:53 -0700 Subject: [PATCH 08/40] clang-format --- src/core/ext/client_config/subchannel.c | 4 +-- .../chttp2/transport/chttp2_transport.c | 27 +++++++++---------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/core/ext/client_config/subchannel.c b/src/core/ext/client_config/subchannel.c index 3854e757c25..69f9ddae138 100644 --- a/src/core/ext/client_config/subchannel.c +++ b/src/core/ext/client_config/subchannel.c @@ -349,8 +349,8 @@ grpc_subchannel *grpc_subchannel_create(grpc_exec_ctx *exec_ctx, if (0 == strcmp(c->args->args[i].key, GRPC_ARG_MAX_RECONNECT_BACKOFF_MS)) { const grpc_integer_options options = {-1, 0, INT_MAX}; - const int value = grpc_channel_arg_get_integer(&c->args->args[i], - options); + const int value = + grpc_channel_arg_get_integer(&c->args->args[i], options); if (value >= 0) { gpr_backoff_init( &c->backoff_state, GRPC_SUBCHANNEL_RECONNECT_BACKOFF_MULTIPLIER, diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index 21759dd9afc..93d21c07d26 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -341,8 +341,8 @@ static void init_transport(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, GRPC_ARG_MAX_CONCURRENT_STREAMS); } else { const grpc_integer_options options = {-1, 0, INT_MAX}; - const int value = grpc_channel_arg_get_integer(&channel_args->args[i], - options); + const int value = + grpc_channel_arg_get_integer(&channel_args->args[i], options); if (value >= 0) { push_setting(exec_ctx, t, GRPC_CHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, @@ -352,8 +352,8 @@ static void init_transport(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, } else if (0 == strcmp(channel_args->args[i].key, GRPC_ARG_HTTP2_INITIAL_SEQUENCE_NUMBER)) { const grpc_integer_options options = {-1, 0, INT_MAX}; - const int value = grpc_channel_arg_get_integer(&channel_args->args[i], - options); + const int value = + grpc_channel_arg_get_integer(&channel_args->args[i], options); if (value >= 0) { if ((t->global.next_stream_id & 1) != (value & 1)) { gpr_log(GPR_ERROR, "%s: low bit must be %d on %s", @@ -367,16 +367,16 @@ static void init_transport(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, } else if (0 == strcmp(channel_args->args[i].key, GRPC_ARG_HTTP2_STREAM_LOOKAHEAD_BYTES)) { const grpc_integer_options options = {-1, 5, INT_MAX}; - const int value = grpc_channel_arg_get_integer(&channel_args->args[i], - options); + const int value = + grpc_channel_arg_get_integer(&channel_args->args[i], options); if (value >= 0) { t->global.stream_lookahead = (uint32_t)value; } } else if (0 == strcmp(channel_args->args[i].key, GRPC_ARG_HTTP2_HPACK_TABLE_SIZE_DECODER)) { const grpc_integer_options options = {-1, 0, INT_MAX}; - const int value = grpc_channel_arg_get_integer(&channel_args->args[i], - options); + const int value = + grpc_channel_arg_get_integer(&channel_args->args[i], options); if (value >= 0) { push_setting(exec_ctx, t, GRPC_CHTTP2_SETTINGS_HEADER_TABLE_SIZE, (uint32_t)value); @@ -384,18 +384,17 @@ static void init_transport(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, } else if (0 == strcmp(channel_args->args[i].key, GRPC_ARG_HTTP2_HPACK_TABLE_SIZE_ENCODER)) { const grpc_integer_options options = {-1, 0, INT_MAX}; - const int value = grpc_channel_arg_get_integer(&channel_args->args[i], - options); + const int value = + grpc_channel_arg_get_integer(&channel_args->args[i], options); if (value >= 0) { grpc_chttp2_hpack_compressor_set_max_usable_size( - &t->writing.hpack_compressor, - (uint32_t)value); + &t->writing.hpack_compressor, (uint32_t)value); } } else if (0 == strcmp(channel_args->args[i].key, GRPC_ARG_MAX_METADATA_SIZE)) { const grpc_integer_options options = {-1, 0, INT_MAX}; - const int value = grpc_channel_arg_get_integer(&channel_args->args[i], - options); + const int value = + grpc_channel_arg_get_integer(&channel_args->args[i], options); if (value >= 0) { push_setting(exec_ctx, t, GRPC_CHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE, (uint32_t)value); From ee7000a5605bd7512259922c59705f13e04066e5 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 26 Aug 2016 13:37:09 -0700 Subject: [PATCH 09/40] Switch back to C-style comment to appease check_include_guards.py. --- src/core/lib/channel/message_size_filter.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/lib/channel/message_size_filter.h b/src/core/lib/channel/message_size_filter.h index 9460360b957..a88ff7f81a1 100644 --- a/src/core/lib/channel/message_size_filter.h +++ b/src/core/lib/channel/message_size_filter.h @@ -36,4 +36,4 @@ extern const grpc_channel_filter grpc_message_size_filter; -#endif // GRPC_CORE_LIB_CHANNEL_MESSAGE_SIZE_FILTER_H +#endif /* GRPC_CORE_LIB_CHANNEL_MESSAGE_SIZE_FILTER_H */ From b71af5f6209055b7653adb382381486219de6835 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 26 Aug 2016 13:38:16 -0700 Subject: [PATCH 10/40] Re-run generate_projects.sh. --- BUILD | 4 ++++ CMakeLists.txt | 2 ++ Makefile | 2 ++ tools/doxygen/Doxyfile.c++.internal | 2 ++ vsprojects/vcxproj/grpc++/grpc++.vcxproj | 3 +++ vsprojects/vcxproj/grpc++/grpc++.vcxproj.filters | 6 ++++++ vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj | 3 +++ .../vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj.filters | 6 ++++++ 8 files changed, 28 insertions(+) diff --git a/BUILD b/BUILD index 6181f66ffa2..cca2803a0a7 100644 --- a/BUILD +++ b/BUILD @@ -1270,6 +1270,7 @@ cc_library( "src/core/lib/channel/handshaker.h", "src/core/lib/channel/http_client_filter.h", "src/core/lib/channel/http_server_filter.h", + "src/core/lib/channel/message_size_filter.h", "src/core/lib/compression/algorithm_metadata.h", "src/core/lib/compression/message_compress.h", "src/core/lib/debug/trace.h", @@ -1382,6 +1383,7 @@ cc_library( "src/core/lib/channel/handshaker.c", "src/core/lib/channel/http_client_filter.c", "src/core/lib/channel/http_server_filter.c", + "src/core/lib/channel/message_size_filter.c", "src/core/lib/compression/compression.c", "src/core/lib/compression/message_compress.c", "src/core/lib/debug/trace.c", @@ -1680,6 +1682,7 @@ cc_library( "src/core/lib/channel/handshaker.h", "src/core/lib/channel/http_client_filter.h", "src/core/lib/channel/http_server_filter.h", + "src/core/lib/channel/message_size_filter.h", "src/core/lib/compression/algorithm_metadata.h", "src/core/lib/compression/message_compress.h", "src/core/lib/debug/trace.h", @@ -1787,6 +1790,7 @@ cc_library( "src/core/lib/channel/handshaker.c", "src/core/lib/channel/http_client_filter.c", "src/core/lib/channel/http_server_filter.c", + "src/core/lib/channel/message_size_filter.c", "src/core/lib/compression/compression.c", "src/core/lib/compression/message_compress.c", "src/core/lib/debug/trace.c", diff --git a/CMakeLists.txt b/CMakeLists.txt index 10dcf440ff1..ddb22549ed2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1049,6 +1049,7 @@ add_library(grpc++ src/core/lib/channel/handshaker.c src/core/lib/channel/http_client_filter.c src/core/lib/channel/http_server_filter.c + src/core/lib/channel/message_size_filter.c src/core/lib/compression/compression.c src/core/lib/compression/message_compress.c src/core/lib/debug/trace.c @@ -1406,6 +1407,7 @@ add_library(grpc++_unsecure src/core/lib/channel/handshaker.c src/core/lib/channel/http_client_filter.c src/core/lib/channel/http_server_filter.c + src/core/lib/channel/message_size_filter.c src/core/lib/compression/compression.c src/core/lib/compression/message_compress.c src/core/lib/debug/trace.c diff --git a/Makefile b/Makefile index ed8080e5d3f..4607cdb1a32 100644 --- a/Makefile +++ b/Makefile @@ -3594,6 +3594,7 @@ LIBGRPC++_SRC = \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ + src/core/lib/channel/message_size_filter.c \ src/core/lib/compression/compression.c \ src/core/lib/compression/message_compress.c \ src/core/lib/debug/trace.c \ @@ -4229,6 +4230,7 @@ LIBGRPC++_UNSECURE_SRC = \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ + src/core/lib/channel/message_size_filter.c \ src/core/lib/compression/compression.c \ src/core/lib/compression/message_compress.c \ src/core/lib/debug/trace.c \ diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index 12eb6513848..1baa5bb7238 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -882,6 +882,7 @@ src/core/lib/channel/context.h \ src/core/lib/channel/handshaker.h \ src/core/lib/channel/http_client_filter.h \ src/core/lib/channel/http_server_filter.h \ +src/core/lib/channel/message_size_filter.h \ src/core/lib/compression/algorithm_metadata.h \ src/core/lib/compression/message_compress.h \ src/core/lib/debug/trace.h \ @@ -994,6 +995,7 @@ src/core/lib/channel/connected_channel.c \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ +src/core/lib/channel/message_size_filter.c \ src/core/lib/compression/compression.c \ src/core/lib/compression/message_compress.c \ src/core/lib/debug/trace.c \ diff --git a/vsprojects/vcxproj/grpc++/grpc++.vcxproj b/vsprojects/vcxproj/grpc++/grpc++.vcxproj index 321a403c497..042a7d59370 100644 --- a/vsprojects/vcxproj/grpc++/grpc++.vcxproj +++ b/vsprojects/vcxproj/grpc++/grpc++.vcxproj @@ -382,6 +382,7 @@ + @@ -537,6 +538,8 @@ + + diff --git a/vsprojects/vcxproj/grpc++/grpc++.vcxproj.filters b/vsprojects/vcxproj/grpc++/grpc++.vcxproj.filters index b34ca03a535..bfd4afaf2b7 100644 --- a/vsprojects/vcxproj/grpc++/grpc++.vcxproj.filters +++ b/vsprojects/vcxproj/grpc++/grpc++.vcxproj.filters @@ -124,6 +124,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\compression @@ -740,6 +743,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\compression diff --git a/vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj b/vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj index a7bb3ef23d1..08a1857f76e 100644 --- a/vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj +++ b/vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj @@ -378,6 +378,7 @@ + @@ -523,6 +524,8 @@ + + diff --git a/vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj.filters b/vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj.filters index 4ad0ae31d93..3466aecfd58 100644 --- a/vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj.filters +++ b/vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj.filters @@ -109,6 +109,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\compression @@ -713,6 +716,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\compression From ed4605b71c8ed5ea1b4d4ff27dbea230a458fb44 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 31 Aug 2016 13:05:46 -0700 Subject: [PATCH 11/40] Change error messages to include actual values and limits. --- src/core/lib/channel/message_size_filter.c | 7 +++++-- test/core/end2end/tests/max_message_length.c | 4 ++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index ee9e3ee7346..e2d7ff795c4 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -69,7 +69,9 @@ static void recv_message_ready(grpc_exec_ctx* exec_ctx, void* user_data, if (*calld->recv_message != NULL && (*calld->recv_message)->length > chand->max_recv_size) { char* message_string; - gpr_asprintf(&message_string, "Received message larger than max (%lu)", + gpr_asprintf(&message_string, + "Received message larger than max (%lu vs. %lu)", + (unsigned long)(*calld->recv_message)->length, (unsigned long)chand->max_recv_size); gpr_slice message = gpr_slice_from_copied_string(message_string); gpr_free(message_string); @@ -91,7 +93,8 @@ static void start_transport_stream_op(grpc_exec_ctx* exec_ctx, if (op->send_message != NULL && op->send_message->length > chand->max_send_size) { char* message_string; - gpr_asprintf(&message_string, "Sent message larger than max (%lu)", + gpr_asprintf(&message_string, "Sent message larger than max (%lu vs. %lu)", + (unsigned long)op->send_message->length, (unsigned long)chand->max_send_size); gpr_slice message = gpr_slice_from_copied_string(message_string); gpr_free(message_string); diff --git a/test/core/end2end/tests/max_message_length.c b/test/core/end2end/tests/max_message_length.c index 33683aaecd3..7368bca308d 100644 --- a/test/core/end2end/tests/max_message_length.c +++ b/test/core/end2end/tests/max_message_length.c @@ -217,6 +217,10 @@ static void test_max_message_length(grpc_end2end_test_config config, done: GPR_ASSERT(status == GRPC_STATUS_INVALID_ARGUMENT); + GPR_ASSERT(strcmp(details, + send_limit + ? "Sent message larger than max (11 vs. 5)" + : "Received message larger than max (11 vs. 5)") == 0); gpr_free(details); grpc_metadata_array_destroy(&initial_metadata_recv); From 7331a7aa6201cc025d09da76056f5aae80184c1f Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 31 Aug 2016 13:09:27 -0700 Subject: [PATCH 12/40] Change wrapped languages to use the new channel arg macro name. --- src/cpp/server/server_builder.cc | 2 +- src/objective-c/GRPCClient/private/GRPCHost.m | 2 +- src/python/grpcio/grpc/_cython/_cygrpc/grpc.pxi | 2 +- src/python/grpcio/grpc/_cython/_cygrpc/records.pyx.pxi | 2 +- src/ruby/ext/grpc/rb_channel.c | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/cpp/server/server_builder.cc b/src/cpp/server/server_builder.cc index 45bb858e2ed..6cd9f982d7f 100644 --- a/src/cpp/server/server_builder.cc +++ b/src/cpp/server/server_builder.cc @@ -164,7 +164,7 @@ std::unique_ptr ServerBuilder::BuildAndStart() { } } if (max_message_size_ > 0) { - args.SetInt(GRPC_ARG_MAX_MESSAGE_LENGTH, max_message_size_); + args.SetInt(GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH, max_message_size_); } args.SetInt(GRPC_COMPRESSION_CHANNEL_ENABLED_ALGORITHMS_BITSET, enabled_compression_algorithms_bitset_); diff --git a/src/objective-c/GRPCClient/private/GRPCHost.m b/src/objective-c/GRPCClient/private/GRPCHost.m index 9cd9593d172..1159aac7a71 100644 --- a/src/objective-c/GRPCClient/private/GRPCHost.m +++ b/src/objective-c/GRPCClient/private/GRPCHost.m @@ -217,7 +217,7 @@ static NSMutableDictionary *kHostCache; } if (_responseSizeLimitOverride) { - args[@GRPC_ARG_MAX_MESSAGE_LENGTH] = _responseSizeLimitOverride; + args[@GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH] = _responseSizeLimitOverride; } return args; } diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/grpc.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/grpc.pxi index 42fced65457..dae28e15f2c 100644 --- a/src/python/grpcio/grpc/_cython/_cygrpc/grpc.pxi +++ b/src/python/grpcio/grpc/_cython/_cygrpc/grpc.pxi @@ -135,7 +135,7 @@ cdef extern from "grpc/grpc.h": const char *GRPC_ARG_PRIMARY_USER_AGENT_STRING const char *GRPC_ARG_ENABLE_CENSUS const char *GRPC_ARG_MAX_CONCURRENT_STREAMS - const char *GRPC_ARG_MAX_MESSAGE_LENGTH + const char *GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH const char *GRPC_ARG_HTTP2_INITIAL_SEQUENCE_NUMBER const char *GRPC_ARG_DEFAULT_AUTHORITY const char *GRPC_ARG_PRIMARY_USER_AGENT_STRING diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/records.pyx.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/records.pyx.pxi index 834a44123d4..f444e33cf07 100644 --- a/src/python/grpcio/grpc/_cython/_cygrpc/records.pyx.pxi +++ b/src/python/grpcio/grpc/_cython/_cygrpc/records.pyx.pxi @@ -39,7 +39,7 @@ class ConnectivityState: class ChannelArgKey: enable_census = GRPC_ARG_ENABLE_CENSUS max_concurrent_streams = GRPC_ARG_MAX_CONCURRENT_STREAMS - max_message_length = GRPC_ARG_MAX_MESSAGE_LENGTH + max_message_length = GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH http2_initial_sequence_number = GRPC_ARG_HTTP2_INITIAL_SEQUENCE_NUMBER default_authority = GRPC_ARG_DEFAULT_AUTHORITY primary_user_agent_string = GRPC_ARG_PRIMARY_USER_AGENT_STRING diff --git a/src/ruby/ext/grpc/rb_channel.c b/src/ruby/ext/grpc/rb_channel.c index e6d30a174b8..3b2b88eb774 100644 --- a/src/ruby/ext/grpc/rb_channel.c +++ b/src/ruby/ext/grpc/rb_channel.c @@ -386,7 +386,7 @@ void Init_grpc_channel() { rb_define_const(grpc_rb_cChannel, "MAX_CONCURRENT_STREAMS", ID2SYM(rb_intern(GRPC_ARG_MAX_CONCURRENT_STREAMS))); rb_define_const(grpc_rb_cChannel, "MAX_MESSAGE_LENGTH", - ID2SYM(rb_intern(GRPC_ARG_MAX_MESSAGE_LENGTH))); + ID2SYM(rb_intern(GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH))); id_insecure_channel = rb_intern("this_channel_is_insecure"); Init_grpc_propagate_masks(); Init_grpc_connectivity_states(); From a4d9ee23c229c1358f177b7e474b8f2842661e18 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 31 Aug 2016 13:17:26 -0700 Subject: [PATCH 13/40] Fix objective-C test case. --- src/objective-c/tests/InteropTests.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/objective-c/tests/InteropTests.m b/src/objective-c/tests/InteropTests.m index f04a7e6441f..44f22c9e85c 100644 --- a/src/objective-c/tests/InteropTests.m +++ b/src/objective-c/tests/InteropTests.m @@ -180,7 +180,7 @@ // - If you're developing the server, consider using response streaming, or let clients filter // responses by setting a google.protobuf.FieldMask in the request: // https://github.com/google/protobuf/blob/master/src/google/protobuf/field_mask.proto - XCTAssertEqualObjects(error.localizedDescription, @"Max message size exceeded"); + XCTAssertEqualObjects(error.localizedDescription, @"Received message larger than max (4194305 vs. 4194304)"); [expectation fulfill]; }]; From 6980362c4edadc833c02fbcb104bf4d4a2a1e081 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 6 Sep 2016 08:15:36 -0700 Subject: [PATCH 14/40] Allow setting max send message size via C++ ServerBuilder API. --- include/grpc++/impl/codegen/call.h | 73 ++++++++++--------- .../grpc++/impl/codegen/method_handler_impl.h | 4 +- include/grpc++/impl/codegen/proto_utils.h | 7 +- .../grpc++/impl/codegen/rpc_service_method.h | 4 +- .../impl/codegen/serialization_traits.h | 8 +- .../grpc++/impl/codegen/server_interface.h | 6 +- include/grpc++/server.h | 12 +-- include/grpc++/server_builder.h | 20 ++++- include/grpc++/support/byte_buffer.h | 2 +- src/cpp/server/server.cc | 10 +-- src/cpp/server/server_builder.cc | 15 ++-- 11 files changed, 94 insertions(+), 67 deletions(-) diff --git a/include/grpc++/impl/codegen/call.h b/include/grpc++/impl/codegen/call.h index dfac177970a..7a8f846744f 100644 --- a/include/grpc++/impl/codegen/call.h +++ b/include/grpc++/impl/codegen/call.h @@ -175,7 +175,7 @@ template class CallNoOp { protected: void AddOp(grpc_op* ops, size_t* nops) {} - void FinishOp(bool* status, int max_message_size) {} + void FinishOp(bool* status, int max_receive_message_size) {} }; class CallOpSendInitialMetadata { @@ -213,7 +213,7 @@ class CallOpSendInitialMetadata { op->data.send_initial_metadata.maybe_compression_level.level = maybe_compression_level_.level; } - void FinishOp(bool* status, int max_message_size) { + void FinishOp(bool* status, int max_receive_message_size) { if (!send_) return; g_core_codegen_interface->gpr_free(initial_metadata_); send_ = false; @@ -253,7 +253,7 @@ class CallOpSendMessage { // Flags are per-message: clear them after use. write_options_.Clear(); } - void FinishOp(bool* status, int max_message_size) { + void FinishOp(bool* status, int max_receive_message_size) { if (own_buf_) g_core_codegen_interface->grpc_byte_buffer_destroy(send_buf_); send_buf_ = nullptr; } @@ -301,13 +301,14 @@ class CallOpRecvMessage { op->data.recv_message = &recv_buf_; } - void FinishOp(bool* status, int max_message_size) { + void FinishOp(bool* status, int max_receive_message_size) { if (message_ == nullptr) return; if (recv_buf_) { if (*status) { - got_message = *status = SerializationTraits::Deserialize( - recv_buf_, message_, max_message_size) - .ok(); + got_message = *status = + SerializationTraits::Deserialize(recv_buf_, message_, + max_receive_message_size) + .ok(); } else { got_message = false; g_core_codegen_interface->grpc_byte_buffer_destroy(recv_buf_); @@ -330,7 +331,8 @@ class CallOpRecvMessage { namespace CallOpGenericRecvMessageHelper { class DeserializeFunc { public: - virtual Status Deserialize(grpc_byte_buffer* buf, int max_message_size) = 0; + virtual Status Deserialize(grpc_byte_buffer* buf, + int max_receive_message_size) = 0; virtual ~DeserializeFunc() {} }; @@ -339,8 +341,9 @@ class DeserializeFuncType GRPC_FINAL : public DeserializeFunc { public: DeserializeFuncType(R* message) : message_(message) {} Status Deserialize(grpc_byte_buffer* buf, - int max_message_size) GRPC_OVERRIDE { - return SerializationTraits::Deserialize(buf, message_, max_message_size); + int max_receive_message_size) GRPC_OVERRIDE { + return SerializationTraits::Deserialize(buf, message_, + max_receive_message_size); } ~DeserializeFuncType() GRPC_OVERRIDE {} @@ -379,12 +382,13 @@ class CallOpGenericRecvMessage { op->data.recv_message = &recv_buf_; } - void FinishOp(bool* status, int max_message_size) { + void FinishOp(bool* status, int max_receive_message_size) { if (!deserialize_) return; if (recv_buf_) { if (*status) { got_message = true; - *status = deserialize_->Deserialize(recv_buf_, max_message_size).ok(); + *status = + deserialize_->Deserialize(recv_buf_, max_receive_message_size).ok(); } else { got_message = false; g_core_codegen_interface->grpc_byte_buffer_destroy(recv_buf_); @@ -418,7 +422,7 @@ class CallOpClientSendClose { op->flags = 0; op->reserved = NULL; } - void FinishOp(bool* status, int max_message_size) { send_ = false; } + void FinishOp(bool* status, int max_receive_message_size) { send_ = false; } private: bool send_; @@ -453,7 +457,7 @@ class CallOpServerSendStatus { op->reserved = NULL; } - void FinishOp(bool* status, int max_message_size) { + void FinishOp(bool* status, int max_receive_message_size) { if (!send_status_available_) return; g_core_codegen_interface->gpr_free(trailing_metadata_); send_status_available_ = false; @@ -486,7 +490,7 @@ class CallOpRecvInitialMetadata { op->flags = 0; op->reserved = NULL; } - void FinishOp(bool* status, int max_message_size) { + void FinishOp(bool* status, int max_receive_message_size) { if (recv_initial_metadata_ == nullptr) return; FillMetadataMap(&recv_initial_metadata_arr_, recv_initial_metadata_); recv_initial_metadata_ = nullptr; @@ -525,7 +529,7 @@ class CallOpClientRecvStatus { op->reserved = NULL; } - void FinishOp(bool* status, int max_message_size) { + void FinishOp(bool* status, int max_receive_message_size) { if (recv_status_ == nullptr) return; FillMetadataMap(&recv_trailing_metadata_arr_, recv_trailing_metadata_); *recv_status_ = Status( @@ -562,13 +566,13 @@ class CallOpSetCollectionInterface /// API. class CallOpSetInterface : public CompletionQueueTag { public: - CallOpSetInterface() : max_message_size_(0) {} + CallOpSetInterface() : max_receive_message_size_(0) {} /// Fills in grpc_op, starting from ops[*nops] and moving /// upwards. virtual void FillOps(grpc_op* ops, size_t* nops) = 0; - void set_max_message_size(int max_message_size) { - max_message_size_ = max_message_size; + void set_max_receive_message_size(int max_receive_message_size) { + max_receive_message_size_ = max_receive_message_size; } /// Mark this as belonging to a collection if needed @@ -577,7 +581,7 @@ class CallOpSetInterface : public CompletionQueueTag { } protected: - int max_message_size_; + int max_receive_message_size_; std::shared_ptr collection_; }; @@ -609,12 +613,12 @@ class CallOpSet : public CallOpSetInterface, } bool FinalizeResult(void** tag, bool* status) GRPC_OVERRIDE { - this->Op1::FinishOp(status, max_message_size_); - this->Op2::FinishOp(status, max_message_size_); - this->Op3::FinishOp(status, max_message_size_); - this->Op4::FinishOp(status, max_message_size_); - this->Op5::FinishOp(status, max_message_size_); - this->Op6::FinishOp(status, max_message_size_); + this->Op1::FinishOp(status, max_receive_message_size_); + this->Op2::FinishOp(status, max_receive_message_size_); + this->Op3::FinishOp(status, max_receive_message_size_); + this->Op4::FinishOp(status, max_receive_message_size_); + this->Op5::FinishOp(status, max_receive_message_size_); + this->Op6::FinishOp(status, max_receive_message_size_); *tag = return_tag_; collection_.reset(); // drop the ref at this point return true; @@ -646,18 +650,21 @@ class Call GRPC_FINAL { public: /* call is owned by the caller */ Call(grpc_call* call, CallHook* call_hook, CompletionQueue* cq) - : call_hook_(call_hook), cq_(cq), call_(call), max_message_size_(-1) {} + : call_hook_(call_hook), + cq_(cq), + call_(call), + max_receive_message_size_(-1) {} Call(grpc_call* call, CallHook* call_hook, CompletionQueue* cq, - int max_message_size) + int max_receive_message_size) : call_hook_(call_hook), cq_(cq), call_(call), - max_message_size_(max_message_size) {} + max_receive_message_size_(max_receive_message_size) {} void PerformOps(CallOpSetInterface* ops) { - if (max_message_size_ > 0) { - ops->set_max_message_size(max_message_size_); + if (max_receive_message_size_ > 0) { + ops->set_max_receive_message_size(max_receive_message_size_); } call_hook_->PerformOpsOnCall(ops, this); } @@ -665,13 +672,13 @@ class Call GRPC_FINAL { grpc_call* call() { return call_; } CompletionQueue* cq() { return cq_; } - int max_message_size() { return max_message_size_; } + int max_receive_message_size() { return max_receive_message_size_; } private: CallHook* call_hook_; CompletionQueue* cq_; grpc_call* call_; - int max_message_size_; + int max_receive_message_size_; }; } // namespace grpc diff --git a/include/grpc++/impl/codegen/method_handler_impl.h b/include/grpc++/impl/codegen/method_handler_impl.h index 2f4be644bae..749924e4ec6 100644 --- a/include/grpc++/impl/codegen/method_handler_impl.h +++ b/include/grpc++/impl/codegen/method_handler_impl.h @@ -53,7 +53,7 @@ class RpcMethodHandler : public MethodHandler { void RunHandler(const HandlerParameter& param) GRPC_FINAL { RequestType req; Status status = SerializationTraits::Deserialize( - param.request, &req, param.max_message_size); + param.request, &req, param.max_receive_message_size); ResponseType rsp; if (status.ok()) { status = func_(service_, param.server_context, &req, &rsp); @@ -139,7 +139,7 @@ class ServerStreamingHandler : public MethodHandler { void RunHandler(const HandlerParameter& param) GRPC_FINAL { RequestType req; Status status = SerializationTraits::Deserialize( - param.request, &req, param.max_message_size); + param.request, &req, param.max_receive_message_size); if (status.ok()) { ServerWriter writer(param.call, param.server_context); diff --git a/include/grpc++/impl/codegen/proto_utils.h b/include/grpc++/impl/codegen/proto_utils.h index d4599c5ffff..e91e0b79fae 100644 --- a/include/grpc++/impl/codegen/proto_utils.h +++ b/include/grpc++/impl/codegen/proto_utils.h @@ -205,7 +205,7 @@ class SerializationTraits 0) { - decoder.SetTotalBytesLimit(max_message_size, max_message_size); + if (max_receive_message_size > 0) { + decoder.SetTotalBytesLimit(max_receive_message_size, + max_receive_message_size); } if (!msg->ParseFromCodedStream(&decoder)) { result = Status(StatusCode::INTERNAL, msg->InitializationErrorString()); diff --git a/include/grpc++/impl/codegen/rpc_service_method.h b/include/grpc++/impl/codegen/rpc_service_method.h index 8b1f026c912..01999ee7b45 100644 --- a/include/grpc++/impl/codegen/rpc_service_method.h +++ b/include/grpc++/impl/codegen/rpc_service_method.h @@ -59,12 +59,12 @@ class MethodHandler { : call(c), server_context(context), request(req), - max_message_size(max_size) {} + max_receive_message_size(max_size) {} Call* call; ServerContext* server_context; // Handler required to grpc_byte_buffer_destroy this grpc_byte_buffer* request; - int max_message_size; + int max_receive_message_size; }; virtual void RunHandler(const HandlerParameter& param) = 0; }; diff --git a/include/grpc++/impl/codegen/serialization_traits.h b/include/grpc++/impl/codegen/serialization_traits.h index fa99dbfa9c3..54e5d476327 100644 --- a/include/grpc++/impl/codegen/serialization_traits.h +++ b/include/grpc++/impl/codegen/serialization_traits.h @@ -43,10 +43,10 @@ namespace grpc { /// functions: /// static Status Serialize(const Message& msg, /// grpc_byte_buffer** buffer, -// bool* own_buffer); +/// bool* own_buffer); /// static Status Deserialize(grpc_byte_buffer* buffer, /// Message* msg, -/// int max_message_size); +/// int max_receive_message_size); /// /// Serialize is required to convert message to a grpc_byte_buffer, and /// to store a pointer to that byte buffer at *buffer. *own_buffer should @@ -54,8 +54,8 @@ namespace grpc { /// ownership is retained elsewhere. /// /// Deserialize is required to convert buffer into the message stored at -/// msg. max_message_size is passed in as a bound on the maximum number of -/// message bytes Deserialize should accept. +/// msg. max_receive_message_size is passed in as a bound on the maximum +/// number of message bytes Deserialize should accept. /// /// Both functions return a Status, allowing them to explain what went /// wrong if required. diff --git a/include/grpc++/impl/codegen/server_interface.h b/include/grpc++/impl/codegen/server_interface.h index 3a3e052d9ef..20e418d76eb 100644 --- a/include/grpc++/impl/codegen/server_interface.h +++ b/include/grpc++/impl/codegen/server_interface.h @@ -129,7 +129,7 @@ class ServerInterface : public CallHook { virtual void ShutdownInternal(gpr_timespec deadline) = 0; - virtual int max_message_size() const = 0; + virtual int max_receive_message_size() const = 0; virtual grpc_server* server() = 0; @@ -200,8 +200,8 @@ class ServerInterface : public CallHook { bool FinalizeResult(void** tag, bool* status) GRPC_OVERRIDE { bool serialization_status = *status && payload_ && - SerializationTraits::Deserialize(payload_, request_, - server_->max_message_size()) + SerializationTraits::Deserialize( + payload_, request_, server_->max_receive_message_size()) .ok(); bool ret = RegisteredAsyncRequest::FinalizeResult(tag, status); *status = serialization_status && *status; diff --git a/include/grpc++/server.h b/include/grpc++/server.h index 6876961e210..6e150351255 100644 --- a/include/grpc++/server.h +++ b/include/grpc++/server.h @@ -116,10 +116,10 @@ class Server GRPC_FINAL : public ServerInterface, private GrpcLibraryCodegen { /// /// \param thread_pool The threadpool instance to use for call processing. /// \param thread_pool_owned Does the server own the \a thread_pool instance? - /// \param max_message_size Maximum message length that the channel can - /// receive. + /// \param max_receive_message_size Maximum message length that the channel + /// can receive. Server(ThreadPoolInterface* thread_pool, bool thread_pool_owned, - int max_message_size, ChannelArguments* args); + int max_receive_message_size, ChannelArguments* args); /// Register a service. This call does not take ownership of the service. /// The service must exist for the lifetime of the Server instance. @@ -164,13 +164,15 @@ class Server GRPC_FINAL : public ServerInterface, private GrpcLibraryCodegen { void ShutdownInternal(gpr_timespec deadline) GRPC_OVERRIDE; - int max_message_size() const GRPC_OVERRIDE { return max_message_size_; }; + int max_receive_message_size() const GRPC_OVERRIDE { + return max_receive_message_size_; + }; grpc_server* server() GRPC_OVERRIDE { return server_; }; ServerInitializer* initializer(); - const int max_message_size_; + const int max_receive_message_size_; // Completion queue. CompletionQueue cq_; diff --git a/include/grpc++/server_builder.h b/include/grpc++/server_builder.h index b9c49f0b192..326962bf782 100644 --- a/include/grpc++/server_builder.h +++ b/include/grpc++/server_builder.h @@ -78,12 +78,23 @@ class ServerBuilder { /// Only matches requests with :authority \a host ServerBuilder& RegisterService(const grpc::string& host, Service* service); - /// Set max message size in bytes. - ServerBuilder& SetMaxMessageSize(int max_message_size) { - max_message_size_ = max_message_size; + /// Set max receive message size in bytes. + ServerBuilder& SetMaxReceiveMessageSize(int max_receive_message_size) { + max_receive_message_size_ = max_receive_message_size; + return *this; + } + + /// Set max send message size in bytes. + ServerBuilder& SetMaxSendMessageSize(int max_send_message_size) { + max_send_message_size_ = max_send_message_size; return *this; } + /// For backward compatibility. + ServerBuilder& SetMaxMessageSize(int max_message_size) { + return SetMaxReceiveMessageSize(max_message_size); + } + /// Set the support status for compression algorithms. All algorithms are /// enabled by default. /// @@ -168,7 +179,8 @@ class ServerBuilder { Service* service; }; - int max_message_size_; + int max_receive_message_size_; + int max_send_message_size_; std::vector> options_; std::vector> services_; std::vector ports_; diff --git a/include/grpc++/support/byte_buffer.h b/include/grpc++/support/byte_buffer.h index 01249a0b88b..06f8969b704 100644 --- a/include/grpc++/support/byte_buffer.h +++ b/include/grpc++/support/byte_buffer.h @@ -96,7 +96,7 @@ template <> class SerializationTraits { public: static Status Deserialize(grpc_byte_buffer* byte_buffer, ByteBuffer* dest, - int max_message_size) { + int max_receive_message_size) { dest->set_buffer(byte_buffer); return Status::OK; } diff --git a/src/cpp/server/server.cc b/src/cpp/server/server.cc index af04fd4ca64..a693ce9b8ee 100644 --- a/src/cpp/server/server.cc +++ b/src/cpp/server/server.cc @@ -220,7 +220,7 @@ class Server::SyncRequest GRPC_FINAL : public CompletionQueueTag { public: explicit CallData(Server* server, SyncRequest* mrd) : cq_(mrd->cq_), - call_(mrd->call_, server, &cq_, server->max_message_size_), + call_(mrd->call_, server, &cq_, server->max_receive_message_size_), ctx_(mrd->deadline_, mrd->request_metadata_.metadata, mrd->request_metadata_.count), has_request_payload_(mrd->has_request_payload_), @@ -243,7 +243,7 @@ class Server::SyncRequest GRPC_FINAL : public CompletionQueueTag { ctx_.BeginCompletionOp(&call_); global_callbacks->PreSynchronousRequest(&ctx_); method_->handler()->RunHandler(MethodHandler::HandlerParameter( - &call_, &ctx_, request_payload_, call_.max_message_size())); + &call_, &ctx_, request_payload_, call_.max_receive_message_size())); global_callbacks->PostSynchronousRequest(&ctx_); request_payload_ = nullptr; void* ignored_tag; @@ -277,8 +277,8 @@ class Server::SyncRequest GRPC_FINAL : public CompletionQueueTag { static internal::GrpcLibraryInitializer g_gli_initializer; Server::Server(ThreadPoolInterface* thread_pool, bool thread_pool_owned, - int max_message_size, ChannelArguments* args) - : max_message_size_(max_message_size), + int max_receive_message_size, ChannelArguments* args) + : max_receive_message_size_(max_receive_message_size), started_(false), shutdown_(false), shutdown_notified_(false), @@ -514,7 +514,7 @@ bool ServerInterface::BaseAsyncRequest::FinalizeResult(void** tag, grpc_metadata_array_destroy(&initial_metadata_array_); context_->set_call(call_); context_->cq_ = call_cq_; - Call call(call_, server_, call_cq_, server_->max_message_size()); + Call call(call_, server_, call_cq_, server_->max_receive_message_size()); if (*status && call_) { context_->BeginCompletionOp(&call); } diff --git a/src/cpp/server/server_builder.cc b/src/cpp/server/server_builder.cc index 6cd9f982d7f..1f6e74938de 100644 --- a/src/cpp/server/server_builder.cc +++ b/src/cpp/server/server_builder.cc @@ -53,7 +53,9 @@ static void do_plugin_list_init(void) { } ServerBuilder::ServerBuilder() - : max_message_size_(-1), generic_service_(nullptr) { + : max_receive_message_size_(-1), + max_send_message_size_(-1), + generic_service_(nullptr) { gpr_once_init(&once_init_plugin_list, do_plugin_list_init); for (auto it = g_plugin_factory_list->begin(); it != g_plugin_factory_list->end(); it++) { @@ -163,8 +165,11 @@ std::unique_ptr ServerBuilder::BuildAndStart() { } } } - if (max_message_size_ > 0) { - args.SetInt(GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH, max_message_size_); + if (max_receive_message_size_ > 0) { + args.SetInt(GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH, max_receive_message_size_); + } + if (max_send_message_size_ > 0) { + args.SetInt(GRPC_ARG_MAX_SEND_MESSAGE_LENGTH, max_send_message_size_); } args.SetInt(GRPC_COMPRESSION_CHANNEL_ENABLED_ALGORITHMS_BITSET, enabled_compression_algorithms_bitset_); @@ -176,8 +181,8 @@ std::unique_ptr ServerBuilder::BuildAndStart() { args.SetInt(GRPC_COMPRESSION_CHANNEL_DEFAULT_ALGORITHM, maybe_default_compression_algorithm_.algorithm); } - std::unique_ptr server( - new Server(thread_pool.release(), true, max_message_size_, &args)); + std::unique_ptr server(new Server(thread_pool.release(), true, + max_receive_message_size_, &args)); ServerInitializer* initializer = server->initializer(); // If the server has atleast one sync methods, we know that this is a Sync From c99fd89d32d8b74791ea0aed1b0e72eed85db85d Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 6 Sep 2016 08:16:13 -0700 Subject: [PATCH 15/40] Increase max send message size on server used in objc tests. --- src/objective-c/tests/run_tests.sh | 4 ++-- test/cpp/interop/interop_server.cc | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/objective-c/tests/run_tests.sh b/src/objective-c/tests/run_tests.sh index a265149f488..81c28853205 100755 --- a/src/objective-c/tests/run_tests.sh +++ b/src/objective-c/tests/run_tests.sh @@ -44,8 +44,8 @@ BINDIR=../../../bins/$CONFIG "interop_server before calling this script." exit 1 } -$BINDIR/interop_server --port=5050 & -$BINDIR/interop_server --port=5051 --use_tls & +$BINDIR/interop_server --port=5050 --max_send_message_size=8388608 & +$BINDIR/interop_server --port=5051 --max_send_message_size=8388608 --use_tls & # Kill them when this script exits. trap 'kill -9 `jobs -p`' EXIT diff --git a/test/cpp/interop/interop_server.cc b/test/cpp/interop/interop_server.cc index e5878bb248c..c05eb5d1461 100644 --- a/test/cpp/interop/interop_server.cc +++ b/test/cpp/interop/interop_server.cc @@ -56,6 +56,7 @@ DEFINE_bool(use_tls, false, "Whether to use tls."); DEFINE_int32(port, 0, "Server port."); +DEFINE_int32(max_send_message_size, -1, "The maximum send message size."); using grpc::Server; using grpc::ServerBuilder; @@ -321,6 +322,9 @@ void grpc::testing::interop::RunServer( ServerBuilder builder; builder.RegisterService(&service); builder.AddListeningPort(server_address.str(), creds); + if (FLAGS_max_send_message_size >= 0) { + builder.SetMaxSendMessageSize(FLAGS_max_send_message_size); + } std::unique_ptr server(builder.BuildAndStart()); gpr_log(GPR_INFO, "Server listening on %s", server_address.str().c_str()); while (!g_got_sigint) { From 3ad61889209a04494580d80ecd023b1fdd6b9895 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 6 Sep 2016 13:03:39 -0700 Subject: [PATCH 16/40] Fix a few lingering references to max_message_size. --- include/grpc++/impl/codegen/sync_stream.h | 8 ++++---- include/grpc++/impl/codegen/thrift_utils.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/grpc++/impl/codegen/sync_stream.h b/include/grpc++/impl/codegen/sync_stream.h index e1d4660ae77..8249717d6c6 100644 --- a/include/grpc++/impl/codegen/sync_stream.h +++ b/include/grpc++/impl/codegen/sync_stream.h @@ -161,7 +161,7 @@ class ClientReader GRPC_FINAL : public ClientReaderInterface { } bool NextMessageSize(uint32_t* sz) GRPC_OVERRIDE { - *sz = call_.max_message_size(); + *sz = call_.max_receive_message_size(); return true; } @@ -311,7 +311,7 @@ class ClientReaderWriter GRPC_FINAL : public ClientReaderWriterInterface { } bool NextMessageSize(uint32_t* sz) GRPC_OVERRIDE { - *sz = call_.max_message_size(); + *sz = call_.max_receive_message_size(); return true; } @@ -383,7 +383,7 @@ class ServerReader GRPC_FINAL : public ServerReaderInterface { } bool NextMessageSize(uint32_t* sz) GRPC_OVERRIDE { - *sz = call_->max_message_size(); + *sz = call_->max_receive_message_size(); return true; } @@ -475,7 +475,7 @@ class ServerReaderWriterBody GRPC_FINAL { } bool NextMessageSize(uint32_t* sz) { - *sz = call_->max_message_size(); + *sz = call_->max_receive_message_size(); return true; } diff --git a/include/grpc++/impl/codegen/thrift_utils.h b/include/grpc++/impl/codegen/thrift_utils.h index 7d19b247f4c..3615e274743 100644 --- a/include/grpc++/impl/codegen/thrift_utils.h +++ b/include/grpc++/impl/codegen/thrift_utils.h @@ -66,7 +66,7 @@ class SerializationTraits Date: Wed, 7 Sep 2016 08:45:26 -0700 Subject: [PATCH 17/40] Fix test broken by merge. --- test/core/end2end/tests/max_message_length.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/core/end2end/tests/max_message_length.c b/test/core/end2end/tests/max_message_length.c index 27434888e24..63bc5ebc0c3 100644 --- a/test/core/end2end/tests/max_message_length.c +++ b/test/core/end2end/tests/max_message_length.c @@ -180,7 +180,7 @@ static void test_max_message_length(grpc_end2end_test_config config, GPR_ASSERT(GRPC_CALL_OK == error); if (send_limit) { - cq_expect_completion(cqv, tag(1), 1); + CQ_EXPECT_COMPLETION(cqv, tag(1), 1); cq_verify(cqv); goto done; } From 0e48a9af490c5c48802ca1f3faab36e022cfca49 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 8 Sep 2016 14:14:39 -0700 Subject: [PATCH 18/40] Move LB policy instantiation from resolvers into client_channel. --- src/core/ext/client_config/client_channel.c | 29 ++++++++---- src/core/ext/client_config/client_channel.h | 13 ++--- src/core/ext/client_config/resolver_factory.h | 5 +- .../ext/client_config/resolver_registry.c | 4 +- .../ext/client_config/resolver_registry.h | 3 +- src/core/ext/client_config/resolver_result.c | 47 ++++++++++--------- src/core/ext/client_config/resolver_result.h | 19 +++++--- .../ext/resolver/dns/native/dns_resolver.c | 27 ++--------- .../ext/resolver/sockaddr/sockaddr_resolver.c | 18 +------ .../chttp2/client/insecure/channel_create.c | 6 +-- .../client/secure/secure_channel_create.c | 6 +-- 11 files changed, 81 insertions(+), 96 deletions(-) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index 61e012578e0..2e6f253d386 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -42,6 +42,7 @@ #include #include +#include "src/core/ext/client_config/lb_policy_registry.h" #include "src/core/ext/client_config/subchannel.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/connected_channel.h" @@ -63,6 +64,8 @@ typedef struct client_channel_channel_data { grpc_resolver *resolver; /** have we started resolving this channel */ bool started_resolving; + /** client channel factory */ + grpc_client_channel_factory *client_channel_factory; /** mutex protecting client configuration, including all variables below in this data structure */ @@ -173,20 +176,24 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *state_error = GRPC_ERROR_CREATE("No load balancing policy"); if (chand->resolver_result != NULL) { - lb_policy = grpc_resolver_result_get_lb_policy(chand->resolver_result); + grpc_lb_policy_args lb_policy_args; + lb_policy_args.addresses = + grpc_resolver_result_get_addresses(chand->resolver_result); + lb_policy_args.client_channel_factory = chand->client_channel_factory; + lb_policy = grpc_lb_policy_create( + exec_ctx, + grpc_resolver_result_get_lb_policy_name(chand->resolver_result), + &lb_policy_args); if (lb_policy != NULL) { - GRPC_LB_POLICY_REF(lb_policy, "channel"); GRPC_LB_POLICY_REF(lb_policy, "config_change"); GRPC_ERROR_UNREF(state_error); state = grpc_lb_policy_check_connectivity(exec_ctx, lb_policy, &state_error); } - grpc_resolver_result_unref(exec_ctx, chand->resolver_result); + chand->resolver_result = NULL; } - chand->resolver_result = NULL; - if (lb_policy != NULL) { grpc_pollset_set_add_pollset_set(exec_ctx, lb_policy->interested_parties, chand->interested_parties); @@ -346,6 +353,9 @@ static void cc_destroy_channel_elem(grpc_exec_ctx *exec_ctx, grpc_resolver_shutdown(exec_ctx, chand->resolver); GRPC_RESOLVER_UNREF(exec_ctx, chand->resolver, "channel"); } + if (chand->client_channel_factory != NULL) { + grpc_client_channel_factory_unref(exec_ctx, chand->client_channel_factory); + } if (chand->lb_policy != NULL) { grpc_pollset_set_del_pollset_set(exec_ctx, chand->lb_policy->interested_parties, @@ -759,9 +769,10 @@ const grpc_channel_filter grpc_client_channel_filter = { "client-channel", }; -void grpc_client_channel_set_resolver(grpc_exec_ctx *exec_ctx, - grpc_channel_stack *channel_stack, - grpc_resolver *resolver) { +void grpc_client_channel_set_resolver_and_client_channel_factory( + grpc_exec_ctx *exec_ctx, grpc_channel_stack *channel_stack, + grpc_resolver *resolver, + grpc_client_channel_factory *client_channel_factory) { /* post construction initialization: set the transport setup pointer */ grpc_channel_element *elem = grpc_channel_stack_last_element(channel_stack); channel_data *chand = elem->channel_data; @@ -776,6 +787,8 @@ void grpc_client_channel_set_resolver(grpc_exec_ctx *exec_ctx, grpc_resolver_next(exec_ctx, resolver, &chand->resolver_result, &chand->on_resolver_result_changed); } + chand->client_channel_factory = client_channel_factory; + grpc_client_channel_factory_ref(client_channel_factory); gpr_mu_unlock(&chand->mu); } diff --git a/src/core/ext/client_config/client_channel.h b/src/core/ext/client_config/client_channel.h index 1e47ad34ad8..b3a9bc79951 100644 --- a/src/core/ext/client_config/client_channel.h +++ b/src/core/ext/client_config/client_channel.h @@ -34,6 +34,7 @@ #ifndef GRPC_CORE_EXT_CLIENT_CONFIG_CLIENT_CHANNEL_H #define GRPC_CORE_EXT_CLIENT_CONFIG_CLIENT_CHANNEL_H +#include "src/core/ext/client_config/client_channel_factory.h" #include "src/core/ext/client_config/resolver.h" #include "src/core/lib/channel/channel_stack.h" @@ -46,12 +47,12 @@ extern const grpc_channel_filter grpc_client_channel_filter; -/* post-construction initializer to let the client channel know which - transport setup it should cancel upon destruction, or initiate when it needs - a connection */ -void grpc_client_channel_set_resolver(grpc_exec_ctx *exec_ctx, - grpc_channel_stack *channel_stack, - grpc_resolver *resolver); +/* Post-construction initializer to give the client channel its resolver + and factory. */ +void grpc_client_channel_set_resolver_and_client_channel_factory( + grpc_exec_ctx *exec_ctx, grpc_channel_stack *channel_stack, + grpc_resolver *resolver, + grpc_client_channel_factory *client_channel_factory); grpc_connectivity_state grpc_client_channel_check_connectivity_state( grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, int try_to_connect); diff --git a/src/core/ext/client_config/resolver_factory.h b/src/core/ext/client_config/resolver_factory.h index f69bf795649..9ec5b9a70e7 100644 --- a/src/core/ext/client_config/resolver_factory.h +++ b/src/core/ext/client_config/resolver_factory.h @@ -47,10 +47,7 @@ struct grpc_resolver_factory { const grpc_resolver_factory_vtable *vtable; }; -typedef struct grpc_resolver_args { - grpc_uri *uri; - grpc_client_channel_factory *client_channel_factory; -} grpc_resolver_args; +typedef struct grpc_resolver_args { grpc_uri *uri; } grpc_resolver_args; struct grpc_resolver_factory_vtable { void (*ref)(grpc_resolver_factory *factory); diff --git a/src/core/ext/client_config/resolver_registry.c b/src/core/ext/client_config/resolver_registry.c index e7a4abd568c..b5308a140c9 100644 --- a/src/core/ext/client_config/resolver_registry.c +++ b/src/core/ext/client_config/resolver_registry.c @@ -128,15 +128,13 @@ static grpc_resolver_factory *resolve_factory(const char *target, return factory; } -grpc_resolver *grpc_resolver_create( - const char *target, grpc_client_channel_factory *client_channel_factory) { +grpc_resolver *grpc_resolver_create(const char *target) { grpc_uri *uri = NULL; grpc_resolver_factory *factory = resolve_factory(target, &uri); grpc_resolver *resolver; grpc_resolver_args args; memset(&args, 0, sizeof(args)); args.uri = uri; - args.client_channel_factory = client_channel_factory; resolver = grpc_resolver_factory_create_resolver(factory, &args); grpc_uri_destroy(uri); return resolver; diff --git a/src/core/ext/client_config/resolver_registry.h b/src/core/ext/client_config/resolver_registry.h index 5ef1383cd35..92e248d5488 100644 --- a/src/core/ext/client_config/resolver_registry.h +++ b/src/core/ext/client_config/resolver_registry.h @@ -55,8 +55,7 @@ void grpc_register_resolver_type(grpc_resolver_factory *factory); If a resolver factory was found, use it to instantiate a resolver and return it. If a resolver factory was not found, return NULL. */ -grpc_resolver *grpc_resolver_create( - const char *target, grpc_client_channel_factory *client_channel_factory); +grpc_resolver *grpc_resolver_create(const char *target); /** Find a resolver factory given a name and return an (owned-by-the-caller) * reference to it */ diff --git a/src/core/ext/client_config/resolver_result.c b/src/core/ext/client_config/resolver_result.c index e14f761f05e..ba21349d2d5 100644 --- a/src/core/ext/client_config/resolver_result.c +++ b/src/core/ext/client_config/resolver_result.c @@ -36,6 +36,7 @@ #include #include +#include grpc_addresses *grpc_addresses_create(size_t num_addresses) { grpc_addresses *addresses = gpr_malloc(sizeof(grpc_addresses)); @@ -63,37 +64,39 @@ void grpc_addresses_destroy(grpc_addresses *addresses) { struct grpc_resolver_result { gpr_refcount refs; - grpc_lb_policy *lb_policy; + grpc_addresses *addresses; + char *lb_policy_name; }; -grpc_resolver_result *grpc_resolver_result_create() { - grpc_resolver_result *c = gpr_malloc(sizeof(*c)); - memset(c, 0, sizeof(*c)); - gpr_ref_init(&c->refs, 1); - return c; +grpc_resolver_result *grpc_resolver_result_create(grpc_addresses *addresses, + const char *lb_policy_name) { + grpc_resolver_result *result = gpr_malloc(sizeof(*result)); + memset(result, 0, sizeof(*result)); + gpr_ref_init(&result->refs, 1); + result->addresses = addresses; + result->lb_policy_name = gpr_strdup(lb_policy_name); + return result; } -void grpc_resolver_result_ref(grpc_resolver_result *c) { gpr_ref(&c->refs); } +void grpc_resolver_result_ref(grpc_resolver_result *result) { + gpr_ref(&result->refs); +} void grpc_resolver_result_unref(grpc_exec_ctx *exec_ctx, - grpc_resolver_result *c) { - if (gpr_unref(&c->refs)) { - if (c->lb_policy != NULL) { - GRPC_LB_POLICY_UNREF(exec_ctx, c->lb_policy, "resolver_result"); - } - gpr_free(c); + grpc_resolver_result *result) { + if (gpr_unref(&result->refs)) { + grpc_addresses_destroy(result->addresses); + gpr_free(result->lb_policy_name); + gpr_free(result); } } -void grpc_resolver_result_set_lb_policy(grpc_resolver_result *c, - grpc_lb_policy *lb_policy) { - GPR_ASSERT(c->lb_policy == NULL); - if (lb_policy) { - GRPC_LB_POLICY_REF(lb_policy, "resolver_result"); - } - c->lb_policy = lb_policy; +grpc_addresses *grpc_resolver_result_get_addresses( + grpc_resolver_result *result) { + return result->addresses; } -grpc_lb_policy *grpc_resolver_result_get_lb_policy(grpc_resolver_result *c) { - return c->lb_policy; +const char *grpc_resolver_result_get_lb_policy_name( + grpc_resolver_result *result) { + return result->lb_policy_name; } diff --git a/src/core/ext/client_config/resolver_result.h b/src/core/ext/client_config/resolver_result.h index 4199ef512ad..df92bec984e 100644 --- a/src/core/ext/client_config/resolver_result.h +++ b/src/core/ext/client_config/resolver_result.h @@ -63,14 +63,19 @@ void grpc_addresses_destroy(grpc_addresses *addresses); /** Results reported from a grpc_resolver. */ typedef struct grpc_resolver_result grpc_resolver_result; -grpc_resolver_result *grpc_resolver_result_create(); -void grpc_resolver_result_ref(grpc_resolver_result *client_config); +/** Takes ownership of \a addresses. */ +grpc_resolver_result *grpc_resolver_result_create(grpc_addresses *addresses, + const char *lb_policy_name); +void grpc_resolver_result_ref(grpc_resolver_result *result); void grpc_resolver_result_unref(grpc_exec_ctx *exec_ctx, - grpc_resolver_result *client_config); + grpc_resolver_result *result); -void grpc_resolver_result_set_lb_policy(grpc_resolver_result *client_config, - grpc_lb_policy *lb_policy); -grpc_lb_policy *grpc_resolver_result_get_lb_policy( - grpc_resolver_result *client_config); +/** Caller does NOT take ownership of result. */ +grpc_addresses *grpc_resolver_result_get_addresses( + grpc_resolver_result *result); + +/** Caller does NOT take ownership of result. */ +const char *grpc_resolver_result_get_lb_policy_name( + grpc_resolver_result *result); #endif /* GRPC_CORE_EXT_CLIENT_CONFIG_RESOLVER_RESULT_H */ diff --git a/src/core/ext/resolver/dns/native/dns_resolver.c b/src/core/ext/resolver/dns/native/dns_resolver.c index 8fc10d98a80..78a996788dc 100644 --- a/src/core/ext/resolver/dns/native/dns_resolver.c +++ b/src/core/ext/resolver/dns/native/dns_resolver.c @@ -37,7 +37,6 @@ #include #include -#include "src/core/ext/client_config/lb_policy_registry.h" #include "src/core/ext/client_config/resolver_registry.h" #include "src/core/lib/iomgr/resolve_address.h" #include "src/core/lib/iomgr/timer.h" @@ -58,8 +57,6 @@ typedef struct { char *name; /** default port to use */ char *default_port; - /** subchannel factory */ - grpc_client_channel_factory *client_channel_factory; /** load balancing policy name */ char *lb_policy_name; @@ -166,29 +163,18 @@ static void dns_on_resolved(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { dns_resolver *r = arg; grpc_resolver_result *result = NULL; - grpc_lb_policy *lb_policy; gpr_mu_lock(&r->mu); GPR_ASSERT(r->resolving); r->resolving = 0; if (r->addresses != NULL) { - grpc_lb_policy_args lb_policy_args; - memset(&lb_policy_args, 0, sizeof(lb_policy_args)); - lb_policy_args.addresses = grpc_addresses_create(r->addresses->naddrs); + grpc_addresses *addresses = grpc_addresses_create(r->addresses->naddrs); for (size_t i = 0; i < r->addresses->naddrs; ++i) { - grpc_addresses_set_address( - lb_policy_args.addresses, i, &r->addresses->addrs[i].addr, - r->addresses->addrs[i].len, false /* is_balancer */); + grpc_addresses_set_address(addresses, i, &r->addresses->addrs[i].addr, + r->addresses->addrs[i].len, + false /* is_balancer */); } grpc_resolved_addresses_destroy(r->addresses); - lb_policy_args.client_channel_factory = r->client_channel_factory; - lb_policy = - grpc_lb_policy_create(exec_ctx, r->lb_policy_name, &lb_policy_args); - grpc_addresses_destroy(lb_policy_args.addresses); - result = grpc_resolver_result_create(); - if (lb_policy != NULL) { - grpc_resolver_result_set_lb_policy(result, lb_policy); - GRPC_LB_POLICY_UNREF(exec_ctx, lb_policy, "construction"); - } + result = grpc_resolver_result_create(addresses, r->lb_policy_name); } else { gpr_timespec now = gpr_now(GPR_CLOCK_MONOTONIC); gpr_timespec next_try = gpr_backoff_step(&r->backoff_state, now); @@ -249,7 +235,6 @@ static void dns_destroy(grpc_exec_ctx *exec_ctx, grpc_resolver *gr) { if (r->resolved_result) { grpc_resolver_result_unref(exec_ctx, r->resolved_result); } - grpc_client_channel_factory_unref(exec_ctx, r->client_channel_factory); gpr_free(r->name); gpr_free(r->default_port); gpr_free(r->lb_policy_name); @@ -276,10 +261,8 @@ static grpc_resolver *dns_create(grpc_resolver_args *args, grpc_resolver_init(&r->base, &dns_resolver_vtable); r->name = gpr_strdup(path); r->default_port = gpr_strdup(default_port); - r->client_channel_factory = args->client_channel_factory; gpr_backoff_init(&r->backoff_state, BACKOFF_MULTIPLIER, BACKOFF_JITTER, BACKOFF_MIN_SECONDS * 1000, BACKOFF_MAX_SECONDS * 1000); - grpc_client_channel_factory_ref(r->client_channel_factory); r->lb_policy_name = gpr_strdup(lb_policy_name); return &r->base; } diff --git a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c index 94d2f892ebb..328c7cb6f93 100644 --- a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c +++ b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c @@ -40,7 +40,6 @@ #include #include -#include "src/core/ext/client_config/lb_policy_registry.h" #include "src/core/ext/client_config/parse_address.h" #include "src/core/ext/client_config/resolver_registry.h" #include "src/core/lib/iomgr/resolve_address.h" @@ -52,8 +51,6 @@ typedef struct { grpc_resolver base; /** refcount */ gpr_refcount refs; - /** subchannel factory */ - grpc_client_channel_factory *client_channel_factory; /** load balancing policy name */ char *lb_policy_name; @@ -122,17 +119,9 @@ static void sockaddr_next(grpc_exec_ctx *exec_ctx, grpc_resolver *resolver, static void sockaddr_maybe_finish_next_locked(grpc_exec_ctx *exec_ctx, sockaddr_resolver *r) { if (r->next_completion != NULL && !r->published) { - grpc_resolver_result *result = grpc_resolver_result_create(); - grpc_lb_policy_args lb_policy_args; - memset(&lb_policy_args, 0, sizeof(lb_policy_args)); - lb_policy_args.addresses = r->addresses; - lb_policy_args.client_channel_factory = r->client_channel_factory; - grpc_lb_policy *lb_policy = - grpc_lb_policy_create(exec_ctx, r->lb_policy_name, &lb_policy_args); - grpc_resolver_result_set_lb_policy(result, lb_policy); - GRPC_LB_POLICY_UNREF(exec_ctx, lb_policy, "sockaddr"); r->published = true; - *r->target_result = result; + *r->target_result = + grpc_resolver_result_create(r->addresses, r->lb_policy_name); grpc_exec_ctx_sched(exec_ctx, r->next_completion, GRPC_ERROR_NONE, NULL); r->next_completion = NULL; } @@ -141,7 +130,6 @@ static void sockaddr_maybe_finish_next_locked(grpc_exec_ctx *exec_ctx, static void sockaddr_destroy(grpc_exec_ctx *exec_ctx, grpc_resolver *gr) { sockaddr_resolver *r = (sockaddr_resolver *)gr; gpr_mu_destroy(&r->mu); - grpc_client_channel_factory_unref(exec_ctx, r->client_channel_factory); grpc_addresses_destroy(r->addresses); gpr_free(r->lb_policy_name); gpr_free(r); @@ -243,8 +231,6 @@ static grpc_resolver *sockaddr_create( gpr_ref_init(&r->refs, 1); gpr_mu_init(&r->mu); grpc_resolver_init(&r->base, &sockaddr_resolver_vtable); - r->client_channel_factory = args->client_channel_factory; - grpc_client_channel_factory_ref(r->client_channel_factory); return &r->base; } diff --git a/src/core/ext/transport/chttp2/client/insecure/channel_create.c b/src/core/ext/transport/chttp2/client/insecure/channel_create.c index cbaa75a90af..550080c7d5e 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.c @@ -210,15 +210,15 @@ static grpc_channel *client_channel_factory_create_channel( grpc_channel *channel = grpc_channel_create(exec_ctx, target, final_args, GRPC_CLIENT_CHANNEL, NULL); grpc_channel_args_destroy(final_args); - grpc_resolver *resolver = grpc_resolver_create(target, &f->base); + grpc_resolver *resolver = grpc_resolver_create(target); if (!resolver) { GRPC_CHANNEL_INTERNAL_UNREF(exec_ctx, channel, "client_channel_factory_create_channel"); return NULL; } - grpc_client_channel_set_resolver( - exec_ctx, grpc_channel_get_channel_stack(channel), resolver); + grpc_client_channel_set_resolver_and_client_channel_factory( + exec_ctx, grpc_channel_get_channel_stack(channel), resolver, &f->base); GRPC_RESOLVER_UNREF(exec_ctx, resolver, "create_channel"); return channel; diff --git a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c index 9e2bdd758f9..3285dbf6125 100644 --- a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c +++ b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c @@ -276,10 +276,10 @@ static grpc_channel *client_channel_factory_create_channel( GRPC_CLIENT_CHANNEL, NULL); grpc_channel_args_destroy(final_args); - grpc_resolver *resolver = grpc_resolver_create(target, &f->base); + grpc_resolver *resolver = grpc_resolver_create(target); if (resolver != NULL) { - grpc_client_channel_set_resolver( - exec_ctx, grpc_channel_get_channel_stack(channel), resolver); + grpc_client_channel_set_resolver_and_client_channel_factory( + exec_ctx, grpc_channel_get_channel_stack(channel), resolver, &f->base); GRPC_RESOLVER_UNREF(exec_ctx, resolver, "create"); } else { GRPC_CHANNEL_INTERNAL_UNREF(exec_ctx, channel, From 9392b045177cf1294e4918392bb300b7e458b232 Mon Sep 17 00:00:00 2001 From: thinkerou Date: Fri, 9 Sep 2016 17:43:00 +0800 Subject: [PATCH 19/40] fix php code style --- src/php/lib/Grpc/BaseStub.php | 1 + src/php/tests/interop/interop_client.php | 102 ++++++++++-------- .../tests/unit_tests/CallCredentials2Test.php | 3 +- 3 files changed, 59 insertions(+), 47 deletions(-) diff --git a/src/php/lib/Grpc/BaseStub.php b/src/php/lib/Grpc/BaseStub.php index d850eebc788..8a7f6572a62 100644 --- a/src/php/lib/Grpc/BaseStub.php +++ b/src/php/lib/Grpc/BaseStub.php @@ -182,6 +182,7 @@ class BaseStub } else { $hostname = $this->hostname; } + return 'https://'.$hostname.$service_name; } diff --git a/src/php/tests/interop/interop_client.php b/src/php/tests/interop/interop_client.php index 94ceeda02c7..4bc4589956c 100755 --- a/src/php/tests/interop/interop_client.php +++ b/src/php/tests/interop/interop_client.php @@ -70,7 +70,8 @@ function hardAssertIfStatusOk($status) */ function emptyUnary($stub) { - list($result, $status) = $stub->EmptyCall(new grpc\testing\EmptyMessage())->wait(); + list($result, $status) = + $stub->EmptyCall(new grpc\testing\EmptyMessage())->wait(); hardAssertIfStatusOk($status); hardAssert($result !== null, 'Call completed with a null response'); } @@ -92,8 +93,8 @@ function largeUnary($stub) * @param $fillUsername boolean whether to fill result with username * @param $fillOauthScope boolean whether to fill result with oauth scope */ -function performLargeUnary($stub, $fillUsername = false, $fillOauthScope = false, - $callback = false) +function performLargeUnary($stub, $fillUsername = false, + $fillOauthScope = false, $callback = false) { $request_len = 271828; $response_len = 314159; @@ -118,11 +119,11 @@ function performLargeUnary($stub, $fillUsername = false, $fillOauthScope = false hardAssert($result !== null, 'Call returned a null response'); $payload = $result->getPayload(); hardAssert($payload->getType() === grpc\testing\PayloadType::COMPRESSABLE, - 'Payload had the wrong type'); + 'Payload had the wrong type'); hardAssert(strlen($payload->getBody()) === $response_len, - 'Payload had the wrong length'); + 'Payload had the wrong length'); hardAssert($payload->getBody() === str_repeat("\0", $response_len), - 'Payload had the wrong content'); + 'Payload had the wrong content'); return $result; } @@ -141,11 +142,12 @@ function serviceAccountCreds($stub, $args) $jsonKey = json_decode( file_get_contents(getenv(CredentialsLoader::ENV_VAR)), true); - $result = performLargeUnary($stub, $fillUsername = true, $fillOauthScope = true); - hardAssert($result->getUsername() == $jsonKey['client_email'], - 'invalid email returned'); + $result = performLargeUnary($stub, $fillUsername = true, + $fillOauthScope = true); + hardAssert($result->getUsername() === $jsonKey['client_email'], + 'invalid email returned'); hardAssert(strpos($args['oauth_scope'], $result->getOauthScope()) !== false, - 'invalid oauth scope returned'); + 'invalid oauth scope returned'); } /** @@ -163,9 +165,10 @@ function computeEngineCreds($stub, $args) if (!array_key_exists('default_service_account', $args)) { throw new Exception('Missing default_service_account'); } - $result = performLargeUnary($stub, $fillUsername = true, $fillOauthScope = true); - hardAssert($args['default_service_account'] == $result->getUsername(), - 'invalid email returned'); + $result = performLargeUnary($stub, $fillUsername = true, + $fillOauthScope = true); + hardAssert($args['default_service_account'] === $result->getUsername(), + 'invalid email returned'); } /** @@ -179,9 +182,10 @@ function jwtTokenCreds($stub, $args) $jsonKey = json_decode( file_get_contents(getenv(CredentialsLoader::ENV_VAR)), true); - $result = performLargeUnary($stub, $fillUsername = true, $fillOauthScope = true); - hardAssert($result->getUsername() == $jsonKey['client_email'], - 'invalid email returned'); + $result = performLargeUnary($stub, $fillUsername = true, + $fillOauthScope = true); + hardAssert($result->getUsername() === $jsonKey['client_email'], + 'invalid email returned'); } /** @@ -195,9 +199,10 @@ function oauth2AuthToken($stub, $args) $jsonKey = json_decode( file_get_contents(getenv(CredentialsLoader::ENV_VAR)), true); - $result = performLargeUnary($stub, $fillUsername = true, $fillOauthScope = true); - hardAssert($result->getUsername() == $jsonKey['client_email'], - 'invalid email returned'); + $result = performLargeUnary($stub, $fillUsername = true, + $fillOauthScope = true); + hardAssert($result->getUsername() === $jsonKey['client_email'], + 'invalid email returned'); } function updateAuthMetadataCallback($context) @@ -209,8 +214,9 @@ function updateAuthMetadataCallback($context) $metadata = []; $result = $auth_credentials->updateMetadata([], $authUri); foreach ($result as $key => $value) { - $metadata[strtolower($key)] = $value; + $metadata[strtolower($key)] = $value; } + return $metadata; } @@ -226,10 +232,11 @@ function perRpcCreds($stub, $args) file_get_contents(getenv(CredentialsLoader::ENV_VAR)), true); - $result = performLargeUnary($stub, $fillUsername = true, $fillOauthScope = true, + $result = performLargeUnary($stub, $fillUsername = true, + $fillOauthScope = true, 'updateAuthMetadataCallback'); - hardAssert($result->getUsername() == $jsonKey['client_email'], - 'invalid email returned'); + hardAssert($result->getUsername() === $jsonKey['client_email'], + 'invalid email returned'); } /** @@ -258,7 +265,7 @@ function clientStreaming($stub) list($result, $status) = $call->wait(); hardAssertIfStatusOk($status); hardAssert($result->getAggregatedPayloadSize() === 74922, - 'aggregated_payload_size was incorrect'); + 'aggregated_payload_size was incorrect'); } /** @@ -283,10 +290,11 @@ function serverStreaming($stub) foreach ($call->responses() as $value) { hardAssert($i < 4, 'Too many responses'); $payload = $value->getPayload(); - hardAssert($payload->getType() === grpc\testing\PayloadType::COMPRESSABLE, - 'Payload '.$i.' had the wrong type'); + hardAssert( + $payload->getType() === grpc\testing\PayloadType::COMPRESSABLE, + 'Payload '.$i.' had the wrong type'); hardAssert(strlen($payload->getBody()) === $sizes[$i], - 'Response '.$i.' had the wrong length'); + 'Response '.$i.' had the wrong length'); $i += 1; } hardAssertIfStatusOk($call->getStatus()); @@ -318,10 +326,11 @@ function pingPong($stub) hardAssert($response !== null, 'Server returned too few responses'); $payload = $response->getPayload(); - hardAssert($payload->getType() === grpc\testing\PayloadType::COMPRESSABLE, - 'Payload '.$i.' had the wrong type'); + hardAssert( + $payload->getType() === grpc\testing\PayloadType::COMPRESSABLE, + 'Payload '.$i.' had the wrong type'); hardAssert(strlen($payload->getBody()) === $response_lengths[$i], - 'Payload '.$i.' had the wrong length'); + 'Payload '.$i.' had the wrong length'); } $call->writesDone(); hardAssert($call->read() === null, 'Server returned too many responses'); @@ -352,7 +361,7 @@ function cancelAfterBegin($stub) $call->cancel(); list($result, $status) = $call->wait(); hardAssert($status->code === Grpc\STATUS_CANCELLED, - 'Call status was not CANCELLED'); + 'Call status was not CANCELLED'); } /** @@ -377,7 +386,7 @@ function cancelAfterFirstResponse($stub) $call->cancel(); hardAssert($call->getStatus()->code === Grpc\STATUS_CANCELLED, - 'Call status was not CANCELLED'); + 'Call status was not CANCELLED'); } function timeoutOnSleepingServer($stub) @@ -396,7 +405,7 @@ function timeoutOnSleepingServer($stub) $response = $call->read(); hardAssert($call->getStatus()->code === Grpc\STATUS_DEADLINE_EXCEEDED, - 'Call status was not DEADLINE_EXCEEDED'); + 'Call status was not DEADLINE_EXCEEDED'); } function customMetadata($stub) @@ -425,9 +434,9 @@ function customMetadata($stub) $initial_metadata = $call->getMetadata(); hardAssert(array_key_exists($ECHO_INITIAL_KEY, $initial_metadata), 'Initial metadata does not contain expected key'); - hardAssert($initial_metadata[$ECHO_INITIAL_KEY][0] == - $ECHO_INITIAL_VALUE, - 'Incorrect initial metadata value'); + hardAssert( + $initial_metadata[$ECHO_INITIAL_KEY][0] === $ECHO_INITIAL_VALUE, + 'Incorrect initial metadata value'); list($result, $status) = $call->wait(); hardAssertIfStatusOk($status); @@ -435,8 +444,9 @@ function customMetadata($stub) $trailing_metadata = $call->getTrailingMetadata(); hardAssert(array_key_exists($ECHO_TRAILING_KEY, $trailing_metadata), 'Trailing metadata does not contain expected key'); - hardAssert($trailing_metadata[$ECHO_TRAILING_KEY][0] == - $ECHO_TRAILING_VALUE, 'Incorrect trailing metadata value'); + hardAssert( + $trailing_metadata[$ECHO_TRAILING_KEY][0] === $ECHO_TRAILING_VALUE, + 'Incorrect trailing metadata value'); $streaming_call = $stub->FullDuplexCall($metadata); @@ -451,7 +461,7 @@ function customMetadata($stub) hardAssert(array_key_exists($ECHO_TRAILING_KEY, $streaming_trailing_metadata), 'Trailing metadata does not contain expected key'); - hardAssert($streaming_trailing_metadata[$ECHO_TRAILING_KEY][0] == + hardAssert($streaming_trailing_metadata[$ECHO_TRAILING_KEY][0] === $ECHO_TRAILING_VALUE, 'Incorrect trailing metadata value'); } @@ -506,7 +516,7 @@ function _makeStub($args) throw new Exception('Missing argument: --test_case is required'); } - if ($args['server_port'] == 443) { + if ($args['server_port'] === 443) { $server_address = $args['server_host']; } else { $server_address = $args['server_host'].':'.$args['server_port']; @@ -548,7 +558,7 @@ function _makeStub($args) if (in_array($test_case, ['service_account_creds', 'compute_engine_creds', 'jwt_token_creds', ])) { - if ($test_case == 'jwt_token_creds') { + if ($test_case === 'jwt_token_creds') { $auth_credentials = ApplicationDefaultCredentials::getCredentials(); } else { $auth_credentials = ApplicationDefaultCredentials::getCredentials( @@ -558,7 +568,7 @@ function _makeStub($args) $opts['update_metadata'] = $auth_credentials->getUpdateMetadataFunc(); } - if ($test_case == 'oauth2_auth_token') { + if ($test_case === 'oauth2_auth_token') { $auth_credentials = ApplicationDefaultCredentials::getCredentials( $args['oauth_scope'] ); @@ -578,8 +588,9 @@ function _makeStub($args) $opts['update_metadata'] = $update_metadata; } - if ($test_case == 'unimplemented_method') { - $stub = new grpc\testing\UnimplementedServiceClient($server_address, $opts); + if ($test_case === 'unimplemented_method') { + $stub = new grpc\testing\UnimplementedServiceClient($server_address, + $opts); } else { $stub = new grpc\testing\TestServiceClient($server_address, $opts); } @@ -656,7 +667,8 @@ function interop_main($args, $stub = false) return $stub; } -if (isset($_SERVER['PHP_SELF']) && preg_match('/interop_client/', $_SERVER['PHP_SELF'])) { +if (isset($_SERVER['PHP_SELF']) && + preg_match('/interop_client/', $_SERVER['PHP_SELF'])) { $args = getopt('', ['server_host:', 'server_port:', 'test_case:', 'use_tls::', 'use_test_ca::', 'server_host_override:', 'oauth_scope:', diff --git a/src/php/tests/unit_tests/CallCredentials2Test.php b/src/php/tests/unit_tests/CallCredentials2Test.php index b3b98a22cae..fa31439c543 100644 --- a/src/php/tests/unit_tests/CallCredentials2Test.php +++ b/src/php/tests/unit_tests/CallCredentials2Test.php @@ -170,7 +170,7 @@ class CallCredentials2Test extends PHPUnit_Framework_TestCase $this->assertTrue(is_string($context->service_url)); $this->assertTrue(is_string($context->method_name)); - return "a string"; + return 'a string'; } public function testCallbackWithInvalidReturnValue() @@ -196,5 +196,4 @@ class CallCredentials2Test extends PHPUnit_Framework_TestCase $this->assertTrue($event->send_close); $this->assertTrue($event->status->code == Grpc\STATUS_UNAUTHENTICATED); } - } From 7309664d032c82724c12e79d8cbb2e18d842598e Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 9 Sep 2016 08:33:35 -0700 Subject: [PATCH 20/40] Avoid circular refcounting. --- .../transport/chttp2/client/insecure/channel_create.c | 11 +---------- .../chttp2/client/secure/secure_channel_create.c | 9 --------- 2 files changed, 1 insertion(+), 19 deletions(-) diff --git a/src/core/ext/transport/chttp2/client/insecure/channel_create.c b/src/core/ext/transport/chttp2/client/insecure/channel_create.c index 550080c7d5e..5b9101e018b 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.c @@ -160,7 +160,6 @@ typedef struct { grpc_client_channel_factory base; gpr_refcount refs; grpc_channel_args *merge_args; - grpc_channel *master; } client_channel_factory; static void client_channel_factory_ref( @@ -173,10 +172,6 @@ static void client_channel_factory_unref( grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *cc_factory) { client_channel_factory *f = (client_channel_factory *)cc_factory; if (gpr_unref(&f->refs)) { - if (f->master != NULL) { - GRPC_CHANNEL_INTERNAL_UNREF(exec_ctx, f->master, - "client_channel_factory"); - } grpc_channel_args_destroy(f->merge_args); gpr_free(f); } @@ -250,12 +245,8 @@ grpc_channel *grpc_insecure_channel_create(const char *target, grpc_channel *channel = client_channel_factory_create_channel( &exec_ctx, &f->base, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, NULL); - if (channel != NULL) { - f->master = channel; - GRPC_CHANNEL_INTERNAL_REF(f->master, "grpc_insecure_channel_create"); - } - grpc_client_channel_factory_unref(&exec_ctx, &f->base); + grpc_client_channel_factory_unref(&exec_ctx, &f->base); grpc_exec_ctx_finish(&exec_ctx); return channel != NULL ? channel : grpc_lame_client_channel_create( diff --git a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c index 3285dbf6125..da738138b51 100644 --- a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c +++ b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c @@ -220,7 +220,6 @@ typedef struct { gpr_refcount refs; grpc_channel_args *merge_args; grpc_channel_security_connector *security_connector; - grpc_channel *master; } client_channel_factory; static void client_channel_factory_ref( @@ -235,10 +234,6 @@ static void client_channel_factory_unref( if (gpr_unref(&f->refs)) { GRPC_SECURITY_CONNECTOR_UNREF(&f->security_connector->base, "client_channel_factory"); - if (f->master != NULL) { - GRPC_CHANNEL_INTERNAL_UNREF(exec_ctx, f->master, - "client_channel_factory"); - } grpc_channel_args_destroy(f->merge_args); gpr_free(f); } @@ -356,10 +351,6 @@ grpc_channel *grpc_secure_channel_create(grpc_channel_credentials *creds, grpc_channel *channel = client_channel_factory_create_channel( &exec_ctx, &f->base, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, NULL); - if (channel != NULL) { - f->master = channel; - GRPC_CHANNEL_INTERNAL_REF(f->master, "grpc_secure_channel_create"); - } grpc_client_channel_factory_unref(&exec_ctx, &f->base); grpc_exec_ctx_finish(&exec_ctx); From bd268492e876d7ea77217148dba45ee7b4979192 Mon Sep 17 00:00:00 2001 From: Siddharth Shukla Date: Fri, 26 Aug 2016 23:42:04 +0200 Subject: [PATCH 21/40] remove erroneously redefined function from run_tests.py The function was added in 824c234de8cc0b9a7e4cfd6618060e4158ecf8e3. --- tools/run_tests/run_tests.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/tools/run_tests/run_tests.py b/tools/run_tests/run_tests.py index 78ef05b6357..c0780814856 100755 --- a/tools/run_tests/run_tests.py +++ b/tools/run_tests/run_tests.py @@ -1204,18 +1204,6 @@ def _shut_down_legacy_server(legacy_server_port): 'http://localhost:%d/quitquitquit' % legacy_server_port).read() -def _shut_down_legacy_server(legacy_server_port): - try: - version = int(urllib2.urlopen( - 'http://localhost:%d/version_number' % legacy_server_port, - timeout=10).read()) - except: - pass - else: - urllib2.urlopen( - 'http://localhost:%d/quitquitquit' % legacy_server_port).read() - - def _start_port_server(port_server_port): # check if a compatible port server is running # if incompatible (version mismatch) ==> start a new one From 90f49f58a555bb24fb13588b638fa4c795514358 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 9 Sep 2016 14:20:42 -0700 Subject: [PATCH 22/40] Fix resolver test build failures. --- .../dns_resolver_connectivity_test.c | 24 ------------------- .../resolvers/dns_resolver_test.c | 24 ------------------- .../resolvers/sockaddr_resolver_test.c | 24 ------------------- 3 files changed, 72 deletions(-) diff --git a/test/core/client_config/resolvers/dns_resolver_connectivity_test.c b/test/core/client_config/resolvers/dns_resolver_connectivity_test.c index 6a33525f629..d3b961959d2 100644 --- a/test/core/client_config/resolvers/dns_resolver_connectivity_test.c +++ b/test/core/client_config/resolvers/dns_resolver_connectivity_test.c @@ -41,29 +41,6 @@ #include "src/core/lib/iomgr/timer.h" #include "test/core/util/test_config.h" -static void client_channel_factory_ref(grpc_client_channel_factory *scv) {} -static void client_channel_factory_unref(grpc_exec_ctx *exec_ctx, - grpc_client_channel_factory *scv) {} -static grpc_subchannel *client_channel_factory_create_subchannel( - grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *factory, - grpc_subchannel_args *args) { - return NULL; -} - -static grpc_channel *client_channel_factory_create_channel( - grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *cc_factory, - const char *target, grpc_client_channel_type type, - grpc_channel_args *args) { - GPR_UNREACHABLE_CODE(return NULL); -} - -static const grpc_client_channel_factory_vtable sc_vtable = { - client_channel_factory_ref, client_channel_factory_unref, - client_channel_factory_create_subchannel, - client_channel_factory_create_channel}; - -static grpc_client_channel_factory cc_factory = {&sc_vtable}; - static gpr_mu g_mu; static bool g_fail_resolution = true; @@ -92,7 +69,6 @@ static grpc_resolver *create_resolver(const char *name) { grpc_resolver_args args; memset(&args, 0, sizeof(args)); args.uri = uri; - args.client_channel_factory = &cc_factory; grpc_resolver *resolver = grpc_resolver_factory_create_resolver(factory, &args); grpc_resolver_factory_unref(factory); diff --git a/test/core/client_config/resolvers/dns_resolver_test.c b/test/core/client_config/resolvers/dns_resolver_test.c index 21dc99cadd8..c3f4cb12444 100644 --- a/test/core/client_config/resolvers/dns_resolver_test.c +++ b/test/core/client_config/resolvers/dns_resolver_test.c @@ -38,29 +38,6 @@ #include "src/core/ext/client_config/resolver_registry.h" #include "test/core/util/test_config.h" -static void client_channel_factory_ref(grpc_client_channel_factory *scv) {} -static void client_channel_factory_unref(grpc_exec_ctx *exec_ctx, - grpc_client_channel_factory *scv) {} -static grpc_subchannel *client_channel_factory_create_subchannel( - grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *factory, - grpc_subchannel_args *args) { - GPR_UNREACHABLE_CODE(return NULL); -} - -static grpc_channel *client_channel_factory_create_channel( - grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *cc_factory, - const char *target, grpc_client_channel_type type, - grpc_channel_args *args) { - GPR_UNREACHABLE_CODE(return NULL); -} - -static const grpc_client_channel_factory_vtable sc_vtable = { - client_channel_factory_ref, client_channel_factory_unref, - client_channel_factory_create_subchannel, - client_channel_factory_create_channel}; - -static grpc_client_channel_factory cc_factory = {&sc_vtable}; - static void test_succeeds(grpc_resolver_factory *factory, const char *string) { grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; grpc_uri *uri = grpc_uri_parse(string, 0); @@ -71,7 +48,6 @@ static void test_succeeds(grpc_resolver_factory *factory, const char *string) { GPR_ASSERT(uri); memset(&args, 0, sizeof(args)); args.uri = uri; - args.client_channel_factory = &cc_factory; resolver = grpc_resolver_factory_create_resolver(factory, &args); GPR_ASSERT(resolver != NULL); GRPC_RESOLVER_UNREF(&exec_ctx, resolver, "test_succeeds"); diff --git a/test/core/client_config/resolvers/sockaddr_resolver_test.c b/test/core/client_config/resolvers/sockaddr_resolver_test.c index b11546b6b1c..d8430d39c4c 100644 --- a/test/core/client_config/resolvers/sockaddr_resolver_test.c +++ b/test/core/client_config/resolvers/sockaddr_resolver_test.c @@ -38,29 +38,6 @@ #include "src/core/ext/client_config/resolver_registry.h" #include "test/core/util/test_config.h" -static void client_channel_factory_ref(grpc_client_channel_factory *scv) {} -static void client_channel_factory_unref(grpc_exec_ctx *exec_ctx, - grpc_client_channel_factory *scv) {} -static grpc_subchannel *client_channel_factory_create_subchannel( - grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *factory, - grpc_subchannel_args *args) { - GPR_UNREACHABLE_CODE(return NULL); -} - -static grpc_channel *client_channel_factory_create_channel( - grpc_exec_ctx *exec_ctx, grpc_client_channel_factory *cc_factory, - const char *target, grpc_client_channel_type type, - grpc_channel_args *args) { - GPR_UNREACHABLE_CODE(return NULL); -} - -static const grpc_client_channel_factory_vtable sc_vtable = { - client_channel_factory_ref, client_channel_factory_unref, - client_channel_factory_create_subchannel, - client_channel_factory_create_channel}; - -static grpc_client_channel_factory cc_factory = {&sc_vtable}; - static void test_succeeds(grpc_resolver_factory *factory, const char *string) { grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; grpc_uri *uri = grpc_uri_parse(string, 0); @@ -71,7 +48,6 @@ static void test_succeeds(grpc_resolver_factory *factory, const char *string) { GPR_ASSERT(uri); memset(&args, 0, sizeof(args)); args.uri = uri; - args.client_channel_factory = &cc_factory; resolver = grpc_resolver_factory_create_resolver(factory, &args); GPR_ASSERT(resolver != NULL); GRPC_RESOLVER_UNREF(&exec_ctx, resolver, "test_succeeds"); From 12d1fc61d85bc06b2f291f341d112f94ea1dee13 Mon Sep 17 00:00:00 2001 From: Vizerai Date: Fri, 9 Sep 2016 14:22:19 -0700 Subject: [PATCH 23/40] initial commit of tracing context files --- BUILD | 6 + CMakeLists.txt | 2 + Makefile | 38 +++ binding.gyp | 1 + build.yaml | 12 + config.m4 | 1 + gRPC-Core.podspec | 5 +- grpc.gemspec | 2 + package.xml | 2 + src/core/ext/census/trace_context.c | 85 +++++++ src/core/ext/census/trace_context.h | 68 ++++++ src/python/grpcio/grpc_core_dependencies.py | 1 + test/core/census/data/context_empty.pb | 0 test/core/census/data/context_empty.txt | 0 test/core/census/data/context_full.pb | Bin 0 -> 31 bytes test/core/census/data/context_full.txt | 3 + test/core/census/data/context_no_sample.pb | Bin 0 -> 29 bytes test/core/census/data/context_no_sample.txt | 2 + test/core/census/data/context_span_only.pb | Bin 0 -> 11 bytes test/core/census/data/context_span_only.txt | 2 + test/core/census/data/context_trace_only.pb | Bin 0 -> 22 bytes test/core/census/data/context_trace_only.txt | 2 + test/core/census/trace_context_test.c | 230 ++++++++++++++++++ tools/doxygen/Doxyfile.core.internal | 2 + tools/run_tests/sources_and_headers.json | 21 +- tools/run_tests/tests.json | 21 ++ vsprojects/buildtests_c.sln | 27 ++ vsprojects/vcxproj/grpc/grpc.vcxproj | 3 + vsprojects/vcxproj/grpc/grpc.vcxproj.filters | 6 + .../grpc_unsecure/grpc_unsecure.vcxproj | 3 + .../grpc_unsecure.vcxproj.filters | 6 + .../census_trace_context_test.vcxproj | 199 +++++++++++++++ .../census_trace_context_test.vcxproj.filters | 21 ++ 33 files changed, 769 insertions(+), 2 deletions(-) create mode 100644 src/core/ext/census/trace_context.c create mode 100644 src/core/ext/census/trace_context.h create mode 100644 test/core/census/data/context_empty.pb create mode 100644 test/core/census/data/context_empty.txt create mode 100644 test/core/census/data/context_full.pb create mode 100644 test/core/census/data/context_full.txt create mode 100644 test/core/census/data/context_no_sample.pb create mode 100644 test/core/census/data/context_no_sample.txt create mode 100644 test/core/census/data/context_span_only.pb create mode 100644 test/core/census/data/context_span_only.txt create mode 100644 test/core/census/data/context_trace_only.pb create mode 100644 test/core/census/data/context_trace_only.txt create mode 100644 test/core/census/trace_context_test.c create mode 100644 vsprojects/vcxproj/test/census_trace_context_test/census_trace_context_test.vcxproj create mode 100644 vsprojects/vcxproj/test/census_trace_context_test/census_trace_context_test.vcxproj.filters diff --git a/BUILD b/BUILD index ee41d1ab217..63770ffb058 100644 --- a/BUILD +++ b/BUILD @@ -318,6 +318,7 @@ cc_library( "src/core/ext/census/mlog.h", "src/core/ext/census/resource.h", "src/core/ext/census/rpc_metric_id.h", + "src/core/ext/census/trace_context.h", "src/core/lib/surface/init.c", "src/core/lib/channel/channel_args.c", "src/core/lib/channel/channel_stack.c", @@ -503,6 +504,7 @@ cc_library( "src/core/ext/census/operation.c", "src/core/ext/census/placeholders.c", "src/core/ext/census/resource.c", + "src/core/ext/census/trace_context.c", "src/core/ext/census/tracing.c", "src/core/plugin_registry/grpc_plugin_registry.c", ], @@ -1041,6 +1043,7 @@ cc_library( "src/core/ext/census/mlog.h", "src/core/ext/census/resource.h", "src/core/ext/census/rpc_metric_id.h", + "src/core/ext/census/trace_context.h", "src/core/lib/surface/init.c", "src/core/lib/surface/init_unsecure.c", "src/core/lib/channel/channel_args.c", @@ -1197,6 +1200,7 @@ cc_library( "src/core/ext/census/operation.c", "src/core/ext/census/placeholders.c", "src/core/ext/census/resource.c", + "src/core/ext/census/trace_context.c", "src/core/ext/census/tracing.c", "src/core/plugin_registry/grpc_unsecure_plugin_registry.c", ], @@ -2349,6 +2353,7 @@ objc_library( "src/core/ext/census/operation.c", "src/core/ext/census/placeholders.c", "src/core/ext/census/resource.c", + "src/core/ext/census/trace_context.c", "src/core/ext/census/tracing.c", "src/core/plugin_registry/grpc_plugin_registry.c", ], @@ -2538,6 +2543,7 @@ objc_library( "src/core/ext/census/mlog.h", "src/core/ext/census/resource.h", "src/core/ext/census/rpc_metric_id.h", + "src/core/ext/census/trace_context.h", ], includes = [ "include", diff --git a/CMakeLists.txt b/CMakeLists.txt index 05aa323ca93..221c1328ef7 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -479,6 +479,7 @@ add_library(grpc src/core/ext/census/operation.c src/core/ext/census/placeholders.c src/core/ext/census/resource.c + src/core/ext/census/trace_context.c src/core/ext/census/tracing.c src/core/plugin_registry/grpc_plugin_registry.c ) @@ -938,6 +939,7 @@ add_library(grpc_unsecure src/core/ext/census/operation.c src/core/ext/census/placeholders.c src/core/ext/census/resource.c + src/core/ext/census/trace_context.c src/core/ext/census/tracing.c src/core/plugin_registry/grpc_unsecure_plugin_registry.c ) diff --git a/Makefile b/Makefile index e454ad6cd76..12eebec8073 100644 --- a/Makefile +++ b/Makefile @@ -909,6 +909,7 @@ bin_decoder_test: $(BINDIR)/$(CONFIG)/bin_decoder_test bin_encoder_test: $(BINDIR)/$(CONFIG)/bin_encoder_test census_context_test: $(BINDIR)/$(CONFIG)/census_context_test census_resource_test: $(BINDIR)/$(CONFIG)/census_resource_test +census_trace_context_test: $(BINDIR)/$(CONFIG)/census_trace_context_test channel_create_test: $(BINDIR)/$(CONFIG)/channel_create_test chttp2_hpack_encoder_test: $(BINDIR)/$(CONFIG)/chttp2_hpack_encoder_test chttp2_status_conversion_test: $(BINDIR)/$(CONFIG)/chttp2_status_conversion_test @@ -1235,6 +1236,7 @@ buildtests_c: privatelibs_c \ $(BINDIR)/$(CONFIG)/bin_encoder_test \ $(BINDIR)/$(CONFIG)/census_context_test \ $(BINDIR)/$(CONFIG)/census_resource_test \ + $(BINDIR)/$(CONFIG)/census_trace_context_test \ $(BINDIR)/$(CONFIG)/channel_create_test \ $(BINDIR)/$(CONFIG)/chttp2_hpack_encoder_test \ $(BINDIR)/$(CONFIG)/chttp2_status_conversion_test \ @@ -1547,6 +1549,8 @@ test_c: buildtests_c $(Q) $(BINDIR)/$(CONFIG)/census_context_test || ( echo test census_context_test failed ; exit 1 ) $(E) "[RUN] Testing census_resource_test" $(Q) $(BINDIR)/$(CONFIG)/census_resource_test || ( echo test census_resource_test failed ; exit 1 ) + $(E) "[RUN] Testing census_trace_context_test" + $(Q) $(BINDIR)/$(CONFIG)/census_trace_context_test || ( echo test census_trace_context_test failed ; exit 1 ) $(E) "[RUN] Testing channel_create_test" $(Q) $(BINDIR)/$(CONFIG)/channel_create_test || ( echo test channel_create_test failed ; exit 1 ) $(E) "[RUN] Testing chttp2_hpack_encoder_test" @@ -2700,6 +2704,7 @@ LIBGRPC_SRC = \ src/core/ext/census/operation.c \ src/core/ext/census/placeholders.c \ src/core/ext/census/resource.c \ + src/core/ext/census/trace_context.c \ src/core/ext/census/tracing.c \ src/core/plugin_registry/grpc_plugin_registry.c \ @@ -3404,6 +3409,7 @@ LIBGRPC_UNSECURE_SRC = \ src/core/ext/census/operation.c \ src/core/ext/census/placeholders.c \ src/core/ext/census/resource.c \ + src/core/ext/census/trace_context.c \ src/core/ext/census/tracing.c \ src/core/plugin_registry/grpc_unsecure_plugin_registry.c \ @@ -7221,6 +7227,38 @@ endif endif +CENSUS_TRACE_CONTEXT_TEST_SRC = \ + test/core/census/trace_context_test.c \ + +CENSUS_TRACE_CONTEXT_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(CENSUS_TRACE_CONTEXT_TEST_SRC)))) +ifeq ($(NO_SECURE),true) + +# You can't build secure targets if you don't have OpenSSL. + +$(BINDIR)/$(CONFIG)/census_trace_context_test: openssl_dep_error + +else + + + +$(BINDIR)/$(CONFIG)/census_trace_context_test: $(CENSUS_TRACE_CONTEXT_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + $(E) "[LD] Linking $@" + $(Q) mkdir -p `dirname $@` + $(Q) $(LD) $(LDFLAGS) $(CENSUS_TRACE_CONTEXT_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBS) $(LDLIBS_SECURE) -o $(BINDIR)/$(CONFIG)/census_trace_context_test + +endif + +$(OBJDIR)/$(CONFIG)/test/core/census/trace_context_test.o: $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + +deps_census_trace_context_test: $(CENSUS_TRACE_CONTEXT_TEST_OBJS:.o=.dep) + +ifneq ($(NO_SECURE),true) +ifneq ($(NO_DEPS),true) +-include $(CENSUS_TRACE_CONTEXT_TEST_OBJS:.o=.dep) +endif +endif + + CHANNEL_CREATE_TEST_SRC = \ test/core/surface/channel_create_test.c \ diff --git a/binding.gyp b/binding.gyp index 7cc8a58c01c..a710e4cae42 100644 --- a/binding.gyp +++ b/binding.gyp @@ -751,6 +751,7 @@ 'src/core/ext/census/operation.c', 'src/core/ext/census/placeholders.c', 'src/core/ext/census/resource.c', + 'src/core/ext/census/trace_context.c', 'src/core/ext/census/tracing.c', 'src/core/plugin_registry/grpc_plugin_registry.c', ], diff --git a/build.yaml b/build.yaml index 6eb23d6fb79..49ba987832b 100644 --- a/build.yaml +++ b/build.yaml @@ -29,6 +29,7 @@ filegroups: - src/core/ext/census/mlog.h - src/core/ext/census/resource.h - src/core/ext/census/rpc_metric_id.h + - src/core/ext/census/trace_context.h src: - src/core/ext/census/base_resources.c - src/core/ext/census/context.c @@ -42,6 +43,7 @@ filegroups: - src/core/ext/census/operation.c - src/core/ext/census/placeholders.c - src/core/ext/census/resource.c + - src/core/ext/census/trace_context.c - src/core/ext/census/tracing.c plugin: census_grpc_plugin uses: @@ -1344,6 +1346,16 @@ targets: - grpc - gpr_test_util - gpr +- name: census_trace_context_test + build: test + language: c + src: + - test/core/census/trace_context_test.c + deps: + - grpc_test_util + - grpc + - gpr_test_util + - gpr - name: channel_create_test build: test language: c diff --git a/config.m4 b/config.m4 index 5384585dd6c..b3e61c87bcd 100644 --- a/config.m4 +++ b/config.m4 @@ -270,6 +270,7 @@ if test "$PHP_GRPC" != "no"; then src/core/ext/census/operation.c \ src/core/ext/census/placeholders.c \ src/core/ext/census/resource.c \ + src/core/ext/census/trace_context.c \ src/core/ext/census/tracing.c \ src/core/plugin_registry/grpc_plugin_registry.c \ src/boringssl/err_data.c \ diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index deffa1653ea..d882685b927 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -413,6 +413,7 @@ Pod::Spec.new do |s| 'src/core/ext/census/mlog.h', 'src/core/ext/census/resource.h', 'src/core/ext/census/rpc_metric_id.h', + 'src/core/ext/census/trace_context.h', 'src/core/lib/surface/init.c', 'src/core/lib/channel/channel_args.c', 'src/core/lib/channel/channel_stack.c', @@ -601,6 +602,7 @@ Pod::Spec.new do |s| 'src/core/ext/census/operation.c', 'src/core/ext/census/placeholders.c', 'src/core/ext/census/resource.c', + 'src/core/ext/census/trace_context.c', 'src/core/ext/census/tracing.c', 'src/core/plugin_registry/grpc_plugin_registry.c' @@ -774,7 +776,8 @@ Pod::Spec.new do |s| 'src/core/ext/census/grpc_filter.h', 'src/core/ext/census/mlog.h', 'src/core/ext/census/resource.h', - 'src/core/ext/census/rpc_metric_id.h' + 'src/core/ext/census/rpc_metric_id.h', + 'src/core/ext/census/trace_context.h' end s.subspec 'Cronet-Interface' do |ss| diff --git a/grpc.gemspec b/grpc.gemspec index f2490060656..8edac26e063 100755 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -333,6 +333,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/ext/census/mlog.h ) s.files += %w( src/core/ext/census/resource.h ) s.files += %w( src/core/ext/census/rpc_metric_id.h ) + s.files += %w( src/core/ext/census/trace_context.h ) s.files += %w( src/core/lib/surface/init.c ) s.files += %w( src/core/lib/channel/channel_args.c ) s.files += %w( src/core/lib/channel/channel_stack.c ) @@ -521,6 +522,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/ext/census/operation.c ) s.files += %w( src/core/ext/census/placeholders.c ) s.files += %w( src/core/ext/census/resource.c ) + s.files += %w( src/core/ext/census/trace_context.c ) s.files += %w( src/core/ext/census/tracing.c ) s.files += %w( src/core/plugin_registry/grpc_plugin_registry.c ) s.files += %w( third_party/boringssl/crypto/aes/internal.h ) diff --git a/package.xml b/package.xml index 93210ee47be..86cf14bf187 100644 --- a/package.xml +++ b/package.xml @@ -340,6 +340,7 @@ + @@ -528,6 +529,7 @@ + diff --git a/src/core/ext/census/trace_context.c b/src/core/ext/census/trace_context.c new file mode 100644 index 00000000000..976d69ab261 --- /dev/null +++ b/src/core/ext/census/trace_context.c @@ -0,0 +1,85 @@ +/* + * + * 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. + * + */ + +#include "src/core/ext/census/trace_context.h" + +#include +#include +#include + +#include "third_party/nanopb/pb_decode.h" +#include "third_party/nanopb/pb_encode.h" + +// This function assumes the TraceContext is valid. +size_t encode_trace_context(google_trace_TraceContext *ctxt, uint8_t *buffer, + const size_t buf_size) { + // Create a stream that will write to our buffer. + pb_ostream_t stream = pb_ostream_from_buffer(buffer, buf_size); + + // encode message + bool status = pb_encode(&stream, google_trace_TraceContext_fields, ctxt); + + if (!status) { + gpr_log(GPR_DEBUG, "TraceContext encoding failed: %s", + PB_GET_ERROR(&stream)); + return 0; + } + + return stream.bytes_written; +} + +bool decode_trace_context(google_trace_TraceContext *ctxt, uint8_t *buffer, + const size_t nbytes) { + // Create a stream that reads nbytes from the buffer. + pb_istream_t stream = pb_istream_from_buffer(buffer, nbytes); + + // decode message + bool status = pb_decode(&stream, google_trace_TraceContext_fields, ctxt); + + if (!status) { + gpr_log(GPR_DEBUG, "TraceContext decoding failed: %s", + PB_GET_ERROR(&stream)); + } + + // check fields + if (!ctxt->has_trace_id) { + gpr_log(GPR_DEBUG, "Invalid TraceContext: missing trace_id"); + return false; + } + if (!ctxt->has_span_id) { + gpr_log(GPR_DEBUG, "Invalid TraceContext: missing span_id"); + return false; + } + + return true; +} diff --git a/src/core/ext/census/trace_context.h b/src/core/ext/census/trace_context.h new file mode 100644 index 00000000000..ee71fef460e --- /dev/null +++ b/src/core/ext/census/trace_context.h @@ -0,0 +1,68 @@ +/* + * + * 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. + * + */ + +/* Functions for manipulating trace contexts as defined in + src/proto/census/trace.proto */ +#ifndef GRPC_CORE_EXT_CENSUS_TRACE_CONTEXT_H +#define GRPC_CORE_EXT_CENSUS_TRACE_CONTEXT_H + +#include "src/core/ext/census/gen/trace_context.pb.h" + +/* Maximum number of bytes required to encode a TraceContext (31) +1 byte for trace_id field +1 byte for trace_id length +1 byte for trace_id.hi field +8 bytes for trace_id.hi (uint64_t) +1 byte for trace_id.lo field +8 bytes for trace_id.lo (uint64_t) +1 byte for span_id field +8 bytes for span_id (uint64_t) +1 byte for is_sampled field +1 byte for is_sampled (bool) */ +#define TRACE_MAX_CONTEXT_SIZE 31 + +/* Encode a trace context (ctxt) into proto format to the buffer provided. The +size of buffer must be at least TRACE_MAX_CONTEXT_SIZE. On success, returns the +number of bytes successfully encoded into buffer. On failure, returns 0. */ +size_t encode_trace_context(google_trace_TraceContext *ctxt, uint8_t *buffer, + const size_t buf_size); + +/* Decode a proto-encoded TraceContext from the provided buffer into the +TraceContext structure (ctxt). The function expects to be supplied the number +of bytes to be read from buffer (nbytes). This function will also validate that +the TraceContext has a span_id and a trace_id, and will return false if either +of these do not exist. On success, returns true and false otherwise. */ +bool decode_trace_context(google_trace_TraceContext *ctxt, uint8_t *buffer, + const size_t nbytes); + +#endif diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index be7f30c29b3..2245b0b9c52 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -264,6 +264,7 @@ CORE_SOURCE_FILES = [ 'src/core/ext/census/operation.c', 'src/core/ext/census/placeholders.c', 'src/core/ext/census/resource.c', + 'src/core/ext/census/trace_context.c', 'src/core/ext/census/tracing.c', 'src/core/plugin_registry/grpc_plugin_registry.c', 'src/boringssl/err_data.c', diff --git a/test/core/census/data/context_empty.pb b/test/core/census/data/context_empty.pb new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test/core/census/data/context_empty.txt b/test/core/census/data/context_empty.txt new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test/core/census/data/context_full.pb b/test/core/census/data/context_full.pb new file mode 100644 index 0000000000000000000000000000000000000000..80ebcf280bc5a358d595eb88ff84420bcfe9d11e GIT binary patch literal 31 Wcmd-Q;$&rj06|6=gB{9{U<3dMn*eSA literal 0 HcmV?d00001 diff --git a/test/core/census/data/context_full.txt b/test/core/census/data/context_full.txt new file mode 100644 index 00000000000..7901a10c336 --- /dev/null +++ b/test/core/census/data/context_full.txt @@ -0,0 +1,3 @@ +trace_id { hi : 5; lo : 1 } +span_id : 7 +is_sampled : true diff --git a/test/core/census/data/context_no_sample.pb b/test/core/census/data/context_no_sample.pb new file mode 100644 index 0000000000000000000000000000000000000000..ab7ad7d1091955636cfaa7e342d94497b717e55b GIT binary patch literal 29 Ucmd-Q;$&rj06|6=gB{8M00zJSRR910 literal 0 HcmV?d00001 diff --git a/test/core/census/data/context_no_sample.txt b/test/core/census/data/context_no_sample.txt new file mode 100644 index 00000000000..150298002fc --- /dev/null +++ b/test/core/census/data/context_no_sample.txt @@ -0,0 +1,2 @@ +trace_id { hi : 5; lo : 1 } +span_id : 7 diff --git a/test/core/census/data/context_span_only.pb b/test/core/census/data/context_span_only.pb new file mode 100644 index 0000000000000000000000000000000000000000..2a9527a75a6ddb2ea7a17d096ad5c70615512819 GIT binary patch literal 11 NcmWe+XMg|+MgRdl05Sjo literal 0 HcmV?d00001 diff --git a/test/core/census/data/context_span_only.txt b/test/core/census/data/context_span_only.txt new file mode 100644 index 00000000000..d90de2e614b --- /dev/null +++ b/test/core/census/data/context_span_only.txt @@ -0,0 +1,2 @@ +span_id : 7 +is_sampled : true diff --git a/test/core/census/data/context_trace_only.pb b/test/core/census/data/context_trace_only.pb new file mode 100644 index 0000000000000000000000000000000000000000..7fdf6f61a3f2e4b72edc05f23a0632043d4f6bcc GIT binary patch literal 22 Tcmd-Q;$&rj06|75LxK?i1a$yb literal 0 HcmV?d00001 diff --git a/test/core/census/data/context_trace_only.txt b/test/core/census/data/context_trace_only.txt new file mode 100644 index 00000000000..9b68a6aa928 --- /dev/null +++ b/test/core/census/data/context_trace_only.txt @@ -0,0 +1,2 @@ +trace_id { hi : 5; lo : 1 } +is_sampled : true diff --git a/test/core/census/trace_context_test.c b/test/core/census/trace_context_test.c new file mode 100644 index 00000000000..1d134285623 --- /dev/null +++ b/test/core/census/trace_context_test.c @@ -0,0 +1,230 @@ +/* + * + * 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. + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include "src/core/ext/census/base_resources.h" +#include "src/core/ext/census/resource.h" +#include "test/core/util/test_config.h" + +#include "src/core/ext/census/gen/trace_context.pb.h" +#include "src/core/ext/census/trace_context.h" +#include "third_party/nanopb/pb_decode.h" +#include "third_party/nanopb/pb_encode.h" + +#define BUF_SIZE 256 + +/* Encodes a TraceContext structure (ctxt1) to a buffer, and then decodes it +to a second TraceContext (ctxt2). Validates that the resulting TraceContext +has a span_id, trace_id, and that the values are equal to those in initial +TraceContext. On success, returns true. If encode_trace_context returns 0, +decode_trace_context fails, or the resulting TraceContext is missing a trace_id +or span_id, it will return false. */ +bool validate_encode_decode_context(google_trace_TraceContext *ctxt1, + uint8_t *buffer, size_t buf_size) { + google_trace_TraceContext ctxt2 = google_trace_TraceContext_init_zero; + size_t msg_length; + GPR_ASSERT(ctxt1->has_trace_id && ctxt1->has_span_id); + + msg_length = encode_trace_context(ctxt1, buffer, buf_size); + if (msg_length == 0) { + return false; + } + + if (!decode_trace_context(&ctxt2, buffer, msg_length)) { + return false; + } + + if (!ctxt2.has_trace_id || !ctxt2.has_span_id) { + return false; + } + + GPR_ASSERT( + ctxt1->trace_id.hi == ctxt2.trace_id.hi && + ctxt1->trace_id.lo == ctxt2.trace_id.lo && + ctxt1->span_id == ctxt2.span_id && + ctxt1->has_is_sampled == ctxt2.has_is_sampled && + (ctxt1->has_is_sampled ? ctxt1->is_sampled == ctxt2.is_sampled : true)); + + return true; +} + +/* Decodes a proto-encoded TraceContext from a buffer. If decode_trace_context +fails or the resulting TraceContext is missing a trace_id or span_id it will +return false, otherwise returns true. */ +bool validate_decode_context(google_trace_TraceContext *ctxt, uint8_t *buffer, + size_t msg_length) { + // Validate the decoding of a context written to buffer. + if (!decode_trace_context(ctxt, buffer, msg_length)) { + return false; + } + + if (!ctxt->has_trace_id || !ctxt->has_span_id) { + return false; + } + + return true; +} + +/* Read an encoded trace context from a file. Validates that the decoding +gives the expected result (succeed). */ +static void read_and_validate_context_from_file(google_trace_TraceContext *ctxt, + const char *file, + const bool succeed) { + uint8_t buffer[BUF_SIZE]; + FILE *input = fopen(file, "rb"); + GPR_ASSERT(input != NULL); + size_t nbytes = fread(buffer, 1, BUF_SIZE, input); + GPR_ASSERT(nbytes <= BUF_SIZE && feof(input) && !ferror(input)); + bool res = validate_decode_context(ctxt, buffer, nbytes); + GPR_ASSERT(res == succeed); + GPR_ASSERT(fclose(input) == 0); +} + +// Test full proto-buffer. +static void test_full() { + google_trace_TraceContext ctxt = google_trace_TraceContext_init_zero; + read_and_validate_context_from_file( + &ctxt, "test/core/census/data/context_full.pb", true); +} + +// Test empty proto-buffer. +static void test_empty() { + google_trace_TraceContext ctxt = google_trace_TraceContext_init_zero; + read_and_validate_context_from_file( + &ctxt, "test/core/census/data/context_empty.pb", false); +} + +// Test proto-buffer with only trace_id. +static void test_trace_only() { + google_trace_TraceContext ctxt = google_trace_TraceContext_init_zero; + read_and_validate_context_from_file( + &ctxt, "test/core/census/data/context_trace_only.pb", false); +} + +// Test proto-buffer with only span_id. +static void test_span_only() { + google_trace_TraceContext ctxt = google_trace_TraceContext_init_zero; + read_and_validate_context_from_file( + &ctxt, "test/core/census/data/context_span_only.pb", false); +} + +// Test proto-buffer without is_sampled value. +static void test_no_sample() { + google_trace_TraceContext ctxt = google_trace_TraceContext_init_zero; + read_and_validate_context_from_file( + &ctxt, "test/core/census/data/context_no_sample.pb", true); + GPR_ASSERT(ctxt.has_is_sampled == false && ctxt.is_sampled == false); +} + +static void test_encode_decode() { + uint8_t buffer[BUF_SIZE] = {0}; + + google_trace_TraceContext ctxt1 = google_trace_TraceContext_init_zero; + ctxt1.has_trace_id = true; + ctxt1.trace_id.has_hi = true; + ctxt1.trace_id.has_lo = true; + ctxt1.trace_id.lo = 1; + ctxt1.trace_id.hi = 2; + ctxt1.has_span_id = true; + ctxt1.span_id = 3; + validate_encode_decode_context(&ctxt1, buffer, sizeof(buffer)); + + google_trace_TraceContext ctxt2 = google_trace_TraceContext_init_zero; + ctxt2.has_trace_id = true; + ctxt2.trace_id.has_hi = false; + ctxt2.trace_id.has_lo = false; + ctxt2.has_span_id = true; + validate_encode_decode_context(&ctxt2, buffer, sizeof(buffer)); +} + +// Test a corrupted proto-buffer. +static void test_corrupt() { + uint8_t buffer[BUF_SIZE] = {0}; + google_trace_TraceContext ctxt1 = google_trace_TraceContext_init_zero; + size_t msg_length; + + ctxt1.has_trace_id = true; + ctxt1.trace_id.has_hi = true; + ctxt1.trace_id.has_lo = true; + ctxt1.trace_id.lo = 1; + ctxt1.trace_id.hi = 2; + ctxt1.has_span_id = true; + ctxt1.span_id = 3; + ctxt1.is_sampled = true; + msg_length = encode_trace_context(&ctxt1, buffer, sizeof(buffer)); + + // Corrupt some bytes. + buffer[0] = 255; + + bool res = validate_decode_context(&ctxt1, buffer, msg_length); + GPR_ASSERT(res == false); +} + +static void test_buffer_size() { + // This buffer is too small, so the encode should fail. + uint8_t buffer[16] = {0}; + google_trace_TraceContext ctxt1 = google_trace_TraceContext_init_zero; + size_t msg_length; + + ctxt1.has_trace_id = true; + ctxt1.trace_id.has_hi = true; + ctxt1.trace_id.has_lo = true; + ctxt1.trace_id.lo = 1; + ctxt1.trace_id.hi = 2; + ctxt1.has_span_id = true; + ctxt1.span_id = 3; + ctxt1.is_sampled = true; + msg_length = encode_trace_context(&ctxt1, buffer, sizeof(buffer)); + + GPR_ASSERT(msg_length == 0); +} + +int main(int argc, char **argv) { + grpc_test_init(argc, argv); + test_full(); + test_empty(); + test_trace_only(); + test_span_only(); + test_encode_decode(); + test_corrupt(); + test_no_sample(); + test_buffer_size(); + + return 0; +} diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 02590db4214..29611e100e9 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -949,6 +949,7 @@ src/core/ext/census/grpc_filter.h \ src/core/ext/census/mlog.h \ src/core/ext/census/resource.h \ src/core/ext/census/rpc_metric_id.h \ +src/core/ext/census/trace_context.h \ src/core/lib/surface/init.c \ src/core/lib/channel/channel_args.c \ src/core/lib/channel/channel_stack.c \ @@ -1137,6 +1138,7 @@ src/core/ext/census/mlog.c \ src/core/ext/census/operation.c \ src/core/ext/census/placeholders.c \ src/core/ext/census/resource.c \ +src/core/ext/census/trace_context.c \ src/core/ext/census/tracing.c \ src/core/plugin_registry/grpc_plugin_registry.c \ include/grpc/support/alloc.h \ diff --git a/tools/run_tests/sources_and_headers.json b/tools/run_tests/sources_and_headers.json index db84f219687..3e6d167d922 100644 --- a/tools/run_tests/sources_and_headers.json +++ b/tools/run_tests/sources_and_headers.json @@ -156,6 +156,22 @@ "third_party": false, "type": "target" }, + { + "deps": [ + "gpr", + "gpr_test_util", + "grpc", + "grpc_test_util" + ], + "headers": [], + "language": "c", + "name": "census_trace_context_test", + "src": [ + "test/core/census/trace_context_test.c" + ], + "third_party": false, + "type": "target" + }, { "deps": [ "gpr", @@ -5665,7 +5681,8 @@ "src/core/ext/census/grpc_filter.h", "src/core/ext/census/mlog.h", "src/core/ext/census/resource.h", - "src/core/ext/census/rpc_metric_id.h" + "src/core/ext/census/rpc_metric_id.h", + "src/core/ext/census/trace_context.h" ], "language": "c", "name": "census", @@ -5693,6 +5710,8 @@ "src/core/ext/census/resource.c", "src/core/ext/census/resource.h", "src/core/ext/census/rpc_metric_id.h", + "src/core/ext/census/trace_context.c", + "src/core/ext/census/trace_context.h", "src/core/ext/census/tracing.c" ], "third_party": false, diff --git a/tools/run_tests/tests.json b/tools/run_tests/tests.json index f97ee835c14..3b9c9ae6e4a 100644 --- a/tools/run_tests/tests.json +++ b/tools/run_tests/tests.json @@ -190,6 +190,27 @@ "windows" ] }, + { + "args": [], + "ci_platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "flaky": false, + "gtest": false, + "language": "c", + "name": "census_trace_context_test", + "platforms": [ + "linux", + "mac", + "posix", + "windows" + ] + }, { "args": [], "ci_platforms": [ diff --git a/vsprojects/buildtests_c.sln b/vsprojects/buildtests_c.sln index 8f3546f7bec..51855d7ca54 100644 --- a/vsprojects/buildtests_c.sln +++ b/vsprojects/buildtests_c.sln @@ -120,6 +120,17 @@ Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "census_resource_test", "vcx {B23D3D1A-9438-4EDA-BEB6-9A0A03D17792} = {B23D3D1A-9438-4EDA-BEB6-9A0A03D17792} EndProjectSection EndProject +Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "census_trace_context_test", "vcxproj\test\census_trace_context_test\census_trace_context_test.vcxproj", "{14511310-FAE4-C287-31DC-D6F456D10AF3}" + ProjectSection(myProperties) = preProject + lib = "False" + EndProjectSection + ProjectSection(ProjectDependencies) = postProject + {17BCAFC0-5FDC-4C94-AEB9-95F3E220614B} = {17BCAFC0-5FDC-4C94-AEB9-95F3E220614B} + {29D16885-7228-4C31-81ED-5F9187C7F2A9} = {29D16885-7228-4C31-81ED-5F9187C7F2A9} + {EAB0A629-17A9-44DB-B5FF-E91A721FE037} = {EAB0A629-17A9-44DB-B5FF-E91A721FE037} + {B23D3D1A-9438-4EDA-BEB6-9A0A03D17792} = {B23D3D1A-9438-4EDA-BEB6-9A0A03D17792} + EndProjectSection +EndProject Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "channel_create_test", "vcxproj\test\channel_create_test\channel_create_test.vcxproj", "{AFC88484-3A2E-32BC-25B2-23DF741D4F3D}" ProjectSection(myProperties) = preProject lib = "False" @@ -1640,6 +1651,22 @@ Global {18CF99B5-3C61-EC3D-9509-3C95334C3B88}.Release-DLL|Win32.Build.0 = Release|Win32 {18CF99B5-3C61-EC3D-9509-3C95334C3B88}.Release-DLL|x64.ActiveCfg = Release|x64 {18CF99B5-3C61-EC3D-9509-3C95334C3B88}.Release-DLL|x64.Build.0 = Release|x64 + {14511310-FAE4-C287-31DC-D6F456D10AF3}.Debug|Win32.ActiveCfg = Debug|Win32 + {14511310-FAE4-C287-31DC-D6F456D10AF3}.Debug|x64.ActiveCfg = Debug|x64 + {14511310-FAE4-C287-31DC-D6F456D10AF3}.Release|Win32.ActiveCfg = Release|Win32 + {14511310-FAE4-C287-31DC-D6F456D10AF3}.Release|x64.ActiveCfg = Release|x64 + {14511310-FAE4-C287-31DC-D6F456D10AF3}.Debug|Win32.Build.0 = Debug|Win32 + {14511310-FAE4-C287-31DC-D6F456D10AF3}.Debug|x64.Build.0 = Debug|x64 + {14511310-FAE4-C287-31DC-D6F456D10AF3}.Release|Win32.Build.0 = Release|Win32 + {14511310-FAE4-C287-31DC-D6F456D10AF3}.Release|x64.Build.0 = Release|x64 + {14511310-FAE4-C287-31DC-D6F456D10AF3}.Debug-DLL|Win32.ActiveCfg = Debug|Win32 + {14511310-FAE4-C287-31DC-D6F456D10AF3}.Debug-DLL|Win32.Build.0 = Debug|Win32 + {14511310-FAE4-C287-31DC-D6F456D10AF3}.Debug-DLL|x64.ActiveCfg = Debug|x64 + {14511310-FAE4-C287-31DC-D6F456D10AF3}.Debug-DLL|x64.Build.0 = Debug|x64 + {14511310-FAE4-C287-31DC-D6F456D10AF3}.Release-DLL|Win32.ActiveCfg = Release|Win32 + {14511310-FAE4-C287-31DC-D6F456D10AF3}.Release-DLL|Win32.Build.0 = Release|Win32 + {14511310-FAE4-C287-31DC-D6F456D10AF3}.Release-DLL|x64.ActiveCfg = Release|x64 + {14511310-FAE4-C287-31DC-D6F456D10AF3}.Release-DLL|x64.Build.0 = Release|x64 {AFC88484-3A2E-32BC-25B2-23DF741D4F3D}.Debug|Win32.ActiveCfg = Debug|Win32 {AFC88484-3A2E-32BC-25B2-23DF741D4F3D}.Debug|x64.ActiveCfg = Debug|x64 {AFC88484-3A2E-32BC-25B2-23DF741D4F3D}.Release|Win32.ActiveCfg = Release|Win32 diff --git a/vsprojects/vcxproj/grpc/grpc.vcxproj b/vsprojects/vcxproj/grpc/grpc.vcxproj index 7c120bcf022..ca5f5541aa6 100644 --- a/vsprojects/vcxproj/grpc/grpc.vcxproj +++ b/vsprojects/vcxproj/grpc/grpc.vcxproj @@ -458,6 +458,7 @@ + @@ -836,6 +837,8 @@ + + diff --git a/vsprojects/vcxproj/grpc/grpc.vcxproj.filters b/vsprojects/vcxproj/grpc/grpc.vcxproj.filters index 9cbb2ce45bb..10f4c28a9ec 100644 --- a/vsprojects/vcxproj/grpc/grpc.vcxproj.filters +++ b/vsprojects/vcxproj/grpc/grpc.vcxproj.filters @@ -565,6 +565,9 @@ src\core\ext\census + + src\core\ext\census + src\core\ext\census @@ -1142,6 +1145,9 @@ src\core\ext\census + + src\core\ext\census + diff --git a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj index d4a85768c33..68be5895a82 100644 --- a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj +++ b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj @@ -424,6 +424,7 @@ + @@ -744,6 +745,8 @@ + + diff --git a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters index fba5a020995..d52022d67b1 100644 --- a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters +++ b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters @@ -478,6 +478,9 @@ src\core\ext\census + + src\core\ext\census + src\core\ext\census @@ -980,6 +983,9 @@ src\core\ext\census + + src\core\ext\census + diff --git a/vsprojects/vcxproj/test/census_trace_context_test/census_trace_context_test.vcxproj b/vsprojects/vcxproj/test/census_trace_context_test/census_trace_context_test.vcxproj new file mode 100644 index 00000000000..f0c76e7f04c --- /dev/null +++ b/vsprojects/vcxproj/test/census_trace_context_test/census_trace_context_test.vcxproj @@ -0,0 +1,199 @@ + + + + + + Debug + Win32 + + + Debug + x64 + + + Release + Win32 + + + Release + x64 + + + + {14511310-FAE4-C287-31DC-D6F456D10AF3} + true + $(SolutionDir)IntDir\$(MSBuildProjectName)\ + + + + v100 + + + v110 + + + v120 + + + v140 + + + Application + true + Unicode + + + Application + false + true + Unicode + + + + + + + + + + + + + + census_trace_context_test + static + Debug + static + Debug + + + census_trace_context_test + static + Release + static + Release + + + + NotUsing + Level3 + Disabled + WIN32;_DEBUG;_LIB;%(PreprocessorDefinitions) + true + MultiThreadedDebug + true + None + false + + + Console + true + false + + + + + + NotUsing + Level3 + Disabled + WIN32;_DEBUG;_LIB;%(PreprocessorDefinitions) + true + MultiThreadedDebug + true + None + false + + + Console + true + false + + + + + + NotUsing + Level3 + MaxSpeed + WIN32;NDEBUG;_LIB;%(PreprocessorDefinitions) + true + true + true + MultiThreaded + true + None + false + + + Console + true + false + true + true + + + + + + NotUsing + Level3 + MaxSpeed + WIN32;NDEBUG;_LIB;%(PreprocessorDefinitions) + true + true + true + MultiThreaded + true + None + false + + + Console + true + false + true + true + + + + + + + + + + {17BCAFC0-5FDC-4C94-AEB9-95F3E220614B} + + + {29D16885-7228-4C31-81ED-5F9187C7F2A9} + + + {EAB0A629-17A9-44DB-B5FF-E91A721FE037} + + + {B23D3D1A-9438-4EDA-BEB6-9A0A03D17792} + + + + + + + + + + + + + + + This project references NuGet package(s) that are missing on this computer. Enable NuGet Package Restore to download them. For more information, see http://go.microsoft.com/fwlink/?LinkID=322105. The missing file is {0}. + + + + + + + + + diff --git a/vsprojects/vcxproj/test/census_trace_context_test/census_trace_context_test.vcxproj.filters b/vsprojects/vcxproj/test/census_trace_context_test/census_trace_context_test.vcxproj.filters new file mode 100644 index 00000000000..1aafe06ff04 --- /dev/null +++ b/vsprojects/vcxproj/test/census_trace_context_test/census_trace_context_test.vcxproj.filters @@ -0,0 +1,21 @@ + + + + + test\core\census + + + + + + {ceb3b75d-b5bc-0966-6724-06fb51237d08} + + + {8d294ce5-d65c-2fef-28ab-43b4d23722c3} + + + {3981af30-20c0-647e-755f-fa184674d50a} + + + + From cb849ef58f95473006361e8889028cb6eaef58eb Mon Sep 17 00:00:00 2001 From: Vizerai Date: Fri, 9 Sep 2016 14:39:31 -0700 Subject: [PATCH 24/40] update --- src/core/ext/census/trace_context.c | 1 + test/core/census/trace_context_test.c | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/core/ext/census/trace_context.c b/src/core/ext/census/trace_context.c index 976d69ab261..fbb20d3448f 100644 --- a/src/core/ext/census/trace_context.c +++ b/src/core/ext/census/trace_context.c @@ -69,6 +69,7 @@ bool decode_trace_context(google_trace_TraceContext *ctxt, uint8_t *buffer, if (!status) { gpr_log(GPR_DEBUG, "TraceContext decoding failed: %s", PB_GET_ERROR(&stream)); + return false; } // check fields diff --git a/test/core/census/trace_context_test.c b/test/core/census/trace_context_test.c index 1d134285623..ee409e8d1a5 100644 --- a/test/core/census/trace_context_test.c +++ b/test/core/census/trace_context_test.c @@ -189,7 +189,8 @@ static void test_corrupt() { ctxt1.is_sampled = true; msg_length = encode_trace_context(&ctxt1, buffer, sizeof(buffer)); - // Corrupt some bytes. + /* Corrupt some bytes. 255 (0xFF) should be illegal for the first byte of the + proto encoded object. */ buffer[0] = 255; bool res = validate_decode_context(&ctxt1, buffer, msg_length); From 38525a9a082df5c8a61efe046cde9cb14ae4170c Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 9 Sep 2016 14:45:00 -0700 Subject: [PATCH 25/40] Fix use-after-free bug. --- src/core/ext/client_config/resolver_result.c | 7 +++++++ src/core/ext/client_config/resolver_result.h | 2 ++ src/core/ext/resolver/sockaddr/sockaddr_resolver.c | 4 ++-- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/core/ext/client_config/resolver_result.c b/src/core/ext/client_config/resolver_result.c index ba21349d2d5..b0602d583d6 100644 --- a/src/core/ext/client_config/resolver_result.c +++ b/src/core/ext/client_config/resolver_result.c @@ -47,6 +47,13 @@ grpc_addresses *grpc_addresses_create(size_t num_addresses) { return addresses; } +grpc_addresses *grpc_addresses_copy(grpc_addresses* addresses) { + grpc_addresses *new = grpc_addresses_create(addresses->num_addresses); + memcpy(new->addresses, addresses->addresses, + sizeof(grpc_address) * addresses->num_addresses); + return new; +} + void grpc_addresses_set_address(grpc_addresses *addresses, size_t index, void *address, size_t address_len, bool is_balancer) { diff --git a/src/core/ext/client_config/resolver_result.h b/src/core/ext/client_config/resolver_result.h index df92bec984e..b1a34575654 100644 --- a/src/core/ext/client_config/resolver_result.h +++ b/src/core/ext/client_config/resolver_result.h @@ -54,6 +54,8 @@ typedef struct grpc_addresses { \a num_addresses addresses. */ grpc_addresses *grpc_addresses_create(size_t num_addresses); +grpc_addresses *grpc_addresses_copy(grpc_addresses* addresses); + void grpc_addresses_set_address(grpc_addresses *addresses, size_t index, void *address, size_t address_len, bool is_balancer); diff --git a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c index 328c7cb6f93..5c59e4397af 100644 --- a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c +++ b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c @@ -120,8 +120,8 @@ static void sockaddr_maybe_finish_next_locked(grpc_exec_ctx *exec_ctx, sockaddr_resolver *r) { if (r->next_completion != NULL && !r->published) { r->published = true; - *r->target_result = - grpc_resolver_result_create(r->addresses, r->lb_policy_name); + *r->target_result = grpc_resolver_result_create( + grpc_addresses_copy(r->addresses), r->lb_policy_name); grpc_exec_ctx_sched(exec_ctx, r->next_completion, GRPC_ERROR_NONE, NULL); r->next_completion = NULL; } From 76988c80a2833134aa481fb3a10f959986f7269a Mon Sep 17 00:00:00 2001 From: Vizerai Date: Tue, 13 Sep 2016 14:22:54 -0700 Subject: [PATCH 26/40] update --- src/core/ext/census/tracing.c | 18 ++++++++++++------ test/core/census/trace_context_test.c | 1 + 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/core/ext/census/tracing.c b/src/core/ext/census/tracing.c index 3b5d6dab2b8..18f8daa1763 100644 --- a/src/core/ext/census/tracing.c +++ b/src/core/ext/census/tracing.c @@ -31,15 +31,21 @@ * */ +//#include "src/core/ext/census/tracing.h" + #include + + /* TODO(aveitch): These are all placeholder implementations. */ -int census_trace_mask(const census_context *context) { - return CENSUS_TRACE_MASK_NONE; -} +// int census_trace_mask(const census_context *context) { +// return CENSUS_TRACE_MASK_NONE; +// } + +// void census_set_trace_mask(int trace_mask) {} -void census_set_trace_mask(int trace_mask) {} +// void census_trace_print(census_context *context, uint32_t type, +// const char *buffer, size_t n) {} -void census_trace_print(census_context *context, uint32_t type, - const char *buffer, size_t n) {} +// void SetTracerParams(const Params& params); diff --git a/test/core/census/trace_context_test.c b/test/core/census/trace_context_test.c index ee409e8d1a5..d98181c03ba 100644 --- a/test/core/census/trace_context_test.c +++ b/test/core/census/trace_context_test.c @@ -227,5 +227,6 @@ int main(int argc, char **argv) { test_no_sample(); test_buffer_size(); + fprintf(stderr, "sizeof(void*): %lu\n", sizeof(void *)); return 0; } From afcb0f7b2d5ffdd9978418692e9a1d73e87c7d76 Mon Sep 17 00:00:00 2001 From: Vizerai Date: Tue, 13 Sep 2016 14:57:51 -0700 Subject: [PATCH 27/40] update --- test/core/census/trace_context_test.c | 1 - 1 file changed, 1 deletion(-) diff --git a/test/core/census/trace_context_test.c b/test/core/census/trace_context_test.c index d98181c03ba..ee409e8d1a5 100644 --- a/test/core/census/trace_context_test.c +++ b/test/core/census/trace_context_test.c @@ -227,6 +227,5 @@ int main(int argc, char **argv) { test_no_sample(); test_buffer_size(); - fprintf(stderr, "sizeof(void*): %lu\n", sizeof(void *)); return 0; } From 6751ac6ffe77c32fe2e4f4873f25ae4e5019bb18 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 15 Sep 2016 08:40:21 -0700 Subject: [PATCH 28/40] Add doc listing API changes to clean up for the next minor-version release. --- doc/core/pending_api_cleanups.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 doc/core/pending_api_cleanups.md diff --git a/doc/core/pending_api_cleanups.md b/doc/core/pending_api_cleanups.md new file mode 100644 index 00000000000..a16d3bf7781 --- /dev/null +++ b/doc/core/pending_api_cleanups.md @@ -0,0 +1,15 @@ +There are times when we make changes that include a temporary shim for +backward-compatibility (e.g., a macro or some other function to preserve +the original API) to avoid having to bump the minor version number in +the next release. However, when we do eventually want to release a +feature that does change the API in a non-backward-compatible way, we +will wind up bumping the minor version number anyway, at which point we +can take the opportunity to clean up any pending backward-compatibility +shims. + +This file lists all pending backward-compatibility changes that should +be cleaned up the next time we are going to bump the minor version +number: + +- remove `GRPC_ARG_MAX_MESSAGE_LENGTH` channel arg from + `include/grpc/impl/codegen/grpc_types.h` (commit `af00d8b`) From 0e256107e812807967aa37ee1dc0ca5e954e9a2c Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 15 Sep 2016 09:01:05 -0700 Subject: [PATCH 29/40] Code review changes. --- include/grpc++/server_builder.h | 2 +- include/grpc/impl/codegen/grpc_types.h | 2 +- src/core/lib/channel/channel_args.h | 2 +- src/core/lib/channel/message_size_filter.c | 13 +++++-------- src/cpp/server/server_builder.cc | 4 ++-- test/core/end2end/tests/max_message_length.c | 2 +- 6 files changed, 11 insertions(+), 14 deletions(-) diff --git a/include/grpc++/server_builder.h b/include/grpc++/server_builder.h index 326962bf782..37f1f8cb803 100644 --- a/include/grpc++/server_builder.h +++ b/include/grpc++/server_builder.h @@ -90,7 +90,7 @@ class ServerBuilder { return *this; } - /// For backward compatibility. + /// \deprecated For backward compatibility. ServerBuilder& SetMaxMessageSize(int max_message_size) { return SetMaxReceiveMessageSize(max_message_size); } diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index 7ba6b96868b..1a3cb650831 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -128,7 +128,7 @@ typedef struct { #define GRPC_ARG_MAX_CONCURRENT_STREAMS "grpc.max_concurrent_streams" /** Maximum message length that the channel can receive. Int valued, bytes. */ #define GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH "grpc.max_receive_message_length" -/** Backward compatibility. */ +/** \deprecated For backward compatibility. */ #define GRPC_ARG_MAX_MESSAGE_LENGTH GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH /** Maximum message length that the channel can send. Int valued, bytes. */ #define GRPC_ARG_MAX_SEND_MESSAGE_LENGTH "grpc.max_send_message_length" diff --git a/src/core/lib/channel/channel_args.h b/src/core/lib/channel/channel_args.h index 71fde5ad7ed..586a296d1f6 100644 --- a/src/core/lib/channel/channel_args.h +++ b/src/core/lib/channel/channel_args.h @@ -89,12 +89,12 @@ uint32_t grpc_channel_args_compression_algorithm_get_states( int grpc_channel_args_compare(const grpc_channel_args *a, const grpc_channel_args *b); -/** Returns the value of \a arg, subject to the contraints in \a options. */ typedef struct grpc_integer_options { int default_value; // Return this if value is outside of expected bounds. int min_value; int max_value; } grpc_integer_options; +/** Returns the value of \a arg, subject to the contraints in \a options. */ int grpc_channel_arg_get_integer(grpc_arg *arg, grpc_integer_options options); #endif /* GRPC_CORE_LIB_CHANNEL_CHANNEL_ARGS_H */ diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index e2d7ff795c4..f8bb1134ec6 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -70,17 +70,15 @@ static void recv_message_ready(grpc_exec_ctx* exec_ctx, void* user_data, (*calld->recv_message)->length > chand->max_recv_size) { char* message_string; gpr_asprintf(&message_string, - "Received message larger than max (%lu vs. %lu)", - (unsigned long)(*calld->recv_message)->length, - (unsigned long)chand->max_recv_size); + "Received message larger than max (%u vs. %zu)", + (*calld->recv_message)->length, chand->max_recv_size); gpr_slice message = gpr_slice_from_copied_string(message_string); gpr_free(message_string); grpc_call_element_send_cancel_with_message( exec_ctx, elem, GRPC_STATUS_INVALID_ARGUMENT, &message); } // Invoke the next callback. - calld->next_recv_message_ready->cb( - exec_ctx, calld->next_recv_message_ready->cb_arg, error); + grpc_exec_ctx_sched(exec_ctx, calld->next_recv_message_ready, error, NULL); } // Start transport op. @@ -93,9 +91,8 @@ static void start_transport_stream_op(grpc_exec_ctx* exec_ctx, if (op->send_message != NULL && op->send_message->length > chand->max_send_size) { char* message_string; - gpr_asprintf(&message_string, "Sent message larger than max (%lu vs. %lu)", - (unsigned long)op->send_message->length, - (unsigned long)chand->max_send_size); + gpr_asprintf(&message_string, "Sent message larger than max (%u vs. %zu)", + op->send_message->length, chand->max_send_size); gpr_slice message = gpr_slice_from_copied_string(message_string); gpr_free(message_string); grpc_call_element_send_cancel_with_message( diff --git a/src/cpp/server/server_builder.cc b/src/cpp/server/server_builder.cc index 960ebd640db..2980b16c56c 100644 --- a/src/cpp/server/server_builder.cc +++ b/src/cpp/server/server_builder.cc @@ -162,10 +162,10 @@ std::unique_ptr ServerBuilder::BuildAndStart() { } (*plugin)->UpdateChannelArguments(&args); } - if (max_receive_message_size_ > 0) { + if (max_receive_message_size_ >= 0) { args.SetInt(GRPC_ARG_MAX_RECEIVE_MESSAGE_LENGTH, max_receive_message_size_); } - if (max_send_message_size_ > 0) { + if (max_send_message_size_ >= 0) { args.SetInt(GRPC_ARG_MAX_SEND_MESSAGE_LENGTH, max_send_message_size_); } args.SetInt(GRPC_COMPRESSION_CHANNEL_ENABLED_ALGORITHMS_BITSET, diff --git a/test/core/end2end/tests/max_message_length.c b/test/core/end2end/tests/max_message_length.c index 63bc5ebc0c3..cdca3e67487 100644 --- a/test/core/end2end/tests/max_message_length.c +++ b/test/core/end2end/tests/max_message_length.c @@ -105,7 +105,7 @@ static void test_max_message_length(grpc_end2end_test_config config, grpc_end2end_test_fixture f; grpc_arg channel_arg; grpc_channel_args channel_args; - grpc_call *c; + grpc_call *c = NULL; grpc_call *s = NULL; cq_verifier *cqv; grpc_op ops[6]; From 1621c26c1ec49207e994e4b11671fb8fb38225ba Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 15 Sep 2016 12:51:29 -0700 Subject: [PATCH 30/40] Allow resolver to pass channel args to LB policy. --- src/core/ext/client_config/client_channel.c | 2 ++ .../ext/client_config/lb_policy_factory.h | 3 +++ src/core/ext/client_config/resolver_result.c | 15 +++++++++++-- src/core/ext/client_config/resolver_result.h | 21 ++++++++++++++++--- .../ext/resolver/dns/native/dns_resolver.c | 2 +- .../ext/resolver/sockaddr/sockaddr_resolver.c | 2 +- 6 files changed, 38 insertions(+), 7 deletions(-) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index 2e6f253d386..0343a5e2544 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -179,6 +179,8 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_lb_policy_args lb_policy_args; lb_policy_args.addresses = grpc_resolver_result_get_addresses(chand->resolver_result); + lb_policy_args.additional_args = + grpc_resolver_result_get_lb_policy_args(chand->resolver_result); lb_policy_args.client_channel_factory = chand->client_channel_factory; lb_policy = grpc_lb_policy_create( exec_ctx, diff --git a/src/core/ext/client_config/lb_policy_factory.h b/src/core/ext/client_config/lb_policy_factory.h index 6919b1eb984..e2364c31a82 100644 --- a/src/core/ext/client_config/lb_policy_factory.h +++ b/src/core/ext/client_config/lb_policy_factory.h @@ -47,9 +47,12 @@ struct grpc_lb_policy_factory { const grpc_lb_policy_factory_vtable *vtable; }; +// TODO(roth, ctiller): Consider replacing this struct with +// grpc_channel_args. See comment in resolver_result.h for details. typedef struct grpc_lb_policy_args { grpc_addresses *addresses; grpc_client_channel_factory *client_channel_factory; + grpc_channel_args *additional_args; } grpc_lb_policy_args; struct grpc_lb_policy_factory_vtable { diff --git a/src/core/ext/client_config/resolver_result.c b/src/core/ext/client_config/resolver_result.c index ac263b9a468..f97dc467e1d 100644 --- a/src/core/ext/client_config/resolver_result.c +++ b/src/core/ext/client_config/resolver_result.c @@ -36,6 +36,8 @@ #include #include +#include "src/core/lib/channel/channel_args.h" + grpc_addresses* grpc_addresses_create(size_t num_addresses) { grpc_addresses* addresses = gpr_malloc(sizeof(grpc_addresses)); addresses->num_addresses = num_addresses; @@ -71,15 +73,18 @@ struct grpc_resolver_result { gpr_refcount refs; grpc_addresses* addresses; char* lb_policy_name; + grpc_channel_args* lb_policy_args; }; -grpc_resolver_result* grpc_resolver_result_create(grpc_addresses* addresses, - const char* lb_policy_name) { +grpc_resolver_result* grpc_resolver_result_create( + grpc_addresses* addresses, const char* lb_policy_name, + grpc_channel_args* lb_policy_args) { grpc_resolver_result* result = gpr_malloc(sizeof(*result)); memset(result, 0, sizeof(*result)); gpr_ref_init(&result->refs, 1); result->addresses = addresses; result->lb_policy_name = gpr_strdup(lb_policy_name); + result->lb_policy_args = lb_policy_args; return result; } @@ -92,6 +97,7 @@ void grpc_resolver_result_unref(grpc_exec_ctx* exec_ctx, if (gpr_unref(&result->refs)) { grpc_addresses_destroy(result->addresses); gpr_free(result->lb_policy_name); + grpc_channel_args_destroy(result->lb_policy_args); gpr_free(result); } } @@ -105,3 +111,8 @@ const char* grpc_resolver_result_get_lb_policy_name( grpc_resolver_result* result) { return result->lb_policy_name; } + +grpc_channel_args* grpc_resolver_result_get_lb_policy_args( + grpc_resolver_result* result) { + return result->lb_policy_args; +} diff --git a/src/core/ext/client_config/resolver_result.h b/src/core/ext/client_config/resolver_result.h index fa60c8cc6d4..821208f7099 100644 --- a/src/core/ext/client_config/resolver_result.h +++ b/src/core/ext/client_config/resolver_result.h @@ -37,6 +37,16 @@ #include "src/core/ext/client_config/lb_policy.h" #include "src/core/lib/iomgr/resolve_address.h" +// TODO(roth, ctiller): In the long term, we are considering replacing +// the resolver_result data structure with grpc_channel_args. The idea is +// that the resolver will return a set of channel args that contains the +// information that is currently in the resolver_result struct. For +// example, there will be specific args indicating the set of addresses +// and the name of the LB policy to instantiate. Note that if we did +// this, we would probably want to change the data structure of +// grpc_channel_args such to a hash table or AVL or some other data +// structure that does not require linear search to find keys. + /// Used to represent addresses returned by the resolver. typedef struct grpc_address { grpc_resolved_address address; @@ -63,9 +73,10 @@ void grpc_addresses_destroy(grpc_addresses* addresses); /// Results reported from a grpc_resolver. typedef struct grpc_resolver_result grpc_resolver_result; -/// Takes ownership of \a addresses. -grpc_resolver_result* grpc_resolver_result_create(grpc_addresses* addresses, - const char* lb_policy_name); +/// Takes ownership of \a addresses and \a lb_policy_args. +grpc_resolver_result* grpc_resolver_result_create( + grpc_addresses* addresses, const char* lb_policy_name, + grpc_channel_args* lb_policy_args); void grpc_resolver_result_ref(grpc_resolver_result* result); void grpc_resolver_result_unref(grpc_exec_ctx* exec_ctx, grpc_resolver_result* result); @@ -78,4 +89,8 @@ grpc_addresses* grpc_resolver_result_get_addresses( const char* grpc_resolver_result_get_lb_policy_name( grpc_resolver_result* result); +/// Caller does NOT take ownership of result. +grpc_channel_args* grpc_resolver_result_get_lb_policy_args( + grpc_resolver_result* result); + #endif /* GRPC_CORE_EXT_CLIENT_CONFIG_RESOLVER_RESULT_H */ diff --git a/src/core/ext/resolver/dns/native/dns_resolver.c b/src/core/ext/resolver/dns/native/dns_resolver.c index 78a996788dc..103d3effbd0 100644 --- a/src/core/ext/resolver/dns/native/dns_resolver.c +++ b/src/core/ext/resolver/dns/native/dns_resolver.c @@ -174,7 +174,7 @@ static void dns_on_resolved(grpc_exec_ctx *exec_ctx, void *arg, false /* is_balancer */); } grpc_resolved_addresses_destroy(r->addresses); - result = grpc_resolver_result_create(addresses, r->lb_policy_name); + result = grpc_resolver_result_create(addresses, r->lb_policy_name, NULL); } else { gpr_timespec now = gpr_now(GPR_CLOCK_MONOTONIC); gpr_timespec next_try = gpr_backoff_step(&r->backoff_state, now); diff --git a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c index 5c59e4397af..fb67f1a227f 100644 --- a/src/core/ext/resolver/sockaddr/sockaddr_resolver.c +++ b/src/core/ext/resolver/sockaddr/sockaddr_resolver.c @@ -121,7 +121,7 @@ static void sockaddr_maybe_finish_next_locked(grpc_exec_ctx *exec_ctx, if (r->next_completion != NULL && !r->published) { r->published = true; *r->target_result = grpc_resolver_result_create( - grpc_addresses_copy(r->addresses), r->lb_policy_name); + grpc_addresses_copy(r->addresses), r->lb_policy_name, NULL); grpc_exec_ctx_sched(exec_ctx, r->next_completion, GRPC_ERROR_NONE, NULL); r->next_completion = NULL; } From 311e7badb80cceac466b57f03094844a2c289cf4 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 16 Sep 2016 07:34:08 -0700 Subject: [PATCH 31/40] s/minor/major/ in API cleanups doc. --- doc/core/pending_api_cleanups.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/core/pending_api_cleanups.md b/doc/core/pending_api_cleanups.md index a16d3bf7781..6d30e0b0e63 100644 --- a/doc/core/pending_api_cleanups.md +++ b/doc/core/pending_api_cleanups.md @@ -1,14 +1,14 @@ There are times when we make changes that include a temporary shim for backward-compatibility (e.g., a macro or some other function to preserve -the original API) to avoid having to bump the minor version number in +the original API) to avoid having to bump the major version number in the next release. However, when we do eventually want to release a feature that does change the API in a non-backward-compatible way, we -will wind up bumping the minor version number anyway, at which point we +will wind up bumping the major version number anyway, at which point we can take the opportunity to clean up any pending backward-compatibility shims. This file lists all pending backward-compatibility changes that should -be cleaned up the next time we are going to bump the minor version +be cleaned up the next time we are going to bump the major version number: - remove `GRPC_ARG_MAX_MESSAGE_LENGTH` channel arg from From 6d5715867f737442805338a139a1fd91bd37238d Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 16 Sep 2016 10:16:49 -0700 Subject: [PATCH 32/40] clang-format --- src/core/ext/client_config/lb_policy_factory.c | 7 +++---- src/core/ext/client_config/lb_policy_factory.h | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/core/ext/client_config/lb_policy_factory.c b/src/core/ext/client_config/lb_policy_factory.c index 2737f141c4e..88e8f15d997 100644 --- a/src/core/ext/client_config/lb_policy_factory.c +++ b/src/core/ext/client_config/lb_policy_factory.c @@ -46,8 +46,8 @@ grpc_lb_addresses* grpc_lb_addresses_create(size_t num_addresses) { return addresses; } -grpc_lb_addresses *grpc_lb_addresses_copy(grpc_lb_addresses* addresses, - void *(*user_data_copy)(void *)) { +grpc_lb_addresses* grpc_lb_addresses_copy(grpc_lb_addresses* addresses, + void* (*user_data_copy)(void*)) { grpc_lb_addresses* new = grpc_lb_addresses_create(addresses->num_addresses); memcpy(new->addresses, addresses->addresses, sizeof(grpc_address) * addresses->num_addresses); @@ -55,8 +55,7 @@ grpc_lb_addresses *grpc_lb_addresses_copy(grpc_lb_addresses* addresses, new->addresses[i].balancer_name = gpr_strdup(new->addresses[i].balancer_name); if (user_data_copy != NULL) { - new->addresses[i].user_data = - user_data_copy(new->addresses[i].user_data); + new->addresses[i].user_data = user_data_copy(new->addresses[i].user_data); } } return new; diff --git a/src/core/ext/client_config/lb_policy_factory.h b/src/core/ext/client_config/lb_policy_factory.h index 70dadff0ab9..ac62dd9bc06 100644 --- a/src/core/ext/client_config/lb_policy_factory.h +++ b/src/core/ext/client_config/lb_policy_factory.h @@ -70,7 +70,7 @@ grpc_lb_addresses *grpc_lb_addresses_create(size_t num_addresses); /** Creates a copy of \a addresses. If \a user_data_copy is not NULL, * it will be invoked to copy the \a user_data field of each address. */ -grpc_lb_addresses *grpc_lb_addresses_copy(grpc_lb_addresses* addresses, +grpc_lb_addresses *grpc_lb_addresses_copy(grpc_lb_addresses *addresses, void *(*user_data_copy)(void *)); /** Sets the value of the address at index \a index of \a addresses. From fef585f1be2dcc03ee813d1083a50e4ca0ba8486 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 16 Sep 2016 10:23:28 -0700 Subject: [PATCH 33/40] Fix bugs from merge. --- src/core/ext/client_config/lb_policy_factory.c | 9 ++++++--- src/core/ext/client_config/lb_policy_factory.h | 2 +- src/core/ext/client_config/resolver_result.c | 8 ++++---- src/core/ext/client_config/resolver_result.h | 2 +- 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/core/ext/client_config/lb_policy_factory.c b/src/core/ext/client_config/lb_policy_factory.c index 88e8f15d997..24895057919 100644 --- a/src/core/ext/client_config/lb_policy_factory.c +++ b/src/core/ext/client_config/lb_policy_factory.c @@ -34,6 +34,7 @@ #include #include +#include #include "src/core/ext/client_config/lb_policy_factory.h" @@ -50,10 +51,12 @@ grpc_lb_addresses* grpc_lb_addresses_copy(grpc_lb_addresses* addresses, void* (*user_data_copy)(void*)) { grpc_lb_addresses* new = grpc_lb_addresses_create(addresses->num_addresses); memcpy(new->addresses, addresses->addresses, - sizeof(grpc_address) * addresses->num_addresses); + sizeof(grpc_lb_address) * addresses->num_addresses); for (size_t i = 0; i < addresses->num_addresses; ++i) { - new->addresses[i].balancer_name = - gpr_strdup(new->addresses[i].balancer_name); + if (new->addresses[i].balancer_name != NULL) { + new->addresses[i].balancer_name = + gpr_strdup(new->addresses[i].balancer_name); + } if (user_data_copy != NULL) { new->addresses[i].user_data = user_data_copy(new->addresses[i].user_data); } diff --git a/src/core/ext/client_config/lb_policy_factory.h b/src/core/ext/client_config/lb_policy_factory.h index ac62dd9bc06..c9499d4f877 100644 --- a/src/core/ext/client_config/lb_policy_factory.h +++ b/src/core/ext/client_config/lb_policy_factory.h @@ -36,9 +36,9 @@ #include "src/core/ext/client_config/client_channel_factory.h" #include "src/core/ext/client_config/lb_policy.h" -#include "src/core/ext/client_config/resolver_result.h" #include "src/core/lib/iomgr/exec_ctx.h" +#include "src/core/lib/iomgr/resolve_address.h" typedef struct grpc_lb_policy_factory grpc_lb_policy_factory; typedef struct grpc_lb_policy_factory_vtable grpc_lb_policy_factory_vtable; diff --git a/src/core/ext/client_config/resolver_result.c b/src/core/ext/client_config/resolver_result.c index c07bd8b4da3..59c9e7dc255 100644 --- a/src/core/ext/client_config/resolver_result.c +++ b/src/core/ext/client_config/resolver_result.c @@ -40,13 +40,13 @@ struct grpc_resolver_result { gpr_refcount refs; - grpc_addresses* addresses; + grpc_lb_addresses* addresses; char* lb_policy_name; grpc_channel_args* lb_policy_args; }; grpc_resolver_result* grpc_resolver_result_create( - grpc_addresses* addresses, const char* lb_policy_name, + grpc_lb_addresses* addresses, const char* lb_policy_name, grpc_channel_args* lb_policy_args) { grpc_resolver_result* result = gpr_malloc(sizeof(*result)); memset(result, 0, sizeof(*result)); @@ -64,14 +64,14 @@ void grpc_resolver_result_ref(grpc_resolver_result* result) { void grpc_resolver_result_unref(grpc_exec_ctx* exec_ctx, grpc_resolver_result* result) { if (gpr_unref(&result->refs)) { - grpc_addresses_destroy(result->addresses); + grpc_lb_addresses_destroy(result->addresses, NULL /* user_data_destroy */); gpr_free(result->lb_policy_name); grpc_channel_args_destroy(result->lb_policy_args); gpr_free(result); } } -grpc_addresses* grpc_resolver_result_get_addresses( +grpc_lb_addresses* grpc_resolver_result_get_addresses( grpc_resolver_result* result) { return result->addresses; } diff --git a/src/core/ext/client_config/resolver_result.h b/src/core/ext/client_config/resolver_result.h index e28a2f5bf3b..d4118b90e8a 100644 --- a/src/core/ext/client_config/resolver_result.h +++ b/src/core/ext/client_config/resolver_result.h @@ -57,7 +57,7 @@ void grpc_resolver_result_unref(grpc_exec_ctx* exec_ctx, grpc_resolver_result* result); /// Caller does NOT take ownership of result. -grpc_addresses* grpc_resolver_result_get_addresses( +grpc_lb_addresses* grpc_resolver_result_get_addresses( grpc_resolver_result* result); /// Caller does NOT take ownership of result. From 859d2a4d5351f16bc2d8f650ca9238b30e88c208 Mon Sep 17 00:00:00 2001 From: Adele Zhou Date: Fri, 16 Sep 2016 15:15:44 -0700 Subject: [PATCH 34/40] More known errors. --- tools/run_tests/run_build_statistics.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/run_tests/run_build_statistics.py b/tools/run_tests/run_build_statistics.py index 89aad2a3991..9c17cd1db91 100755 --- a/tools/run_tests/run_build_statistics.py +++ b/tools/run_tests/run_build_statistics.py @@ -63,6 +63,8 @@ _URL_BASE = 'https://grpc-testing.appspot.com/job' _KNOWN_ERRORS = [ 'Failed to build workspace Tests with scheme AllTests', 'Build timed out', + 'TIMEOUT: tools/run_tests/pre_build_node.sh', + 'TIMEOUT: tools/run_tests/pre_build_ruby.sh', 'FATAL: Unable to produce a script file', 'FAILED: build_docker_c\+\+', 'cannot find package \"cloud.google.com/go/compute/metadata\"', From 88671a8ae80c749d04ead83d282fe46e64d2e32d Mon Sep 17 00:00:00 2001 From: thinkerou Date: Sun, 18 Sep 2016 15:28:41 +0800 Subject: [PATCH 35/40] update route_guide_db path in php example --- examples/php/route_guide/run_route_guide_client.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/examples/php/route_guide/run_route_guide_client.sh b/examples/php/route_guide/run_route_guide_client.sh index 90ecc6270bb..10e6f00238a 100755 --- a/examples/php/route_guide/run_route_guide_client.sh +++ b/examples/php/route_guide/run_route_guide_client.sh @@ -30,5 +30,6 @@ set -e cd $(dirname $0) -php $extension_dir -d extension=grpc.so -d max_execution_time=300 \ - route_guide_client.php ../../node/route_guide/route_guide_db.json +php -d extension=grpc.so -d max_execution_time=300 \ + route_guide_client.php \ + ../../node/static_codegen/route_guide/route_guide_db.json From 843ec78263a2b19f935855bb58f858abd9ac0054 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 19 Sep 2016 07:19:15 -0700 Subject: [PATCH 36/40] Fix Windows printf formatting bug. --- src/core/lib/channel/message_size_filter.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/core/lib/channel/message_size_filter.c b/src/core/lib/channel/message_size_filter.c index f8bb1134ec6..6613785deaa 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -69,9 +69,9 @@ static void recv_message_ready(grpc_exec_ctx* exec_ctx, void* user_data, if (*calld->recv_message != NULL && (*calld->recv_message)->length > chand->max_recv_size) { char* message_string; - gpr_asprintf(&message_string, - "Received message larger than max (%u vs. %zu)", - (*calld->recv_message)->length, chand->max_recv_size); + gpr_asprintf( + &message_string, "Received message larger than max (%u vs. %lu)", + (*calld->recv_message)->length, (unsigned long)chand->max_recv_size); gpr_slice message = gpr_slice_from_copied_string(message_string); gpr_free(message_string); grpc_call_element_send_cancel_with_message( @@ -91,8 +91,8 @@ static void start_transport_stream_op(grpc_exec_ctx* exec_ctx, if (op->send_message != NULL && op->send_message->length > chand->max_send_size) { char* message_string; - gpr_asprintf(&message_string, "Sent message larger than max (%u vs. %zu)", - op->send_message->length, chand->max_send_size); + gpr_asprintf(&message_string, "Sent message larger than max (%u vs. %lu)", + op->send_message->length, (unsigned long)chand->max_send_size); gpr_slice message = gpr_slice_from_copied_string(message_string); gpr_free(message_string); grpc_call_element_send_cancel_with_message( From 63d8af2b0043ccbff04cf3f6bac0e58aba06f257 Mon Sep 17 00:00:00 2001 From: Ken Payson Date: Sun, 18 Sep 2016 22:49:03 -0700 Subject: [PATCH 37/40] Add parameter for server options --- src/python/grpcio/grpc/__init__.py | 12 +++-- src/python/grpcio/grpc/_channel.py | 17 ++---- src/python/grpcio/grpc/_common.py | 10 ++++ .../grpc/_cython/_cygrpc/channel.pyx.pxi | 5 +- .../grpc/_cython/_cygrpc/server.pyx.pxi | 4 +- src/python/grpcio/grpc/_server.py | 5 +- src/python/grpcio_tests/tests/tests.json | 1 + .../tests/unit/_channel_args_test.py | 53 +++++++++++++++++++ .../tests/unit/_channel_connectivity_test.py | 10 ++-- .../tests/unit/_channel_ready_future_test.py | 2 +- .../unit/_cython/_cancel_many_calls_test.py | 5 +- .../_read_some_but_not_all_responses_test.py | 5 +- .../tests/unit/_cython/cygrpc_test.py | 9 ++-- 13 files changed, 100 insertions(+), 38 deletions(-) create mode 100644 src/python/grpcio_tests/tests/unit/_channel_args_test.py diff --git a/src/python/grpcio/grpc/__init__.py b/src/python/grpcio/grpc/__init__.py index faf3ab5f0d5..b9b662c0329 100644 --- a/src/python/grpcio/grpc/__init__.py +++ b/src/python/grpcio/grpc/__init__.py @@ -1189,7 +1189,7 @@ def insecure_channel(target, options=None): A Channel to the target through which RPCs may be conducted. """ from grpc import _channel - return _channel.Channel(target, options, None) + return _channel.Channel(target, () if options is None else options, None) def secure_channel(target, credentials, options=None): @@ -1205,10 +1205,11 @@ def secure_channel(target, credentials, options=None): A Channel to the target through which RPCs may be conducted. """ from grpc import _channel - return _channel.Channel(target, options, credentials._credentials) + return _channel.Channel(target, () if options is None else options, + credentials._credentials) -def server(thread_pool, handlers=None): +def server(thread_pool, handlers=None, options=None): """Creates a Server with which RPCs can be serviced. Args: @@ -1219,12 +1220,15 @@ def server(thread_pool, handlers=None): only handlers the server will use to service RPCs; other handlers may later be added by calling add_generic_rpc_handlers any time before the returned Server is started. + options: A sequence of string-value pairs according to which to configure + the created server. Returns: A Server with which RPCs can be serviced. """ from grpc import _server - return _server.Server(thread_pool, () if handlers is None else handlers) + return _server.Server(thread_pool, () if handlers is None else handlers, + () if options is None else options) ################################### __all__ ################################# diff --git a/src/python/grpcio/grpc/_channel.py b/src/python/grpcio/grpc/_channel.py index 3117dd1cb31..53a26727abe 100644 --- a/src/python/grpcio/grpc/_channel.py +++ b/src/python/grpcio/grpc/_channel.py @@ -842,18 +842,8 @@ def _unsubscribe(state, callback): def _options(options): - if options is None: - pairs = ((cygrpc.ChannelArgKey.primary_user_agent_string, _USER_AGENT),) - else: - pairs = list(options) + [ - (cygrpc.ChannelArgKey.primary_user_agent_string, _USER_AGENT)] - encoded_pairs = [ - (_common.encode(arg_name), arg_value) if isinstance(arg_value, int) - else (_common.encode(arg_name), _common.encode(arg_value)) - for arg_name, arg_value in pairs] - return cygrpc.ChannelArgs([ - cygrpc.ChannelArg(arg_name, arg_value) - for arg_name, arg_value in encoded_pairs]) + return list(options) + [ + (cygrpc.ChannelArgKey.primary_user_agent_string, _USER_AGENT)] class Channel(grpc.Channel): @@ -867,7 +857,8 @@ class Channel(grpc.Channel): credentials: A cygrpc.ChannelCredentials or None. """ self._channel = cygrpc.Channel( - _common.encode(target), _options(options), credentials) + _common.encode(target), _common.channel_args(_options(options)), + credentials) self._call_state = _ChannelCallState(self._channel) self._connectivity_state = _ChannelConnectivityState(self._channel) diff --git a/src/python/grpcio/grpc/_common.py b/src/python/grpcio/grpc/_common.py index 4d7d5214195..cc0984c8c63 100644 --- a/src/python/grpcio/grpc/_common.py +++ b/src/python/grpcio/grpc/_common.py @@ -94,6 +94,16 @@ def decode(b): return b.decode('latin1') +def channel_args(options): + channel_args = [] + for key, value in options: + if isinstance(value, six.string_types): + channel_args.append(cygrpc.ChannelArg(encode(key), encode(value))) + else: + channel_args.append(cygrpc.ChannelArg(encode(key), value)) + return cygrpc.ChannelArgs(channel_args) + + def cygrpc_metadata(application_metadata): return _EMPTY_METADATA if application_metadata is None else cygrpc.Metadata( cygrpc.Metadatum(encode(key), encode(value)) diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/channel.pyx.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/channel.pyx.pxi index 3df937eb14f..73d1ff7b97d 100644 --- a/src/python/grpcio/grpc/_cython/_cygrpc/channel.pyx.pxi +++ b/src/python/grpcio/grpc/_cython/_cygrpc/channel.pyx.pxi @@ -32,15 +32,16 @@ cimport cpython cdef class Channel: - def __cinit__(self, bytes target, ChannelArgs arguments=None, + def __cinit__(self, bytes target, ChannelArgs arguments, ChannelCredentials channel_credentials=None): grpc_init() cdef grpc_channel_args *c_arguments = NULL cdef char *c_target = NULL self.c_channel = NULL self.references = [] - if arguments is not None: + if len(arguments) > 0: c_arguments = &arguments.c_args + self.references.append(arguments) c_target = target if channel_credentials is None: with nogil: diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/server.pyx.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/server.pyx.pxi index ca2b8311147..18db38b6861 100644 --- a/src/python/grpcio/grpc/_cython/_cygrpc/server.pyx.pxi +++ b/src/python/grpcio/grpc/_cython/_cygrpc/server.pyx.pxi @@ -34,12 +34,12 @@ import time cdef class Server: - def __cinit__(self, ChannelArgs arguments=None): + def __cinit__(self, ChannelArgs arguments): grpc_init() cdef grpc_channel_args *c_arguments = NULL self.references = [] self.registered_completion_queues = [] - if arguments is not None: + if len(arguments) > 0: c_arguments = &arguments.c_args self.references.append(arguments) with nogil: diff --git a/src/python/grpcio/grpc/_server.py b/src/python/grpcio/grpc/_server.py index 94a13bfb2fb..5f846ce7732 100644 --- a/src/python/grpcio/grpc/_server.py +++ b/src/python/grpcio/grpc/_server.py @@ -728,12 +728,11 @@ def _start(state): cleanup_server, target=_serve, args=(state,)) thread.start() - class Server(grpc.Server): - def __init__(self, thread_pool, generic_handlers): + def __init__(self, thread_pool, generic_handlers, options): completion_queue = cygrpc.CompletionQueue() - server = cygrpc.Server() + server = cygrpc.Server(_common.channel_args(options)) server.register_completion_queue(completion_queue) self._state = _ServerState( completion_queue, server, generic_handlers, thread_pool) diff --git a/src/python/grpcio_tests/tests/tests.json b/src/python/grpcio_tests/tests/tests.json index dcaef0db1fa..2071a33e13a 100644 --- a/src/python/grpcio_tests/tests/tests.json +++ b/src/python/grpcio_tests/tests/tests.json @@ -7,6 +7,7 @@ "_beta_features_test.BetaFeaturesTest", "_beta_features_test.ContextManagementAndLifecycleTest", "_cancel_many_calls_test.CancelManyCallsTest", + "_channel_args_test.ChannelArgsTest", "_channel_connectivity_test.ChannelConnectivityTest", "_channel_ready_future_test.ChannelReadyFutureTest", "_channel_test.ChannelTest", diff --git a/src/python/grpcio_tests/tests/unit/_channel_args_test.py b/src/python/grpcio_tests/tests/unit/_channel_args_test.py new file mode 100644 index 00000000000..6a636d79935 --- /dev/null +++ b/src/python/grpcio_tests/tests/unit/_channel_args_test.py @@ -0,0 +1,53 @@ +# 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. + +"""Tests of Channel Args on client/server side.""" + +import unittest + +import grpc + +TEST_CHANNEL_ARGS = ( + ('arg1', b'bytes_val'), + ('arg2', 'str_val'), + ('arg3', 1), + (b'arg4', 'str_val'), +) + + +class ChannelArgsTest(unittest.TestCase): + + def test_client(self): + grpc.insecure_channel('localhost:8080', options=TEST_CHANNEL_ARGS) + + def test_server(self): + grpc.server(None, options=TEST_CHANNEL_ARGS) + +if __name__ == '__main__': + unittest.main(verbosity=2) diff --git a/src/python/grpcio_tests/tests/unit/_channel_connectivity_test.py b/src/python/grpcio_tests/tests/unit/_channel_connectivity_test.py index 9cae96a00d8..1324ad37b6d 100644 --- a/src/python/grpcio_tests/tests/unit/_channel_connectivity_test.py +++ b/src/python/grpcio_tests/tests/unit/_channel_connectivity_test.py @@ -78,7 +78,7 @@ class ChannelConnectivityTest(unittest.TestCase): def test_lonely_channel_connectivity(self): callback = _Callback() - channel = _channel.Channel('localhost:12345', None, None) + channel = _channel.Channel('localhost:12345', (), None) channel.subscribe(callback.update, try_to_connect=False) first_connectivities = callback.block_until_connectivities_satisfy(bool) channel.subscribe(callback.update, try_to_connect=True) @@ -105,13 +105,13 @@ class ChannelConnectivityTest(unittest.TestCase): def test_immediately_connectable_channel_connectivity(self): thread_pool = _thread_pool.RecordingThreadPool(max_workers=None) - server = _server.Server(thread_pool, ()) + server = _server.Server(thread_pool, (), ()) port = server.add_insecure_port('[::]:0') server.start() first_callback = _Callback() second_callback = _Callback() - channel = _channel.Channel('localhost:{}'.format(port), None, None) + channel = _channel.Channel('localhost:{}'.format(port), (), None) channel.subscribe(first_callback.update, try_to_connect=False) first_connectivities = first_callback.block_until_connectivities_satisfy( bool) @@ -146,12 +146,12 @@ class ChannelConnectivityTest(unittest.TestCase): def test_reachable_then_unreachable_channel_connectivity(self): thread_pool = _thread_pool.RecordingThreadPool(max_workers=None) - server = _server.Server(thread_pool, ()) + server = _server.Server(thread_pool, (), ()) port = server.add_insecure_port('[::]:0') server.start() callback = _Callback() - channel = _channel.Channel('localhost:{}'.format(port), None, None) + channel = _channel.Channel('localhost:{}'.format(port), (), None) channel.subscribe(callback.update, try_to_connect=True) callback.block_until_connectivities_satisfy(_ready_in_connectivities) # Now take down the server and confirm that channel readiness is repudiated. diff --git a/src/python/grpcio_tests/tests/unit/_channel_ready_future_test.py b/src/python/grpcio_tests/tests/unit/_channel_ready_future_test.py index 24f5b45b18e..60d478bcd96 100644 --- a/src/python/grpcio_tests/tests/unit/_channel_ready_future_test.py +++ b/src/python/grpcio_tests/tests/unit/_channel_ready_future_test.py @@ -79,7 +79,7 @@ class ChannelReadyFutureTest(unittest.TestCase): def test_immediately_connectable_channel_connectivity(self): thread_pool = _thread_pool.RecordingThreadPool(max_workers=None) - server = _server.Server(thread_pool, ()) + server = _server.Server(thread_pool, (), ()) port = server.add_insecure_port('[::]:0') server.start() channel = grpc.insecure_channel('localhost:{}'.format(port)) diff --git a/src/python/grpcio_tests/tests/unit/_cython/_cancel_many_calls_test.py b/src/python/grpcio_tests/tests/unit/_cython/_cancel_many_calls_test.py index cf212c56539..20115fb22cb 100644 --- a/src/python/grpcio_tests/tests/unit/_cython/_cancel_many_calls_test.py +++ b/src/python/grpcio_tests/tests/unit/_cython/_cancel_many_calls_test.py @@ -157,11 +157,12 @@ class CancelManyCallsTest(unittest.TestCase): server_thread_pool = logging_pool.pool(test_constants.THREAD_CONCURRENCY) server_completion_queue = cygrpc.CompletionQueue() - server = cygrpc.Server() + server = cygrpc.Server(cygrpc.ChannelArgs([])) server.register_completion_queue(server_completion_queue) port = server.add_http2_port(b'[::]:0') server.start() - channel = cygrpc.Channel('localhost:{}'.format(port).encode()) + channel = cygrpc.Channel('localhost:{}'.format(port).encode(), + cygrpc.ChannelArgs([])) state = _State() diff --git a/src/python/grpcio_tests/tests/unit/_cython/_read_some_but_not_all_responses_test.py b/src/python/grpcio_tests/tests/unit/_cython/_read_some_but_not_all_responses_test.py index 152d8edde3b..2ae5285232c 100644 --- a/src/python/grpcio_tests/tests/unit/_cython/_read_some_but_not_all_responses_test.py +++ b/src/python/grpcio_tests/tests/unit/_cython/_read_some_but_not_all_responses_test.py @@ -124,11 +124,12 @@ class ReadSomeButNotAllResponsesTest(unittest.TestCase): def testReadSomeButNotAllResponses(self): server_completion_queue = cygrpc.CompletionQueue() - server = cygrpc.Server() + server = cygrpc.Server(cygrpc.ChannelArgs([])) server.register_completion_queue(server_completion_queue) port = server.add_http2_port(b'[::]:0') server.start() - channel = cygrpc.Channel('localhost:{}'.format(port).encode()) + channel = cygrpc.Channel('localhost:{}'.format(port).encode(), + cygrpc.ChannelArgs([])) server_shutdown_tag = 'server_shutdown_tag' server_driver = _ServerDriver(server_completion_queue, server_shutdown_tag) diff --git a/src/python/grpcio_tests/tests/unit/_cython/cygrpc_test.py b/src/python/grpcio_tests/tests/unit/_cython/cygrpc_test.py index 142387d810a..8dedebfabee 100644 --- a/src/python/grpcio_tests/tests/unit/_cython/cygrpc_test.py +++ b/src/python/grpcio_tests/tests/unit/_cython/cygrpc_test.py @@ -121,7 +121,7 @@ class TypeSmokeTest(unittest.TestCase): del call_credentials def testServerStartNoExplicitShutdown(self): - server = cygrpc.Server() + server = cygrpc.Server(cygrpc.ChannelArgs([])) completion_queue = cygrpc.CompletionQueue() server.register_completion_queue(completion_queue) port = server.add_http2_port(b'[::]:0') @@ -131,7 +131,7 @@ class TypeSmokeTest(unittest.TestCase): def testServerStartShutdown(self): completion_queue = cygrpc.CompletionQueue() - server = cygrpc.Server() + server = cygrpc.Server(cygrpc.ChannelArgs([])) server.add_http2_port(b'[::]:0') server.register_completion_queue(completion_queue) server.start() @@ -148,7 +148,7 @@ class ServerClientMixin(object): def setUpMixin(self, server_credentials, client_credentials, host_override): self.server_completion_queue = cygrpc.CompletionQueue() - self.server = cygrpc.Server() + self.server = cygrpc.Server(cygrpc.ChannelArgs([])) self.server.register_completion_queue(self.server_completion_queue) if server_credentials: self.port = self.server.add_http2_port(b'[::]:0', server_credentials) @@ -164,7 +164,8 @@ class ServerClientMixin(object): 'localhost:{}'.format(self.port).encode(), client_channel_arguments, client_credentials) else: - self.client_channel = cygrpc.Channel('localhost:{}'.format(self.port).encode()) + self.client_channel = cygrpc.Channel( + 'localhost:{}'.format(self.port).encode(), cygrpc.ChannelArgs([])) if host_override: self.host_argument = None # default host self.expected_host = host_override From 6053497f21de429640758cebad6014aa16177417 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 20 Sep 2016 08:37:12 -0700 Subject: [PATCH 38/40] Code review changes. --- src/core/ext/client_config/client_channel.c | 2 +- src/core/ext/client_config/client_channel.h | 2 +- src/core/ext/transport/chttp2/client/insecure/channel_create.c | 2 +- .../ext/transport/chttp2/client/secure/secure_channel_create.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index e4500cc50da..946fff9cf21 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -779,7 +779,7 @@ const grpc_channel_filter grpc_client_channel_filter = { "client-channel", }; -void grpc_client_channel_set_resolver_and_client_channel_factory( +void grpc_client_channel_finish_initialization( grpc_exec_ctx *exec_ctx, grpc_channel_stack *channel_stack, grpc_resolver *resolver, grpc_client_channel_factory *client_channel_factory) { diff --git a/src/core/ext/client_config/client_channel.h b/src/core/ext/client_config/client_channel.h index b3a9bc79951..abb5ac4d878 100644 --- a/src/core/ext/client_config/client_channel.h +++ b/src/core/ext/client_config/client_channel.h @@ -49,7 +49,7 @@ extern const grpc_channel_filter grpc_client_channel_filter; /* Post-construction initializer to give the client channel its resolver and factory. */ -void grpc_client_channel_set_resolver_and_client_channel_factory( +void grpc_client_channel_finish_initialization( grpc_exec_ctx *exec_ctx, grpc_channel_stack *channel_stack, grpc_resolver *resolver, grpc_client_channel_factory *client_channel_factory); diff --git a/src/core/ext/transport/chttp2/client/insecure/channel_create.c b/src/core/ext/transport/chttp2/client/insecure/channel_create.c index 5b9101e018b..ddc00bd79f8 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.c @@ -212,7 +212,7 @@ static grpc_channel *client_channel_factory_create_channel( return NULL; } - grpc_client_channel_set_resolver_and_client_channel_factory( + grpc_client_channel_finish_initialization( exec_ctx, grpc_channel_get_channel_stack(channel), resolver, &f->base); GRPC_RESOLVER_UNREF(exec_ctx, resolver, "create_channel"); diff --git a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c index da738138b51..f36fbbfc579 100644 --- a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c +++ b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c @@ -273,7 +273,7 @@ static grpc_channel *client_channel_factory_create_channel( grpc_resolver *resolver = grpc_resolver_create(target); if (resolver != NULL) { - grpc_client_channel_set_resolver_and_client_channel_factory( + grpc_client_channel_finish_initialization( exec_ctx, grpc_channel_get_channel_stack(channel), resolver, &f->base); GRPC_RESOLVER_UNREF(exec_ctx, resolver, "create"); } else { From 7f7d165fa8123ef0346f3d3314c21ee27519a188 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 20 Sep 2016 10:46:15 -0700 Subject: [PATCH 39/40] Code review changes. --- src/core/ext/client_config/client_channel.c | 1 + src/core/ext/client_config/lb_policy_factory.c | 16 +++++++++------- src/core/ext/client_config/lb_policy_factory.h | 1 + 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index 946fff9cf21..63797bfa1e3 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -784,6 +784,7 @@ void grpc_client_channel_finish_initialization( grpc_resolver *resolver, grpc_client_channel_factory *client_channel_factory) { /* post construction initialization: set the transport setup pointer */ + GPR_ASSERT(client_channel_factory != NULL); grpc_channel_element *elem = grpc_channel_stack_last_element(channel_stack); channel_data *chand = elem->channel_data; gpr_mu_lock(&chand->mu); diff --git a/src/core/ext/client_config/lb_policy_factory.c b/src/core/ext/client_config/lb_policy_factory.c index 8ce87455046..c17af91a09e 100644 --- a/src/core/ext/client_config/lb_policy_factory.c +++ b/src/core/ext/client_config/lb_policy_factory.c @@ -49,19 +49,21 @@ grpc_lb_addresses* grpc_lb_addresses_create(size_t num_addresses) { grpc_lb_addresses* grpc_lb_addresses_copy(grpc_lb_addresses* addresses, void* (*user_data_copy)(void*)) { - grpc_lb_addresses* new = grpc_lb_addresses_create(addresses->num_addresses); - memcpy(new->addresses, addresses->addresses, + grpc_lb_addresses* new_addresses = + grpc_lb_addresses_create(addresses->num_addresses); + memcpy(new_addresses->addresses, addresses->addresses, sizeof(grpc_lb_address) * addresses->num_addresses); for (size_t i = 0; i < addresses->num_addresses; ++i) { - if (new->addresses[i].balancer_name != NULL) { - new->addresses[i].balancer_name = - gpr_strdup(new->addresses[i].balancer_name); + if (new_addresses->addresses[i].balancer_name != NULL) { + new_addresses->addresses[i].balancer_name = + gpr_strdup(new_addresses->addresses[i].balancer_name); } if (user_data_copy != NULL) { - new->addresses[i].user_data = user_data_copy(new->addresses[i].user_data); + new_addresses->addresses[i].user_data = + user_data_copy(new_addresses->addresses[i].user_data); } } - return new; + return new_addresses; } void grpc_lb_addresses_set_address(grpc_lb_addresses* addresses, size_t index, diff --git a/src/core/ext/client_config/lb_policy_factory.h b/src/core/ext/client_config/lb_policy_factory.h index c9499d4f877..54408c63080 100644 --- a/src/core/ext/client_config/lb_policy_factory.h +++ b/src/core/ext/client_config/lb_policy_factory.h @@ -92,6 +92,7 @@ void grpc_lb_addresses_destroy(grpc_lb_addresses *addresses, typedef struct grpc_lb_policy_args { grpc_lb_addresses *addresses; grpc_client_channel_factory *client_channel_factory; + /* Can be used to pass implementation-specific parameters to the LB policy. */ grpc_channel_args *additional_args; } grpc_lb_policy_args; From d8a76d19683a458b807e1a07b587dcdefdf53f0b Mon Sep 17 00:00:00 2001 From: "Nicolas \"Pixel\" Noble" Date: Tue, 20 Sep 2016 22:17:28 +0200 Subject: [PATCH 40/40] Sanitize master. --- src/core/ext/census/tracing.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/core/ext/census/tracing.c b/src/core/ext/census/tracing.c index 18f8daa1763..8f0e12296dd 100644 --- a/src/core/ext/census/tracing.c +++ b/src/core/ext/census/tracing.c @@ -35,8 +35,6 @@ #include - - /* TODO(aveitch): These are all placeholder implementations. */ // int census_trace_mask(const census_context *context) {