From 9d01848ef2a933d50e5d7ed95598788614d70bc8 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 18 Jul 2016 08:53:49 -0700 Subject: [PATCH] Fixes --- .../client_config/subchannel_call_holder.c | 4 ++++ .../chttp2/transport/chttp2_transport.c | 4 ++-- .../ext/transport/chttp2/transport/writing.c | 19 ++++++++++--------- src/core/lib/iomgr/ev_epoll_linux.c | 4 ++++ .../security/transport/client_auth_filter.c | 5 +++++ .../lib/security/transport/secure_endpoint.c | 5 +++++ src/core/lib/surface/channel_ping.c | 2 ++ src/core/lib/surface/lame_client.c | 6 +++--- src/core/lib/surface/server.c | 7 +++++-- test/core/surface/lame_client_test.c | 6 +++--- 10 files changed, 43 insertions(+), 19 deletions(-) diff --git a/src/core/ext/client_config/subchannel_call_holder.c b/src/core/ext/client_config/subchannel_call_holder.c index 7eb17c713a7..b10b204ff26 100644 --- a/src/core/ext/client_config/subchannel_call_holder.c +++ b/src/core/ext/client_config/subchannel_call_holder.c @@ -207,6 +207,10 @@ typedef struct { static void retry_waiting_locked(grpc_exec_ctx *exec_ctx, grpc_subchannel_call_holder *holder) { + if (holder->waiting_ops_count == 0) { + return; + } + retry_ops_args *a = gpr_malloc(sizeof(*a)); a->ops = holder->waiting_ops; a->nops = holder->waiting_ops_count; diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index 98b01426a4c..edb5346dbec 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -311,7 +311,7 @@ static void init_transport(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, if (is_client) { gpr_slice_buffer_add( - &t->global.qbuf, + &t->writing.outbuf, gpr_slice_from_copied_string(GRPC_CHTTP2_CLIENT_CONNECT_STRING)); grpc_chttp2_initiate_write(exec_ctx, &t->global, false, "initial_write"); } @@ -768,7 +768,7 @@ static void start_writing(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t) { set_write_state(t, GRPC_CHTTP2_WRITING_INACTIVE, "start_writing:nothing_to_write"); } - end_waiting_for_write(exec_ctx, t, GRPC_ERROR_CREATE("Nothing to write")); + end_waiting_for_write(exec_ctx, t, GRPC_ERROR_NONE); UNREF_TRANSPORT(exec_ctx, t, "writing"); } GPR_TIMER_END("start_writing", 0); diff --git a/src/core/ext/transport/chttp2/transport/writing.c b/src/core/ext/transport/chttp2/transport/writing.c index ad3441cfeb5..9e6555cdd3d 100644 --- a/src/core/ext/transport/chttp2/transport/writing.c +++ b/src/core/ext/transport/chttp2/transport/writing.c @@ -51,15 +51,6 @@ int grpc_chttp2_unlocking_check_writes( GPR_TIMER_BEGIN("grpc_chttp2_unlocking_check_writes", 0); - /* simple writes are queued to qbuf, and flushed here */ - gpr_slice_buffer_swap(&transport_global->qbuf, &transport_writing->outbuf); - GPR_ASSERT(transport_global->qbuf.count == 0); - - grpc_chttp2_hpack_compressor_set_max_table_size( - &transport_writing->hpack_compressor, - transport_global->settings[GRPC_PEER_SETTINGS] - [GRPC_CHTTP2_SETTINGS_HEADER_TABLE_SIZE]); - if (transport_global->dirtied_local_settings && !transport_global->sent_local_settings) { gpr_slice_buffer_add( @@ -73,6 +64,16 @@ int grpc_chttp2_unlocking_check_writes( transport_global->sent_local_settings = 1; } + /* simple writes are queued to qbuf, and flushed here */ + gpr_slice_buffer_move_into(&transport_global->qbuf, + &transport_writing->outbuf); + GPR_ASSERT(transport_global->qbuf.count == 0); + + grpc_chttp2_hpack_compressor_set_max_table_size( + &transport_writing->hpack_compressor, + transport_global->settings[GRPC_PEER_SETTINGS] + [GRPC_CHTTP2_SETTINGS_HEADER_TABLE_SIZE]); + GRPC_CHTTP2_FLOW_MOVE_TRANSPORT("write", transport_writing, outgoing_window, transport_global, outgoing_window); if (transport_writing->outgoing_window > 0) { diff --git a/src/core/lib/iomgr/ev_epoll_linux.c b/src/core/lib/iomgr/ev_epoll_linux.c index 6a63c4d1d18..57c26138be8 100644 --- a/src/core/lib/iomgr/ev_epoll_linux.c +++ b/src/core/lib/iomgr/ev_epoll_linux.c @@ -1526,6 +1526,8 @@ static grpc_error *pollset_work(grpc_exec_ctx *exec_ctx, grpc_pollset *pollset, static void pollset_add_fd(grpc_exec_ctx *exec_ctx, grpc_pollset *pollset, grpc_fd *fd) { + GPR_TIMER_BEGIN("pollset_add_fd", 0); + grpc_error *error = GRPC_ERROR_NONE; gpr_mu_lock(&pollset->mu); @@ -1638,6 +1640,8 @@ retry: gpr_mu_unlock(&pollset->mu); GRPC_LOG_IF_ERROR("pollset_add_fd", error); + + GPR_TIMER_END("pollset_add_fd", 0); } /******************************************************************************* diff --git a/src/core/lib/security/transport/client_auth_filter.c b/src/core/lib/security/transport/client_auth_filter.c index ed7929aa27e..b13bd55df17 100644 --- a/src/core/lib/security/transport/client_auth_filter.c +++ b/src/core/lib/security/transport/client_auth_filter.c @@ -40,6 +40,7 @@ #include #include "src/core/lib/channel/channel_stack.h" +#include "src/core/lib/profiling/timers.h" #include "src/core/lib/security/context/security_context.h" #include "src/core/lib/security/credentials/credentials.h" #include "src/core/lib/security/transport/security_connector.h" @@ -218,6 +219,8 @@ static void on_host_checked(grpc_exec_ctx *exec_ctx, void *user_data, static void auth_start_transport_op(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, grpc_transport_stream_op *op) { + GPR_TIMER_BEGIN("auth_start_transport_op", 0); + /* grab pointers to our data from the call element */ call_data *calld = elem->call_data; channel_data *chand = elem->channel_data; @@ -258,12 +261,14 @@ static void auth_start_transport_op(grpc_exec_ctx *exec_ctx, grpc_channel_security_connector_check_call_host( exec_ctx, chand->security_connector, call_host, chand->auth_context, on_host_checked, elem); + GPR_TIMER_END("auth_start_transport_op", 0); return; /* early exit */ } } /* pass control down the stack */ grpc_call_next_op(exec_ctx, elem, op); + GPR_TIMER_END("auth_start_transport_op", 0); } /* Constructor for call_data */ diff --git a/src/core/lib/security/transport/secure_endpoint.c b/src/core/lib/security/transport/secure_endpoint.c index 0169ccd9ef9..acb0113ea8c 100644 --- a/src/core/lib/security/transport/secure_endpoint.c +++ b/src/core/lib/security/transport/secure_endpoint.c @@ -38,6 +38,7 @@ #include #include #include "src/core/lib/debug/trace.h" +#include "src/core/lib/profiling/timers.h" #include "src/core/lib/security/transport/tsi_error.h" #include "src/core/lib/support/string.h" #include "src/core/lib/tsi/transport_security_interface.h" @@ -248,6 +249,8 @@ static void flush_write_staging_buffer(secure_endpoint *ep, uint8_t **cur, static void endpoint_write(grpc_exec_ctx *exec_ctx, grpc_endpoint *secure_ep, gpr_slice_buffer *slices, grpc_closure *cb) { + GPR_TIMER_BEGIN("secure_endpoint.endpoint_write", 0); + unsigned i; tsi_result result = TSI_OK; secure_endpoint *ep = (secure_endpoint *)secure_ep; @@ -323,10 +326,12 @@ static void endpoint_write(grpc_exec_ctx *exec_ctx, grpc_endpoint *secure_ep, exec_ctx, cb, grpc_set_tsi_error_result(GRPC_ERROR_CREATE("Wrap failed"), result), NULL); + GPR_TIMER_END("secure_endpoint.endpoint_write", 0); return; } grpc_endpoint_write(exec_ctx, ep->wrapped_ep, &ep->output_buffer, cb); + GPR_TIMER_END("secure_endpoint.endpoint_write", 0); } static void endpoint_shutdown(grpc_exec_ctx *exec_ctx, diff --git a/src/core/lib/surface/channel_ping.c b/src/core/lib/surface/channel_ping.c index 5f9f07f89a4..0d2f01a6492 100644 --- a/src/core/lib/surface/channel_ping.c +++ b/src/core/lib/surface/channel_ping.c @@ -61,6 +61,8 @@ static void ping_done(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { void grpc_channel_ping(grpc_channel *channel, grpc_completion_queue *cq, void *tag, void *reserved) { + GRPC_API_TRACE("grpc_channel_ping(channel=%p, cq=%p, tag=%p, reserved=%p)", 4, + (channel, cq, tag, reserved)); grpc_transport_op *op = grpc_make_transport_op(NULL); ping_result *pr = gpr_malloc(sizeof(*pr)); grpc_channel_element *top_elem = diff --git a/src/core/lib/surface/lame_client.c b/src/core/lib/surface/lame_client.c index 5ea4cba5d1a..203d614b310 100644 --- a/src/core/lib/surface/lame_client.c +++ b/src/core/lib/surface/lame_client.c @@ -97,14 +97,14 @@ static void lame_start_transport_op(grpc_exec_ctx *exec_ctx, grpc_exec_ctx_sched(exec_ctx, op->on_connectivity_state_change, GRPC_ERROR_NONE, NULL); } - if (op->on_consumed != NULL) { - grpc_exec_ctx_sched(exec_ctx, op->on_consumed, GRPC_ERROR_NONE, NULL); - } if (op->send_ping != NULL) { grpc_exec_ctx_sched(exec_ctx, op->send_ping, GRPC_ERROR_CREATE("lame client channel"), NULL); } GRPC_ERROR_UNREF(op->disconnect_with_error); + if (op->on_consumed != NULL) { + grpc_exec_ctx_sched(exec_ctx, op->on_consumed, GRPC_ERROR_NONE, NULL); + } } static void init_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, diff --git a/src/core/lib/surface/server.c b/src/core/lib/surface/server.c index e0b5ed62978..b6f4b5e427b 100644 --- a/src/core/lib/surface/server.c +++ b/src/core/lib/surface/server.c @@ -428,7 +428,8 @@ static void finish_destroy_channel(grpc_exec_ctx *exec_ctx, void *cd, server_unref(exec_ctx, server); } -static void destroy_channel(grpc_exec_ctx *exec_ctx, channel_data *chand) { +static void destroy_channel(grpc_exec_ctx *exec_ctx, channel_data *chand, + grpc_error *error) { if (is_channel_orphaned(chand)) return; GPR_ASSERT(chand->server != NULL); orphan_channel(chand); @@ -444,6 +445,8 @@ static void destroy_channel(grpc_exec_ctx *exec_ctx, channel_data *chand) { grpc_channel_stack_element( grpc_channel_get_channel_stack(chand->channel), 0), op); + + GRPC_LOG_IF_ERROR("disconnecting client", error); } static void cpstr(char **dest, size_t *capacity, grpc_mdstr *value) { @@ -845,7 +848,7 @@ static void channel_connectivity_changed(grpc_exec_ctx *exec_ctx, void *cd, op); } else { gpr_mu_lock(&server->mu_global); - destroy_channel(exec_ctx, chand); + destroy_channel(exec_ctx, chand, GRPC_ERROR_REF(error)); gpr_mu_unlock(&server->mu_global); GRPC_CHANNEL_INTERNAL_UNREF(exec_ctx, chand->channel, "connectivity"); } diff --git a/test/core/surface/lame_client_test.c b/test/core/surface/lame_client_test.c index cad45cc9fa1..68c1e0eab02 100644 --- a/test/core/surface/lame_client_test.c +++ b/test/core/surface/lame_client_test.c @@ -49,8 +49,8 @@ static void *tag(intptr_t x) { return (void *)x; } void verify_connectivity(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { - grpc_transport_op *op = arg; - GPR_ASSERT(GRPC_CHANNEL_SHUTDOWN == *op->connectivity_state); + grpc_connectivity_state *state = arg; + GPR_ASSERT(GRPC_CHANNEL_SHUTDOWN == *state); GPR_ASSERT(error == GRPC_ERROR_NONE); } @@ -62,7 +62,7 @@ void test_transport_op(grpc_channel *channel) { grpc_connectivity_state state = GRPC_CHANNEL_IDLE; grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; - grpc_closure_init(&transport_op_cb, verify_connectivity, &op); + grpc_closure_init(&transport_op_cb, verify_connectivity, &state); op = grpc_make_transport_op(NULL); op->on_connectivity_state_change = &transport_op_cb;