From 14c072ccc0ee33c26c55999344a501d2f1f2d93d Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 26 Aug 2016 08:31:34 -0700 Subject: [PATCH 01/26] Initial code to move deadline timer into a filter. Timer cancellation not working right -- will fix in subsequent commit. --- BUILD | 12 + CMakeLists.txt | 5 + Makefile | 6 + binding.gyp | 1 + build.yaml | 2 + config.m4 | 1 + gRPC-Core.podspec | 3 + grpc.gemspec | 2 + package.xml | 2 + src/core/lib/channel/deadline_filter.c | 209 ++++++++++++++++++ src/core/lib/channel/deadline_filter.h | 40 ++++ src/core/lib/surface/call.c | 54 +---- src/core/lib/surface/init.c | 10 + src/python/grpcio/grpc_core_dependencies.py | 1 + tools/doxygen/Doxyfile.c++.internal | 2 + tools/doxygen/Doxyfile.core.internal | 2 + tools/run_tests/sources_and_headers.json | 3 + vsprojects/vcxproj/grpc++/grpc++.vcxproj | 3 + .../vcxproj/grpc++/grpc++.vcxproj.filters | 6 + .../grpc++_unsecure/grpc++_unsecure.vcxproj | 3 + .../grpc++_unsecure.vcxproj.filters | 6 + 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 + 27 files changed, 348 insertions(+), 52 deletions(-) create mode 100644 src/core/lib/channel/deadline_filter.c create mode 100644 src/core/lib/channel/deadline_filter.h diff --git a/BUILD b/BUILD index b36ce254e9e..2b8c1180d2a 100644 --- a/BUILD +++ b/BUILD @@ -165,6 +165,7 @@ cc_library( "src/core/lib/channel/compress_filter.h", "src/core/lib/channel/connected_channel.h", "src/core/lib/channel/context.h", + "src/core/lib/channel/deadline_filter.h", "src/core/lib/channel/handshaker.h", "src/core/lib/channel/http_client_filter.h", "src/core/lib/channel/http_server_filter.h", @@ -320,6 +321,7 @@ cc_library( "src/core/lib/channel/channel_stack_builder.c", "src/core/lib/channel/compress_filter.c", "src/core/lib/channel/connected_channel.c", + "src/core/lib/channel/deadline_filter.c", "src/core/lib/channel/handshaker.c", "src/core/lib/channel/http_client_filter.c", "src/core/lib/channel/http_server_filter.c", @@ -560,6 +562,7 @@ cc_library( "src/core/lib/channel/compress_filter.h", "src/core/lib/channel/connected_channel.h", "src/core/lib/channel/context.h", + "src/core/lib/channel/deadline_filter.h", "src/core/lib/channel/handshaker.h", "src/core/lib/channel/http_client_filter.h", "src/core/lib/channel/http_server_filter.h", @@ -702,6 +705,7 @@ cc_library( "src/core/lib/channel/channel_stack_builder.c", "src/core/lib/channel/compress_filter.c", "src/core/lib/channel/connected_channel.c", + "src/core/lib/channel/deadline_filter.c", "src/core/lib/channel/handshaker.c", "src/core/lib/channel/http_client_filter.c", "src/core/lib/channel/http_server_filter.c", @@ -914,6 +918,7 @@ cc_library( "src/core/lib/channel/compress_filter.h", "src/core/lib/channel/connected_channel.h", "src/core/lib/channel/context.h", + "src/core/lib/channel/deadline_filter.h", "src/core/lib/channel/handshaker.h", "src/core/lib/channel/http_client_filter.h", "src/core/lib/channel/http_server_filter.h", @@ -1046,6 +1051,7 @@ cc_library( "src/core/lib/channel/channel_stack_builder.c", "src/core/lib/channel/compress_filter.c", "src/core/lib/channel/connected_channel.c", + "src/core/lib/channel/deadline_filter.c", "src/core/lib/channel/handshaker.c", "src/core/lib/channel/http_client_filter.c", "src/core/lib/channel/http_server_filter.c", @@ -1261,6 +1267,7 @@ cc_library( "src/core/lib/channel/compress_filter.h", "src/core/lib/channel/connected_channel.h", "src/core/lib/channel/context.h", + "src/core/lib/channel/deadline_filter.h", "src/core/lib/channel/handshaker.h", "src/core/lib/channel/http_client_filter.h", "src/core/lib/channel/http_server_filter.h", @@ -1373,6 +1380,7 @@ cc_library( "src/core/lib/channel/channel_stack_builder.c", "src/core/lib/channel/compress_filter.c", "src/core/lib/channel/connected_channel.c", + "src/core/lib/channel/deadline_filter.c", "src/core/lib/channel/handshaker.c", "src/core/lib/channel/http_client_filter.c", "src/core/lib/channel/http_server_filter.c", @@ -1671,6 +1679,7 @@ cc_library( "src/core/lib/channel/compress_filter.h", "src/core/lib/channel/connected_channel.h", "src/core/lib/channel/context.h", + "src/core/lib/channel/deadline_filter.h", "src/core/lib/channel/handshaker.h", "src/core/lib/channel/http_client_filter.h", "src/core/lib/channel/http_server_filter.h", @@ -1778,6 +1787,7 @@ cc_library( "src/core/lib/channel/channel_stack_builder.c", "src/core/lib/channel/compress_filter.c", "src/core/lib/channel/connected_channel.c", + "src/core/lib/channel/deadline_filter.c", "src/core/lib/channel/handshaker.c", "src/core/lib/channel/http_client_filter.c", "src/core/lib/channel/http_server_filter.c", @@ -2166,6 +2176,7 @@ objc_library( "src/core/lib/channel/channel_stack_builder.c", "src/core/lib/channel/compress_filter.c", "src/core/lib/channel/connected_channel.c", + "src/core/lib/channel/deadline_filter.c", "src/core/lib/channel/handshaker.c", "src/core/lib/channel/http_client_filter.c", "src/core/lib/channel/http_server_filter.c", @@ -2385,6 +2396,7 @@ objc_library( "src/core/lib/channel/compress_filter.h", "src/core/lib/channel/connected_channel.h", "src/core/lib/channel/context.h", + "src/core/lib/channel/deadline_filter.h", "src/core/lib/channel/handshaker.h", "src/core/lib/channel/http_client_filter.h", "src/core/lib/channel/http_server_filter.h", diff --git a/CMakeLists.txt b/CMakeLists.txt index fbcc6bbeaec..6eda5fe4f71 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -296,6 +296,7 @@ add_library(grpc src/core/lib/channel/channel_stack_builder.c src/core/lib/channel/compress_filter.c src/core/lib/channel/connected_channel.c + src/core/lib/channel/deadline_filter.c src/core/lib/channel/handshaker.c src/core/lib/channel/http_client_filter.c src/core/lib/channel/http_server_filter.c @@ -554,6 +555,7 @@ add_library(grpc_cronet src/core/lib/channel/channel_stack_builder.c src/core/lib/channel/compress_filter.c src/core/lib/channel/connected_channel.c + src/core/lib/channel/deadline_filter.c src/core/lib/channel/handshaker.c src/core/lib/channel/http_client_filter.c src/core/lib/channel/http_server_filter.c @@ -786,6 +788,7 @@ add_library(grpc_unsecure src/core/lib/channel/channel_stack_builder.c src/core/lib/channel/compress_filter.c src/core/lib/channel/connected_channel.c + src/core/lib/channel/deadline_filter.c src/core/lib/channel/handshaker.c src/core/lib/channel/http_client_filter.c src/core/lib/channel/http_server_filter.c @@ -1043,6 +1046,7 @@ add_library(grpc++ src/core/lib/channel/channel_stack_builder.c src/core/lib/channel/compress_filter.c src/core/lib/channel/connected_channel.c + src/core/lib/channel/deadline_filter.c src/core/lib/channel/handshaker.c src/core/lib/channel/http_client_filter.c src/core/lib/channel/http_server_filter.c @@ -1400,6 +1404,7 @@ add_library(grpc++_unsecure src/core/lib/channel/channel_stack_builder.c src/core/lib/channel/compress_filter.c src/core/lib/channel/connected_channel.c + src/core/lib/channel/deadline_filter.c src/core/lib/channel/handshaker.c src/core/lib/channel/http_client_filter.c src/core/lib/channel/http_server_filter.c diff --git a/Makefile b/Makefile index c7622c0b9a3..2a25ccb47f2 100644 --- a/Makefile +++ b/Makefile @@ -2512,6 +2512,7 @@ LIBGRPC_SRC = \ src/core/lib/channel/channel_stack_builder.c \ src/core/lib/channel/compress_filter.c \ src/core/lib/channel/connected_channel.c \ + src/core/lib/channel/deadline_filter.c \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ @@ -2788,6 +2789,7 @@ LIBGRPC_CRONET_SRC = \ src/core/lib/channel/channel_stack_builder.c \ src/core/lib/channel/compress_filter.c \ src/core/lib/channel/connected_channel.c \ + src/core/lib/channel/deadline_filter.c \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ @@ -3054,6 +3056,7 @@ LIBGRPC_TEST_UTIL_SRC = \ src/core/lib/channel/channel_stack_builder.c \ src/core/lib/channel/compress_filter.c \ src/core/lib/channel/connected_channel.c \ + src/core/lib/channel/deadline_filter.c \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ @@ -3247,6 +3250,7 @@ LIBGRPC_UNSECURE_SRC = \ src/core/lib/channel/channel_stack_builder.c \ src/core/lib/channel/compress_filter.c \ src/core/lib/channel/connected_channel.c \ + src/core/lib/channel/deadline_filter.c \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ @@ -3587,6 +3591,7 @@ LIBGRPC++_SRC = \ src/core/lib/channel/channel_stack_builder.c \ src/core/lib/channel/compress_filter.c \ src/core/lib/channel/connected_channel.c \ + src/core/lib/channel/deadline_filter.c \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ @@ -4222,6 +4227,7 @@ LIBGRPC++_UNSECURE_SRC = \ src/core/lib/channel/channel_stack_builder.c \ src/core/lib/channel/compress_filter.c \ src/core/lib/channel/connected_channel.c \ + src/core/lib/channel/deadline_filter.c \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ diff --git a/binding.gyp b/binding.gyp index 17dbfc0d381..1c29798bd52 100644 --- a/binding.gyp +++ b/binding.gyp @@ -568,6 +568,7 @@ 'src/core/lib/channel/channel_stack_builder.c', 'src/core/lib/channel/compress_filter.c', 'src/core/lib/channel/connected_channel.c', + 'src/core/lib/channel/deadline_filter.c', 'src/core/lib/channel/handshaker.c', 'src/core/lib/channel/http_client_filter.c', 'src/core/lib/channel/http_server_filter.c', diff --git a/build.yaml b/build.yaml index 506a02920c1..7f8a7d032b6 100644 --- a/build.yaml +++ b/build.yaml @@ -161,6 +161,7 @@ filegroups: - src/core/lib/channel/compress_filter.h - src/core/lib/channel/connected_channel.h - src/core/lib/channel/context.h + - src/core/lib/channel/deadline_filter.h - src/core/lib/channel/handshaker.h - src/core/lib/channel/http_client_filter.h - src/core/lib/channel/http_server_filter.h @@ -241,6 +242,7 @@ filegroups: - src/core/lib/channel/channel_stack_builder.c - src/core/lib/channel/compress_filter.c - src/core/lib/channel/connected_channel.c + - src/core/lib/channel/deadline_filter.c - src/core/lib/channel/handshaker.c - src/core/lib/channel/http_client_filter.c - src/core/lib/channel/http_server_filter.c diff --git a/config.m4 b/config.m4 index b37658dc617..2fff96f14a5 100644 --- a/config.m4 +++ b/config.m4 @@ -87,6 +87,7 @@ if test "$PHP_GRPC" != "no"; then src/core/lib/channel/channel_stack_builder.c \ src/core/lib/channel/compress_filter.c \ src/core/lib/channel/connected_channel.c \ + src/core/lib/channel/deadline_filter.c \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index 86b811a3ec6..5eee9c486ea 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -258,6 +258,7 @@ Pod::Spec.new do |s| 'src/core/lib/channel/compress_filter.h', 'src/core/lib/channel/connected_channel.h', 'src/core/lib/channel/context.h', + 'src/core/lib/channel/deadline_filter.h', 'src/core/lib/channel/handshaker.h', 'src/core/lib/channel/http_client_filter.h', 'src/core/lib/channel/http_server_filter.h', @@ -417,6 +418,7 @@ Pod::Spec.new do |s| 'src/core/lib/channel/channel_stack_builder.c', 'src/core/lib/channel/compress_filter.c', 'src/core/lib/channel/connected_channel.c', + 'src/core/lib/channel/deadline_filter.c', 'src/core/lib/channel/handshaker.c', 'src/core/lib/channel/http_client_filter.c', 'src/core/lib/channel/http_server_filter.c', @@ -619,6 +621,7 @@ Pod::Spec.new do |s| 'src/core/lib/channel/compress_filter.h', 'src/core/lib/channel/connected_channel.h', 'src/core/lib/channel/context.h', + 'src/core/lib/channel/deadline_filter.h', 'src/core/lib/channel/handshaker.h', 'src/core/lib/channel/http_client_filter.h', 'src/core/lib/channel/http_server_filter.h', diff --git a/grpc.gemspec b/grpc.gemspec index 8d74e36e7b6..4b79e6cda09 100755 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -177,6 +177,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/channel/compress_filter.h ) s.files += %w( src/core/lib/channel/connected_channel.h ) s.files += %w( src/core/lib/channel/context.h ) + s.files += %w( src/core/lib/channel/deadline_filter.h ) 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 ) @@ -336,6 +337,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/channel/channel_stack_builder.c ) s.files += %w( src/core/lib/channel/compress_filter.c ) s.files += %w( src/core/lib/channel/connected_channel.c ) + s.files += %w( src/core/lib/channel/deadline_filter.c ) 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 ) diff --git a/package.xml b/package.xml index 387afc34982..5f774ef5c5b 100644 --- a/package.xml +++ b/package.xml @@ -185,6 +185,7 @@ + @@ -344,6 +345,7 @@ + diff --git a/src/core/lib/channel/deadline_filter.c b/src/core/lib/channel/deadline_filter.c new file mode 100644 index 00000000000..032dea02213 --- /dev/null +++ b/src/core/lib/channel/deadline_filter.c @@ -0,0 +1,209 @@ +// +// 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/deadline_filter.h" + +#include +#include + +#include +#include + +#include "src/core/lib/iomgr/timer.h" + +// Used for both client and server filters. +typedef struct channel_data { +} channel_data; + +// Call data used for both client and server filter. +typedef struct base_call_data { + grpc_call_stack* call_stack; + bool timer_pending; + grpc_timer timer; +} base_call_data; + +// Additional call data used only for the server filter. +typedef struct server_call_data { + base_call_data base; // Must be first. + // The closure for receiving initial metadata. + grpc_closure recv_initial_metadata_ready; + // Received initial metadata batch. + grpc_metadata_batch* recv_initial_metadata; + // The original recv_initial_metadata_ready closure, which we chain to + // after our own closure is invoked. + grpc_closure* next_recv_initial_metadata_ready; +} server_call_data; + +// Constructor for channel_data. Used for both client and server filters. +static void init_channel_elem(grpc_exec_ctx* exec_ctx, + grpc_channel_element* elem, + grpc_channel_element_args* args) { + GPR_ASSERT(!args->is_last); +} + +// Destructor for channel_data. Used for both client and server filters. +static void destroy_channel_elem(grpc_exec_ctx* exec_ctx, + grpc_channel_element* elem) { +} + +// Constructor for call_data. Used for both client and server filters. +static grpc_error *init_call_elem(grpc_exec_ctx* exec_ctx, + grpc_call_element* elem, + grpc_call_element_args* args) { + base_call_data* calld = elem->call_data; + // Note: size of call data is different between client and server. + memset(calld, 0, elem->filter->sizeof_call_data); + calld->call_stack = args->call_stack; + return GRPC_ERROR_NONE; +} + +// Destructor for call_data. Used for both client and server filters. +static void destroy_call_elem(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, + const grpc_call_final_info* final_info, + void* and_free_memory) { + base_call_data* calld = elem->call_data; +gpr_log(GPR_INFO, "==> destroy_call_elem()"); +// FIXME: this is not working -- timer holds a ref, so we won't get +// called until after timer pops + if (calld->timer_pending) +{ +gpr_log(GPR_INFO, "CANCELLING TIMER"); + grpc_timer_cancel(exec_ctx, &calld->timer); +} + +} + +// Timer callback. +static void timer_callback(grpc_exec_ctx *exec_ctx, void *arg, + grpc_error *error) { + grpc_call_element* elem = arg; + base_call_data* calld = elem->call_data; + calld->timer_pending = false; + if (error != GRPC_ERROR_CANCELLED) { +gpr_log(GPR_INFO, "DEADLINE EXCEEDED"); + gpr_slice message = gpr_slice_from_static_string("Deadline Exceeded"); + grpc_call_element_send_cancel_with_message( + exec_ctx, elem, GRPC_STATUS_DEADLINE_EXCEEDED, &message); + } +else gpr_log(GPR_INFO, "TIMER CANCELLED"); +gpr_log(GPR_INFO, "UNREF"); + GRPC_CALL_STACK_UNREF(exec_ctx, calld->call_stack, "deadline"); +} + +// Starts the deadline timer. +static void start_timer_if_needed(grpc_exec_ctx *exec_ctx, + grpc_call_element* elem, + gpr_timespec deadline) { + base_call_data* calld = elem->call_data; + deadline = gpr_convert_clock_type(deadline, GPR_CLOCK_MONOTONIC); + if (gpr_time_cmp(deadline, gpr_inf_future(GPR_CLOCK_MONOTONIC)) != 0) { + // Take a reference to the call stack, to be owned by the timer. +gpr_log(GPR_INFO, "REF"); + GRPC_CALL_STACK_REF(calld->call_stack, "deadline"); + grpc_timer_init(exec_ctx, &calld->timer, deadline, timer_callback, elem, + gpr_now(GPR_CLOCK_MONOTONIC)); + calld->timer_pending = true; + } +} + +// Method for starting a call op for client filter. +static void client_start_transport_stream_op(grpc_exec_ctx* exec_ctx, + grpc_call_element* elem, + grpc_transport_stream_op* op) { + // If we're sending initial metadata, get the deadline from the metadata + // and start the timer if needed. + if (op->send_initial_metadata != NULL) { + start_timer_if_needed(exec_ctx, elem, + op->send_initial_metadata->deadline); + } + // Chain to next filter. + grpc_call_next_op(exec_ctx, elem, op); +} + +// Callback for receiving initial metadata on the server. +static void recv_initial_metadata_ready(grpc_exec_ctx *exec_ctx, void *arg, + grpc_error *error) { + grpc_call_element* elem = arg; + server_call_data* calld = elem->call_data; + // Get deadline from metadata and start the timer if needed. + start_timer_if_needed(exec_ctx, elem, + calld->recv_initial_metadata->deadline); + // Invoke the next callback. + calld->next_recv_initial_metadata_ready->cb( + exec_ctx, calld->next_recv_initial_metadata_ready->cb_arg, error); +} + +// Method for starting a call op for server filter. +static void server_start_transport_stream_op(grpc_exec_ctx* exec_ctx, + grpc_call_element* elem, + grpc_transport_stream_op* op) { + server_call_data* calld = elem->call_data; + // If we're receiving initial metadata, we need to get the deadline + // from the recv_initial_metadata_ready callback. So we inject our + // own callback into that hook. + if (op->recv_initial_metadata_ready != NULL) { + calld->next_recv_initial_metadata_ready = op->recv_initial_metadata_ready; + calld->recv_initial_metadata = op->recv_initial_metadata; + grpc_closure_init(&calld->recv_initial_metadata_ready, + recv_initial_metadata_ready, elem); + op->recv_initial_metadata_ready = &calld->recv_initial_metadata_ready; + } + // Chain to next filter. + grpc_call_next_op(exec_ctx, elem, op); +} + +const grpc_channel_filter grpc_client_deadline_filter = { + client_start_transport_stream_op, + grpc_channel_next_op, + sizeof(base_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, + "deadline", +}; + +const grpc_channel_filter grpc_server_deadline_filter = { + server_start_transport_stream_op, + grpc_channel_next_op, + sizeof(server_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, + "deadline", +}; diff --git a/src/core/lib/channel/deadline_filter.h b/src/core/lib/channel/deadline_filter.h new file mode 100644 index 00000000000..323cb4e10cf --- /dev/null +++ b/src/core/lib/channel/deadline_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_DEADLINE_FILTER_H +#define GRPC_CORE_LIB_CHANNEL_DEADLINE_FILTER_H + +#include "src/core/lib/channel/channel_stack.h" + +extern const grpc_channel_filter grpc_client_deadline_filter; +extern const grpc_channel_filter grpc_server_deadline_filter; + +#endif // GRPC_CORE_LIB_CHANNEL_DEADLINE_FILTER_H diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c index 772681109a0..c05ed67c432 100644 --- a/src/core/lib/surface/call.c +++ b/src/core/lib/surface/call.c @@ -122,8 +122,6 @@ struct grpc_call { /* client or server call */ bool is_client; - /* is the alarm set */ - bool have_alarm; /** has grpc_call_destroy been called */ bool destroy_called; /** flag indicating that cancellation is inherited */ @@ -166,9 +164,6 @@ struct grpc_call { /* Contexts for various subsystems (security, tracing, ...). */ grpc_call_context_element context[GRPC_CONTEXT_COUNT]; - /* Deadline alarm - if have_alarm is non-zero */ - grpc_timer alarm; - /* for the client, extra metadata is initial metadata; for the server, it's trailing metadata */ grpc_linked_mdelem send_extra_metadata[MAX_SEND_EXTRA_METADATA_COUNT]; @@ -211,8 +206,6 @@ struct grpc_call { #define CALL_FROM_TOP_ELEM(top_elem) \ CALL_FROM_CALL_STACK(grpc_call_stack_from_top_element(top_elem)) -static void set_deadline_alarm(grpc_exec_ctx *exec_ctx, grpc_call *call, - gpr_timespec deadline); static void execute_op(grpc_exec_ctx *exec_ctx, grpc_call *call, grpc_transport_stream_op *op); static grpc_call_error cancel_with_status(grpc_exec_ctx *exec_ctx, grpc_call *c, @@ -260,7 +253,7 @@ grpc_call *grpc_call_create( call->metadata_batch[i][j].deadline = gpr_inf_future(GPR_CLOCK_MONOTONIC); } } - call->send_deadline = + send_deadline = gpr_convert_clock_type(send_deadline, GPR_CLOCK_MONOTONIC); GRPC_CHANNEL_INTERNAL_REF(channel, "call"); /* initial refcount dropped by grpc_call_destroy */ @@ -334,10 +327,7 @@ grpc_call *grpc_call_create( gpr_mu_unlock(&parent_call->mu); } - if (gpr_time_cmp(send_deadline, gpr_inf_future(send_deadline.clock_type)) != - 0) { - set_deadline_alarm(&exec_ctx, call, send_deadline); - } + call->send_deadline = send_deadline; grpc_exec_ctx_finish(&exec_ctx); GPR_TIMER_END("grpc_call_create", 0); return call; @@ -736,9 +726,6 @@ void grpc_call_destroy(grpc_call *c) { gpr_mu_lock(&c->mu); GPR_ASSERT(!c->destroy_called); c->destroy_called = 1; - if (c->have_alarm) { - grpc_timer_cancel(&exec_ctx, &c->alarm); - } cancel = !c->received_final_op; gpr_mu_unlock(&c->mu); if (cancel) grpc_call_cancel(c, NULL); @@ -897,32 +884,6 @@ grpc_call *grpc_call_from_top_element(grpc_call_element *elem) { return CALL_FROM_TOP_ELEM(elem); } -static void call_alarm(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { - grpc_call *call = arg; - gpr_mu_lock(&call->mu); - call->have_alarm = 0; - if (error != GRPC_ERROR_CANCELLED) { - cancel_with_status(exec_ctx, call, GRPC_STATUS_DEADLINE_EXCEEDED, - "Deadline Exceeded"); - } - gpr_mu_unlock(&call->mu); - GRPC_CALL_INTERNAL_UNREF(exec_ctx, call, "alarm"); -} - -static void set_deadline_alarm(grpc_exec_ctx *exec_ctx, grpc_call *call, - gpr_timespec deadline) { - if (call->have_alarm) { - gpr_log(GPR_ERROR, "Attempt to set deadline alarm twice"); - assert(0); - return; - } - GRPC_CALL_INTERNAL_REF(call, "alarm"); - call->have_alarm = 1; - call->send_deadline = gpr_convert_clock_type(deadline, GPR_CLOCK_MONOTONIC); - grpc_timer_init(exec_ctx, &call->alarm, call->send_deadline, call_alarm, call, - gpr_now(GPR_CLOCK_MONOTONIC)); -} - /* we offset status by a small amount when storing it into transport metadata as metadata cannot store a 0 value (which is used as OK for grpc_status_codes */ @@ -1271,14 +1232,6 @@ static void receiving_initial_metadata_ready(grpc_exec_ctx *exec_ctx, GPR_TIMER_BEGIN("validate_filtered_metadata", 0); validate_filtered_metadata(exec_ctx, bctl); GPR_TIMER_END("validate_filtered_metadata", 0); - - if (gpr_time_cmp(md->deadline, gpr_inf_future(md->deadline.clock_type)) != - 0 && - !call->is_client) { - GPR_TIMER_BEGIN("set_deadline_alarm", 0); - set_deadline_alarm(exec_ctx, call, md->deadline); - GPR_TIMER_END("set_deadline_alarm", 0); - } } call->has_initial_md_been_received = true; @@ -1326,9 +1279,6 @@ static void finish_batch(grpc_exec_ctx *exec_ctx, void *bctlp, grpc_metadata_batch_filter(md, recv_trailing_filter, call); call->received_final_op = true; - if (call->have_alarm) { - grpc_timer_cancel(exec_ctx, &call->alarm); - } /* propagate cancellation to any interested children */ child_call = call->first_child; if (child_call != NULL) { diff --git a/src/core/lib/surface/init.c b/src/core/lib/surface/init.c index 5397913a21f..4a3c03b915b 100644 --- a/src/core/lib/surface/init.c +++ b/src/core/lib/surface/init.c @@ -43,6 +43,7 @@ #include "src/core/lib/channel/channel_stack.h" #include "src/core/lib/channel/compress_filter.h" #include "src/core/lib/channel/connected_channel.h" +#include "src/core/lib/channel/deadline_filter.h" #include "src/core/lib/channel/http_client_filter.h" #include "src/core/lib/channel/http_server_filter.h" #include "src/core/lib/debug/trace.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_SUBCHANNEL, GRPC_CHANNEL_INIT_BUILTIN_PRIORITY, + prepend_filter, (void *)&grpc_client_deadline_filter); + grpc_channel_init_register_stage( + GRPC_CLIENT_DIRECT_CHANNEL, GRPC_CHANNEL_INIT_BUILTIN_PRIORITY, + prepend_filter, (void *)&grpc_client_deadline_filter); + grpc_channel_init_register_stage( + GRPC_SERVER_CHANNEL, GRPC_CHANNEL_INIT_BUILTIN_PRIORITY, prepend_filter, + (void *)&grpc_server_deadline_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 7ae76f52c1f..bc8e8ea4447 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -81,6 +81,7 @@ CORE_SOURCE_FILES = [ 'src/core/lib/channel/channel_stack_builder.c', 'src/core/lib/channel/compress_filter.c', 'src/core/lib/channel/connected_channel.c', + 'src/core/lib/channel/deadline_filter.c', 'src/core/lib/channel/handshaker.c', 'src/core/lib/channel/http_client_filter.c', 'src/core/lib/channel/http_server_filter.c', diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index 12eb6513848..ebaca73ed24 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -879,6 +879,7 @@ src/core/lib/channel/channel_stack_builder.h \ src/core/lib/channel/compress_filter.h \ src/core/lib/channel/connected_channel.h \ src/core/lib/channel/context.h \ +src/core/lib/channel/deadline_filter.h \ src/core/lib/channel/handshaker.h \ src/core/lib/channel/http_client_filter.h \ src/core/lib/channel/http_server_filter.h \ @@ -991,6 +992,7 @@ src/core/lib/channel/channel_stack.c \ src/core/lib/channel/channel_stack_builder.c \ src/core/lib/channel/compress_filter.c \ src/core/lib/channel/connected_channel.c \ +src/core/lib/channel/deadline_filter.c \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 426c6d91e36..71ac3496163 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -796,6 +796,7 @@ src/core/lib/channel/channel_stack_builder.h \ src/core/lib/channel/compress_filter.h \ src/core/lib/channel/connected_channel.h \ src/core/lib/channel/context.h \ +src/core/lib/channel/deadline_filter.h \ src/core/lib/channel/handshaker.h \ src/core/lib/channel/http_client_filter.h \ src/core/lib/channel/http_server_filter.h \ @@ -955,6 +956,7 @@ src/core/lib/channel/channel_stack.c \ src/core/lib/channel/channel_stack_builder.c \ src/core/lib/channel/compress_filter.c \ src/core/lib/channel/connected_channel.c \ +src/core/lib/channel/deadline_filter.c \ src/core/lib/channel/handshaker.c \ src/core/lib/channel/http_client_filter.c \ src/core/lib/channel/http_server_filter.c \ diff --git a/tools/run_tests/sources_and_headers.json b/tools/run_tests/sources_and_headers.json index 11daf24e0a2..adc37dff83e 100644 --- a/tools/run_tests/sources_and_headers.json +++ b/tools/run_tests/sources_and_headers.json @@ -5823,6 +5823,7 @@ "src/core/lib/channel/compress_filter.h", "src/core/lib/channel/connected_channel.h", "src/core/lib/channel/context.h", + "src/core/lib/channel/deadline_filter.h", "src/core/lib/channel/handshaker.h", "src/core/lib/channel/http_client_filter.h", "src/core/lib/channel/http_server_filter.h", @@ -5919,6 +5920,8 @@ "src/core/lib/channel/connected_channel.c", "src/core/lib/channel/connected_channel.h", "src/core/lib/channel/context.h", + "src/core/lib/channel/deadline_filter.c", + "src/core/lib/channel/deadline_filter.h", "src/core/lib/channel/handshaker.c", "src/core/lib/channel/handshaker.h", "src/core/lib/channel/http_client_filter.c", diff --git a/vsprojects/vcxproj/grpc++/grpc++.vcxproj b/vsprojects/vcxproj/grpc++/grpc++.vcxproj index 321a403c497..baee3a309e2 100644 --- a/vsprojects/vcxproj/grpc++/grpc++.vcxproj +++ b/vsprojects/vcxproj/grpc++/grpc++.vcxproj @@ -379,6 +379,7 @@ + @@ -531,6 +532,8 @@ + + diff --git a/vsprojects/vcxproj/grpc++/grpc++.vcxproj.filters b/vsprojects/vcxproj/grpc++/grpc++.vcxproj.filters index b34ca03a535..fec96cda4c2 100644 --- a/vsprojects/vcxproj/grpc++/grpc++.vcxproj.filters +++ b/vsprojects/vcxproj/grpc++/grpc++.vcxproj.filters @@ -115,6 +115,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\channel @@ -731,6 +734,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\channel diff --git a/vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj b/vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj index a7bb3ef23d1..a1a42d82b9d 100644 --- a/vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj +++ b/vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj @@ -375,6 +375,7 @@ + @@ -517,6 +518,8 @@ + + diff --git a/vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj.filters b/vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj.filters index 4ad0ae31d93..78c0a924ed4 100644 --- a/vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj.filters +++ b/vsprojects/vcxproj/grpc++_unsecure/grpc++_unsecure.vcxproj.filters @@ -100,6 +100,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\channel @@ -704,6 +707,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\channel diff --git a/vsprojects/vcxproj/grpc/grpc.vcxproj b/vsprojects/vcxproj/grpc/grpc.vcxproj index 252704756d1..e892f0d57d2 100644 --- a/vsprojects/vcxproj/grpc/grpc.vcxproj +++ b/vsprojects/vcxproj/grpc/grpc.vcxproj @@ -305,6 +305,7 @@ + @@ -472,6 +473,8 @@ + + diff --git a/vsprojects/vcxproj/grpc/grpc.vcxproj.filters b/vsprojects/vcxproj/grpc/grpc.vcxproj.filters index 1fdf0f5ecad..d4a6d8834a7 100644 --- a/vsprojects/vcxproj/grpc/grpc.vcxproj.filters +++ b/vsprojects/vcxproj/grpc/grpc.vcxproj.filters @@ -19,6 +19,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\channel @@ -683,6 +686,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\channel diff --git a/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj b/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj index 80dd6b2dcb2..1a74cf4b39e 100644 --- a/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj +++ b/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj @@ -196,6 +196,7 @@ + @@ -316,6 +317,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 8dc28d1cb99..ce5a1326dc3 100644 --- a/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj.filters +++ b/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj.filters @@ -67,6 +67,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\channel @@ -464,6 +467,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\channel diff --git a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj index fffb409781d..96b6569e3e9 100644 --- a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj +++ b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj @@ -295,6 +295,7 @@ + @@ -440,6 +441,8 @@ + + diff --git a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters index 8c8afb6b861..0e2c27779b9 100644 --- a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters +++ b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters @@ -22,6 +22,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\channel @@ -593,6 +596,9 @@ src\core\lib\channel + + src\core\lib\channel + src\core\lib\channel From d2b4533df27c54fc782be88b0b0ea581066fac40 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 26 Aug 2016 11:18:00 -0700 Subject: [PATCH 02/26] Cancel deadline timer from on_complete instead of destroy_call_elem(). --- src/core/lib/channel/deadline_filter.c | 49 ++++++++++++++++++-------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/src/core/lib/channel/deadline_filter.c b/src/core/lib/channel/deadline_filter.c index 032dea02213..852fbaf0036 100644 --- a/src/core/lib/channel/deadline_filter.c +++ b/src/core/lib/channel/deadline_filter.c @@ -45,9 +45,18 @@ typedef struct channel_data { // Call data used for both client and server filter. typedef struct base_call_data { + // We take a reference to the call stack for the timer callback. grpc_call_stack* call_stack; + // True if the timer callback is currently pending. bool timer_pending; + // The deadline timer. grpc_timer timer; + // Closure to invoke when the call is complete. + // We use this to cancel the timer. + grpc_closure on_complete; + // The original on_complete closure, which we chain to after our own + // closure is invoked. + grpc_closure* next_on_complete; } base_call_data; // Additional call data used only for the server filter. @@ -89,16 +98,6 @@ static grpc_error *init_call_elem(grpc_exec_ctx* exec_ctx, static void destroy_call_elem(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, const grpc_call_final_info* final_info, void* and_free_memory) { - base_call_data* calld = elem->call_data; -gpr_log(GPR_INFO, "==> destroy_call_elem()"); -// FIXME: this is not working -- timer holds a ref, so we won't get -// called until after timer pops - if (calld->timer_pending) -{ -gpr_log(GPR_INFO, "CANCELLING TIMER"); - grpc_timer_cancel(exec_ctx, &calld->timer); -} - } // Timer callback. @@ -108,13 +107,10 @@ static void timer_callback(grpc_exec_ctx *exec_ctx, void *arg, base_call_data* calld = elem->call_data; calld->timer_pending = false; if (error != GRPC_ERROR_CANCELLED) { -gpr_log(GPR_INFO, "DEADLINE EXCEEDED"); gpr_slice message = gpr_slice_from_static_string("Deadline Exceeded"); grpc_call_element_send_cancel_with_message( exec_ctx, elem, GRPC_STATUS_DEADLINE_EXCEEDED, &message); } -else gpr_log(GPR_INFO, "TIMER CANCELLED"); -gpr_log(GPR_INFO, "UNREF"); GRPC_CALL_STACK_UNREF(exec_ctx, calld->call_stack, "deadline"); } @@ -126,7 +122,6 @@ static void start_timer_if_needed(grpc_exec_ctx *exec_ctx, deadline = gpr_convert_clock_type(deadline, GPR_CLOCK_MONOTONIC); if (gpr_time_cmp(deadline, gpr_inf_future(GPR_CLOCK_MONOTONIC)) != 0) { // Take a reference to the call stack, to be owned by the timer. -gpr_log(GPR_INFO, "REF"); GRPC_CALL_STACK_REF(calld->call_stack, "deadline"); grpc_timer_init(exec_ctx, &calld->timer, deadline, timer_callback, elem, gpr_now(GPR_CLOCK_MONOTONIC)); @@ -134,16 +129,35 @@ gpr_log(GPR_INFO, "REF"); } } +// Callback run when the call is complete. +static void on_complete(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { + base_call_data* calld = arg; + if (calld->timer_pending) { + grpc_timer_cancel(exec_ctx, &calld->timer); + calld->timer_pending = false; + } + // Invoke the next callback. + calld->next_on_complete->cb(exec_ctx, calld->next_on_complete->cb_arg, error); +} + // Method for starting a call op for client filter. static void client_start_transport_stream_op(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, grpc_transport_stream_op* op) { + base_call_data* calld = elem->call_data; // If we're sending initial metadata, get the deadline from the metadata // and start the timer if needed. if (op->send_initial_metadata != NULL) { start_timer_if_needed(exec_ctx, elem, op->send_initial_metadata->deadline); } + // Make sure we know when the call is complete, so that we can cancel + // the timer. + if (op->recv_trailing_metadata != NULL) { + calld->next_on_complete = op->on_complete; + grpc_closure_init(&calld->on_complete, on_complete, calld); + op->on_complete = &calld->on_complete; + } // Chain to next filter. grpc_call_next_op(exec_ctx, elem, op); } @@ -176,6 +190,13 @@ static void server_start_transport_stream_op(grpc_exec_ctx* exec_ctx, recv_initial_metadata_ready, elem); op->recv_initial_metadata_ready = &calld->recv_initial_metadata_ready; } + // Make sure we know when the call is complete, so that we can cancel + // the timer. + if (op->send_trailing_metadata != NULL) { + calld->base.next_on_complete = op->on_complete; + grpc_closure_init(&calld->base.on_complete, on_complete, calld); + op->on_complete = &calld->base.on_complete; + } // Chain to next filter. grpc_call_next_op(exec_ctx, elem, op); } From d59a5fc9ee39b32bcb076e80ff81d9e3170bc9cb Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 29 Aug 2016 10:55:57 -0700 Subject: [PATCH 03/26] Fix build problems when refcount debugging is enabled. --- src/core/ext/client_config/subchannel.c | 4 ++-- src/core/lib/surface/completion_queue.h | 2 ++ src/core/lib/transport/transport.c | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/core/ext/client_config/subchannel.c b/src/core/ext/client_config/subchannel.c index df35904b85d..3d4318774b1 100644 --- a/src/core/ext/client_config/subchannel.c +++ b/src/core/ext/client_config/subchannel.c @@ -219,8 +219,8 @@ static gpr_atm ref_mutate(grpc_subchannel *c, gpr_atm delta, : gpr_atm_no_barrier_fetch_add(&c->ref_pair, delta); #ifdef GRPC_STREAM_REFCOUNT_DEBUG gpr_log(file, line, GPR_LOG_SEVERITY_DEBUG, - "SUBCHANNEL: %p % 12s 0x%08x -> 0x%08x [%s]", c, purpose, old_val, - old_val + delta, reason); + "SUBCHANNEL: %p %12s 0x%08d -> 0x%08d [%s]", c, purpose, (int)old_val, + (int)(old_val + delta), reason); #endif return old_val; } diff --git a/src/core/lib/surface/completion_queue.h b/src/core/lib/surface/completion_queue.h index 3049284f686..4dbf3aae634 100644 --- a/src/core/lib/surface/completion_queue.h +++ b/src/core/lib/surface/completion_queue.h @@ -57,6 +57,8 @@ typedef struct grpc_cq_completion { uintptr_t next; } grpc_cq_completion; +//#define GRPC_CQ_REF_COUNT_DEBUG + #ifdef GRPC_CQ_REF_COUNT_DEBUG void grpc_cq_internal_ref(grpc_completion_queue *cc, const char *reason, const char *file, int line); diff --git a/src/core/lib/transport/transport.c b/src/core/lib/transport/transport.c index 857c3909d26..36672bdbc5c 100644 --- a/src/core/lib/transport/transport.c +++ b/src/core/lib/transport/transport.c @@ -43,7 +43,7 @@ void grpc_stream_ref(grpc_stream_refcount *refcount, const char *reason) { gpr_atm val = gpr_atm_no_barrier_load(&refcount->refs.count); gpr_log(GPR_DEBUG, "%s %p:%p REF %d->%d %s", refcount->object_type, - refcount, refcount->destroy.cb_arg, val, val + 1, reason); + refcount, refcount->destroy.cb_arg, (int)val, (int)val + 1, reason); #else void grpc_stream_ref(grpc_stream_refcount *refcount) { #endif @@ -55,7 +55,7 @@ void grpc_stream_unref(grpc_exec_ctx *exec_ctx, grpc_stream_refcount *refcount, const char *reason) { gpr_atm val = gpr_atm_no_barrier_load(&refcount->refs.count); gpr_log(GPR_DEBUG, "%s %p:%p UNREF %d->%d %s", refcount->object_type, - refcount, refcount->destroy.cb_arg, val, val - 1, reason); + refcount, refcount->destroy.cb_arg, (int)val, (int)val - 1, reason); #else void grpc_stream_unref(grpc_exec_ctx *exec_ctx, grpc_stream_refcount *refcount) { From 1bbe6cb143e98805a5f9dc2745a45bff85bd768c Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 31 Aug 2016 08:33:37 -0700 Subject: [PATCH 04/26] Add locking. Add cancellation check. Use grpc_call_element_send_cancel(). --- src/core/lib/channel/deadline_filter.c | 105 ++++++++++++++++--------- src/core/lib/transport/transport.c | 4 + 2 files changed, 72 insertions(+), 37 deletions(-) diff --git a/src/core/lib/channel/deadline_filter.c b/src/core/lib/channel/deadline_filter.c index 852fbaf0036..9dff1c4d636 100644 --- a/src/core/lib/channel/deadline_filter.c +++ b/src/core/lib/channel/deadline_filter.c @@ -35,6 +35,7 @@ #include #include +#include #include #include "src/core/lib/iomgr/timer.h" @@ -47,6 +48,8 @@ typedef struct channel_data { typedef struct base_call_data { // We take a reference to the call stack for the timer callback. grpc_call_stack* call_stack; + // Guards access to timer_pending and timer. + gpr_mu timer_mu; // True if the timer callback is currently pending. bool timer_pending; // The deadline timer. @@ -91,6 +94,7 @@ static grpc_error *init_call_elem(grpc_exec_ctx* exec_ctx, // Note: size of call data is different between client and server. memset(calld, 0, elem->filter->sizeof_call_data); calld->call_stack = args->call_stack; + gpr_mu_init(&calld->timer_mu); return GRPC_ERROR_NONE; } @@ -98,6 +102,8 @@ static grpc_error *init_call_elem(grpc_exec_ctx* exec_ctx, static void destroy_call_elem(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, const grpc_call_final_info* final_info, void* and_free_memory) { + base_call_data* calld = elem->call_data; + gpr_mu_destroy(&calld->timer_mu); } // Timer callback. @@ -105,13 +111,13 @@ static void timer_callback(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { grpc_call_element* elem = arg; base_call_data* calld = elem->call_data; + gpr_mu_lock(&calld->timer_mu); calld->timer_pending = false; + gpr_mu_unlock(&calld->timer_mu); if (error != GRPC_ERROR_CANCELLED) { - gpr_slice message = gpr_slice_from_static_string("Deadline Exceeded"); - grpc_call_element_send_cancel_with_message( - exec_ctx, elem, GRPC_STATUS_DEADLINE_EXCEEDED, &message); + grpc_call_element_send_cancel(exec_ctx, elem); } - GRPC_CALL_STACK_UNREF(exec_ctx, calld->call_stack, "deadline"); + GRPC_CALL_STACK_UNREF(exec_ctx, calld->call_stack, "deadline_timer"); } // Starts the deadline timer. @@ -122,20 +128,30 @@ static void start_timer_if_needed(grpc_exec_ctx *exec_ctx, deadline = gpr_convert_clock_type(deadline, GPR_CLOCK_MONOTONIC); if (gpr_time_cmp(deadline, gpr_inf_future(GPR_CLOCK_MONOTONIC)) != 0) { // Take a reference to the call stack, to be owned by the timer. - GRPC_CALL_STACK_REF(calld->call_stack, "deadline"); + GRPC_CALL_STACK_REF(calld->call_stack, "deadline_timer"); + gpr_mu_lock(&calld->timer_mu); + calld->timer_pending = true; grpc_timer_init(exec_ctx, &calld->timer, deadline, timer_callback, elem, gpr_now(GPR_CLOCK_MONOTONIC)); - calld->timer_pending = true; + gpr_mu_unlock(&calld->timer_mu); } } -// Callback run when the call is complete. -static void on_complete(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { - base_call_data* calld = arg; +// Cancels the deadline timer. +static void cancel_timer_if_needed(grpc_exec_ctx* exec_ctx, + base_call_data* calld) { + gpr_mu_lock(&calld->timer_mu); if (calld->timer_pending) { grpc_timer_cancel(exec_ctx, &calld->timer); calld->timer_pending = false; } + gpr_mu_unlock(&calld->timer_mu); +} + +// Callback run when the call is complete. +static void on_complete(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { + base_call_data* calld = arg; + cancel_timer_if_needed(exec_ctx, calld); // Invoke the next callback. calld->next_on_complete->cb(exec_ctx, calld->next_on_complete->cb_arg, error); } @@ -145,18 +161,24 @@ static void client_start_transport_stream_op(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, grpc_transport_stream_op* op) { base_call_data* calld = elem->call_data; - // If we're sending initial metadata, get the deadline from the metadata - // and start the timer if needed. - if (op->send_initial_metadata != NULL) { - start_timer_if_needed(exec_ctx, elem, - op->send_initial_metadata->deadline); - } - // Make sure we know when the call is complete, so that we can cancel - // the timer. - if (op->recv_trailing_metadata != NULL) { - calld->next_on_complete = op->on_complete; - grpc_closure_init(&calld->on_complete, on_complete, calld); - op->on_complete = &calld->on_complete; + // If the call is cancelled or closed, cancel the timer. + if (op->cancel_error != GRPC_ERROR_NONE || + op->close_error != GRPC_ERROR_NONE) { + cancel_timer_if_needed(exec_ctx, calld); + } else { + // If we're sending initial metadata, get the deadline from the metadata + // and start the timer if needed. + if (op->send_initial_metadata != NULL) { + start_timer_if_needed(exec_ctx, elem, + op->send_initial_metadata->deadline); + } + // Make sure we know when the call is complete, so that we can cancel + // the timer. + if (op->recv_trailing_metadata != NULL) { + calld->next_on_complete = op->on_complete; + grpc_closure_init(&calld->on_complete, on_complete, calld); + op->on_complete = &calld->on_complete; + } } // Chain to next filter. grpc_call_next_op(exec_ctx, elem, op); @@ -180,22 +202,31 @@ static void server_start_transport_stream_op(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, grpc_transport_stream_op* op) { server_call_data* calld = elem->call_data; - // If we're receiving initial metadata, we need to get the deadline - // from the recv_initial_metadata_ready callback. So we inject our - // own callback into that hook. - if (op->recv_initial_metadata_ready != NULL) { - calld->next_recv_initial_metadata_ready = op->recv_initial_metadata_ready; - calld->recv_initial_metadata = op->recv_initial_metadata; - grpc_closure_init(&calld->recv_initial_metadata_ready, - recv_initial_metadata_ready, elem); - op->recv_initial_metadata_ready = &calld->recv_initial_metadata_ready; - } - // Make sure we know when the call is complete, so that we can cancel - // the timer. - if (op->send_trailing_metadata != NULL) { - calld->base.next_on_complete = op->on_complete; - grpc_closure_init(&calld->base.on_complete, on_complete, calld); - op->on_complete = &calld->base.on_complete; + // If the call is cancelled or closed, cancel the timer. + if (op->cancel_error != GRPC_ERROR_NONE || + op->close_error != GRPC_ERROR_NONE) { + cancel_timer_if_needed(exec_ctx, &calld->base); + } else { + // If we're receiving initial metadata, we need to get the deadline + // from the recv_initial_metadata_ready callback. So we inject our + // own callback into that hook. + if (op->recv_initial_metadata_ready != NULL) { + calld->next_recv_initial_metadata_ready = op->recv_initial_metadata_ready; + calld->recv_initial_metadata = op->recv_initial_metadata; + grpc_closure_init(&calld->recv_initial_metadata_ready, + recv_initial_metadata_ready, elem); + op->recv_initial_metadata_ready = &calld->recv_initial_metadata_ready; + } + // Make sure we know when the call is complete, so that we can cancel + // the timer. + // Note that we trigger this on recv_trailing_metadata, even though + // the client never sends trailing metadata, because this is the + // hook that tells us when the call is complete on the server side. + if (op->recv_trailing_metadata != NULL) { + calld->base.next_on_complete = op->on_complete; + grpc_closure_init(&calld->base.on_complete, on_complete, calld); + op->on_complete = &calld->base.on_complete; + } } // Chain to next filter. grpc_call_next_op(exec_ctx, elem, op); diff --git a/src/core/lib/transport/transport.c b/src/core/lib/transport/transport.c index 36672bdbc5c..d4e197fa5cd 100644 --- a/src/core/lib/transport/transport.c +++ b/src/core/lib/transport/transport.c @@ -220,6 +220,10 @@ void grpc_transport_stream_op_add_cancellation_with_message( error = GRPC_ERROR_CREATE("Call cancelled"); } error = grpc_error_set_int(error, GRPC_ERROR_INT_GRPC_STATUS, status); + // TODO(ctiller): We are intentionally setting close_error instead of + // cancel_error here. This is an ugly hack and should be replaced + // by a more general-purpose mechanism that allows us to control + // cancel/close behavior. add_error(op, &op->close_error, error); } From 72f6da8bacd15e878071b982c5e17409004da730 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 2 Sep 2016 13:42:38 -0700 Subject: [PATCH 05/26] Move client-side deadline handling to client_channel filter. --- src/core/ext/client_config/client_channel.c | 41 +++- src/core/lib/channel/deadline_filter.c | 244 ++++++++++++-------- src/core/lib/channel/deadline_filter.h | 41 ++++ src/core/lib/surface/init.c | 3 - 4 files changed, 215 insertions(+), 114 deletions(-) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index 61e012578e0..bbae37d7af8 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -45,6 +45,7 @@ #include "src/core/ext/client_config/subchannel.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/connected_channel.h" +#include "src/core/lib/channel/deadline_filter.h" #include "src/core/lib/iomgr/iomgr.h" #include "src/core/lib/iomgr/polling_entity.h" #include "src/core/lib/profiling/timers.h" @@ -377,6 +378,15 @@ typedef enum { for initial metadata before trying to create a call object, and handling cancellation gracefully. */ typedef struct client_channel_call_data { + // State for handling deadlines. + // The code in deadline_filter.c requires this to be the first field. +// FIXME + // TODO(roth): This is slightly sub-optimal in that grpc_deadline_state + // and this struct both independently store a pointer to the call + // stack and each has its own mutex. If/when we have time, find a way + // to avoid this without breaking either abstraction. + grpc_deadline_state deadline_state; + /** either 0 for no call, 1 for cancelled, or a pointer to a grpc_subchannel_call */ gpr_atm subchannel_call; @@ -465,7 +475,7 @@ static void subchannel_ready(grpc_exec_ctx *exec_ctx, void *arg, gpr_atm_no_barrier_store(&calld->subchannel_call, 1); fail_locked(exec_ctx, calld, GRPC_ERROR_CREATE_REFERENCING( "Failed to create subchannel", &error, 1)); - } else if (1 == gpr_atm_acq_load(&calld->subchannel_call)) { + } else if (GET_CALL(calld) == CANCELLED_CALL) { /* already cancelled before subchannel became ready */ fail_locked(exec_ctx, calld, GRPC_ERROR_CREATE_REFERENCING( @@ -511,7 +521,7 @@ static bool pick_subchannel(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, grpc_metadata_batch *initial_metadata, uint32_t initial_metadata_flags, grpc_connected_subchannel **connected_subchannel, - grpc_closure *on_ready); + grpc_closure *on_ready, grpc_error *error); static void continue_picking(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { @@ -522,7 +532,8 @@ static void continue_picking(grpc_exec_ctx *exec_ctx, void *arg, grpc_exec_ctx_sched(exec_ctx, cpa->on_ready, GRPC_ERROR_REF(error), NULL); } else if (pick_subchannel(exec_ctx, cpa->elem, cpa->initial_metadata, cpa->initial_metadata_flags, - cpa->connected_subchannel, cpa->on_ready)) { + cpa->connected_subchannel, cpa->on_ready, + GRPC_ERROR_NONE)) { grpc_exec_ctx_sched(exec_ctx, cpa->on_ready, GRPC_ERROR_NONE, NULL); } gpr_free(cpa); @@ -532,7 +543,7 @@ static bool pick_subchannel(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, grpc_metadata_batch *initial_metadata, uint32_t initial_metadata_flags, grpc_connected_subchannel **connected_subchannel, - grpc_closure *on_ready) { + grpc_closure *on_ready, grpc_error *error) { GPR_TIMER_BEGIN("pick_subchannel", 0); channel_data *chand = elem->channel_data; @@ -554,7 +565,8 @@ static bool pick_subchannel(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, if (cpa->connected_subchannel == connected_subchannel) { cpa->connected_subchannel = NULL; grpc_exec_ctx_sched(exec_ctx, cpa->on_ready, - GRPC_ERROR_CREATE("Pick cancelled"), NULL); + GRPC_ERROR_CREATE_REFERENCING( + "Pick cancelled", &error, 1), NULL); } } gpr_mu_unlock(&chand->mu); @@ -609,6 +621,7 @@ static void cc_start_transport_stream_op(grpc_exec_ctx *exec_ctx, grpc_transport_stream_op *op) { call_data *calld = elem->call_data; GRPC_CALL_LOG_OP(GPR_INFO, elem, op); + grpc_deadline_state_client_start_transport_stream_op(exec_ctx, elem, op); /* try to (atomically) get the call */ grpc_subchannel_call *call = GET_CALL(calld); GPR_TIMER_BEGIN("cc_start_transport_stream_op", 0); @@ -645,20 +658,23 @@ retry: if (op->cancel_error != GRPC_ERROR_NONE) { if (!gpr_atm_rel_cas(&calld->subchannel_call, 0, (gpr_atm)(uintptr_t)CANCELLED_CALL)) { +gpr_log(GPR_INFO, "CANCELLED_CALL"); goto retry; } else { switch (calld->creation_phase) { case GRPC_SUBCHANNEL_CALL_HOLDER_NOT_CREATING: +gpr_log(GPR_INFO, "GRPC_SUBCHANNEL_CALL_HOLDER_NOT_CREATING"); fail_locked(exec_ctx, calld, GRPC_ERROR_REF(op->cancel_error)); break; case GRPC_SUBCHANNEL_CALL_HOLDER_PICKING_SUBCHANNEL: +gpr_log(GPR_INFO, "GRPC_SUBCHANNEL_CALL_HOLDER_PICKING_SUBCHANNEL"); pick_subchannel(exec_ctx, elem, NULL, 0, &calld->connected_subchannel, - NULL); + NULL, GRPC_ERROR_REF(op->cancel_error)); break; } gpr_mu_unlock(&calld->mu); - grpc_transport_stream_op_finish_with_failure(exec_ctx, op, - GRPC_ERROR_CANCELLED); + grpc_transport_stream_op_finish_with_failure( + exec_ctx, op, GRPC_ERROR_REF(op->cancel_error)); GPR_TIMER_END("cc_start_transport_stream_op", 0); return; } @@ -672,7 +688,8 @@ retry: GRPC_CALL_STACK_REF(calld->owning_call, "pick_subchannel"); if (pick_subchannel(exec_ctx, elem, op->send_initial_metadata, op->send_initial_metadata_flags, - &calld->connected_subchannel, &calld->next_step)) { + &calld->connected_subchannel, &calld->next_step, + GRPC_ERROR_NONE)) { calld->creation_phase = GRPC_SUBCHANNEL_CALL_HOLDER_NOT_CREATING; GRPC_CALL_STACK_UNREF(exec_ctx, calld->owning_call, "pick_subchannel"); } @@ -705,6 +722,11 @@ static grpc_error *cc_init_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, grpc_call_element_args *args) { call_data *calld = elem->call_data; + grpc_deadline_state_init(&calld->deadline_state, args->call_stack); + +// FIXME: remove +calld->deadline_state.is_client = true; + gpr_atm_rel_store(&calld->subchannel_call, 0); gpr_mu_init(&calld->mu); calld->connected_subchannel = NULL; @@ -723,6 +745,7 @@ static void cc_destroy_call_elem(grpc_exec_ctx *exec_ctx, const grpc_call_final_info *final_info, void *and_free_memory) { call_data *calld = elem->call_data; + grpc_deadline_state_destroy(exec_ctx, &calld->deadline_state); grpc_subchannel_call *call = GET_CALL(calld); if (call != NULL && call != CANCELLED_CALL) { GRPC_SUBCHANNEL_CALL_UNREF(exec_ctx, call, "client_channel_destroy_call"); diff --git a/src/core/lib/channel/deadline_filter.c b/src/core/lib/channel/deadline_filter.c index 9dff1c4d636..e4f132624fe 100644 --- a/src/core/lib/channel/deadline_filter.c +++ b/src/core/lib/channel/deadline_filter.c @@ -40,131 +40,107 @@ #include "src/core/lib/iomgr/timer.h" -// Used for both client and server filters. -typedef struct channel_data { -} channel_data; - -// Call data used for both client and server filter. -typedef struct base_call_data { - // We take a reference to the call stack for the timer callback. - grpc_call_stack* call_stack; - // Guards access to timer_pending and timer. - gpr_mu timer_mu; - // True if the timer callback is currently pending. - bool timer_pending; - // The deadline timer. - grpc_timer timer; - // Closure to invoke when the call is complete. - // We use this to cancel the timer. - grpc_closure on_complete; - // The original on_complete closure, which we chain to after our own - // closure is invoked. - grpc_closure* next_on_complete; -} base_call_data; - -// Additional call data used only for the server filter. -typedef struct server_call_data { - base_call_data base; // Must be first. - // The closure for receiving initial metadata. - grpc_closure recv_initial_metadata_ready; - // Received initial metadata batch. - grpc_metadata_batch* recv_initial_metadata; - // The original recv_initial_metadata_ready closure, which we chain to - // after our own closure is invoked. - grpc_closure* next_recv_initial_metadata_ready; -} server_call_data; - -// Constructor for channel_data. Used for both client and server filters. -static void init_channel_elem(grpc_exec_ctx* exec_ctx, - grpc_channel_element* elem, - grpc_channel_element_args* args) { - GPR_ASSERT(!args->is_last); -} - -// Destructor for channel_data. Used for both client and server filters. -static void destroy_channel_elem(grpc_exec_ctx* exec_ctx, - grpc_channel_element* elem) { -} - -// Constructor for call_data. Used for both client and server filters. -static grpc_error *init_call_elem(grpc_exec_ctx* exec_ctx, - grpc_call_element* elem, - grpc_call_element_args* args) { - base_call_data* calld = elem->call_data; - // Note: size of call data is different between client and server. - memset(calld, 0, elem->filter->sizeof_call_data); - calld->call_stack = args->call_stack; - gpr_mu_init(&calld->timer_mu); - return GRPC_ERROR_NONE; -} - -// Destructor for call_data. Used for both client and server filters. -static void destroy_call_elem(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, - const grpc_call_final_info* final_info, - void* and_free_memory) { - base_call_data* calld = elem->call_data; - gpr_mu_destroy(&calld->timer_mu); -} +// +// grpc_deadline_state +// // Timer callback. static void timer_callback(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { grpc_call_element* elem = arg; - base_call_data* calld = elem->call_data; - gpr_mu_lock(&calld->timer_mu); - calld->timer_pending = false; - gpr_mu_unlock(&calld->timer_mu); + grpc_deadline_state* deadline_state = elem->call_data; +gpr_log(GPR_INFO, "==> %s(), is_client=%d", __func__, deadline_state->is_client); + gpr_mu_lock(&deadline_state->timer_mu); + deadline_state->timer_pending = false; + gpr_mu_unlock(&deadline_state->timer_mu); if (error != GRPC_ERROR_CANCELLED) { - grpc_call_element_send_cancel(exec_ctx, elem); +gpr_log(GPR_INFO, "DEADLINE_EXCEEDED"); +// FIXME: change grpc_call_element_send_cancel_with_message to not call close +// grpc_call_element_send_cancel(exec_ctx, elem); + grpc_transport_stream_op op; + memset(&op, 0, sizeof(op)); + op.cancel_error = grpc_error_set_int( + GRPC_ERROR_CREATE("Deadline Exceeded"), + GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_DEADLINE_EXCEEDED); + elem->filter->start_transport_stream_op(exec_ctx, elem, &op); } - GRPC_CALL_STACK_UNREF(exec_ctx, calld->call_stack, "deadline_timer"); + GRPC_CALL_STACK_UNREF(exec_ctx, deadline_state->call_stack, "deadline_timer"); } // Starts the deadline timer. static void start_timer_if_needed(grpc_exec_ctx *exec_ctx, grpc_call_element* elem, gpr_timespec deadline) { - base_call_data* calld = elem->call_data; + grpc_deadline_state* deadline_state = elem->call_data; +gpr_log(GPR_INFO, "==> %s(), is_client=%d", __func__, deadline_state->is_client); deadline = gpr_convert_clock_type(deadline, GPR_CLOCK_MONOTONIC); if (gpr_time_cmp(deadline, gpr_inf_future(GPR_CLOCK_MONOTONIC)) != 0) { // Take a reference to the call stack, to be owned by the timer. - GRPC_CALL_STACK_REF(calld->call_stack, "deadline_timer"); - gpr_mu_lock(&calld->timer_mu); - calld->timer_pending = true; - grpc_timer_init(exec_ctx, &calld->timer, deadline, timer_callback, elem, - gpr_now(GPR_CLOCK_MONOTONIC)); - gpr_mu_unlock(&calld->timer_mu); + GRPC_CALL_STACK_REF(deadline_state->call_stack, "deadline_timer"); + gpr_mu_lock(&deadline_state->timer_mu); +gpr_log(GPR_INFO, "STARTING TIMER -- is_client=%d", deadline_state->is_client); + deadline_state->timer_pending = true; + grpc_timer_init(exec_ctx, &deadline_state->timer, deadline, timer_callback, + elem, gpr_now(GPR_CLOCK_MONOTONIC)); + gpr_mu_unlock(&deadline_state->timer_mu); } } // Cancels the deadline timer. static void cancel_timer_if_needed(grpc_exec_ctx* exec_ctx, - base_call_data* calld) { - gpr_mu_lock(&calld->timer_mu); - if (calld->timer_pending) { - grpc_timer_cancel(exec_ctx, &calld->timer); - calld->timer_pending = false; + grpc_deadline_state* deadline_state) { +gpr_log(GPR_INFO, "==> %s(), is_client=%d", __func__, deadline_state->is_client); + gpr_mu_lock(&deadline_state->timer_mu); + if (deadline_state->timer_pending) { +gpr_log(GPR_INFO, "CANCELLING TIMER -- is_client=%d", deadline_state->is_client); + grpc_timer_cancel(exec_ctx, &deadline_state->timer); + deadline_state->timer_pending = false; } - gpr_mu_unlock(&calld->timer_mu); + gpr_mu_unlock(&deadline_state->timer_mu); } // Callback run when the call is complete. static void on_complete(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { - base_call_data* calld = arg; - cancel_timer_if_needed(exec_ctx, calld); + grpc_deadline_state* deadline_state = arg; +gpr_log(GPR_INFO, "==> %s(), is_client=%d, next_on_complete->cb=%p", __func__, deadline_state->is_client, deadline_state->next_on_complete->cb); + cancel_timer_if_needed(exec_ctx, deadline_state); // Invoke the next callback. - calld->next_on_complete->cb(exec_ctx, calld->next_on_complete->cb_arg, error); + deadline_state->next_on_complete->cb( + exec_ctx, deadline_state->next_on_complete->cb_arg, error); } -// Method for starting a call op for client filter. -static void client_start_transport_stream_op(grpc_exec_ctx* exec_ctx, - grpc_call_element* elem, - grpc_transport_stream_op* op) { - base_call_data* calld = elem->call_data; - // If the call is cancelled or closed, cancel the timer. +// Inject our own on_complete callback into op. +static void inject_on_complete_cb(grpc_deadline_state* deadline_state, + grpc_transport_stream_op* op) { +gpr_log(GPR_INFO, "==> %s(), is_client=%d", __func__, deadline_state->is_client); + deadline_state->next_on_complete = op->on_complete; + grpc_closure_init(&deadline_state->on_complete, on_complete, deadline_state); + op->on_complete = &deadline_state->on_complete; +} + +void grpc_deadline_state_init(grpc_deadline_state* deadline_state, + grpc_call_stack* call_stack) { +gpr_log(GPR_INFO, "==> %s()", __func__); + memset(deadline_state, 0, sizeof(*deadline_state)); + deadline_state->call_stack = call_stack; + gpr_mu_init(&deadline_state->timer_mu); +} + +void grpc_deadline_state_destroy(grpc_exec_ctx* exec_ctx, + grpc_deadline_state* deadline_state) { +gpr_log(GPR_INFO, "==> %s(), is_client=%d", __func__, deadline_state->is_client); + cancel_timer_if_needed(exec_ctx, deadline_state); + gpr_mu_destroy(&deadline_state->timer_mu); +} + +void grpc_deadline_state_client_start_transport_stream_op( + grpc_exec_ctx* exec_ctx, grpc_call_element* elem, + grpc_transport_stream_op* op) { +gpr_log(GPR_INFO, "==> %s(): op=%p {on_complete=%p, cancel_error=%p, close_error=%p, send_initial_metadata=%p, send_trailing_metadata=%p, send_message=%p, recv_initial_metadata_ready=%p, recv_trailing_metadata=%p}", __func__, op, op->on_complete, op->cancel_error, op->close_error, op->send_initial_metadata, op->send_trailing_metadata, op->send_message, op->recv_initial_metadata_ready, op->recv_trailing_metadata); + grpc_deadline_state* deadline_state = elem->call_data; if (op->cancel_error != GRPC_ERROR_NONE || op->close_error != GRPC_ERROR_NONE) { - cancel_timer_if_needed(exec_ctx, calld); + cancel_timer_if_needed(exec_ctx, deadline_state); } else { // If we're sending initial metadata, get the deadline from the metadata // and start the timer if needed. @@ -175,11 +151,77 @@ static void client_start_transport_stream_op(grpc_exec_ctx* exec_ctx, // Make sure we know when the call is complete, so that we can cancel // the timer. if (op->recv_trailing_metadata != NULL) { - calld->next_on_complete = op->on_complete; - grpc_closure_init(&calld->on_complete, on_complete, calld); - op->on_complete = &calld->on_complete; + inject_on_complete_cb(deadline_state, op); } } +} + +// +// filter code +// + +// Used for both client and server filters. +typedef struct channel_data { +} channel_data; + +// Constructor for channel_data. Used for both client and server filters. +static void init_channel_elem(grpc_exec_ctx* exec_ctx, + grpc_channel_element* elem, + grpc_channel_element_args* args) { + GPR_ASSERT(!args->is_last); +} + +// Destructor for channel_data. Used for both client and server filters. +static void destroy_channel_elem(grpc_exec_ctx* exec_ctx, + grpc_channel_element* elem) { +} + +// Call data used for both client and server filter. +typedef struct base_call_data { + grpc_deadline_state deadline_state; +} base_call_data; + +// Additional call data used only for the server filter. +typedef struct server_call_data { + base_call_data base; // Must be first. + // The closure for receiving initial metadata. + grpc_closure recv_initial_metadata_ready; + // Received initial metadata batch. + grpc_metadata_batch* recv_initial_metadata; + // The original recv_initial_metadata_ready closure, which we chain to + // after our own closure is invoked. + grpc_closure* next_recv_initial_metadata_ready; +} server_call_data; + +// Constructor for call_data. Used for both client and server filters. +static grpc_error *init_call_elem(grpc_exec_ctx* exec_ctx, + grpc_call_element* elem, + grpc_call_element_args* args) { +gpr_log(GPR_INFO, "==> %s() -- call_data_size=%lu", __func__, elem->filter->sizeof_call_data); + base_call_data* calld = elem->call_data; + // Note: size of call data is different between client and server. + memset(calld, 0, elem->filter->sizeof_call_data); + grpc_deadline_state_init(&calld->deadline_state, args->call_stack); + +calld->deadline_state.is_client = elem->filter->sizeof_call_data == sizeof(base_call_data); + + return GRPC_ERROR_NONE; +} + +// Destructor for call_data. Used for both client and server filters. +static void destroy_call_elem(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, + const grpc_call_final_info* final_info, + void* and_free_memory) { + base_call_data* calld = elem->call_data; + grpc_deadline_state_destroy(exec_ctx, &calld->deadline_state); +} + +// Method for starting a call op for client filter. +static void client_start_transport_stream_op(grpc_exec_ctx* exec_ctx, + grpc_call_element* elem, + grpc_transport_stream_op* op) { +gpr_log(GPR_INFO, "==> %s()", __func__); + grpc_deadline_state_client_start_transport_stream_op(exec_ctx, elem, op); // Chain to next filter. grpc_call_next_op(exec_ctx, elem, op); } @@ -201,11 +243,11 @@ static void recv_initial_metadata_ready(grpc_exec_ctx *exec_ctx, void *arg, static void server_start_transport_stream_op(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, grpc_transport_stream_op* op) { +gpr_log(GPR_INFO, "==> %s(): op=%p {on_complete=%p, cancel_error=%p, close_error=%p, send_initial_metadata=%p, send_trailing_metadata=%p, send_message=%p, recv_initial_metadata_ready=%p, recv_trailing_metadata=%p}", __func__, op, op->on_complete, op->cancel_error, op->close_error, op->send_initial_metadata, op->send_trailing_metadata, op->send_message, op->recv_initial_metadata_ready, op->recv_trailing_metadata); server_call_data* calld = elem->call_data; - // If the call is cancelled or closed, cancel the timer. if (op->cancel_error != GRPC_ERROR_NONE || op->close_error != GRPC_ERROR_NONE) { - cancel_timer_if_needed(exec_ctx, &calld->base); + cancel_timer_if_needed(exec_ctx, &calld->base.deadline_state); } else { // If we're receiving initial metadata, we need to get the deadline // from the recv_initial_metadata_ready callback. So we inject our @@ -223,9 +265,7 @@ static void server_start_transport_stream_op(grpc_exec_ctx* exec_ctx, // the client never sends trailing metadata, because this is the // hook that tells us when the call is complete on the server side. if (op->recv_trailing_metadata != NULL) { - calld->base.next_on_complete = op->on_complete; - grpc_closure_init(&calld->base.on_complete, on_complete, calld); - op->on_complete = &calld->base.on_complete; + inject_on_complete_cb(&calld->base.deadline_state, op); } } // Chain to next filter. diff --git a/src/core/lib/channel/deadline_filter.h b/src/core/lib/channel/deadline_filter.h index 323cb4e10cf..793b76f9b68 100644 --- a/src/core/lib/channel/deadline_filter.h +++ b/src/core/lib/channel/deadline_filter.h @@ -33,7 +33,48 @@ #define GRPC_CORE_LIB_CHANNEL_DEADLINE_FILTER_H #include "src/core/lib/channel/channel_stack.h" +#include "src/core/lib/iomgr/timer.h" +// State used for filters that enforce call deadlines. +// Should be the first field in the filter's call_data. +typedef struct grpc_deadline_state { + // We take a reference to the call stack for the timer callback. + grpc_call_stack* call_stack; + // Guards access to timer_pending and timer. + gpr_mu timer_mu; + // True if the timer callback is currently pending. + bool timer_pending; + // The deadline timer. + grpc_timer timer; + // Closure to invoke when the call is complete. + // We use this to cancel the timer. + grpc_closure on_complete; + // The original on_complete closure, which we chain to after our own + // closure is invoked. + grpc_closure* next_on_complete; + +// FIXME: remove +bool is_client; + +} grpc_deadline_state; + +void grpc_deadline_state_init(grpc_deadline_state* call_data, + grpc_call_stack* call_stack); +void grpc_deadline_state_destroy(grpc_exec_ctx* exec_ctx, + grpc_deadline_state* call_data); + +// To be used in a filter's start_transport_stream_op() method to +// enforce call deadlines. +// It is the caller's responsibility to chain to the next filter if +// necessary after this function returns. +// REQUIRES: The first field in elem is a grpc_deadline_state struct. +void grpc_deadline_state_client_start_transport_stream_op( + grpc_exec_ctx* exec_ctx, grpc_call_element* elem, + grpc_transport_stream_op* op); + +// Deadline filters for direct client channels and server channels. +// Note: Deadlines for non-direct client channels are handled by the +// client_channel filter. extern const grpc_channel_filter grpc_client_deadline_filter; extern const grpc_channel_filter grpc_server_deadline_filter; diff --git a/src/core/lib/surface/init.c b/src/core/lib/surface/init.c index 4a3c03b915b..158dca09647 100644 --- a/src/core/lib/surface/init.c +++ b/src/core/lib/surface/init.c @@ -98,9 +98,6 @@ static bool maybe_add_http_filter(grpc_channel_stack_builder *builder, } static void register_builtin_channel_init() { - grpc_channel_init_register_stage( - GRPC_CLIENT_SUBCHANNEL, GRPC_CHANNEL_INIT_BUILTIN_PRIORITY, - prepend_filter, (void *)&grpc_client_deadline_filter); grpc_channel_init_register_stage( GRPC_CLIENT_DIRECT_CHANNEL, GRPC_CHANNEL_INIT_BUILTIN_PRIORITY, prepend_filter, (void *)&grpc_client_deadline_filter); From dabb376f50289025402044ec1736c050dc073121 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 2 Sep 2016 13:43:20 -0700 Subject: [PATCH 06/26] Start cancellations at the current filter instead of the next one. --- src/core/lib/channel/channel_stack.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/lib/channel/channel_stack.c b/src/core/lib/channel/channel_stack.c index 98f304f2da0..fb0c70986f3 100644 --- a/src/core/lib/channel/channel_stack.c +++ b/src/core/lib/channel/channel_stack.c @@ -271,20 +271,20 @@ grpc_call_stack *grpc_call_stack_from_top_element(grpc_call_element *elem) { } void grpc_call_element_send_cancel(grpc_exec_ctx *exec_ctx, - grpc_call_element *cur_elem) { + grpc_call_element *elem) { grpc_transport_stream_op op; memset(&op, 0, sizeof(op)); op.cancel_error = GRPC_ERROR_CANCELLED; - grpc_call_next_op(exec_ctx, cur_elem, &op); + elem->filter->start_transport_stream_op(exec_ctx, elem, &op); } void grpc_call_element_send_cancel_with_message(grpc_exec_ctx *exec_ctx, - grpc_call_element *cur_elem, + grpc_call_element *elem, grpc_status_code status, gpr_slice *optional_message) { grpc_transport_stream_op op; memset(&op, 0, sizeof(op)); grpc_transport_stream_op_add_cancellation_with_message(&op, status, optional_message); - grpc_call_next_op(exec_ctx, cur_elem, &op); + elem->filter->start_transport_stream_op(exec_ctx, elem, &op); } From 1e35b69788242095fe781bf730b45a0a65549ca0 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 2 Sep 2016 13:44:32 -0700 Subject: [PATCH 07/26] Set status from the grpc_error or whichever of its children has the data. --- src/core/lib/iomgr/error.c | 62 +++++++++++++++++++++++++++++++++++++ src/core/lib/iomgr/error.h | 8 +++++ src/core/lib/surface/call.c | 34 ++++++++------------ 3 files changed, 83 insertions(+), 21 deletions(-) diff --git a/src/core/lib/iomgr/error.c b/src/core/lib/iomgr/error.c index 149c55663c0..e54154421ae 100644 --- a/src/core/lib/iomgr/error.c +++ b/src/core/lib/iomgr/error.c @@ -324,6 +324,68 @@ const char *grpc_error_get_str(grpc_error *err, grpc_error_strs which) { return gpr_avl_get(err->strs, (void *)(uintptr_t)which); } +typedef struct { + grpc_error *error; + grpc_status_code code; + const char *msg; +} special_error_status_map; +static special_error_status_map error_status_map[] = { + { GRPC_ERROR_NONE, GRPC_STATUS_OK, "" }, + { GRPC_ERROR_CANCELLED, GRPC_STATUS_CANCELLED, "RPC cancelled" }, + { GRPC_ERROR_OOM, GRPC_STATUS_RESOURCE_EXHAUSTED, "Out of memory" }, +}; + +static grpc_error *recursively_find_error_with_status(grpc_error* error, + intptr_t* status) { + // If the error itself has a status code, return it. + if (grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, status)) { + return error; + } + // Otherwise, search through its children. + intptr_t key = 0; + while (true) { + grpc_error *child_error = gpr_avl_get(error->errs, (void *)key++); + if (child_error == NULL) + break; + grpc_error *result = + recursively_find_error_with_status(child_error, status); + if (result != NULL) + return result; + } + return NULL; +} + +void grpc_error_get_status(grpc_error *error, grpc_status_code *code, + const char **msg) { + // Handle special errors via the static map. + for (size_t i = 0; + i < sizeof(error_status_map) / sizeof(special_error_status_map); + ++i) { + if (error == error_status_map[i].error) { + *code = error_status_map[i].code; + *msg = error_status_map[i].msg; + return; + } + } + // Populate code. + // Start with the parent error and recurse through the tree of children + // until we find the first one that has a status code. + intptr_t status = GRPC_STATUS_UNKNOWN; // Default in case we don't find one. + grpc_error* found_error = recursively_find_error_with_status(error, &status); + *code = status; + // Now populate msg. + // If we found an error with a status code above, use that; otherwise, + // fall back to using the parent error. + if (found_error == NULL) found_error = error; + // If the error has a status message, use it. Otherwise, fall back to + // the error description. + *msg = grpc_error_get_str(found_error, GRPC_ERROR_STR_GRPC_MESSAGE); + if (*msg == NULL) { + *msg = grpc_error_get_str(found_error, GRPC_ERROR_STR_DESCRIPTION); + if (*msg == NULL) *msg = "uknown error"; // Just in case. + } +} + grpc_error *grpc_error_add_child(grpc_error *src, grpc_error *child) { GPR_TIMER_BEGIN("grpc_error_add_child", 0); grpc_error *new = copy_error_and_unref(src); diff --git a/src/core/lib/iomgr/error.h b/src/core/lib/iomgr/error.h index 6c769accdb6..7e2fd7a3bd6 100644 --- a/src/core/lib/iomgr/error.h +++ b/src/core/lib/iomgr/error.h @@ -37,6 +37,7 @@ #include #include +#include #include /// Opaque representation of an error. @@ -175,6 +176,13 @@ grpc_error *grpc_error_set_str(grpc_error *src, grpc_error_strs which, /// Returns NULL if the specified string is not set. /// Caller does NOT own return value. const char *grpc_error_get_str(grpc_error *error, grpc_error_strs which); + +/// A utility function to get the status code and message to be returned +/// to the application. If not set in the top-level message, looks +/// through child errors until it finds the first one with these attributes. +void grpc_error_get_status(grpc_error *error, grpc_status_code *code, + const char **msg); + /// Add a child error: an error that is believed to have contributed to this /// error occurring. Allows root causing high level errors from lower level /// errors that contributed to them. diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c index c05ed67c432..7ab97f3140f 100644 --- a/src/core/lib/surface/call.c +++ b/src/core/lib/surface/call.c @@ -261,13 +261,10 @@ grpc_call *grpc_call_create( &exec_ctx, channel_stack, 1, destroy_call, call, call->context, server_transport_data, CALL_STACK_FROM_CALL(call)); if (error != GRPC_ERROR_NONE) { - intptr_t status; - if (!grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, &status)) - status = GRPC_STATUS_UNKNOWN; - const char *error_str = - grpc_error_get_str(error, GRPC_ERROR_STR_DESCRIPTION); - close_with_status(&exec_ctx, call, (grpc_status_code)status, - error_str == NULL ? "unknown error" : error_str); + grpc_status_code status; + const char *error_str; + grpc_error_get_status(error, &status, &error_str); + close_with_status(&exec_ctx, call, status, error_str); GRPC_ERROR_UNREF(error); } if (cq != NULL) { @@ -440,20 +437,11 @@ static void set_status_details(grpc_call *call, status_source source, static void set_status_from_error(grpc_call *call, status_source source, grpc_error *error) { - intptr_t status; - if (grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, &status)) { - set_status_code(call, source, (uint32_t)status); - } else { - set_status_code(call, source, GRPC_STATUS_INTERNAL); - } - const char *msg = grpc_error_get_str(error, GRPC_ERROR_STR_GRPC_MESSAGE); - bool free_msg = false; - if (msg == NULL) { - free_msg = true; - msg = grpc_error_string(error); - } + grpc_status_code status; + const char* msg; + grpc_error_get_status(error, &status, &msg); + set_status_code(call, source, (uint32_t)status); set_status_details(call, source, grpc_mdstr_from_string(msg)); - if (free_msg) grpc_error_free_string(msg); } static void set_incoming_compression_algorithm( @@ -1256,12 +1244,16 @@ static void finish_batch(grpc_exec_ctx *exec_ctx, void *bctlp, grpc_call *child_call; grpc_call *next_child_call; +const char* msg = grpc_error_string(error); +gpr_log(GPR_INFO, "==> finish_batch(): is_client=%d, error=%s", call->is_client, msg); +grpc_error_free_string(msg); + GRPC_ERROR_REF(error); gpr_mu_lock(&call->mu); if (bctl->send_initial_metadata) { if (error != GRPC_ERROR_NONE) { - set_status_code(call, STATUS_FROM_CORE, GRPC_STATUS_UNAVAILABLE); + set_status_from_error(call, STATUS_FROM_CORE, error); } grpc_metadata_batch_destroy( &call->metadata_batch[0 /* is_receiving */][0 /* is_trailing */]); From b3a4f906af6823fef1c75bd1d37e5737a0ae98db Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 2 Sep 2016 13:45:20 -0700 Subject: [PATCH 08/26] Fix a couple of tests. --- test/core/end2end/invalid_call_argument_test.c | 10 ++++++++++ test/core/end2end/tests/negative_deadline.c | 16 ++++++++++------ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/test/core/end2end/invalid_call_argument_test.c b/test/core/end2end/invalid_call_argument_test.c index 2b9904a2441..dd2147e6097 100644 --- a/test/core/end2end/invalid_call_argument_test.c +++ b/test/core/end2end/invalid_call_argument_test.c @@ -304,6 +304,11 @@ static void test_receive_initial_metadata_twice_at_client() { grpc_op *op; prepare_test(1); op = g_state.ops; + op->op = GRPC_OP_SEND_INITIAL_METADATA; + op->data.send_initial_metadata.count = 0; + op->flags = 0; + op->reserved = NULL; + op++; op->op = GRPC_OP_RECV_INITIAL_METADATA; op->data.recv_initial_metadata = &g_state.initial_metadata_recv; op->flags = 0; @@ -392,6 +397,11 @@ static void test_recv_status_on_client_twice() { prepare_test(1); op = g_state.ops; + op->op = GRPC_OP_SEND_INITIAL_METADATA; + op->data.send_initial_metadata.count = 0; + op->flags = 0; + op->reserved = NULL; + op++; op->op = GRPC_OP_RECV_STATUS_ON_CLIENT; op->data.recv_status_on_client.trailing_metadata = &g_state.trailing_metadata_recv; diff --git a/test/core/end2end/tests/negative_deadline.c b/test/core/end2end/tests/negative_deadline.c index c999ac116aa..36632f6a5dd 100644 --- a/test/core/end2end/tests/negative_deadline.c +++ b/test/core/end2end/tests/negative_deadline.c @@ -122,6 +122,11 @@ static void simple_request_body(grpc_end2end_test_fixture f, size_t num_ops) { memset(ops, 0, sizeof(ops)); op = ops; + op->op = GRPC_OP_SEND_INITIAL_METADATA; + op->data.send_initial_metadata.count = 0; + op->flags = 0; + op->reserved = NULL; + op++; op->op = GRPC_OP_RECV_STATUS_ON_CLIENT; op->data.recv_status_on_client.trailing_metadata = &trailing_metadata_recv; op->data.recv_status_on_client.status = &status; @@ -135,15 +140,14 @@ static void simple_request_body(grpc_end2end_test_fixture f, size_t num_ops) { op->flags = 0; op->reserved = NULL; op++; - op->op = GRPC_OP_SEND_INITIAL_METADATA; - op->data.send_initial_metadata.count = 0; - op->flags = 0; - op->reserved = NULL; - op++; op->op = GRPC_OP_SEND_CLOSE_FROM_CLIENT; op->flags = 0; op->reserved = NULL; op++; + // Need to send at least the SEND_INITIAL_METADATA and + // RECV_STATUS_ON_CLIENT ops, since the former allows the client to set + // the deadline timer, and the latter returns status to the test. + GPR_ASSERT(num_ops >= 2); GPR_ASSERT(num_ops <= (size_t)(op - ops)); error = grpc_call_start_batch(c, ops, num_ops, tag(1), NULL); GPR_ASSERT(GRPC_CALL_OK == error); @@ -174,7 +178,7 @@ static void test_invoke_simple_request(grpc_end2end_test_config config, void negative_deadline(grpc_end2end_test_config config) { size_t i; - for (i = 1; i <= 4; i++) { + for (i = 2; i <= 4; i++) { test_invoke_simple_request(config, i); } } From e5f434913975f039b434d05852c6b7ab12774076 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 6 Sep 2016 10:49:52 -0700 Subject: [PATCH 09/26] Fix portability problems. --- src/core/lib/channel/deadline_filter.c | 2 +- src/core/lib/iomgr/error.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/lib/channel/deadline_filter.c b/src/core/lib/channel/deadline_filter.c index e4f132624fe..2ca43e105bd 100644 --- a/src/core/lib/channel/deadline_filter.c +++ b/src/core/lib/channel/deadline_filter.c @@ -197,7 +197,7 @@ typedef struct server_call_data { static grpc_error *init_call_elem(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, grpc_call_element_args* args) { -gpr_log(GPR_INFO, "==> %s() -- call_data_size=%lu", __func__, elem->filter->sizeof_call_data); +gpr_log(GPR_INFO, "==> %s() -- call_data_size=%lu", __func__, (unsigned long)elem->filter->sizeof_call_data); base_call_data* calld = elem->call_data; // Note: size of call data is different between client and server. memset(calld, 0, elem->filter->sizeof_call_data); diff --git a/src/core/lib/iomgr/error.c b/src/core/lib/iomgr/error.c index e54154421ae..17f19423a3f 100644 --- a/src/core/lib/iomgr/error.c +++ b/src/core/lib/iomgr/error.c @@ -372,7 +372,7 @@ void grpc_error_get_status(grpc_error *error, grpc_status_code *code, // until we find the first one that has a status code. intptr_t status = GRPC_STATUS_UNKNOWN; // Default in case we don't find one. grpc_error* found_error = recursively_find_error_with_status(error, &status); - *code = status; + *code = (grpc_status_code)status; // Now populate msg. // If we found an error with a status code above, use that; otherwise, // fall back to using the parent error. From 697a1f602325518cdd4e48a4ef44e49a09f0579a Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 7 Sep 2016 13:35:07 -0700 Subject: [PATCH 10/26] Fix error refcounting. --- src/core/ext/client_config/client_channel.c | 2 ++ src/core/lib/channel/deadline_filter.c | 1 + 2 files changed, 3 insertions(+) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index bbae37d7af8..f1eb0eaebc6 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -571,8 +571,10 @@ static bool pick_subchannel(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, } gpr_mu_unlock(&chand->mu); GPR_TIMER_END("pick_subchannel", 0); + GRPC_ERROR_UNREF(error); return true; } + GPR_ASSERT(error == GRPC_ERROR_NONE); if (chand->lb_policy != NULL) { grpc_lb_policy *lb_policy = chand->lb_policy; int r; diff --git a/src/core/lib/channel/deadline_filter.c b/src/core/lib/channel/deadline_filter.c index 2ca43e105bd..5e91d524af0 100644 --- a/src/core/lib/channel/deadline_filter.c +++ b/src/core/lib/channel/deadline_filter.c @@ -63,6 +63,7 @@ gpr_log(GPR_INFO, "DEADLINE_EXCEEDED"); GRPC_ERROR_CREATE("Deadline Exceeded"), GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_DEADLINE_EXCEEDED); elem->filter->start_transport_stream_op(exec_ctx, elem, &op); + GRPC_ERROR_UNREF(op.cancel_error); } GRPC_CALL_STACK_UNREF(exec_ctx, deadline_state->call_stack, "deadline_timer"); } From f7dd851c2a00d34e51d34555deac5c9bbee528c5 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 7 Sep 2016 13:35:31 -0700 Subject: [PATCH 11/26] Remove unused code for chaining on_complete closures in call.c. --- src/core/lib/surface/call.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c index 7ab97f3140f..15c002015ce 100644 --- a/src/core/lib/surface/call.c +++ b/src/core/lib/surface/call.c @@ -751,7 +751,6 @@ typedef struct termination_closure { grpc_closure closure; grpc_call *call; grpc_error *error; - grpc_closure *op_closure; enum { TC_CANCEL, TC_CLOSE } type; } termination_closure; @@ -767,13 +766,12 @@ static void done_termination(grpc_exec_ctx *exec_ctx, void *tcp, break; } GRPC_ERROR_UNREF(tc->error); - grpc_exec_ctx_sched(exec_ctx, tc->op_closure, GRPC_ERROR_NONE, NULL); gpr_free(tc); } static void send_cancel(grpc_exec_ctx *exec_ctx, void *tcp, grpc_error *error) { - grpc_transport_stream_op op; termination_closure *tc = tcp; + grpc_transport_stream_op op; memset(&op, 0, sizeof(op)); op.cancel_error = tc->error; /* reuse closure to catch completion */ @@ -783,13 +781,12 @@ static void send_cancel(grpc_exec_ctx *exec_ctx, void *tcp, grpc_error *error) { } static void send_close(grpc_exec_ctx *exec_ctx, void *tcp, grpc_error *error) { - grpc_transport_stream_op op; termination_closure *tc = tcp; + grpc_transport_stream_op op; memset(&op, 0, sizeof(op)); op.close_error = tc->error; /* reuse closure to catch completion */ grpc_closure_init(&tc->closure, done_termination, tc); - tc->op_closure = op.on_complete; op.on_complete = &tc->closure; execute_op(exec_ctx, tc->call, &op); } From 5f84400ba85e18044ea7dea302c8c23174b0b912 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 8 Sep 2016 08:20:53 -0700 Subject: [PATCH 12/26] Propagate errors through grpc_lb_policy_cancel_pick(). --- src/core/ext/client_config/client_channel.c | 11 ++++++++++- src/core/ext/client_config/lb_policy.c | 5 +++-- src/core/ext/client_config/lb_policy.h | 5 +++-- src/core/ext/lb_policy/grpclb/grpclb.c | 7 +++++-- src/core/ext/lb_policy/pick_first/pick_first.c | 7 +++++-- src/core/ext/lb_policy/round_robin/round_robin.c | 9 ++++++--- 6 files changed, 32 insertions(+), 12 deletions(-) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index f1eb0eaebc6..2708e2d4814 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -466,6 +466,9 @@ static void retry_waiting_locked(grpc_exec_ctx *exec_ctx, call_data *calld) { static void subchannel_ready(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { +const char* msg = grpc_error_string(error); +gpr_log(GPR_INFO, "==> %s(): error=%s", __func__, msg); +grpc_error_free_string(msg); call_data *calld = arg; gpr_mu_lock(&calld->mu); GPR_ASSERT(calld->creation_phase == @@ -544,6 +547,8 @@ static bool pick_subchannel(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, uint32_t initial_metadata_flags, grpc_connected_subchannel **connected_subchannel, grpc_closure *on_ready, grpc_error *error) { +gpr_log(GPR_INFO, "==> %s()", __func__); + GPR_TIMER_BEGIN("pick_subchannel", 0); channel_data *chand = elem->channel_data; @@ -556,11 +561,13 @@ static bool pick_subchannel(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, gpr_mu_lock(&chand->mu); if (initial_metadata == NULL) { if (chand->lb_policy != NULL) { +gpr_log(GPR_INFO, "asking LB policy to cancel pick"); grpc_lb_policy_cancel_pick(exec_ctx, chand->lb_policy, - connected_subchannel); + connected_subchannel, GRPC_ERROR_REF(error)); } for (closure = chand->waiting_for_config_closures.head; closure != NULL; closure = closure->next_data.next) { +gpr_log(GPR_INFO, "top of closure loop"); cpa = closure->cb_arg; if (cpa->connected_subchannel == connected_subchannel) { cpa->connected_subchannel = NULL; @@ -572,6 +579,7 @@ static bool pick_subchannel(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, gpr_mu_unlock(&chand->mu); GPR_TIMER_END("pick_subchannel", 0); GRPC_ERROR_UNREF(error); +gpr_log(GPR_INFO, "returning from pick_subchannel()"); return true; } GPR_ASSERT(error == GRPC_ERROR_NONE); @@ -621,6 +629,7 @@ static bool pick_subchannel(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, static void cc_start_transport_stream_op(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, grpc_transport_stream_op *op) { +gpr_log(GPR_INFO, "==> %s()", __func__); call_data *calld = elem->call_data; GRPC_CALL_LOG_OP(GPR_INFO, elem, op); grpc_deadline_state_client_start_transport_stream_op(exec_ctx, elem, op); diff --git a/src/core/ext/client_config/lb_policy.c b/src/core/ext/client_config/lb_policy.c index 8b980b2cca2..5e605491e75 100644 --- a/src/core/ext/client_config/lb_policy.c +++ b/src/core/ext/client_config/lb_policy.c @@ -110,8 +110,9 @@ int grpc_lb_policy_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy, } void grpc_lb_policy_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy, - grpc_connected_subchannel **target) { - policy->vtable->cancel_pick(exec_ctx, policy, target); + grpc_connected_subchannel **target, + grpc_error *error) { + policy->vtable->cancel_pick(exec_ctx, policy, target, error); } void grpc_lb_policy_cancel_picks(grpc_exec_ctx *exec_ctx, diff --git a/src/core/ext/client_config/lb_policy.h b/src/core/ext/client_config/lb_policy.h index a2f5446fc6c..9650110cce3 100644 --- a/src/core/ext/client_config/lb_policy.h +++ b/src/core/ext/client_config/lb_policy.h @@ -65,7 +65,7 @@ struct grpc_lb_policy_vtable { uint32_t initial_metadata_flags, grpc_connected_subchannel **target, grpc_closure *on_complete); void (*cancel_pick)(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy, - grpc_connected_subchannel **target); + grpc_connected_subchannel **target, grpc_error *error); void (*cancel_picks)(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy, uint32_t initial_metadata_flags_mask, uint32_t initial_metadata_flags_eq); @@ -139,7 +139,8 @@ void grpc_lb_policy_ping_one(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy, grpc_closure *closure); void grpc_lb_policy_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy, - grpc_connected_subchannel **target); + grpc_connected_subchannel **target, + grpc_error *error); /** Cancel all pending picks which have: (initial_metadata_flags & initial_metadata_flags_mask) == diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index af913d8a9df..9b7fcea1b07 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -533,7 +533,8 @@ static void glb_shutdown(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { } static void glb_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, - grpc_connected_subchannel **target) { + grpc_connected_subchannel **target, + grpc_error *error) { glb_lb_policy *glb_policy = (glb_lb_policy *)pol; gpr_mu_lock(&glb_policy->mu); pending_pick *pp = glb_policy->pending_picks; @@ -545,7 +546,8 @@ static void glb_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, exec_ctx, pp->pollent, glb_policy->base.interested_parties); *target = NULL; grpc_exec_ctx_sched(exec_ctx, &pp->wrapped_on_complete, - GRPC_ERROR_CANCELLED, NULL); + GRPC_ERROR_CREATE_REFERENCING( + "Pick Cancelled", &error, 1), NULL); gpr_free(pp); } else { pp->next = glb_policy->pending_picks; @@ -554,6 +556,7 @@ static void glb_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, pp = next; } gpr_mu_unlock(&glb_policy->mu); + GRPC_ERROR_UNREF(error); } static grpc_call *lb_client_data_get_call(struct lb_client_data *lb_client); diff --git a/src/core/ext/lb_policy/pick_first/pick_first.c b/src/core/ext/lb_policy/pick_first/pick_first.c index 9decf70692a..089d6f45fb7 100644 --- a/src/core/ext/lb_policy/pick_first/pick_first.c +++ b/src/core/ext/lb_policy/pick_first/pick_first.c @@ -128,7 +128,8 @@ static void pf_shutdown(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { } static void pf_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, - grpc_connected_subchannel **target) { + grpc_connected_subchannel **target, + grpc_error *error) { pick_first_lb_policy *p = (pick_first_lb_policy *)pol; pending_pick *pp; gpr_mu_lock(&p->mu); @@ -141,7 +142,8 @@ static void pf_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, p->base.interested_parties); *target = NULL; grpc_exec_ctx_sched(exec_ctx, pp->on_complete, - GRPC_ERROR_CREATE("Pick Cancelled"), NULL); + GRPC_ERROR_CREATE_REFERENCING( + "Pick Cancelled", &error, 1), NULL); gpr_free(pp); } else { pp->next = p->pending_picks; @@ -150,6 +152,7 @@ static void pf_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, pp = next; } gpr_mu_unlock(&p->mu); + GRPC_ERROR_UNREF(error); } static void pf_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, diff --git a/src/core/ext/lb_policy/round_robin/round_robin.c b/src/core/ext/lb_policy/round_robin/round_robin.c index 7bcf608ab9b..1134495755c 100644 --- a/src/core/ext/lb_policy/round_robin/round_robin.c +++ b/src/core/ext/lb_policy/round_robin/round_robin.c @@ -281,7 +281,8 @@ static void rr_shutdown(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { } static void rr_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, - grpc_connected_subchannel **target) { + grpc_connected_subchannel **target, + grpc_error *error) { round_robin_lb_policy *p = (round_robin_lb_policy *)pol; pending_pick *pp; gpr_mu_lock(&p->mu); @@ -293,8 +294,9 @@ static void rr_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, grpc_polling_entity_del_from_pollset_set(exec_ctx, pp->pollent, p->base.interested_parties); *target = NULL; - grpc_exec_ctx_sched(exec_ctx, pp->on_complete, GRPC_ERROR_CANCELLED, - NULL); + grpc_exec_ctx_sched(exec_ctx, pp->on_complete, + GRPC_ERROR_CREATE_REFERENCING( + "Pick cancelled", &error, 1), NULL); gpr_free(pp); } else { pp->next = p->pending_picks; @@ -303,6 +305,7 @@ static void rr_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, pp = next; } gpr_mu_unlock(&p->mu); + GRPC_ERROR_UNREF(error); } static void rr_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, From 75d7478d122de8142f9306463058e8d18dfd614a Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 9 Sep 2016 07:46:01 -0700 Subject: [PATCH 13/26] Fix close vs. cancel confusion in transport helper functions. --- src/core/lib/channel/channel_stack.c | 10 ++++++++++ src/core/lib/channel/channel_stack.h | 5 +++++ src/core/lib/channel/deadline_filter.c | 14 +++++--------- src/core/lib/channel/http_client_filter.c | 4 ++-- src/core/lib/transport/transport.c | 6 +----- 5 files changed, 23 insertions(+), 16 deletions(-) diff --git a/src/core/lib/channel/channel_stack.c b/src/core/lib/channel/channel_stack.c index fb0c70986f3..f5fa0b03907 100644 --- a/src/core/lib/channel/channel_stack.c +++ b/src/core/lib/channel/channel_stack.c @@ -288,3 +288,13 @@ void grpc_call_element_send_cancel_with_message(grpc_exec_ctx *exec_ctx, optional_message); elem->filter->start_transport_stream_op(exec_ctx, elem, &op); } + +void grpc_call_element_send_close_with_message(grpc_exec_ctx *exec_ctx, + grpc_call_element *elem, + grpc_status_code status, + gpr_slice *optional_message) { + grpc_transport_stream_op op; + memset(&op, 0, sizeof(op)); + grpc_transport_stream_op_add_close(&op, status, optional_message); + elem->filter->start_transport_stream_op(exec_ctx, elem, &op); +} diff --git a/src/core/lib/channel/channel_stack.h b/src/core/lib/channel/channel_stack.h index 6b73cce380f..eeaab17d39a 100644 --- a/src/core/lib/channel/channel_stack.h +++ b/src/core/lib/channel/channel_stack.h @@ -290,6 +290,11 @@ void grpc_call_element_send_cancel_with_message(grpc_exec_ctx *exec_ctx, grpc_status_code status, gpr_slice *optional_message); +void grpc_call_element_send_close_with_message(grpc_exec_ctx *exec_ctx, + grpc_call_element *cur_elem, + grpc_status_code status, + gpr_slice *optional_message); + extern int grpc_trace_channel; #define GRPC_CALL_LOG_OP(sev, elem, op) \ diff --git a/src/core/lib/channel/deadline_filter.c b/src/core/lib/channel/deadline_filter.c index 5e91d524af0..9597783b63d 100644 --- a/src/core/lib/channel/deadline_filter.c +++ b/src/core/lib/channel/deadline_filter.c @@ -55,15 +55,11 @@ gpr_log(GPR_INFO, "==> %s(), is_client=%d", __func__, deadline_state->is_client) gpr_mu_unlock(&deadline_state->timer_mu); if (error != GRPC_ERROR_CANCELLED) { gpr_log(GPR_INFO, "DEADLINE_EXCEEDED"); -// FIXME: change grpc_call_element_send_cancel_with_message to not call close -// grpc_call_element_send_cancel(exec_ctx, elem); - grpc_transport_stream_op op; - memset(&op, 0, sizeof(op)); - op.cancel_error = grpc_error_set_int( - GRPC_ERROR_CREATE("Deadline Exceeded"), - GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_DEADLINE_EXCEEDED); - elem->filter->start_transport_stream_op(exec_ctx, elem, &op); - GRPC_ERROR_UNREF(op.cancel_error); + gpr_slice msg = gpr_slice_from_static_string("Deadline Exceeded"); + grpc_call_element_send_cancel_with_message(exec_ctx, elem, + GRPC_STATUS_DEADLINE_EXCEEDED, + &msg); + gpr_slice_unref(msg); } GRPC_CALL_STACK_UNREF(exec_ctx, deadline_state->call_stack, "deadline_timer"); } diff --git a/src/core/lib/channel/http_client_filter.c b/src/core/lib/channel/http_client_filter.c index edcc741ff6f..1dc05fb20d1 100644 --- a/src/core/lib/channel/http_client_filter.c +++ b/src/core/lib/channel/http_client_filter.c @@ -103,8 +103,8 @@ static grpc_mdelem *client_recv_filter(void *user_data, grpc_mdelem *md) { grpc_mdstr_as_c_string(md->value)); gpr_slice message = gpr_slice_from_copied_string(message_string); gpr_free(message_string); - grpc_call_element_send_cancel_with_message(a->exec_ctx, a->elem, - GRPC_STATUS_CANCELLED, &message); + grpc_call_element_send_close_with_message(a->exec_ctx, a->elem, + GRPC_STATUS_CANCELLED, &message); return NULL; } else if (md == GRPC_MDELEM_CONTENT_TYPE_APPLICATION_SLASH_GRPC) { return NULL; diff --git a/src/core/lib/transport/transport.c b/src/core/lib/transport/transport.c index d4e197fa5cd..205a1367422 100644 --- a/src/core/lib/transport/transport.c +++ b/src/core/lib/transport/transport.c @@ -220,11 +220,7 @@ void grpc_transport_stream_op_add_cancellation_with_message( error = GRPC_ERROR_CREATE("Call cancelled"); } error = grpc_error_set_int(error, GRPC_ERROR_INT_GRPC_STATUS, status); - // TODO(ctiller): We are intentionally setting close_error instead of - // cancel_error here. This is an ugly hack and should be replaced - // by a more general-purpose mechanism that allows us to control - // cancel/close behavior. - add_error(op, &op->close_error, error); + add_error(op, &op->cancel_error, error); } void grpc_transport_stream_op_add_close(grpc_transport_stream_op *op, From 6ad991783f35bf24b1f31a6aa302774206666d78 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 9 Sep 2016 07:52:48 -0700 Subject: [PATCH 14/26] Remove debugging code. --- src/core/ext/client_config/client_channel.c | 19 +------------------ src/core/lib/channel/deadline_filter.c | 17 ----------------- src/core/lib/channel/deadline_filter.h | 6 +----- src/core/lib/surface/call.c | 4 ---- 4 files changed, 2 insertions(+), 44 deletions(-) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index 2708e2d4814..569ca38b878 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -380,11 +380,10 @@ typedef enum { typedef struct client_channel_call_data { // State for handling deadlines. // The code in deadline_filter.c requires this to be the first field. -// FIXME // TODO(roth): This is slightly sub-optimal in that grpc_deadline_state // and this struct both independently store a pointer to the call // stack and each has its own mutex. If/when we have time, find a way - // to avoid this without breaking either abstraction. + // to avoid this without breaking the grpc_deadline_state abstraction. grpc_deadline_state deadline_state; /** either 0 for no call, 1 for cancelled, or a pointer to a @@ -466,9 +465,6 @@ static void retry_waiting_locked(grpc_exec_ctx *exec_ctx, call_data *calld) { static void subchannel_ready(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { -const char* msg = grpc_error_string(error); -gpr_log(GPR_INFO, "==> %s(): error=%s", __func__, msg); -grpc_error_free_string(msg); call_data *calld = arg; gpr_mu_lock(&calld->mu); GPR_ASSERT(calld->creation_phase == @@ -547,8 +543,6 @@ static bool pick_subchannel(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, uint32_t initial_metadata_flags, grpc_connected_subchannel **connected_subchannel, grpc_closure *on_ready, grpc_error *error) { -gpr_log(GPR_INFO, "==> %s()", __func__); - GPR_TIMER_BEGIN("pick_subchannel", 0); channel_data *chand = elem->channel_data; @@ -561,13 +555,11 @@ gpr_log(GPR_INFO, "==> %s()", __func__); gpr_mu_lock(&chand->mu); if (initial_metadata == NULL) { if (chand->lb_policy != NULL) { -gpr_log(GPR_INFO, "asking LB policy to cancel pick"); grpc_lb_policy_cancel_pick(exec_ctx, chand->lb_policy, connected_subchannel, GRPC_ERROR_REF(error)); } for (closure = chand->waiting_for_config_closures.head; closure != NULL; closure = closure->next_data.next) { -gpr_log(GPR_INFO, "top of closure loop"); cpa = closure->cb_arg; if (cpa->connected_subchannel == connected_subchannel) { cpa->connected_subchannel = NULL; @@ -579,7 +571,6 @@ gpr_log(GPR_INFO, "top of closure loop"); gpr_mu_unlock(&chand->mu); GPR_TIMER_END("pick_subchannel", 0); GRPC_ERROR_UNREF(error); -gpr_log(GPR_INFO, "returning from pick_subchannel()"); return true; } GPR_ASSERT(error == GRPC_ERROR_NONE); @@ -629,7 +620,6 @@ gpr_log(GPR_INFO, "returning from pick_subchannel()"); static void cc_start_transport_stream_op(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, grpc_transport_stream_op *op) { -gpr_log(GPR_INFO, "==> %s()", __func__); call_data *calld = elem->call_data; GRPC_CALL_LOG_OP(GPR_INFO, elem, op); grpc_deadline_state_client_start_transport_stream_op(exec_ctx, elem, op); @@ -669,16 +659,13 @@ retry: if (op->cancel_error != GRPC_ERROR_NONE) { if (!gpr_atm_rel_cas(&calld->subchannel_call, 0, (gpr_atm)(uintptr_t)CANCELLED_CALL)) { -gpr_log(GPR_INFO, "CANCELLED_CALL"); goto retry; } else { switch (calld->creation_phase) { case GRPC_SUBCHANNEL_CALL_HOLDER_NOT_CREATING: -gpr_log(GPR_INFO, "GRPC_SUBCHANNEL_CALL_HOLDER_NOT_CREATING"); fail_locked(exec_ctx, calld, GRPC_ERROR_REF(op->cancel_error)); break; case GRPC_SUBCHANNEL_CALL_HOLDER_PICKING_SUBCHANNEL: -gpr_log(GPR_INFO, "GRPC_SUBCHANNEL_CALL_HOLDER_PICKING_SUBCHANNEL"); pick_subchannel(exec_ctx, elem, NULL, 0, &calld->connected_subchannel, NULL, GRPC_ERROR_REF(op->cancel_error)); break; @@ -734,10 +721,6 @@ static grpc_error *cc_init_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element_args *args) { call_data *calld = elem->call_data; grpc_deadline_state_init(&calld->deadline_state, args->call_stack); - -// FIXME: remove -calld->deadline_state.is_client = true; - gpr_atm_rel_store(&calld->subchannel_call, 0); gpr_mu_init(&calld->mu); calld->connected_subchannel = NULL; diff --git a/src/core/lib/channel/deadline_filter.c b/src/core/lib/channel/deadline_filter.c index 9597783b63d..424d09d3d8d 100644 --- a/src/core/lib/channel/deadline_filter.c +++ b/src/core/lib/channel/deadline_filter.c @@ -49,12 +49,10 @@ static void timer_callback(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { grpc_call_element* elem = arg; grpc_deadline_state* deadline_state = elem->call_data; -gpr_log(GPR_INFO, "==> %s(), is_client=%d", __func__, deadline_state->is_client); gpr_mu_lock(&deadline_state->timer_mu); deadline_state->timer_pending = false; gpr_mu_unlock(&deadline_state->timer_mu); if (error != GRPC_ERROR_CANCELLED) { -gpr_log(GPR_INFO, "DEADLINE_EXCEEDED"); gpr_slice msg = gpr_slice_from_static_string("Deadline Exceeded"); grpc_call_element_send_cancel_with_message(exec_ctx, elem, GRPC_STATUS_DEADLINE_EXCEEDED, @@ -69,13 +67,11 @@ static void start_timer_if_needed(grpc_exec_ctx *exec_ctx, grpc_call_element* elem, gpr_timespec deadline) { grpc_deadline_state* deadline_state = elem->call_data; -gpr_log(GPR_INFO, "==> %s(), is_client=%d", __func__, deadline_state->is_client); deadline = gpr_convert_clock_type(deadline, GPR_CLOCK_MONOTONIC); if (gpr_time_cmp(deadline, gpr_inf_future(GPR_CLOCK_MONOTONIC)) != 0) { // Take a reference to the call stack, to be owned by the timer. GRPC_CALL_STACK_REF(deadline_state->call_stack, "deadline_timer"); gpr_mu_lock(&deadline_state->timer_mu); -gpr_log(GPR_INFO, "STARTING TIMER -- is_client=%d", deadline_state->is_client); deadline_state->timer_pending = true; grpc_timer_init(exec_ctx, &deadline_state->timer, deadline, timer_callback, elem, gpr_now(GPR_CLOCK_MONOTONIC)); @@ -86,10 +82,8 @@ gpr_log(GPR_INFO, "STARTING TIMER -- is_client=%d", deadline_state->is_client); // Cancels the deadline timer. static void cancel_timer_if_needed(grpc_exec_ctx* exec_ctx, grpc_deadline_state* deadline_state) { -gpr_log(GPR_INFO, "==> %s(), is_client=%d", __func__, deadline_state->is_client); gpr_mu_lock(&deadline_state->timer_mu); if (deadline_state->timer_pending) { -gpr_log(GPR_INFO, "CANCELLING TIMER -- is_client=%d", deadline_state->is_client); grpc_timer_cancel(exec_ctx, &deadline_state->timer); deadline_state->timer_pending = false; } @@ -99,7 +93,6 @@ gpr_log(GPR_INFO, "CANCELLING TIMER -- is_client=%d", deadline_state->is_client) // Callback run when the call is complete. static void on_complete(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { grpc_deadline_state* deadline_state = arg; -gpr_log(GPR_INFO, "==> %s(), is_client=%d, next_on_complete->cb=%p", __func__, deadline_state->is_client, deadline_state->next_on_complete->cb); cancel_timer_if_needed(exec_ctx, deadline_state); // Invoke the next callback. deadline_state->next_on_complete->cb( @@ -109,7 +102,6 @@ gpr_log(GPR_INFO, "==> %s(), is_client=%d, next_on_complete->cb=%p", __func__, d // Inject our own on_complete callback into op. static void inject_on_complete_cb(grpc_deadline_state* deadline_state, grpc_transport_stream_op* op) { -gpr_log(GPR_INFO, "==> %s(), is_client=%d", __func__, deadline_state->is_client); deadline_state->next_on_complete = op->on_complete; grpc_closure_init(&deadline_state->on_complete, on_complete, deadline_state); op->on_complete = &deadline_state->on_complete; @@ -117,7 +109,6 @@ gpr_log(GPR_INFO, "==> %s(), is_client=%d", __func__, deadline_state->is_client) void grpc_deadline_state_init(grpc_deadline_state* deadline_state, grpc_call_stack* call_stack) { -gpr_log(GPR_INFO, "==> %s()", __func__); memset(deadline_state, 0, sizeof(*deadline_state)); deadline_state->call_stack = call_stack; gpr_mu_init(&deadline_state->timer_mu); @@ -125,7 +116,6 @@ gpr_log(GPR_INFO, "==> %s()", __func__); void grpc_deadline_state_destroy(grpc_exec_ctx* exec_ctx, grpc_deadline_state* deadline_state) { -gpr_log(GPR_INFO, "==> %s(), is_client=%d", __func__, deadline_state->is_client); cancel_timer_if_needed(exec_ctx, deadline_state); gpr_mu_destroy(&deadline_state->timer_mu); } @@ -133,7 +123,6 @@ gpr_log(GPR_INFO, "==> %s(), is_client=%d", __func__, deadline_state->is_client) void grpc_deadline_state_client_start_transport_stream_op( grpc_exec_ctx* exec_ctx, grpc_call_element* elem, grpc_transport_stream_op* op) { -gpr_log(GPR_INFO, "==> %s(): op=%p {on_complete=%p, cancel_error=%p, close_error=%p, send_initial_metadata=%p, send_trailing_metadata=%p, send_message=%p, recv_initial_metadata_ready=%p, recv_trailing_metadata=%p}", __func__, op, op->on_complete, op->cancel_error, op->close_error, op->send_initial_metadata, op->send_trailing_metadata, op->send_message, op->recv_initial_metadata_ready, op->recv_trailing_metadata); grpc_deadline_state* deadline_state = elem->call_data; if (op->cancel_error != GRPC_ERROR_NONE || op->close_error != GRPC_ERROR_NONE) { @@ -194,14 +183,10 @@ typedef struct server_call_data { static grpc_error *init_call_elem(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, grpc_call_element_args* args) { -gpr_log(GPR_INFO, "==> %s() -- call_data_size=%lu", __func__, (unsigned long)elem->filter->sizeof_call_data); base_call_data* calld = elem->call_data; // Note: size of call data is different between client and server. memset(calld, 0, elem->filter->sizeof_call_data); grpc_deadline_state_init(&calld->deadline_state, args->call_stack); - -calld->deadline_state.is_client = elem->filter->sizeof_call_data == sizeof(base_call_data); - return GRPC_ERROR_NONE; } @@ -217,7 +202,6 @@ static void destroy_call_elem(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, static void client_start_transport_stream_op(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, grpc_transport_stream_op* op) { -gpr_log(GPR_INFO, "==> %s()", __func__); grpc_deadline_state_client_start_transport_stream_op(exec_ctx, elem, op); // Chain to next filter. grpc_call_next_op(exec_ctx, elem, op); @@ -240,7 +224,6 @@ static void recv_initial_metadata_ready(grpc_exec_ctx *exec_ctx, void *arg, static void server_start_transport_stream_op(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, grpc_transport_stream_op* op) { -gpr_log(GPR_INFO, "==> %s(): op=%p {on_complete=%p, cancel_error=%p, close_error=%p, send_initial_metadata=%p, send_trailing_metadata=%p, send_message=%p, recv_initial_metadata_ready=%p, recv_trailing_metadata=%p}", __func__, op, op->on_complete, op->cancel_error, op->close_error, op->send_initial_metadata, op->send_trailing_metadata, op->send_message, op->recv_initial_metadata_ready, op->recv_trailing_metadata); server_call_data* calld = elem->call_data; if (op->cancel_error != GRPC_ERROR_NONE || op->close_error != GRPC_ERROR_NONE) { diff --git a/src/core/lib/channel/deadline_filter.h b/src/core/lib/channel/deadline_filter.h index 793b76f9b68..bfe2cab90e7 100644 --- a/src/core/lib/channel/deadline_filter.h +++ b/src/core/lib/channel/deadline_filter.h @@ -52,10 +52,6 @@ typedef struct grpc_deadline_state { // The original on_complete closure, which we chain to after our own // closure is invoked. grpc_closure* next_on_complete; - -// FIXME: remove -bool is_client; - } grpc_deadline_state; void grpc_deadline_state_init(grpc_deadline_state* call_data, @@ -67,7 +63,7 @@ void grpc_deadline_state_destroy(grpc_exec_ctx* exec_ctx, // enforce call deadlines. // It is the caller's responsibility to chain to the next filter if // necessary after this function returns. -// REQUIRES: The first field in elem is a grpc_deadline_state struct. +// REQUIRES: The first field in elem->call_data is a grpc_deadline_state. void grpc_deadline_state_client_start_transport_stream_op( grpc_exec_ctx* exec_ctx, grpc_call_element* elem, grpc_transport_stream_op* op); diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c index 15c002015ce..fa0ee46da6b 100644 --- a/src/core/lib/surface/call.c +++ b/src/core/lib/surface/call.c @@ -1241,10 +1241,6 @@ static void finish_batch(grpc_exec_ctx *exec_ctx, void *bctlp, grpc_call *child_call; grpc_call *next_child_call; -const char* msg = grpc_error_string(error); -gpr_log(GPR_INFO, "==> finish_batch(): is_client=%d, error=%s", call->is_client, msg); -grpc_error_free_string(msg); - GRPC_ERROR_REF(error); gpr_mu_lock(&call->mu); From 932b10c75bb4674650a6862d2f155f3548ebfede Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 9 Sep 2016 08:44:30 -0700 Subject: [PATCH 15/26] clang-format --- src/core/ext/client_config/client_channel.c | 6 ++--- src/core/ext/lb_policy/grpclb/grpclb.c | 6 ++--- .../ext/lb_policy/pick_first/pick_first.c | 6 ++--- .../ext/lb_policy/round_robin/round_robin.c | 6 ++--- src/core/lib/channel/deadline_filter.c | 25 ++++++++----------- src/core/lib/channel/deadline_filter.h | 2 +- src/core/lib/iomgr/error.c | 21 +++++++--------- src/core/lib/surface/call.c | 5 ++-- 8 files changed, 35 insertions(+), 42 deletions(-) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index 569ca38b878..762fa271b20 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -563,9 +563,9 @@ static bool pick_subchannel(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, cpa = closure->cb_arg; if (cpa->connected_subchannel == connected_subchannel) { cpa->connected_subchannel = NULL; - grpc_exec_ctx_sched(exec_ctx, cpa->on_ready, - GRPC_ERROR_CREATE_REFERENCING( - "Pick cancelled", &error, 1), NULL); + grpc_exec_ctx_sched( + exec_ctx, cpa->on_ready, + GRPC_ERROR_CREATE_REFERENCING("Pick cancelled", &error, 1), NULL); } } gpr_mu_unlock(&chand->mu); diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index 9b7fcea1b07..cf4fae1d55f 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -545,9 +545,9 @@ static void glb_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, grpc_polling_entity_del_from_pollset_set( exec_ctx, pp->pollent, glb_policy->base.interested_parties); *target = NULL; - grpc_exec_ctx_sched(exec_ctx, &pp->wrapped_on_complete, - GRPC_ERROR_CREATE_REFERENCING( - "Pick Cancelled", &error, 1), NULL); + grpc_exec_ctx_sched( + exec_ctx, &pp->wrapped_on_complete, + GRPC_ERROR_CREATE_REFERENCING("Pick Cancelled", &error, 1), NULL); gpr_free(pp); } else { pp->next = glb_policy->pending_picks; diff --git a/src/core/ext/lb_policy/pick_first/pick_first.c b/src/core/ext/lb_policy/pick_first/pick_first.c index 089d6f45fb7..dcee25b2c22 100644 --- a/src/core/ext/lb_policy/pick_first/pick_first.c +++ b/src/core/ext/lb_policy/pick_first/pick_first.c @@ -141,9 +141,9 @@ static void pf_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, grpc_polling_entity_del_from_pollset_set(exec_ctx, pp->pollent, p->base.interested_parties); *target = NULL; - grpc_exec_ctx_sched(exec_ctx, pp->on_complete, - GRPC_ERROR_CREATE_REFERENCING( - "Pick Cancelled", &error, 1), NULL); + grpc_exec_ctx_sched( + exec_ctx, pp->on_complete, + GRPC_ERROR_CREATE_REFERENCING("Pick Cancelled", &error, 1), NULL); gpr_free(pp); } else { pp->next = p->pending_picks; diff --git a/src/core/ext/lb_policy/round_robin/round_robin.c b/src/core/ext/lb_policy/round_robin/round_robin.c index 1134495755c..83aba0f8ba4 100644 --- a/src/core/ext/lb_policy/round_robin/round_robin.c +++ b/src/core/ext/lb_policy/round_robin/round_robin.c @@ -294,9 +294,9 @@ static void rr_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, grpc_polling_entity_del_from_pollset_set(exec_ctx, pp->pollent, p->base.interested_parties); *target = NULL; - grpc_exec_ctx_sched(exec_ctx, pp->on_complete, - GRPC_ERROR_CREATE_REFERENCING( - "Pick cancelled", &error, 1), NULL); + grpc_exec_ctx_sched( + exec_ctx, pp->on_complete, + GRPC_ERROR_CREATE_REFERENCING("Pick cancelled", &error, 1), NULL); gpr_free(pp); } else { pp->next = p->pending_picks; diff --git a/src/core/lib/channel/deadline_filter.c b/src/core/lib/channel/deadline_filter.c index 424d09d3d8d..d83a231caca 100644 --- a/src/core/lib/channel/deadline_filter.c +++ b/src/core/lib/channel/deadline_filter.c @@ -45,8 +45,8 @@ // // Timer callback. -static void timer_callback(grpc_exec_ctx *exec_ctx, void *arg, - grpc_error *error) { +static void timer_callback(grpc_exec_ctx* exec_ctx, void* arg, + grpc_error* error) { grpc_call_element* elem = arg; grpc_deadline_state* deadline_state = elem->call_data; gpr_mu_lock(&deadline_state->timer_mu); @@ -54,16 +54,15 @@ static void timer_callback(grpc_exec_ctx *exec_ctx, void *arg, gpr_mu_unlock(&deadline_state->timer_mu); if (error != GRPC_ERROR_CANCELLED) { gpr_slice msg = gpr_slice_from_static_string("Deadline Exceeded"); - grpc_call_element_send_cancel_with_message(exec_ctx, elem, - GRPC_STATUS_DEADLINE_EXCEEDED, - &msg); + grpc_call_element_send_cancel_with_message( + exec_ctx, elem, GRPC_STATUS_DEADLINE_EXCEEDED, &msg); gpr_slice_unref(msg); } GRPC_CALL_STACK_UNREF(exec_ctx, deadline_state->call_stack, "deadline_timer"); } // Starts the deadline timer. -static void start_timer_if_needed(grpc_exec_ctx *exec_ctx, +static void start_timer_if_needed(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, gpr_timespec deadline) { grpc_deadline_state* deadline_state = elem->call_data; @@ -91,7 +90,7 @@ static void cancel_timer_if_needed(grpc_exec_ctx* exec_ctx, } // Callback run when the call is complete. -static void on_complete(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { +static void on_complete(grpc_exec_ctx* exec_ctx, void* arg, grpc_error* error) { grpc_deadline_state* deadline_state = arg; cancel_timer_if_needed(exec_ctx, deadline_state); // Invoke the next callback. @@ -159,8 +158,7 @@ static void init_channel_elem(grpc_exec_ctx* exec_ctx, // Destructor for channel_data. Used for both client and server filters. static void destroy_channel_elem(grpc_exec_ctx* exec_ctx, - grpc_channel_element* elem) { -} + grpc_channel_element* elem) {} // Call data used for both client and server filter. typedef struct base_call_data { @@ -180,7 +178,7 @@ typedef struct server_call_data { } server_call_data; // Constructor for call_data. Used for both client and server filters. -static grpc_error *init_call_elem(grpc_exec_ctx* exec_ctx, +static grpc_error* init_call_elem(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, grpc_call_element_args* args) { base_call_data* calld = elem->call_data; @@ -208,13 +206,12 @@ static void client_start_transport_stream_op(grpc_exec_ctx* exec_ctx, } // Callback for receiving initial metadata on the server. -static void recv_initial_metadata_ready(grpc_exec_ctx *exec_ctx, void *arg, - grpc_error *error) { +static void recv_initial_metadata_ready(grpc_exec_ctx* exec_ctx, void* arg, + grpc_error* error) { grpc_call_element* elem = arg; server_call_data* calld = elem->call_data; // Get deadline from metadata and start the timer if needed. - start_timer_if_needed(exec_ctx, elem, - calld->recv_initial_metadata->deadline); + start_timer_if_needed(exec_ctx, elem, calld->recv_initial_metadata->deadline); // Invoke the next callback. calld->next_recv_initial_metadata_ready->cb( exec_ctx, calld->next_recv_initial_metadata_ready->cb_arg, error); diff --git a/src/core/lib/channel/deadline_filter.h b/src/core/lib/channel/deadline_filter.h index bfe2cab90e7..a09f85afe6b 100644 --- a/src/core/lib/channel/deadline_filter.h +++ b/src/core/lib/channel/deadline_filter.h @@ -74,4 +74,4 @@ void grpc_deadline_state_client_start_transport_stream_op( extern const grpc_channel_filter grpc_client_deadline_filter; extern const grpc_channel_filter grpc_server_deadline_filter; -#endif // GRPC_CORE_LIB_CHANNEL_DEADLINE_FILTER_H +#endif /* GRPC_CORE_LIB_CHANNEL_DEADLINE_FILTER_H */ diff --git a/src/core/lib/iomgr/error.c b/src/core/lib/iomgr/error.c index 17f19423a3f..e13f37f8ed1 100644 --- a/src/core/lib/iomgr/error.c +++ b/src/core/lib/iomgr/error.c @@ -330,13 +330,13 @@ typedef struct { const char *msg; } special_error_status_map; static special_error_status_map error_status_map[] = { - { GRPC_ERROR_NONE, GRPC_STATUS_OK, "" }, - { GRPC_ERROR_CANCELLED, GRPC_STATUS_CANCELLED, "RPC cancelled" }, - { GRPC_ERROR_OOM, GRPC_STATUS_RESOURCE_EXHAUSTED, "Out of memory" }, + {GRPC_ERROR_NONE, GRPC_STATUS_OK, ""}, + {GRPC_ERROR_CANCELLED, GRPC_STATUS_CANCELLED, "RPC cancelled"}, + {GRPC_ERROR_OOM, GRPC_STATUS_RESOURCE_EXHAUSTED, "Out of memory"}, }; -static grpc_error *recursively_find_error_with_status(grpc_error* error, - intptr_t* status) { +static grpc_error *recursively_find_error_with_status(grpc_error *error, + intptr_t *status) { // If the error itself has a status code, return it. if (grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, status)) { return error; @@ -345,12 +345,10 @@ static grpc_error *recursively_find_error_with_status(grpc_error* error, intptr_t key = 0; while (true) { grpc_error *child_error = gpr_avl_get(error->errs, (void *)key++); - if (child_error == NULL) - break; + if (child_error == NULL) break; grpc_error *result = recursively_find_error_with_status(child_error, status); - if (result != NULL) - return result; + if (result != NULL) return result; } return NULL; } @@ -359,8 +357,7 @@ void grpc_error_get_status(grpc_error *error, grpc_status_code *code, const char **msg) { // Handle special errors via the static map. for (size_t i = 0; - i < sizeof(error_status_map) / sizeof(special_error_status_map); - ++i) { + i < sizeof(error_status_map) / sizeof(special_error_status_map); ++i) { if (error == error_status_map[i].error) { *code = error_status_map[i].code; *msg = error_status_map[i].msg; @@ -371,7 +368,7 @@ void grpc_error_get_status(grpc_error *error, grpc_status_code *code, // Start with the parent error and recurse through the tree of children // until we find the first one that has a status code. intptr_t status = GRPC_STATUS_UNKNOWN; // Default in case we don't find one. - grpc_error* found_error = recursively_find_error_with_status(error, &status); + grpc_error *found_error = recursively_find_error_with_status(error, &status); *code = (grpc_status_code)status; // Now populate msg. // If we found an error with a status code above, use that; otherwise, diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c index fa0ee46da6b..424cd00d962 100644 --- a/src/core/lib/surface/call.c +++ b/src/core/lib/surface/call.c @@ -253,8 +253,7 @@ grpc_call *grpc_call_create( call->metadata_batch[i][j].deadline = gpr_inf_future(GPR_CLOCK_MONOTONIC); } } - send_deadline = - gpr_convert_clock_type(send_deadline, GPR_CLOCK_MONOTONIC); + send_deadline = gpr_convert_clock_type(send_deadline, GPR_CLOCK_MONOTONIC); GRPC_CHANNEL_INTERNAL_REF(channel, "call"); /* initial refcount dropped by grpc_call_destroy */ grpc_error *error = grpc_call_stack_init( @@ -438,7 +437,7 @@ static void set_status_details(grpc_call *call, status_source source, static void set_status_from_error(grpc_call *call, status_source source, grpc_error *error) { grpc_status_code status; - const char* msg; + const char *msg; grpc_error_get_status(error, &status, &msg); set_status_code(call, source, (uint32_t)status); set_status_details(call, source, grpc_mdstr_from_string(msg)); From b3405f0a7370857373d71bb6a62e056f37493529 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 9 Sep 2016 08:46:28 -0700 Subject: [PATCH 16/26] Fix build problem on Windows. --- src/core/lib/channel/deadline_filter.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/core/lib/channel/deadline_filter.c b/src/core/lib/channel/deadline_filter.c index d83a231caca..010fedd7b73 100644 --- a/src/core/lib/channel/deadline_filter.c +++ b/src/core/lib/channel/deadline_filter.c @@ -145,10 +145,6 @@ void grpc_deadline_state_client_start_transport_stream_op( // filter code // -// Used for both client and server filters. -typedef struct channel_data { -} channel_data; - // Constructor for channel_data. Used for both client and server filters. static void init_channel_elem(grpc_exec_ctx* exec_ctx, grpc_channel_element* elem, @@ -256,7 +252,7 @@ const grpc_channel_filter grpc_client_deadline_filter = { init_call_elem, grpc_call_stack_ignore_set_pollset_or_pollset_set, destroy_call_elem, - sizeof(channel_data), + 0, // sizeof(channel_data) init_channel_elem, destroy_channel_elem, grpc_call_next_get_peer, @@ -270,7 +266,7 @@ const grpc_channel_filter grpc_server_deadline_filter = { init_call_elem, grpc_call_stack_ignore_set_pollset_or_pollset_set, destroy_call_elem, - sizeof(channel_data), + 0, // sizeof(channel_data) init_channel_elem, destroy_channel_elem, grpc_call_next_get_peer, From e65ff1116158958febacd1131a7c2f84f4e0eb1b Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 9 Sep 2016 13:48:38 -0700 Subject: [PATCH 17/26] Propagate error through grpc_lb_policy_cancel_picks(). Also fix error reporting (and improve control flow) in on_writable() in tcp_client_posix.c. --- src/core/ext/client_config/client_channel.c | 3 +- src/core/ext/client_config/lb_policy.c | 5 +- src/core/ext/client_config/lb_policy.h | 5 +- src/core/ext/client_config/subchannel.c | 4 +- src/core/ext/lb_policy/grpclb/grpclb.c | 7 +- .../ext/lb_policy/pick_first/pick_first.c | 7 +- .../ext/lb_policy/round_robin/round_robin.c | 9 +- src/core/lib/iomgr/tcp_client_posix.c | 96 +++++++++---------- 8 files changed, 73 insertions(+), 63 deletions(-) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index 762fa271b20..245cfbb8c4f 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -112,7 +112,8 @@ static void set_channel_connectivity_state_locked(grpc_exec_ctx *exec_ctx, grpc_lb_policy_cancel_picks( exec_ctx, chand->lb_policy, /* mask= */ GRPC_INITIAL_METADATA_IGNORE_CONNECTIVITY, - /* check= */ 0); + /* check= */ 0, + GRPC_ERROR_REF(error)); } grpc_connectivity_state_set(exec_ctx, &chand->state_tracker, state, error, reason); diff --git a/src/core/ext/client_config/lb_policy.c b/src/core/ext/client_config/lb_policy.c index 5e605491e75..3e498f5f561 100644 --- a/src/core/ext/client_config/lb_policy.c +++ b/src/core/ext/client_config/lb_policy.c @@ -118,9 +118,10 @@ void grpc_lb_policy_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy, void grpc_lb_policy_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy, uint32_t initial_metadata_flags_mask, - uint32_t initial_metadata_flags_eq) { + uint32_t initial_metadata_flags_eq, + grpc_error *error) { policy->vtable->cancel_picks(exec_ctx, policy, initial_metadata_flags_mask, - initial_metadata_flags_eq); + initial_metadata_flags_eq, error); } void grpc_lb_policy_exit_idle(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy) { diff --git a/src/core/ext/client_config/lb_policy.h b/src/core/ext/client_config/lb_policy.h index 9650110cce3..eb07ad3360c 100644 --- a/src/core/ext/client_config/lb_policy.h +++ b/src/core/ext/client_config/lb_policy.h @@ -68,7 +68,7 @@ struct grpc_lb_policy_vtable { grpc_connected_subchannel **target, grpc_error *error); void (*cancel_picks)(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy, uint32_t initial_metadata_flags_mask, - uint32_t initial_metadata_flags_eq); + uint32_t initial_metadata_flags_eq, grpc_error *error); void (*ping_one)(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy, grpc_closure *closure); @@ -148,7 +148,8 @@ void grpc_lb_policy_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy, void grpc_lb_policy_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy, uint32_t initial_metadata_flags_mask, - uint32_t initial_metadata_flags_eq); + uint32_t initial_metadata_flags_eq, + grpc_error *error); /** Try to enter a READY connectivity state */ void grpc_lb_policy_exit_idle(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy); diff --git a/src/core/ext/client_config/subchannel.c b/src/core/ext/client_config/subchannel.c index 3d4318774b1..456cc446350 100644 --- a/src/core/ext/client_config/subchannel.c +++ b/src/core/ext/client_config/subchannel.c @@ -635,7 +635,9 @@ static void subchannel_connected(grpc_exec_ctx *exec_ctx, void *arg, c->have_alarm = 1; grpc_connectivity_state_set( exec_ctx, &c->state_tracker, GRPC_CHANNEL_TRANSIENT_FAILURE, - GRPC_ERROR_CREATE_REFERENCING("Connect Failed", &error, 1), + grpc_error_set_int( + GRPC_ERROR_CREATE_REFERENCING("Connect Failed", &error, 1), + GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_UNAVAILABLE), "connect_failed"); gpr_timespec time_til_next = gpr_time_sub(c->next_attempt, now); const char *errmsg = grpc_error_string(error); diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index cf4fae1d55f..50eb54f1cc3 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -562,7 +562,8 @@ static void glb_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, static grpc_call *lb_client_data_get_call(struct lb_client_data *lb_client); static void glb_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, uint32_t initial_metadata_flags_mask, - uint32_t initial_metadata_flags_eq) { + uint32_t initial_metadata_flags_eq, + grpc_error *error) { glb_lb_policy *glb_policy = (glb_lb_policy *)pol; gpr_mu_lock(&glb_policy->mu); if (glb_policy->lb_client != NULL) { @@ -578,7 +579,8 @@ static void glb_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, grpc_polling_entity_del_from_pollset_set( exec_ctx, pp->pollent, glb_policy->base.interested_parties); grpc_exec_ctx_sched(exec_ctx, &pp->wrapped_on_complete, - GRPC_ERROR_CANCELLED, NULL); + GRPC_ERROR_CREATE_REFERENCING("Pick Cancelled", + &error, 1), NULL); gpr_free(pp); } else { pp->next = glb_policy->pending_picks; @@ -587,6 +589,7 @@ static void glb_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, pp = next; } gpr_mu_unlock(&glb_policy->mu); + GRPC_ERROR_UNREF(error); } static void query_for_backends(grpc_exec_ctx *exec_ctx, diff --git a/src/core/ext/lb_policy/pick_first/pick_first.c b/src/core/ext/lb_policy/pick_first/pick_first.c index dcee25b2c22..9e83de28e5c 100644 --- a/src/core/ext/lb_policy/pick_first/pick_first.c +++ b/src/core/ext/lb_policy/pick_first/pick_first.c @@ -157,7 +157,8 @@ static void pf_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, static void pf_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, uint32_t initial_metadata_flags_mask, - uint32_t initial_metadata_flags_eq) { + uint32_t initial_metadata_flags_eq, + grpc_error *error) { pick_first_lb_policy *p = (pick_first_lb_policy *)pol; pending_pick *pp; gpr_mu_lock(&p->mu); @@ -170,7 +171,8 @@ static void pf_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, grpc_polling_entity_del_from_pollset_set(exec_ctx, pp->pollent, p->base.interested_parties); grpc_exec_ctx_sched(exec_ctx, pp->on_complete, - GRPC_ERROR_CREATE("Pick Cancelled"), NULL); + GRPC_ERROR_CREATE_REFERENCING("Pick Cancelled", + &error, 1), NULL); gpr_free(pp); } else { pp->next = p->pending_picks; @@ -179,6 +181,7 @@ static void pf_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, pp = next; } gpr_mu_unlock(&p->mu); + GRPC_ERROR_UNREF(error); } static void start_picking(grpc_exec_ctx *exec_ctx, pick_first_lb_policy *p) { diff --git a/src/core/ext/lb_policy/round_robin/round_robin.c b/src/core/ext/lb_policy/round_robin/round_robin.c index 83aba0f8ba4..0c40dcf9615 100644 --- a/src/core/ext/lb_policy/round_robin/round_robin.c +++ b/src/core/ext/lb_policy/round_robin/round_robin.c @@ -310,7 +310,8 @@ static void rr_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, static void rr_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, uint32_t initial_metadata_flags_mask, - uint32_t initial_metadata_flags_eq) { + uint32_t initial_metadata_flags_eq, + grpc_error *error) { round_robin_lb_policy *p = (round_robin_lb_policy *)pol; pending_pick *pp; gpr_mu_lock(&p->mu); @@ -323,8 +324,9 @@ static void rr_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, grpc_polling_entity_del_from_pollset_set(exec_ctx, pp->pollent, p->base.interested_parties); *pp->target = NULL; - grpc_exec_ctx_sched(exec_ctx, pp->on_complete, GRPC_ERROR_CANCELLED, - NULL); + grpc_exec_ctx_sched(exec_ctx, pp->on_complete, + GRPC_ERROR_CREATE_REFERENCING("Pick cancelled", + &error, 1), NULL); gpr_free(pp); } else { pp->next = p->pending_picks; @@ -333,6 +335,7 @@ static void rr_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, pp = next; } gpr_mu_unlock(&p->mu); + GRPC_ERROR_UNREF(error); } static void start_picking(grpc_exec_ctx *exec_ctx, round_robin_lb_policy *p) { diff --git a/src/core/lib/iomgr/tcp_client_posix.c b/src/core/lib/iomgr/tcp_client_posix.c index 80c7a3f1287..3496b6094f2 100644 --- a/src/core/lib/iomgr/tcp_client_posix.c +++ b/src/core/lib/iomgr/tcp_client_posix.c @@ -146,61 +146,57 @@ static void on_writable(grpc_exec_ctx *exec_ctx, void *acp, grpc_error *error) { grpc_timer_cancel(exec_ctx, &ac->alarm); gpr_mu_lock(&ac->mu); - if (error == GRPC_ERROR_NONE) { - do { - so_error_size = sizeof(so_error); - err = getsockopt(grpc_fd_wrapped_fd(fd), SOL_SOCKET, SO_ERROR, &so_error, - &so_error_size); - } while (err < 0 && errno == EINTR); - if (err < 0) { - error = GRPC_OS_ERROR(errno, "getsockopt"); - goto finish; - } else if (so_error != 0) { - if (so_error == ENOBUFS) { - /* We will get one of these errors if we have run out of - memory in the kernel for the data structures allocated - when you connect a socket. If this happens it is very - likely that if we wait a little bit then try again the - connection will work (since other programs or this - program will close their network connections and free up - memory). This does _not_ indicate that there is anything - wrong with the server we are connecting to, this is a - local problem. - - If you are looking at this code, then chances are that - your program or another program on the same computer - opened too many network connections. The "easy" fix: - don't do that! */ - gpr_log(GPR_ERROR, "kernel out of buffers"); - gpr_mu_unlock(&ac->mu); - grpc_fd_notify_on_write(exec_ctx, fd, &ac->write_closure); - return; - } else { - switch (so_error) { - case ECONNREFUSED: - error = grpc_error_set_int(error, GRPC_ERROR_INT_ERRNO, errno); - error = grpc_error_set_str(error, GRPC_ERROR_STR_OS_ERROR, - "Connection refused"); - break; - default: - error = GRPC_OS_ERROR(errno, "getsockopt(SO_ERROR)"); - break; - } - goto finish; - } - } else { - grpc_pollset_set_del_fd(exec_ctx, ac->interested_parties, fd); - *ep = grpc_tcp_create(fd, GRPC_TCP_DEFAULT_READ_SLICE_SIZE, ac->addr_str); - fd = NULL; - goto finish; - } - } else { + if (error != GRPC_ERROR_NONE) { error = grpc_error_set_str(error, GRPC_ERROR_STR_OS_ERROR, "Timeout occurred"); goto finish; } - GPR_UNREACHABLE_CODE(return ); + do { + so_error_size = sizeof(so_error); + err = getsockopt(grpc_fd_wrapped_fd(fd), SOL_SOCKET, SO_ERROR, &so_error, + &so_error_size); + } while (err < 0 && errno == EINTR); + if (err < 0) { + error = GRPC_OS_ERROR(errno, "getsockopt"); + goto finish; + } + + switch (so_error) { + case 0: + grpc_pollset_set_del_fd(exec_ctx, ac->interested_parties, fd); + *ep = grpc_tcp_create(fd, GRPC_TCP_DEFAULT_READ_SLICE_SIZE, ac->addr_str); + fd = NULL; + break; + case ENOBUFS: + /* We will get one of these errors if we have run out of + memory in the kernel for the data structures allocated + when you connect a socket. If this happens it is very + likely that if we wait a little bit then try again the + connection will work (since other programs or this + program will close their network connections and free up + memory). This does _not_ indicate that there is anything + wrong with the server we are connecting to, this is a + local problem. + + If you are looking at this code, then chances are that + your program or another program on the same computer + opened too many network connections. The "easy" fix: + don't do that! */ + gpr_log(GPR_ERROR, "kernel out of buffers"); + gpr_mu_unlock(&ac->mu); + grpc_fd_notify_on_write(exec_ctx, fd, &ac->write_closure); + return; + case ECONNREFUSED: + /* This error shouldn't happen for anything other than connect(). */ + error = GRPC_OS_ERROR(so_error, "connect"); + break; + default: + /* We don't really know which syscall triggered the problem here, + so punt by reporting getsockopt(). */ + error = GRPC_OS_ERROR(so_error, "getsockopt(SO_ERROR)"); + break; + } finish: if (fd != NULL) { From 58f52b789185d696d98be3c8ef4571499c8101dd Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 9 Sep 2016 13:55:18 -0700 Subject: [PATCH 18/26] clang-format --- src/core/ext/client_config/client_channel.c | 3 +-- src/core/ext/lb_policy/grpclb/grpclb.c | 6 +++--- src/core/ext/lb_policy/pick_first/pick_first.c | 6 +++--- src/core/ext/lb_policy/round_robin/round_robin.c | 6 +++--- 4 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index 245cfbb8c4f..5a8030b23e9 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -112,8 +112,7 @@ static void set_channel_connectivity_state_locked(grpc_exec_ctx *exec_ctx, grpc_lb_policy_cancel_picks( exec_ctx, chand->lb_policy, /* mask= */ GRPC_INITIAL_METADATA_IGNORE_CONNECTIVITY, - /* check= */ 0, - GRPC_ERROR_REF(error)); + /* check= */ 0, GRPC_ERROR_REF(error)); } grpc_connectivity_state_set(exec_ctx, &chand->state_tracker, state, error, reason); diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index 50eb54f1cc3..550f5c5972d 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -578,9 +578,9 @@ static void glb_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, initial_metadata_flags_eq) { grpc_polling_entity_del_from_pollset_set( exec_ctx, pp->pollent, glb_policy->base.interested_parties); - grpc_exec_ctx_sched(exec_ctx, &pp->wrapped_on_complete, - GRPC_ERROR_CREATE_REFERENCING("Pick Cancelled", - &error, 1), NULL); + grpc_exec_ctx_sched( + exec_ctx, &pp->wrapped_on_complete, + GRPC_ERROR_CREATE_REFERENCING("Pick Cancelled", &error, 1), NULL); gpr_free(pp); } else { pp->next = glb_policy->pending_picks; diff --git a/src/core/ext/lb_policy/pick_first/pick_first.c b/src/core/ext/lb_policy/pick_first/pick_first.c index 9e83de28e5c..5962ec3327c 100644 --- a/src/core/ext/lb_policy/pick_first/pick_first.c +++ b/src/core/ext/lb_policy/pick_first/pick_first.c @@ -170,9 +170,9 @@ static void pf_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, initial_metadata_flags_eq) { grpc_polling_entity_del_from_pollset_set(exec_ctx, pp->pollent, p->base.interested_parties); - grpc_exec_ctx_sched(exec_ctx, pp->on_complete, - GRPC_ERROR_CREATE_REFERENCING("Pick Cancelled", - &error, 1), NULL); + grpc_exec_ctx_sched( + exec_ctx, pp->on_complete, + GRPC_ERROR_CREATE_REFERENCING("Pick Cancelled", &error, 1), NULL); gpr_free(pp); } else { pp->next = p->pending_picks; diff --git a/src/core/ext/lb_policy/round_robin/round_robin.c b/src/core/ext/lb_policy/round_robin/round_robin.c index 0c40dcf9615..654fcf2d2ac 100644 --- a/src/core/ext/lb_policy/round_robin/round_robin.c +++ b/src/core/ext/lb_policy/round_robin/round_robin.c @@ -324,9 +324,9 @@ static void rr_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, grpc_polling_entity_del_from_pollset_set(exec_ctx, pp->pollent, p->base.interested_parties); *pp->target = NULL; - grpc_exec_ctx_sched(exec_ctx, pp->on_complete, - GRPC_ERROR_CREATE_REFERENCING("Pick cancelled", - &error, 1), NULL); + grpc_exec_ctx_sched( + exec_ctx, pp->on_complete, + GRPC_ERROR_CREATE_REFERENCING("Pick cancelled", &error, 1), NULL); gpr_free(pp); } else { pp->next = p->pending_picks; From d5130f2fb3c03310b90607957beea026ab428618 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 13 Sep 2016 11:33:54 -0700 Subject: [PATCH 19/26] Fix build problem from merge. --- src/core/lib/channel/channel_stack.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/lib/channel/channel_stack.c b/src/core/lib/channel/channel_stack.c index 4994b2484ca..f5fa0b03907 100644 --- a/src/core/lib/channel/channel_stack.c +++ b/src/core/lib/channel/channel_stack.c @@ -275,7 +275,7 @@ void grpc_call_element_send_cancel(grpc_exec_ctx *exec_ctx, grpc_transport_stream_op op; memset(&op, 0, sizeof(op)); op.cancel_error = GRPC_ERROR_CANCELLED; - elem->filter->start_transport_stream_op(exec_ctx, elem, op); + elem->filter->start_transport_stream_op(exec_ctx, elem, &op); } void grpc_call_element_send_cancel_with_message(grpc_exec_ctx *exec_ctx, @@ -286,7 +286,7 @@ void grpc_call_element_send_cancel_with_message(grpc_exec_ctx *exec_ctx, memset(&op, 0, sizeof(op)); grpc_transport_stream_op_add_cancellation_with_message(&op, status, optional_message); - elem->filter->start_transport_stream_op(exec_ctx, elem, op); + elem->filter->start_transport_stream_op(exec_ctx, elem, &op); } void grpc_call_element_send_close_with_message(grpc_exec_ctx *exec_ctx, From bf3596ef5f552190e9350e7376b3b533753e890c Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 13 Sep 2016 11:35:35 -0700 Subject: [PATCH 20/26] Minor cleanup. --- src/core/lib/surface/call.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c index ad5a7223844..424cd00d962 100644 --- a/src/core/lib/surface/call.c +++ b/src/core/lib/surface/call.c @@ -769,8 +769,8 @@ static void done_termination(grpc_exec_ctx *exec_ctx, void *tcp, } static void send_cancel(grpc_exec_ctx *exec_ctx, void *tcp, grpc_error *error) { - grpc_transport_stream_op op; termination_closure *tc = tcp; + grpc_transport_stream_op op; memset(&op, 0, sizeof(op)); op.cancel_error = tc->error; /* reuse closure to catch completion */ @@ -780,8 +780,8 @@ static void send_cancel(grpc_exec_ctx *exec_ctx, void *tcp, grpc_error *error) { } static void send_close(grpc_exec_ctx *exec_ctx, void *tcp, grpc_error *error) { - grpc_transport_stream_op op; termination_closure *tc = tcp; + grpc_transport_stream_op op; memset(&op, 0, sizeof(op)); op.close_error = tc->error; /* reuse closure to catch completion */ From e2bdd54126ac0003f3b7b5880f2c0c69743f5b82 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 13 Sep 2016 14:30:18 -0700 Subject: [PATCH 21/26] Fix setting deadline in server call. --- src/core/lib/surface/call.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c index 424cd00d962..0ced42a5717 100644 --- a/src/core/lib/surface/call.c +++ b/src/core/lib/surface/call.c @@ -1216,6 +1216,13 @@ static void receiving_initial_metadata_ready(grpc_exec_ctx *exec_ctx, GPR_TIMER_BEGIN("validate_filtered_metadata", 0); validate_filtered_metadata(exec_ctx, bctl); GPR_TIMER_END("validate_filtered_metadata", 0); + + if (gpr_time_cmp(md->deadline, gpr_inf_future(md->deadline.clock_type)) != + 0 && + !call->is_client) { + call->send_deadline = gpr_convert_clock_type(md->deadline, + GPR_CLOCK_MONOTONIC); + } } call->has_initial_md_been_received = true; From a99e02cc0d165a226bf57eb49a290f3a626168d7 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 14 Sep 2016 09:39:49 -0700 Subject: [PATCH 22/26] Use GPR_ARRAY_SIZE(). --- src/core/lib/iomgr/error.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/core/lib/iomgr/error.c b/src/core/lib/iomgr/error.c index e13f37f8ed1..3d9f1a8ca58 100644 --- a/src/core/lib/iomgr/error.c +++ b/src/core/lib/iomgr/error.c @@ -356,8 +356,7 @@ static grpc_error *recursively_find_error_with_status(grpc_error *error, void grpc_error_get_status(grpc_error *error, grpc_status_code *code, const char **msg) { // Handle special errors via the static map. - for (size_t i = 0; - i < sizeof(error_status_map) / sizeof(special_error_status_map); ++i) { + for (size_t i = 0; i < GPR_ARRAY_SIZE(error_status_map); ++i) { if (error == error_status_map[i].error) { *code = error_status_map[i].code; *msg = error_status_map[i].msg; From f28763c68c9b3caf539f4d38ff123ae5de69e6d8 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 14 Sep 2016 15:18:40 -0700 Subject: [PATCH 23/26] Pass deadline into filters via grpc_call_element_args, so that we can start the timer before the first op is sent down. --- src/core/ext/client_config/client_channel.c | 28 +++++--- src/core/ext/client_config/subchannel.c | 5 +- src/core/ext/client_config/subchannel.h | 3 +- src/core/lib/channel/channel_stack.c | 13 ++-- src/core/lib/channel/channel_stack.h | 13 ++-- src/core/lib/channel/deadline_filter.c | 58 +++++++++++---- src/core/lib/channel/deadline_filter.h | 22 +++--- src/core/lib/surface/call.c | 72 +++++++++++-------- .../core/end2end/invalid_call_argument_test.c | 10 --- test/core/end2end/tests/negative_deadline.c | 16 ++--- 10 files changed, 140 insertions(+), 100 deletions(-) diff --git a/src/core/ext/client_config/client_channel.c b/src/core/ext/client_config/client_channel.c index 5a8030b23e9..76c2f38a5db 100644 --- a/src/core/ext/client_config/client_channel.c +++ b/src/core/ext/client_config/client_channel.c @@ -385,6 +385,9 @@ typedef struct client_channel_call_data { // stack and each has its own mutex. If/when we have time, find a way // to avoid this without breaking the grpc_deadline_state abstraction. grpc_deadline_state deadline_state; + gpr_timespec deadline; + + grpc_error *cancel_error; /** either 0 for no call, 1 for cancelled, or a pointer to a grpc_subchannel_call */ @@ -482,7 +485,7 @@ static void subchannel_ready(grpc_exec_ctx *exec_ctx, void *arg, } else { grpc_subchannel_call *subchannel_call = NULL; grpc_error *new_error = grpc_connected_subchannel_create_call( - exec_ctx, calld->connected_subchannel, calld->pollent, + exec_ctx, calld->connected_subchannel, calld->pollent, calld->deadline, &subchannel_call); if (new_error != GRPC_ERROR_NONE) { new_error = grpc_error_add_child(new_error, error); @@ -627,8 +630,8 @@ static void cc_start_transport_stream_op(grpc_exec_ctx *exec_ctx, grpc_subchannel_call *call = GET_CALL(calld); GPR_TIMER_BEGIN("cc_start_transport_stream_op", 0); if (call == CANCELLED_CALL) { - grpc_transport_stream_op_finish_with_failure(exec_ctx, op, - GRPC_ERROR_CANCELLED); + grpc_transport_stream_op_finish_with_failure( + exec_ctx, op, GRPC_ERROR_REF(calld->cancel_error)); GPR_TIMER_END("cc_start_transport_stream_op", 0); return; } @@ -644,8 +647,8 @@ retry: call = GET_CALL(calld); if (call == CANCELLED_CALL) { gpr_mu_unlock(&calld->mu); - grpc_transport_stream_op_finish_with_failure(exec_ctx, op, - GRPC_ERROR_CANCELLED); + grpc_transport_stream_op_finish_with_failure( + exec_ctx, op, GRPC_ERROR_REF(calld->cancel_error)); GPR_TIMER_END("cc_start_transport_stream_op", 0); return; } @@ -661,6 +664,12 @@ retry: (gpr_atm)(uintptr_t)CANCELLED_CALL)) { goto retry; } else { + // Stash a copy of cancel_error in our call data, so that we can use + // it for subsequent operations. This ensures that if the call is + // cancelled before any ops are passed down (e.g., if the deadline + // is in the past when the call starts), we can return the right + // error to the caller when the first op does get passed down. + calld->cancel_error = GRPC_ERROR_REF(op->cancel_error); switch (calld->creation_phase) { case GRPC_SUBCHANNEL_CALL_HOLDER_NOT_CREATING: fail_locked(exec_ctx, calld, GRPC_ERROR_REF(op->cancel_error)); @@ -697,7 +706,7 @@ retry: calld->connected_subchannel != NULL) { grpc_subchannel_call *subchannel_call = NULL; grpc_error *error = grpc_connected_subchannel_create_call( - exec_ctx, calld->connected_subchannel, calld->pollent, + exec_ctx, calld->connected_subchannel, calld->pollent, calld->deadline, &subchannel_call); if (error != GRPC_ERROR_NONE) { subchannel_call = CANCELLED_CALL; @@ -720,7 +729,9 @@ static grpc_error *cc_init_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, grpc_call_element_args *args) { call_data *calld = elem->call_data; - grpc_deadline_state_init(&calld->deadline_state, args->call_stack); + grpc_deadline_state_init(exec_ctx, elem, args); + calld->deadline = args->deadline; + calld->cancel_error = GRPC_ERROR_NONE; gpr_atm_rel_store(&calld->subchannel_call, 0); gpr_mu_init(&calld->mu); calld->connected_subchannel = NULL; @@ -739,7 +750,8 @@ static void cc_destroy_call_elem(grpc_exec_ctx *exec_ctx, const grpc_call_final_info *final_info, void *and_free_memory) { call_data *calld = elem->call_data; - grpc_deadline_state_destroy(exec_ctx, &calld->deadline_state); + grpc_deadline_state_destroy(exec_ctx, elem); + GRPC_ERROR_UNREF(calld->cancel_error); grpc_subchannel_call *call = GET_CALL(calld); if (call != NULL && call != CANCELLED_CALL) { GRPC_SUBCHANNEL_CALL_UNREF(exec_ctx, call, "client_channel_destroy_call"); diff --git a/src/core/ext/client_config/subchannel.c b/src/core/ext/client_config/subchannel.c index 456cc446350..8f4a2f9e3ec 100644 --- a/src/core/ext/client_config/subchannel.c +++ b/src/core/ext/client_config/subchannel.c @@ -706,14 +706,15 @@ grpc_connected_subchannel *grpc_subchannel_get_connected_subchannel( grpc_error *grpc_connected_subchannel_create_call( grpc_exec_ctx *exec_ctx, grpc_connected_subchannel *con, - grpc_polling_entity *pollent, grpc_subchannel_call **call) { + grpc_polling_entity *pollent, gpr_timespec deadline, + grpc_subchannel_call **call) { grpc_channel_stack *chanstk = CHANNEL_STACK_FROM_CONNECTION(con); *call = gpr_malloc(sizeof(grpc_subchannel_call) + chanstk->call_stack_size); grpc_call_stack *callstk = SUBCHANNEL_CALL_TO_CALL_STACK(*call); (*call)->connection = con; // Ref is added below. grpc_error *error = grpc_call_stack_init(exec_ctx, chanstk, 1, subchannel_call_destroy, *call, - NULL, NULL, callstk); + NULL, NULL, deadline, callstk); if (error != GRPC_ERROR_NONE) { const char *error_string = grpc_error_string(error); gpr_log(GPR_ERROR, "error: %s", error_string); diff --git a/src/core/ext/client_config/subchannel.h b/src/core/ext/client_config/subchannel.h index ae1d96e6400..763ff857571 100644 --- a/src/core/ext/client_config/subchannel.h +++ b/src/core/ext/client_config/subchannel.h @@ -110,7 +110,8 @@ void grpc_subchannel_call_unref(grpc_exec_ctx *exec_ctx, /** construct a subchannel call */ grpc_error *grpc_connected_subchannel_create_call( grpc_exec_ctx *exec_ctx, grpc_connected_subchannel *connected_subchannel, - grpc_polling_entity *pollent, grpc_subchannel_call **subchannel_call); + grpc_polling_entity *pollent, gpr_timespec deadline, + grpc_subchannel_call **subchannel_call); /** process a transport level op */ void grpc_connected_subchannel_process_transport_op( diff --git a/src/core/lib/channel/channel_stack.c b/src/core/lib/channel/channel_stack.c index f5fa0b03907..98177f439b0 100644 --- a/src/core/lib/channel/channel_stack.c +++ b/src/core/lib/channel/channel_stack.c @@ -157,13 +157,11 @@ void grpc_channel_stack_destroy(grpc_exec_ctx *exec_ctx, } } -grpc_error *grpc_call_stack_init(grpc_exec_ctx *exec_ctx, - grpc_channel_stack *channel_stack, - int initial_refs, grpc_iomgr_cb_func destroy, - void *destroy_arg, - grpc_call_context_element *context, - const void *transport_server_data, - grpc_call_stack *call_stack) { +grpc_error *grpc_call_stack_init( + grpc_exec_ctx *exec_ctx, grpc_channel_stack *channel_stack, + int initial_refs, grpc_iomgr_cb_func destroy, void *destroy_arg, + grpc_call_context_element *context, const void *transport_server_data, + gpr_timespec deadline, grpc_call_stack *call_stack) { grpc_channel_element *channel_elems = CHANNEL_ELEMS_FROM_STACK(channel_stack); grpc_call_element_args args; size_t count = channel_stack->count; @@ -184,6 +182,7 @@ grpc_error *grpc_call_stack_init(grpc_exec_ctx *exec_ctx, args.call_stack = call_stack; args.server_transport_data = transport_server_data; args.context = context; + args.deadline = deadline; call_elems[i].filter = channel_elems[i].filter; call_elems[i].channel_data = channel_elems[i].channel_data; call_elems[i].call_data = user_data; diff --git a/src/core/lib/channel/channel_stack.h b/src/core/lib/channel/channel_stack.h index eeaab17d39a..1cfe2885d81 100644 --- a/src/core/lib/channel/channel_stack.h +++ b/src/core/lib/channel/channel_stack.h @@ -74,6 +74,7 @@ typedef struct { grpc_call_stack *call_stack; const void *server_transport_data; grpc_call_context_element *context; + gpr_timespec deadline; } grpc_call_element_args; typedef struct { @@ -220,13 +221,11 @@ void grpc_channel_stack_destroy(grpc_exec_ctx *exec_ctx, /* Initialize a call stack given a channel stack. transport_server_data is expected to be NULL on a client, or an opaque transport owned pointer on the server. */ -grpc_error *grpc_call_stack_init(grpc_exec_ctx *exec_ctx, - grpc_channel_stack *channel_stack, - int initial_refs, grpc_iomgr_cb_func destroy, - void *destroy_arg, - grpc_call_context_element *context, - const void *transport_server_data, - grpc_call_stack *call_stack); +grpc_error *grpc_call_stack_init( + grpc_exec_ctx *exec_ctx, grpc_channel_stack *channel_stack, + int initial_refs, grpc_iomgr_cb_func destroy, void *destroy_arg, + grpc_call_context_element *context, const void *transport_server_data, + gpr_timespec deadline, grpc_call_stack *call_stack); /* Set a pollset or a pollset_set for a call stack: must occur before the first * op is started */ void grpc_call_stack_set_pollset_or_pollset_set(grpc_exec_ctx *exec_ctx, diff --git a/src/core/lib/channel/deadline_filter.c b/src/core/lib/channel/deadline_filter.c index 010fedd7b73..079b98a2f85 100644 --- a/src/core/lib/channel/deadline_filter.c +++ b/src/core/lib/channel/deadline_filter.c @@ -34,10 +34,12 @@ #include #include +#include #include #include #include +#include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/iomgr/timer.h" // @@ -106,15 +108,49 @@ static void inject_on_complete_cb(grpc_deadline_state* deadline_state, op->on_complete = &deadline_state->on_complete; } -void grpc_deadline_state_init(grpc_deadline_state* deadline_state, - grpc_call_stack* call_stack) { +// Callback and associated state for starting the timer after call stack +// initialization has been completed. +struct start_timer_after_init_state { + grpc_call_element* elem; + gpr_timespec deadline; + grpc_closure closure; +}; +static void start_timer_after_init(grpc_exec_ctx* exec_ctx, void* arg, + grpc_error* error) { + struct start_timer_after_init_state* state = arg; + start_timer_if_needed(exec_ctx, state->elem, state->deadline); + gpr_free(state); +} + +void grpc_deadline_state_init(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, + grpc_call_element_args* args) { + grpc_deadline_state* deadline_state = elem->call_data; memset(deadline_state, 0, sizeof(*deadline_state)); - deadline_state->call_stack = call_stack; + deadline_state->call_stack = args->call_stack; gpr_mu_init(&deadline_state->timer_mu); + // Deadline will always be infinite on servers, so the timer will only be + // set on clients with a finite deadline. + const gpr_timespec deadline = + gpr_convert_clock_type(args->deadline, GPR_CLOCK_MONOTONIC); + if (gpr_time_cmp(deadline, gpr_inf_future(GPR_CLOCK_MONOTONIC)) != 0) { + // When the deadline passes, we indicate the failure by sending down + // an op with cancel_error set. However, we can't send down any ops + // until after the call stack is fully initialized. If we start the + // timer here, we have no guarantee that the timer won't pop before + // call stack initialization is finished. To avoid that problem, we + // create a closure to start the timer, and we schedule that closure + // to be run after call stack initialization is done. + struct start_timer_after_init_state* state = gpr_malloc(sizeof(*state)); + state->elem = elem; + state->deadline = deadline; + grpc_closure_init(&state->closure, start_timer_after_init, state); + grpc_exec_ctx_sched(exec_ctx, &state->closure, GRPC_ERROR_NONE, NULL); + } } void grpc_deadline_state_destroy(grpc_exec_ctx* exec_ctx, - grpc_deadline_state* deadline_state) { + grpc_call_element* elem) { + grpc_deadline_state* deadline_state = elem->call_data; cancel_timer_if_needed(exec_ctx, deadline_state); gpr_mu_destroy(&deadline_state->timer_mu); } @@ -127,12 +163,6 @@ void grpc_deadline_state_client_start_transport_stream_op( op->close_error != GRPC_ERROR_NONE) { cancel_timer_if_needed(exec_ctx, deadline_state); } else { - // If we're sending initial metadata, get the deadline from the metadata - // and start the timer if needed. - if (op->send_initial_metadata != NULL) { - start_timer_if_needed(exec_ctx, elem, - op->send_initial_metadata->deadline); - } // Make sure we know when the call is complete, so that we can cancel // the timer. if (op->recv_trailing_metadata != NULL) { @@ -177,10 +207,9 @@ typedef struct server_call_data { static grpc_error* init_call_elem(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, grpc_call_element_args* args) { - base_call_data* calld = elem->call_data; // Note: size of call data is different between client and server. - memset(calld, 0, elem->filter->sizeof_call_data); - grpc_deadline_state_init(&calld->deadline_state, args->call_stack); + memset(elem->call_data, 0, elem->filter->sizeof_call_data); + grpc_deadline_state_init(exec_ctx, elem, args); return GRPC_ERROR_NONE; } @@ -188,8 +217,7 @@ static grpc_error* init_call_elem(grpc_exec_ctx* exec_ctx, static void destroy_call_elem(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, const grpc_call_final_info* final_info, void* and_free_memory) { - base_call_data* calld = elem->call_data; - grpc_deadline_state_destroy(exec_ctx, &calld->deadline_state); + grpc_deadline_state_destroy(exec_ctx, elem); } // Method for starting a call op for client filter. diff --git a/src/core/lib/channel/deadline_filter.h b/src/core/lib/channel/deadline_filter.h index a09f85afe6b..685df877617 100644 --- a/src/core/lib/channel/deadline_filter.h +++ b/src/core/lib/channel/deadline_filter.h @@ -36,7 +36,7 @@ #include "src/core/lib/iomgr/timer.h" // State used for filters that enforce call deadlines. -// Should be the first field in the filter's call_data. +// Must be the first field in the filter's call_data. typedef struct grpc_deadline_state { // We take a reference to the call stack for the timer callback. grpc_call_stack* call_stack; @@ -54,16 +54,18 @@ typedef struct grpc_deadline_state { grpc_closure* next_on_complete; } grpc_deadline_state; -void grpc_deadline_state_init(grpc_deadline_state* call_data, - grpc_call_stack* call_stack); -void grpc_deadline_state_destroy(grpc_exec_ctx* exec_ctx, - grpc_deadline_state* call_data); - -// To be used in a filter's start_transport_stream_op() method to -// enforce call deadlines. -// It is the caller's responsibility to chain to the next filter if -// necessary after this function returns. +// To be used in a filter's init_call_elem(), destroy_call_elem(), and +// start_transport_stream_op() methods to enforce call deadlines. +// // REQUIRES: The first field in elem->call_data is a grpc_deadline_state. +// +// For grpc_deadline_state_client_start_transport_stream_op(), it is the +// caller's responsibility to chain to the next filter if necessary +// after the function returns. +void grpc_deadline_state_init(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, + grpc_call_element_args* args); +void grpc_deadline_state_destroy(grpc_exec_ctx* exec_ctx, + grpc_call_element* elem); void grpc_deadline_state_client_start_transport_stream_op( grpc_exec_ctx* exec_ctx, grpc_call_element* elem, grpc_transport_stream_op* op); diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c index 0ced42a5717..f8b7c9bf90d 100644 --- a/src/core/lib/surface/call.c +++ b/src/core/lib/surface/call.c @@ -254,34 +254,7 @@ grpc_call *grpc_call_create( } } send_deadline = gpr_convert_clock_type(send_deadline, GPR_CLOCK_MONOTONIC); - GRPC_CHANNEL_INTERNAL_REF(channel, "call"); - /* initial refcount dropped by grpc_call_destroy */ - grpc_error *error = grpc_call_stack_init( - &exec_ctx, channel_stack, 1, destroy_call, call, call->context, - server_transport_data, CALL_STACK_FROM_CALL(call)); - if (error != GRPC_ERROR_NONE) { - grpc_status_code status; - const char *error_str; - grpc_error_get_status(error, &status, &error_str); - close_with_status(&exec_ctx, call, status, error_str); - GRPC_ERROR_UNREF(error); - } - if (cq != NULL) { - GPR_ASSERT( - pollset_set_alternative == NULL && - "Only one of 'cq' and 'pollset_set_alternative' should be non-NULL."); - GRPC_CQ_INTERNAL_REF(cq, "bind"); - call->pollent = - grpc_polling_entity_create_from_pollset(grpc_cq_pollset(cq)); - } - if (pollset_set_alternative != NULL) { - call->pollent = - grpc_polling_entity_create_from_pollset_set(pollset_set_alternative); - } - if (!grpc_polling_entity_is_empty(&call->pollent)) { - grpc_call_stack_set_pollset_or_pollset_set( - &exec_ctx, CALL_STACK_FROM_CALL(call), &call->pollent); - } + if (parent_call != NULL) { GRPC_CALL_INTERNAL_REF(parent_call, "child"); GPR_ASSERT(call->is_client); @@ -323,7 +296,38 @@ grpc_call *grpc_call_create( gpr_mu_unlock(&parent_call->mu); } + call->send_deadline = send_deadline; + + GRPC_CHANNEL_INTERNAL_REF(channel, "call"); + /* initial refcount dropped by grpc_call_destroy */ + grpc_error *error = grpc_call_stack_init( + &exec_ctx, channel_stack, 1, destroy_call, call, call->context, + server_transport_data, send_deadline, CALL_STACK_FROM_CALL(call)); + if (error != GRPC_ERROR_NONE) { + grpc_status_code status; + const char *error_str; + grpc_error_get_status(error, &status, &error_str); + close_with_status(&exec_ctx, call, status, error_str); + GRPC_ERROR_UNREF(error); + } + if (cq != NULL) { + GPR_ASSERT( + pollset_set_alternative == NULL && + "Only one of 'cq' and 'pollset_set_alternative' should be non-NULL."); + GRPC_CQ_INTERNAL_REF(cq, "bind"); + call->pollent = + grpc_polling_entity_create_from_pollset(grpc_cq_pollset(cq)); + } + if (pollset_set_alternative != NULL) { + call->pollent = + grpc_polling_entity_create_from_pollset_set(pollset_set_alternative); + } + if (!grpc_polling_entity_is_empty(&call->pollent)) { + grpc_call_stack_set_pollset_or_pollset_set( + &exec_ctx, CALL_STACK_FROM_CALL(call), &call->pollent); + } + grpc_exec_ctx_finish(&exec_ctx); GPR_TIMER_END("grpc_call_create", 0); return call; @@ -1220,8 +1224,8 @@ static void receiving_initial_metadata_ready(grpc_exec_ctx *exec_ctx, if (gpr_time_cmp(md->deadline, gpr_inf_future(md->deadline.clock_type)) != 0 && !call->is_client) { - call->send_deadline = gpr_convert_clock_type(md->deadline, - GPR_CLOCK_MONOTONIC); + call->send_deadline = + gpr_convert_clock_type(md->deadline, GPR_CLOCK_MONOTONIC); } } @@ -1250,6 +1254,14 @@ static void finish_batch(grpc_exec_ctx *exec_ctx, void *bctlp, GRPC_ERROR_REF(error); gpr_mu_lock(&call->mu); + + // If the error has an associated status code, set the call's status. + intptr_t status; + if (error != GRPC_ERROR_NONE && + grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, &status)) { + set_status_from_error(call, STATUS_FROM_CORE, error); + } + if (bctl->send_initial_metadata) { if (error != GRPC_ERROR_NONE) { set_status_from_error(call, STATUS_FROM_CORE, error); diff --git a/test/core/end2end/invalid_call_argument_test.c b/test/core/end2end/invalid_call_argument_test.c index dd2147e6097..2b9904a2441 100644 --- a/test/core/end2end/invalid_call_argument_test.c +++ b/test/core/end2end/invalid_call_argument_test.c @@ -304,11 +304,6 @@ static void test_receive_initial_metadata_twice_at_client() { grpc_op *op; prepare_test(1); op = g_state.ops; - op->op = GRPC_OP_SEND_INITIAL_METADATA; - op->data.send_initial_metadata.count = 0; - op->flags = 0; - op->reserved = NULL; - op++; op->op = GRPC_OP_RECV_INITIAL_METADATA; op->data.recv_initial_metadata = &g_state.initial_metadata_recv; op->flags = 0; @@ -397,11 +392,6 @@ static void test_recv_status_on_client_twice() { prepare_test(1); op = g_state.ops; - op->op = GRPC_OP_SEND_INITIAL_METADATA; - op->data.send_initial_metadata.count = 0; - op->flags = 0; - op->reserved = NULL; - op++; op->op = GRPC_OP_RECV_STATUS_ON_CLIENT; op->data.recv_status_on_client.trailing_metadata = &g_state.trailing_metadata_recv; diff --git a/test/core/end2end/tests/negative_deadline.c b/test/core/end2end/tests/negative_deadline.c index 36632f6a5dd..c999ac116aa 100644 --- a/test/core/end2end/tests/negative_deadline.c +++ b/test/core/end2end/tests/negative_deadline.c @@ -122,11 +122,6 @@ static void simple_request_body(grpc_end2end_test_fixture f, size_t num_ops) { memset(ops, 0, sizeof(ops)); op = ops; - op->op = GRPC_OP_SEND_INITIAL_METADATA; - op->data.send_initial_metadata.count = 0; - op->flags = 0; - op->reserved = NULL; - op++; op->op = GRPC_OP_RECV_STATUS_ON_CLIENT; op->data.recv_status_on_client.trailing_metadata = &trailing_metadata_recv; op->data.recv_status_on_client.status = &status; @@ -140,14 +135,15 @@ static void simple_request_body(grpc_end2end_test_fixture f, size_t num_ops) { op->flags = 0; op->reserved = NULL; op++; + op->op = GRPC_OP_SEND_INITIAL_METADATA; + op->data.send_initial_metadata.count = 0; + op->flags = 0; + op->reserved = NULL; + op++; op->op = GRPC_OP_SEND_CLOSE_FROM_CLIENT; op->flags = 0; op->reserved = NULL; op++; - // Need to send at least the SEND_INITIAL_METADATA and - // RECV_STATUS_ON_CLIENT ops, since the former allows the client to set - // the deadline timer, and the latter returns status to the test. - GPR_ASSERT(num_ops >= 2); GPR_ASSERT(num_ops <= (size_t)(op - ops)); error = grpc_call_start_batch(c, ops, num_ops, tag(1), NULL); GPR_ASSERT(GRPC_CALL_OK == error); @@ -178,7 +174,7 @@ static void test_invoke_simple_request(grpc_end2end_test_config config, void negative_deadline(grpc_end2end_test_config config) { size_t i; - for (i = 2; i <= 4; i++) { + for (i = 1; i <= 4; i++) { test_invoke_simple_request(config, i); } } From 6e3084b02ce806d57225d6404a7866349e98f5c7 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 15 Sep 2016 08:23:18 -0700 Subject: [PATCH 24/26] Fix build failure. --- test/core/channel/channel_stack_test.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/core/channel/channel_stack_test.c b/test/core/channel/channel_stack_test.c index 569b3f7cd23..372cf1cf892 100644 --- a/test/core/channel/channel_stack_test.c +++ b/test/core/channel/channel_stack_test.c @@ -137,7 +137,8 @@ static void test_create_channel_stack(void) { call_stack = gpr_malloc(channel_stack->call_stack_size); grpc_error *error = grpc_call_stack_init(&exec_ctx, channel_stack, 1, free_call, call_stack, - NULL, NULL, call_stack); + NULL, NULL, gpr_inf_future(GPR_CLOCK_MONOTONIC), + call_stack); GPR_ASSERT(error == GRPC_ERROR_NONE); GPR_ASSERT(call_stack->count == 1); call_elem = grpc_call_stack_element(call_stack, 0); From 590007f341288270edce7f80f76e726b7c2d148b Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 16 Sep 2016 13:26:03 -0700 Subject: [PATCH 25/26] clang-format --- test/core/channel/channel_stack_test.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/test/core/channel/channel_stack_test.c b/test/core/channel/channel_stack_test.c index 372cf1cf892..b1c1ed9039e 100644 --- a/test/core/channel/channel_stack_test.c +++ b/test/core/channel/channel_stack_test.c @@ -135,10 +135,9 @@ static void test_create_channel_stack(void) { GPR_ASSERT(*channel_data == 0); call_stack = gpr_malloc(channel_stack->call_stack_size); - grpc_error *error = - grpc_call_stack_init(&exec_ctx, channel_stack, 1, free_call, call_stack, - NULL, NULL, gpr_inf_future(GPR_CLOCK_MONOTONIC), - call_stack); + grpc_error *error = grpc_call_stack_init( + &exec_ctx, channel_stack, 1, free_call, call_stack, NULL, NULL, + gpr_inf_future(GPR_CLOCK_MONOTONIC), call_stack); GPR_ASSERT(error == GRPC_ERROR_NONE); GPR_ASSERT(call_stack->count == 1); call_elem = grpc_call_stack_element(call_stack, 0); From 219db50d39c2b5754e86838a56d7c8b0bc7273df Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 20 Sep 2016 08:13:16 -0700 Subject: [PATCH 26/26] Fix message size filter to use close instead of cancel. --- 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 6613785deaa..a254fe3997f 100644 --- a/src/core/lib/channel/message_size_filter.c +++ b/src/core/lib/channel/message_size_filter.c @@ -74,7 +74,7 @@ static void recv_message_ready(grpc_exec_ctx* exec_ctx, void* user_data, (*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( + grpc_call_element_send_close_with_message( exec_ctx, elem, GRPC_STATUS_INVALID_ARGUMENT, &message); } // Invoke the next callback. @@ -95,7 +95,7 @@ static void start_transport_stream_op(grpc_exec_ctx* exec_ctx, 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( + grpc_call_element_send_close_with_message( exec_ctx, elem, GRPC_STATUS_INVALID_ARGUMENT, &message); } // Inject callback for receiving a message.