From b4b6a0e5e1a6077e69dcf51f1d7e42cf6d1be8bd Mon Sep 17 00:00:00 2001 From: Yuchen Zeng Date: Tue, 28 Feb 2017 22:06:52 -0800 Subject: [PATCH 1/5] Retry sending pings if they are delayed --- .../chttp2/transport/chttp2_transport.c | 14 +++++++++++--- .../ext/transport/chttp2/transport/internal.h | 2 ++ .../ext/transport/chttp2/transport/writing.c | 6 ++++++ test/core/end2end/tests/ping.c | 18 +++++++++++++----- 4 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index da4c7dc7b23..96e46c29ca7 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -142,6 +142,8 @@ static void send_ping_locked(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, grpc_chttp2_ping_type ping_type, grpc_closure *on_initiate, grpc_closure *on_complete); +static void retry_initiate_ping_locked(grpc_exec_ctx *exec_ctx, void *tp, + grpc_error *error); #define DEFAULT_MIN_TIME_BETWEEN_PINGS_MS 0 #define DEFAULT_MAX_PINGS_BETWEEN_DATA 3 @@ -266,6 +268,8 @@ static void init_transport(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, grpc_closure_init(&t->destructive_reclaimer_locked, destructive_reclaimer_locked, t, grpc_combiner_scheduler(t->combiner, false)); + grpc_closure_init(&t->retry_initiate_ping_locked, retry_initiate_ping_locked, + t, grpc_combiner_scheduler(t->combiner, false)); grpc_closure_init(&t->start_bdp_ping_locked, start_bdp_ping_locked, t, grpc_combiner_scheduler(t->combiner, false)); grpc_closure_init(&t->finish_bdp_ping_locked, finish_bdp_ping_locked, t, @@ -1388,6 +1392,12 @@ static void send_ping_locked(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, } } +static void retry_initiate_ping_locked(grpc_exec_ctx *exec_ctx, void *tp, + grpc_error *error) { + grpc_chttp2_transport *t = tp; + grpc_chttp2_initiate_write(exec_ctx, t, false, "retry_send_ping"); +} + void grpc_chttp2_ack_ping(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, uint64_t id) { grpc_chttp2_ping_queue *pq = @@ -2114,9 +2124,7 @@ static void finish_keepalive_ping_locked(grpc_exec_ctx *exec_ctx, void *arg, grpc_timer_init( exec_ctx, &t->keepalive_ping_timer, gpr_time_add(gpr_now(GPR_CLOCK_MONOTONIC), t->keepalive_time), - grpc_closure_create(init_keepalive_ping_locked, t, - grpc_combiner_scheduler(t->combiner, false)), - gpr_now(GPR_CLOCK_MONOTONIC)); + &t->init_keepalive_ping_locked, gpr_now(GPR_CLOCK_MONOTONIC)); } } GRPC_CHTTP2_UNREF_TRANSPORT(exec_ctx, t, "keepalive ping end"); diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index d26812ad6be..a23aa60dbdb 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -102,6 +102,7 @@ typedef struct { typedef struct { gpr_timespec last_ping_sent_time; int pings_before_data_required; + grpc_timer delayed_ping_timer; } grpc_chttp2_repeated_ping_state; /* deframer state for the overall http2 stream of bytes */ @@ -308,6 +309,7 @@ struct grpc_chttp2_transport { grpc_chttp2_repeated_ping_policy ping_policy; grpc_chttp2_repeated_ping_state ping_state; uint64_t ping_ctr; /* unique id for pings */ + grpc_closure retry_initiate_ping_locked; /** ping acks */ size_t ping_ack_count; diff --git a/src/core/ext/transport/chttp2/transport/writing.c b/src/core/ext/transport/chttp2/transport/writing.c index 2b9d93cae7c..b9c2f15face 100644 --- a/src/core/ext/transport/chttp2/transport/writing.c +++ b/src/core/ext/transport/chttp2/transport/writing.c @@ -101,6 +101,12 @@ static void maybe_initiate_ping(grpc_exec_ctx *exec_ctx, "Ping delayed [%p]: not enough time elapsed since last ping", t->peer_string); } + + grpc_timer_init(exec_ctx, &t->ping_state.delayed_ping_timer, + gpr_time_add(t->ping_state.last_ping_sent_time, + t->ping_policy.min_time_between_pings), + &t->retry_initiate_ping_locked, + gpr_now(GPR_CLOCK_MONOTONIC)); return; } /* coalesce equivalent pings into this one */ diff --git a/test/core/end2end/tests/ping.c b/test/core/end2end/tests/ping.c index f5bfac2255c..082ac641f0b 100644 --- a/test/core/end2end/tests/ping.c +++ b/test/core/end2end/tests/ping.c @@ -41,9 +41,12 @@ #include "test/core/end2end/cq_verifier.h" +#define PING_NUM 5 + static void *tag(intptr_t t) { return (void *)t; } -static void test_ping(grpc_end2end_test_config config) { +static void test_ping(grpc_end2end_test_config config, + int min_time_between_pings_ms) { grpc_end2end_test_fixture f = config.create_fixture(NULL, NULL); cq_verifier *cqv = cq_verifier_create(f.cq); grpc_connectivity_state state = GRPC_CHANNEL_IDLE; @@ -51,7 +54,7 @@ static void test_ping(grpc_end2end_test_config config) { grpc_arg a[] = {{.type = GRPC_ARG_INTEGER, .key = GRPC_ARG_HTTP2_MIN_TIME_BETWEEN_PINGS_MS, - .value.integer = 0}, + .value.integer = min_time_between_pings_ms}, {.type = GRPC_ARG_INTEGER, .key = GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA, .value.integer = 20}}; @@ -70,7 +73,11 @@ static void test_ping(grpc_end2end_test_config config) { READY is reached */ while (state != GRPC_CHANNEL_READY) { grpc_channel_watch_connectivity_state( - f.client, state, grpc_timeout_seconds_to_deadline(3), f.cq, tag(99)); + f.client, state, + gpr_time_add(grpc_timeout_seconds_to_deadline(3), + gpr_time_from_millis(min_time_between_pings_ms * PING_NUM, + GPR_TIMESPAN)), + f.cq, tag(99)); CQ_EXPECT_COMPLETION(cqv, tag(99), 1); cq_verify(cqv); state = grpc_channel_check_connectivity_state(f.client, 0); @@ -79,7 +86,7 @@ static void test_ping(grpc_end2end_test_config config) { state == GRPC_CHANNEL_TRANSIENT_FAILURE); } - for (i = 1; i <= 5; i++) { + for (i = 1; i <= PING_NUM; i++) { grpc_channel_ping(f.client, f.cq, tag(i), NULL); CQ_EXPECT_COMPLETION(cqv, tag(i), 1); cq_verify(cqv); @@ -102,7 +109,8 @@ static void test_ping(grpc_end2end_test_config config) { void ping(grpc_end2end_test_config config) { GPR_ASSERT(config.feature_mask & FEATURE_MASK_SUPPORTS_DELAYED_CONNECTION); - test_ping(config); + test_ping(config, 0); + test_ping(config, 100); } void ping_pre_init(void) {} From cbcdb4023f8e2fecaae3ec4abb21974fbe75024e Mon Sep 17 00:00:00 2001 From: Yuchen Zeng Date: Wed, 1 Mar 2017 01:42:18 -0800 Subject: [PATCH 2/5] Fix mac windows failures --- .../transport/chttp2/transport/chttp2_transport.c | 2 ++ src/core/ext/transport/chttp2/transport/internal.h | 1 + src/core/ext/transport/chttp2/transport/writing.c | 14 ++++++++------ 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index 96e46c29ca7..024d9d649f8 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -477,6 +477,7 @@ static void init_transport(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, t->ping_state.pings_before_data_required = t->ping_policy.max_pings_without_data; + t->ping_state.is_delayed_ping_timer_set = false; /** Start client-side keepalive pings */ if (t->is_client) { @@ -1395,6 +1396,7 @@ static void send_ping_locked(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, static void retry_initiate_ping_locked(grpc_exec_ctx *exec_ctx, void *tp, grpc_error *error) { grpc_chttp2_transport *t = tp; + t->ping_state.is_delayed_ping_timer_set = false; grpc_chttp2_initiate_write(exec_ctx, t, false, "retry_send_ping"); } diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index a23aa60dbdb..bfbfd5fd97c 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -103,6 +103,7 @@ typedef struct { gpr_timespec last_ping_sent_time; int pings_before_data_required; grpc_timer delayed_ping_timer; + bool is_delayed_ping_timer_set; } grpc_chttp2_repeated_ping_state; /* deframer state for the overall http2 stream of bytes */ diff --git a/src/core/ext/transport/chttp2/transport/writing.c b/src/core/ext/transport/chttp2/transport/writing.c index b9c2f15face..0869056f56d 100644 --- a/src/core/ext/transport/chttp2/transport/writing.c +++ b/src/core/ext/transport/chttp2/transport/writing.c @@ -101,12 +101,14 @@ static void maybe_initiate_ping(grpc_exec_ctx *exec_ctx, "Ping delayed [%p]: not enough time elapsed since last ping", t->peer_string); } - - grpc_timer_init(exec_ctx, &t->ping_state.delayed_ping_timer, - gpr_time_add(t->ping_state.last_ping_sent_time, - t->ping_policy.min_time_between_pings), - &t->retry_initiate_ping_locked, - gpr_now(GPR_CLOCK_MONOTONIC)); + if (!t->ping_state.is_delayed_ping_timer_set) { + t->ping_state.is_delayed_ping_timer_set = true; + grpc_timer_init(exec_ctx, &t->ping_state.delayed_ping_timer, + gpr_time_add(t->ping_state.last_ping_sent_time, + t->ping_policy.min_time_between_pings), + &t->retry_initiate_ping_locked, + gpr_now(GPR_CLOCK_MONOTONIC)); + } return; } /* coalesce equivalent pings into this one */ From 9f00073f3cf61b5d12814d2290da7a344b6e0068 Mon Sep 17 00:00:00 2001 From: thinkerou Date: Sun, 26 Mar 2017 00:52:56 +0800 Subject: [PATCH 3/5] fix markdown render error --- src/compiler/README.md | 2 +- src/core/README.md | 2 +- src/core/ext/census/gen/README.md | 2 +- src/cpp/README.md | 16 ++++++++-------- src/php/README.md | 2 +- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/compiler/README.md b/src/compiler/README.md index a2f49b3cd53..d5684af7ff2 100644 --- a/src/compiler/README.md +++ b/src/compiler/README.md @@ -1,4 +1,4 @@ -#Overview +# Overview This directory contains source code for gRPC protocol buffer compiler (*protoc*) plugins. Along with `protoc`, these plugins are used to generate gRPC client and server stubs from `.proto` files. diff --git a/src/core/README.md b/src/core/README.md index 44c6f247725..130d2652b39 100644 --- a/src/core/README.md +++ b/src/core/README.md @@ -1,4 +1,4 @@ -#Overview +# Overview This directory contains source code for C library (a.k.a the *gRPC C core*) that provides all gRPC's core functionality through a low level API. Libraries in other languages in this repository (C++, Ruby, Python, PHP, NodeJS, Objective-C) are layered on top of this library. diff --git a/src/core/ext/census/gen/README.md b/src/core/ext/census/gen/README.md index fdbac1084cd..d4612bc7c85 100644 --- a/src/core/ext/census/gen/README.md +++ b/src/core/ext/census/gen/README.md @@ -1,6 +1,6 @@ Files generated for use by Census stats and trace recording subsystem. -#Files +# Files * census.pb.{h,c} - Generated from src/core/ext/census/census.proto, using the script `tools/codegen/core/gen_nano_proto.sh src/proto/census/census.proto $PWD/src/core/ext/census/gen src/core/ext/census/gen` diff --git a/src/cpp/README.md b/src/cpp/README.md index d9b521317a6..e9ef489a7ca 100644 --- a/src/cpp/README.md +++ b/src/cpp/README.md @@ -1,17 +1,17 @@ -#Overview +# Overview This directory contains source code for C++ implementation of gRPC. -#Pre-requisites +# Pre-requisites -##Linux +## Linux ```sh $ [sudo] apt-get install build-essential autoconf libtool ``` -##Mac OSX +## Mac OSX For a Mac system, git is not available by default. You will first need to install Xcode from the Mac AppStore and then run the following command from a @@ -21,7 +21,7 @@ terminal: $ [sudo] xcode-select --install ``` -##Protoc +## Protoc By default gRPC uses [protocol buffers](https://github.com/google/protobuf), you will need the `protoc` compiler to generate stub server and client code. @@ -39,12 +39,12 @@ $ sudo make install # 'make' should have been run by core grpc Alternatively, you can download `protoc` binaries from [the protocol buffers Github repository](https://github.com/google/protobuf/releases). -#Installation +# Installation Currently to install gRPC for C++, you need to build from source as described below. -#Build from Source +# Build from Source ```sh $ git clone -b $(curl -L http://grpc.io/release) https://github.com/grpc/grpc @@ -54,7 +54,7 @@ below. $ [sudo] make install ``` -#Documentation +# Documentation You can find out how to build and run our simplest gRPC C++ example in our [C++ quick start](../../examples/cpp). diff --git a/src/php/README.md b/src/php/README.md index f08541f16cf..f9f93ba8159 100644 --- a/src/php/README.md +++ b/src/php/README.md @@ -1,5 +1,5 @@ -#Overview +# Overview This directory contains source code for PHP implementation of gRPC layered on shared C library. From 34d21ceadcc7e83f15f3bdcfa6955940c6e4d30a Mon Sep 17 00:00:00 2001 From: thinkerou Date: Sun, 26 Mar 2017 01:07:27 +0800 Subject: [PATCH 4/5] fix php style --- src/php/lib/Grpc/AbstractCall.php | 2 +- src/php/tests/unit_tests/ServerTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/php/lib/Grpc/AbstractCall.php b/src/php/lib/Grpc/AbstractCall.php index 4833fdc7b6f..a59bfa3ba38 100644 --- a/src/php/lib/Grpc/AbstractCall.php +++ b/src/php/lib/Grpc/AbstractCall.php @@ -131,7 +131,7 @@ abstract class AbstractCall // Proto3 implementation if (method_exists($data, 'encode')) { return $data->encode(); - } else if (method_exists($data, 'serializeToString')) { + } elseif (method_exists($data, 'serializeToString')) { return $data->serializeToString(); } diff --git a/src/php/tests/unit_tests/ServerTest.php b/src/php/tests/unit_tests/ServerTest.php index 5f40202f182..3e7c01f20e2 100644 --- a/src/php/tests/unit_tests/ServerTest.php +++ b/src/php/tests/unit_tests/ServerTest.php @@ -69,7 +69,7 @@ class ServerTest extends PHPUnit_Framework_TestCase $this->server = new Grpc\Server(); $port = $this->server->addHttp2Port('0.0.0.0:0'); $this->server->start(); - $channel = new Grpc\Channel('localhost:' . $port, + $channel = new Grpc\Channel('localhost:'.$port, ['credentials' => Grpc\ChannelCredentials::createInsecure()]); $deadline = Grpc\Timeval::infFuture(); From eb2b11534233aa439c743142a47f683a05a42878 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 28 Mar 2017 15:27:27 -0700 Subject: [PATCH 5/5] Fix error slice bug --- src/core/lib/surface/call.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c index 895a8a3b060..9342c5f8e95 100644 --- a/src/core/lib/surface/call.c +++ b/src/core/lib/surface/call.c @@ -625,7 +625,7 @@ static bool get_final_status_from( void (*set_value)(grpc_status_code code, void *user_data), void *set_value_user_data, grpc_slice *details) { grpc_status_code code; - grpc_slice slice; + grpc_slice slice = grpc_empty_slice(); grpc_error_get_status(error, call->send_deadline, &code, &slice, NULL); if (code == GRPC_STATUS_OK && !allow_ok_status) { return false;