Fix the client-side ping policy #11587

y-zeng · 2017-06-23T21:59:21Z

chttp2_transport was too strict about sending ping. It may prevent legal keepalive pings from being sent. This PR relaxes the restriction as described in Client-side Keepalive:

The number of ping frames allowed between data frames will no longer be limited.
If no data frame is received, the client is allowed to send a ping every 5 minutes.
If a data frame is received, the client is allowed to send the next ping immediately.

grpc-kokoro · 2017-06-23T22:23:01Z

[trickle] No significant performance differences

grpc-kokoro · 2017-06-23T23:24:32Z

[qps] No significant performance differences

y-zeng · 2017-06-28T00:00:03Z

Jenkins: test this please

grpc-kokoro · 2017-06-28T00:24:07Z

[trickle] No significant performance differences

grpc-kokoro · 2017-06-28T00:41:56Z

[microbenchmarks] No significant performance differences

grpc-kokoro · 2017-06-30T02:21:12Z

[trickle] No significant performance differences

grpc-kokoro · 2017-06-30T02:38:44Z

[microbenchmarks] No significant performance differences

grpc-kokoro · 2017-07-12T21:52:01Z

[trickle] No significant performance differences

grpc-kokoro · 2017-07-12T22:08:45Z

[microbenchmarks] No significant performance differences

ctiller · 2017-07-13T16:35:41Z

src/core/ext/transport/chttp2/transport/chttp2_transport.c

@@ -2398,6 +2418,36 @@ void grpc_chttp2_config_default_keepalive_args(grpc_channel_args *args,
                &args->args[i],
                (grpc_integer_options){g_default_keepalive_permit_without_calls,
                                       0, 1});
