Set last_ping_sent_time to inf_past after receiving data
diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h
index 748dc71..90f03f4 100644
--- a/include/grpc/impl/codegen/grpc_types.h
+++ b/include/grpc/impl/codegen/grpc_types.h
@@ -188,9 +188,14 @@
#define GRPC_ARG_HTTP2_MAX_FRAME_SIZE "grpc.http2.max_frame_size"
/** Should BDP probing be performed? */
#define GRPC_ARG_HTTP2_BDP_PROBE "grpc.http2.bdp_probe"
-/** Minimum time (in milliseconds) between successive ping frames being sent */
-#define GRPC_ARG_HTTP2_MIN_TIME_BETWEEN_PINGS_MS \
+/** Minimum time between sending successive ping frames without receiving any
+ data frame, Int valued, milliseconds. */
+#define GRPC_ARG_HTTP2_MIN_SENT_PING_INTERVAL_WITHOUT_DATA_MS \
"grpc.http2.min_time_between_pings_ms"
+/** Minimum allowed time between receiving successive ping frames without
+ sending any data frame. Int valued, milliseconds */
+#define GRPC_ARG_HTTP2_MIN_RECV_PING_INTERVAL_WITHOUT_DATA_MS \
+ "grpc.http2.min_ping_interval_without_data_ms"
/** Channel arg to override the http2 :scheme header */
#define GRPC_ARG_HTTP2_SCHEME "grpc.http2_scheme"
/** How many pings can we send before needing to send a data frame or header
@@ -202,10 +207,6 @@
closing the transport? (0 indicates that the server can bear an infinite
number of misbehaving pings) */
#define GRPC_ARG_HTTP2_MAX_PING_STRIKES "grpc.http2.max_ping_strikes"
-/** Minimum allowed time between two pings without sending any data frame. Int
- valued, seconds */
-#define GRPC_ARG_HTTP2_MIN_PING_INTERVAL_WITHOUT_DATA_MS \
- "grpc.http2.min_ping_interval_without_data_ms"
/** How much data are we willing to queue up per stream if
GRPC_WRITE_BUFFER_HINT is set? This is an upper bound */
#define GRPC_ARG_HTTP2_WRITE_BUFFER_SIZE "grpc.http2.write_buffer_size"
diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c
index 3a28762..c742444 100644
--- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c
+++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c
@@ -152,10 +152,10 @@
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
+#define DEFAULT_MIN_SENT_PING_INTERVAL_WITHOUT_DATA_MS 300000 /* 5 minutes */
+#define DEFAULT_MIN_RECV_PING_INTERVAL_WITHOUT_DATA_MS 300000 /* 5 minutes */
+#define DEFAULT_MAX_PINGS_BETWEEN_DATA 2
#define DEFAULT_MAX_PING_STRIKES 2
-#define DEFAULT_MIN_PING_INTERVAL_WITHOUT_DATA_MS 300000 /* 5 minutes */
/** keepalive-relevant functions */
static void init_keepalive_ping_locked(grpc_exec_ctx *exec_ctx, void *arg,
@@ -364,11 +364,11 @@
t->ping_policy = (grpc_chttp2_repeated_ping_policy){
.max_pings_without_data = DEFAULT_MAX_PINGS_BETWEEN_DATA,
- .min_time_between_pings =
- gpr_time_from_millis(DEFAULT_MIN_TIME_BETWEEN_PINGS_MS, GPR_TIMESPAN),
+ .min_sent_ping_interval_without_data = gpr_time_from_millis(
+ DEFAULT_MIN_SENT_PING_INTERVAL_WITHOUT_DATA_MS, GPR_TIMESPAN),
.max_ping_strikes = DEFAULT_MAX_PING_STRIKES,
- .min_ping_interval_without_data = gpr_time_from_millis(
- DEFAULT_MIN_PING_INTERVAL_WITHOUT_DATA_MS, GPR_TIMESPAN),
+ .min_recv_ping_interval_without_data = gpr_time_from_millis(
+ DEFAULT_MIN_RECV_PING_INTERVAL_WITHOUT_DATA_MS, GPR_TIMESPAN),
};
/* Keepalive setting */
@@ -434,23 +434,30 @@
t->ping_policy.max_ping_strikes = grpc_channel_arg_get_integer(
&channel_args->args[i],
(grpc_integer_options){DEFAULT_MAX_PING_STRIKES, 0, INT_MAX});
- } else if (0 == strcmp(channel_args->args[i].key,
- GRPC_ARG_HTTP2_MIN_TIME_BETWEEN_PINGS_MS)) {
- t->ping_policy.min_time_between_pings = gpr_time_from_millis(
- grpc_channel_arg_get_integer(
- &channel_args->args[i],
- (grpc_integer_options){DEFAULT_MIN_TIME_BETWEEN_PINGS_MS, 0,
- INT_MAX}),
- GPR_TIMESPAN);
} else if (0 ==
- strcmp(channel_args->args[i].key,
- GRPC_ARG_HTTP2_MIN_PING_INTERVAL_WITHOUT_DATA_MS)) {
- t->ping_policy.min_ping_interval_without_data = gpr_time_from_millis(
- grpc_channel_arg_get_integer(
- &channel_args->args[i],
- (grpc_integer_options){
- DEFAULT_MIN_PING_INTERVAL_WITHOUT_DATA_MS, 0, INT_MAX}),
- GPR_TIMESPAN);
+ strcmp(
+ channel_args->args[i].key,
+ GRPC_ARG_HTTP2_MIN_SENT_PING_INTERVAL_WITHOUT_DATA_MS)) {
+ t->ping_policy.min_sent_ping_interval_without_data =
+ gpr_time_from_millis(
+ grpc_channel_arg_get_integer(
+ &channel_args->args[i],
+ (grpc_integer_options){
+ DEFAULT_MIN_SENT_PING_INTERVAL_WITHOUT_DATA_MS, 0,
+ INT_MAX}),
+ GPR_TIMESPAN);
+ } else if (0 ==
+ strcmp(
+ channel_args->args[i].key,
+ GRPC_ARG_HTTP2_MIN_RECV_PING_INTERVAL_WITHOUT_DATA_MS)) {
+ t->ping_policy.min_recv_ping_interval_without_data =
+ gpr_time_from_millis(
+ grpc_channel_arg_get_integer(
+ &channel_args->args[i],
+ (grpc_integer_options){
+ DEFAULT_MIN_RECV_PING_INTERVAL_WITHOUT_DATA_MS, 0,
+ INT_MAX}),
+ GPR_TIMESPAN);
} else if (0 == strcmp(channel_args->args[i].key,
GRPC_ARG_HTTP2_WRITE_BUFFER_SIZE)) {
t->write_buffer_size = (uint32_t)grpc_channel_arg_get_integer(
@@ -625,6 +632,9 @@
connectivity_state_set(exec_ctx, t, GRPC_CHANNEL_SHUTDOWN,
GRPC_ERROR_REF(error), "close_transport");
grpc_endpoint_shutdown(exec_ctx, t->ep, GRPC_ERROR_REF(error));
+ if (t->ping_state.is_delayed_ping_timer_set) {
+ grpc_timer_cancel(exec_ctx, &t->ping_state.delayed_ping_timer);
+ }
switch (t->keepalive_state) {
case GRPC_CHTTP2_KEEPALIVE_STATE_WAITING:
grpc_timer_cancel(exec_ctx, &t->keepalive_ping_timer);
@@ -1729,8 +1739,10 @@
grpc_error *error) {
grpc_chttp2_transport *t = (grpc_chttp2_transport *)tp;
t->ping_state.is_delayed_ping_timer_set = false;
- grpc_chttp2_initiate_write(exec_ctx, t,
- GRPC_CHTTP2_INITIATE_WRITE_RETRY_SEND_PING);
+ if (error == GRPC_ERROR_NONE) {
+ grpc_chttp2_initiate_write(exec_ctx, t,
+ GRPC_CHTTP2_INITIATE_WRITE_RETRY_SEND_PING);
+ }
}
void grpc_chttp2_ack_ping(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t,
diff --git a/src/core/ext/transport/chttp2/transport/frame_ping.c b/src/core/ext/transport/chttp2/transport/frame_ping.c
index 81bd02a..d431d6b 100644
--- a/src/core/ext/transport/chttp2/transport/frame_ping.c
+++ b/src/core/ext/transport/chttp2/transport/frame_ping.c
@@ -92,7 +92,7 @@
gpr_timespec now = gpr_now(GPR_CLOCK_MONOTONIC);
gpr_timespec next_allowed_ping =
gpr_time_add(t->ping_recv_state.last_ping_recv_time,
- t->ping_policy.min_ping_interval_without_data);
+ t->ping_policy.min_recv_ping_interval_without_data);
if (t->keepalive_permit_without_calls == 0 &&
grpc_chttp2_stream_map_size(&t->stream_map) == 0) {
diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h
index c2dfce7..1682be2 100644
--- a/src/core/ext/transport/chttp2/transport/internal.h
+++ b/src/core/ext/transport/chttp2/transport/internal.h
@@ -112,10 +112,10 @@
} grpc_chttp2_ping_queue;
typedef struct {
- gpr_timespec min_time_between_pings;
int max_pings_without_data;
int max_ping_strikes;
- gpr_timespec min_ping_interval_without_data;
+ gpr_timespec min_sent_ping_interval_without_data;
+ gpr_timespec min_recv_ping_interval_without_data;
} grpc_chttp2_repeated_ping_policy;
typedef struct {
diff --git a/src/core/ext/transport/chttp2/transport/parsing.c b/src/core/ext/transport/chttp2/transport/parsing.c
index e52e19f..3db1ad4 100644
--- a/src/core/ext/transport/chttp2/transport/parsing.c
+++ b/src/core/ext/transport/chttp2/transport/parsing.c
@@ -385,6 +385,7 @@
t->parser_data = &s->data_parser;
t->ping_state.pings_before_data_required =
t->ping_policy.max_pings_without_data;
+ t->ping_state.last_ping_sent_time = gpr_inf_past(GPR_CLOCK_MONOTONIC);
return GRPC_ERROR_NONE;
} else if (grpc_error_get_int(err, GRPC_ERROR_INT_STREAM_ID, NULL)) {
/* handle stream errors by closing the stream */
@@ -563,6 +564,7 @@
t->ping_state.pings_before_data_required =
t->ping_policy.max_pings_without_data;
+ t->ping_state.last_ping_sent_time = gpr_inf_past(GPR_CLOCK_MONOTONIC);
/* could be a new grpc_chttp2_stream or an existing grpc_chttp2_stream */
s = grpc_chttp2_parsing_lookup_stream(t, t->incoming_stream_id);
diff --git a/src/core/ext/transport/chttp2/transport/writing.c b/src/core/ext/transport/chttp2/transport/writing.c
index d369930..d61063e 100644
--- a/src/core/ext/transport/chttp2/transport/writing.c
+++ b/src/core/ext/transport/chttp2/transport/writing.c
@@ -66,14 +66,6 @@
}
return;
}
- if (t->keepalive_permit_without_calls == 0 &&
- grpc_chttp2_stream_map_size(&t->stream_map) == 0) {
- if (GRPC_TRACER_ON(grpc_http_trace) ||
- GRPC_TRACER_ON(grpc_bdp_estimator_trace)) {
- gpr_log(GPR_DEBUG, "Ping delayed [%p]: no active stream", t->peer_string);
- }
- return;
- }
if (t->ping_state.pings_before_data_required == 0 &&
t->ping_policy.max_pings_without_data != 0) {
/* need to receive something of substance before sending a ping again */
@@ -86,11 +78,18 @@
return;
}
gpr_timespec now = gpr_now(GPR_CLOCK_MONOTONIC);
- gpr_timespec elapsed = gpr_time_sub(now, t->ping_state.last_ping_sent_time);
- /*gpr_log(GPR_DEBUG, "elapsed:%d.%09d min:%d.%09d", (int)elapsed.tv_sec,
- elapsed.tv_nsec, (int)t->ping_policy.min_time_between_pings.tv_sec,
- (int)t->ping_policy.min_time_between_pings.tv_nsec);*/
- if (gpr_time_cmp(elapsed, t->ping_policy.min_time_between_pings) < 0) {
+ gpr_timespec next_allowed_ping =
+ gpr_time_add(t->ping_state.last_ping_sent_time,
+ t->ping_policy.min_sent_ping_interval_without_data);
+ if (t->keepalive_permit_without_calls == 0 &&
+ grpc_chttp2_stream_map_size(&t->stream_map) == 0) {
+ next_allowed_ping = gpr_time_add(t->ping_recv_state.last_ping_recv_time,
+ gpr_time_from_seconds(7200, GPR_TIMESPAN));
+ }
+ /*gpr_log(GPR_DEBUG, "next_allowed_ping:%d.%09d now:%d.%09d",
+ (int)next_allowed_ping.tv_sec, (int)next_allowed_pingpsed.tv_nsec,
+ (int)now.tv_sec, (int)now.tv_nsec);*/
+ if (gpr_time_cmp(next_allowed_ping, now) > 0) {
/* not enough elapsed time between successive pings */
if (GRPC_TRACER_ON(grpc_http_trace) ||
GRPC_TRACER_ON(grpc_bdp_estimator_trace)) {
@@ -100,11 +99,11 @@
}
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));
+ 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_sent_ping_interval_without_data),
+ &t->retry_initiate_ping_locked, gpr_now(GPR_CLOCK_MONOTONIC));
}
return;
}
@@ -127,6 +126,12 @@
grpc_chttp2_ping_create(false, pq->inflight_id));
GRPC_STATS_INC_HTTP2_PINGS_SENT(exec_ctx);
t->ping_state.last_ping_sent_time = now;
+ if (GRPC_TRACER_ON(grpc_http_trace) ||
+ GRPC_TRACER_ON(grpc_bdp_estimator_trace)) {
+ gpr_log(GPR_DEBUG, "Ping sent [%p]: %d/%d", t->peer_string,
+ t->ping_state.pings_before_data_required,
+ t->ping_policy.max_pings_without_data);
+ }
t->ping_state.pings_before_data_required -=
(t->ping_state.pings_before_data_required != 0);
}
diff --git a/test/core/client_channel/lb_policies_test.c b/test/core/client_channel/lb_policies_test.c
index f70a9fc..ba37cd6 100644
--- a/test/core/client_channel/lb_policies_test.c
+++ b/test/core/client_channel/lb_policies_test.c
@@ -519,7 +519,7 @@
arg_array[1].key = GRPC_ARG_LB_POLICY_NAME;
arg_array[1].value.string = "ROUND_ROBIN";
arg_array[2].type = GRPC_ARG_INTEGER;
- arg_array[2].key = GRPC_ARG_HTTP2_MIN_TIME_BETWEEN_PINGS_MS;
+ arg_array[2].key = GRPC_ARG_HTTP2_MIN_SENT_PING_INTERVAL_WITHOUT_DATA_MS;
arg_array[2].value.integer = 0;
args.num_args = GPR_ARRAY_SIZE(arg_array);
args.args = arg_array;
diff --git a/test/core/end2end/tests/bad_ping.c b/test/core/end2end/tests/bad_ping.c
index 6713597..c97d11b 100644
--- a/test/core/end2end/tests/bad_ping.c
+++ b/test/core/end2end/tests/bad_ping.c
@@ -66,18 +66,19 @@
static void test_bad_ping(grpc_end2end_test_config config) {
grpc_end2end_test_fixture f = config.create_fixture(NULL, NULL);
cq_verifier *cqv = cq_verifier_create(f.cq);
- grpc_arg client_a[] = {{.type = GRPC_ARG_INTEGER,
- .key = GRPC_ARG_HTTP2_MIN_TIME_BETWEEN_PINGS_MS,
- .value.integer = 0},
- {.type = GRPC_ARG_INTEGER,
- .key = GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA,
- .value.integer = 0},
- {.type = GRPC_ARG_INTEGER,
- .key = GRPC_ARG_HTTP2_BDP_PROBE,
- .value.integer = 0}};
+ grpc_arg client_a[] = {
+ {.type = GRPC_ARG_INTEGER,
+ .key = GRPC_ARG_HTTP2_MIN_SENT_PING_INTERVAL_WITHOUT_DATA_MS,
+ .value.integer = 10},
+ {.type = GRPC_ARG_INTEGER,
+ .key = GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA,
+ .value.integer = 0},
+ {.type = GRPC_ARG_INTEGER,
+ .key = GRPC_ARG_HTTP2_BDP_PROBE,
+ .value.integer = 0}};
grpc_arg server_a[] = {
{.type = GRPC_ARG_INTEGER,
- .key = GRPC_ARG_HTTP2_MIN_PING_INTERVAL_WITHOUT_DATA_MS,
+ .key = GRPC_ARG_HTTP2_MIN_RECV_PING_INTERVAL_WITHOUT_DATA_MS,
.value.integer = 300000 /* 5 minutes */},
{.type = GRPC_ARG_INTEGER,
.key = GRPC_ARG_HTTP2_MAX_PING_STRIKES,
diff --git a/test/core/end2end/tests/ping.c b/test/core/end2end/tests/ping.c
index b4d9b13..23c8256 100644
--- a/test/core/end2end/tests/ping.c
+++ b/test/core/end2end/tests/ping.c
@@ -37,18 +37,19 @@
grpc_connectivity_state state = GRPC_CHANNEL_IDLE;
int i;
- grpc_arg client_a[] = {{.type = GRPC_ARG_INTEGER,
- .key = GRPC_ARG_HTTP2_MIN_TIME_BETWEEN_PINGS_MS,
- .value.integer = 0},
- {.type = GRPC_ARG_INTEGER,
- .key = GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA,
- .value.integer = 0},
- {.type = GRPC_ARG_INTEGER,
- .key = GRPC_ARG_KEEPALIVE_PERMIT_WITHOUT_CALLS,
- .value.integer = 1}};
+ grpc_arg client_a[] = {
+ {.type = GRPC_ARG_INTEGER,
+ .key = GRPC_ARG_HTTP2_MIN_SENT_PING_INTERVAL_WITHOUT_DATA_MS,
+ .value.integer = 10},
+ {.type = GRPC_ARG_INTEGER,
+ .key = GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA,
+ .value.integer = 0},
+ {.type = GRPC_ARG_INTEGER,
+ .key = GRPC_ARG_KEEPALIVE_PERMIT_WITHOUT_CALLS,
+ .value.integer = 1}};
grpc_arg server_a[] = {
{.type = GRPC_ARG_INTEGER,
- .key = GRPC_ARG_HTTP2_MIN_PING_INTERVAL_WITHOUT_DATA_MS,
+ .key = GRPC_ARG_HTTP2_MIN_RECV_PING_INTERVAL_WITHOUT_DATA_MS,
.value.integer = 0},
{.type = GRPC_ARG_INTEGER,
.key = GRPC_ARG_KEEPALIVE_PERMIT_WITHOUT_CALLS,