+      } else if (0 ==
+                 strcmp(args->args[i].key, GRPC_ARG_HTTP2_MAX_PING_STRIKES)) {
+        g_default_max_ping_strikes = grpc_channel_arg_get_integer(


Setting global state from transport construction is the wrong thing to do:

it's not thread safe (what if I create transports from two threads)

it's order dependent (max_ping_strikes now depends on the construction order of transports, which might change even due to network latency - meaning this could become a random number)

Better would be to hold these values as properties of the transport (preferably as some sub-struct... grpc_chttp2_ping_enforcement_args?)

Thanks for the review!
These global values are not set from transport construction, they are set in grpc_chttp2_config_default_keepalive_args(). It's an internal interface, and must only be called at the initialization stage, as stated here.
The transport struct has corresponding properties in grpc_chttp2_repeated_ping_policy. When constructing a transport, we will use g_default_max_ping_strikes as the default value of grpc_chttp2_transport.ping_policy.max_ping_strikes.

What does initialization stage mean?

before grpc_init?

after grpc_init?

before transport construction?

If it's an internal interface, who is supposed to call it? (no code can, because it's internal)

What is the justification for introducing yet another configuration mechanism?

ctiller · 2017-09-06T17:08:12Z

Needs conflict resolution

y-zeng · 2017-09-11T07:41:36Z

Resolved conflict. Please see the email thread for the explanation of grpc_chttp2_config_default_keepalive_args().

grpc-kokoro · 2017-09-11T08:50:21Z

[microbenchmarks] No significant performance differences

grpc-kokoro · 2017-09-11T09:04:06Z

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_trickle.BM_PumpStreamServerToClient_Trickle_8_4k.opt.new: 1


[trickle] No significant performance differences

grpc-kokoro · 2017-09-18T21:09:23Z

[trickle] No significant performance differences

grpc-kokoro · 2017-09-18T21:49:42Z

[microbenchmarks] Performance differences noted:
Benchmark                                                                       atm_add_per_iteration
------------------------------------------------------------------------------  -----------------------
BM_StreamCreateSendInitialMetadataDestroy<RepresentativeClientInitialMetadata>  +7%

ctiller · 2017-09-18T22:47:28Z

Please let me know once merged.

y-zeng · 2017-09-18T23:23:39Z

Known failures:
Asan: #12245
Asan (Internal CI): #12544
Basic Tests Linux (opt): #12313, #12428
Basic Tests MacOS [dbg] (internal CI): #12138, #12538
Basic Tests MacOS [opt] (internal CI): #12138, #12538
Basic Tests Multi-language Linux (internal CI): #9538
Basic Tests Windows (internal CI): #12595
Msan C (internal CI): #12394
gRPC_interop_pull_requests: #12261
Tsan C: #11618
UBsan C: #12544
Basic Tests Mac: #12138
Basic Tests Windows: #11745

ctiller · 2017-09-19T16:03:09Z

Bad news: This isn't actually fixing the problem I was seeing with #11072

googlebot added the cla: yes label Jun 23, 2017

y-zeng requested a review from sreecha June 30, 2017 01:13

y-zeng assigned sreecha Jun 30, 2017

y-zeng mentioned this pull request Jul 12, 2017

Ping simplifications, BDP estimation improvements #11072

Merged

y-zeng assigned ctiller Jul 12, 2017

y-zeng requested a review from ctiller July 12, 2017 18:02

y-zeng force-pushed the pure_ping branch from db26433 to 0177053 Compare July 12, 2017 21:05

ctiller reviewed Jul 13, 2017

View reviewed changes

y-zeng force-pushed the pure_ping branch from 0177053 to e1a243c Compare September 11, 2017 07:39

ctiller approved these changes Sep 15, 2017

View reviewed changes

y-zeng added 6 commits September 18, 2017 13:32

Fix ping policy

6a8c0ac

Fix keepalive_timeout

2741149

Set last_ping_sent_time to inf_past after receiving data

ad32075

Set the ping behavior as described in the proposal

1ecbebc

Update writing.c with the trailers-only response change

18ea66f

Add more options in grpc_chttp2_config_default_keepalive_args

d7eda61

y-zeng force-pushed the pure_ping branch from e1a243c to d7eda61 Compare September 18, 2017 20:36

ctiller mentioned this pull request Sep 19, 2017

Flow control fixes #12636

Closed

y-zeng merged commit 0aedb81 into grpc:master Sep 19, 2017

y-zeng mentioned this pull request Sep 22, 2017

Fix ping policy #11129

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019

lock bot unassigned sreecha Jan 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the client-side ping policy #11587

Fix the client-side ping policy #11587

y-zeng commented Jun 23, 2017 •

edited

Loading

grpc-kokoro commented Jun 23, 2017

grpc-kokoro commented Jun 23, 2017

y-zeng commented Jun 28, 2017

grpc-kokoro commented Jun 28, 2017

grpc-kokoro commented Jun 28, 2017

grpc-kokoro commented Jun 30, 2017

grpc-kokoro commented Jun 30, 2017

grpc-kokoro commented Jul 12, 2017

grpc-kokoro commented Jul 12, 2017

ctiller Jul 13, 2017

y-zeng Jul 13, 2017

ctiller Jul 17, 2017

ctiller commented Sep 6, 2017

y-zeng commented Sep 11, 2017

grpc-kokoro commented Sep 11, 2017

grpc-kokoro commented Sep 11, 2017

grpc-kokoro commented Sep 18, 2017

grpc-kokoro commented Sep 18, 2017

ctiller commented Sep 18, 2017

y-zeng commented Sep 18, 2017 •

edited

Loading

ctiller commented Sep 19, 2017

Fix the client-side ping policy #11587

Fix the client-side ping policy #11587

Conversation

y-zeng commented Jun 23, 2017 • edited Loading

grpc-kokoro commented Jun 23, 2017

grpc-kokoro commented Jun 23, 2017

y-zeng commented Jun 28, 2017

grpc-kokoro commented Jun 28, 2017

grpc-kokoro commented Jun 28, 2017

grpc-kokoro commented Jun 30, 2017

grpc-kokoro commented Jun 30, 2017

grpc-kokoro commented Jul 12, 2017

grpc-kokoro commented Jul 12, 2017

ctiller Jul 13, 2017

Choose a reason for hiding this comment

y-zeng Jul 13, 2017

Choose a reason for hiding this comment

ctiller Jul 17, 2017

Choose a reason for hiding this comment

ctiller commented Sep 6, 2017

y-zeng commented Sep 11, 2017

grpc-kokoro commented Sep 11, 2017

grpc-kokoro commented Sep 11, 2017

grpc-kokoro commented Sep 18, 2017

grpc-kokoro commented Sep 18, 2017

ctiller commented Sep 18, 2017

y-zeng commented Sep 18, 2017 • edited Loading

ctiller commented Sep 19, 2017

y-zeng commented Jun 23, 2017 •

edited

Loading

y-zeng commented Sep 18, 2017 •

edited

Loading