[chttp2] Continue refactoring towards promises#34437
Merged
ctiller merged 58 commits intogrpc:masterfrom Sep 28, 2023
Merged
Conversation
Automated fix for refs/heads/bitgen
Automated fix for refs/heads/bitgen
Automated fix for refs/heads/bitgen
Automated fix for refs/heads/bitgen
Automated fix for refs/heads/bitgen
Automated fix for refs/heads/ping-v2
Automated fix for refs/heads/ping-v2
Automated fix for refs/heads/ping-v2
yashykt
reviewed
Sep 27, 2023
| t->keepalive_ping_timer_handle.reset(); | ||
| } | ||
| } | ||
| if (t->keepalive_watchdog_timer_handle.has_value()) { |
Member
Author
There was a problem hiding this comment.
it's subsumed by the timeout per-outstanding-ping
| /** How many pings do we allow to be inflight at any given time? | ||
| In older versions of gRPC this was implicitly 1. | ||
| With the multiping experiment we allow this to rise to 100 by default. */ | ||
| #define GRPC_ARG_HTTP2_MAX_INFLIGHT_PINGS "grpc.http2.max_inflight_pings" |
Member
There was a problem hiding this comment.
maybe we don't need to expose this channel arg
Member
Author
There was a problem hiding this comment.
happy to move the definition internally for a bit... would like to keep the escape hatch for the moment though
Member
Author
There was a problem hiding this comment.
moved it private, but left the knob for now
yashykt
approved these changes
Sep 28, 2023
Member
yashykt
left a comment
There was a problem hiding this comment.
My only comment is around documentation, for some explanation whereever it makes sense. Thanks!
| grpc_chttp2_transport::~grpc_chttp2_transport() { | ||
| size_t i; | ||
|
|
||
| cancel_pings(this, GRPC_ERROR_CREATE("Transport destroyed")); |
Member
There was a problem hiding this comment.
note that pings need event engine so this needs to be cancelled before event engine
| explicit GracefulGoaway(grpc_chttp2_transport* t) : t_(t->Ref()) { | ||
| t->sent_goaway_state = GRPC_CHTTP2_GRACEFUL_GOAWAY; | ||
| grpc_chttp2_goaway_append((1u << 31) - 1, 0, grpc_empty_slice(), &t->qbuf); | ||
| t->keepalive_timeout = |
Member
There was a problem hiding this comment.
let's just add a comment over here
|
|
||
| namespace grpc_core { | ||
|
|
||
| class Chttp2PingCallbacks { |
| #include "src/core/lib/experiments/experiments.h" | ||
| #include "src/core/lib/gprpp/match.h" | ||
|
|
||
| /** How many pings do we allow to be inflight at any given time? |
ctiller
added a commit
to ctiller/grpc
that referenced
this pull request
Oct 20, 2023
Isolate ping callback tracking to its own file. Also takes the opportunity to simplify keepalive code by applying the ping timeout to all pings. Adds an experiment to allow multiple pings outstanding too (this was originally an accidental behavior change of the work, but one that I think may be useful going forward). --------- Co-authored-by: ctiller <ctiller@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Isolate ping callback tracking to its own file.
Also takes the opportunity to simplify keepalive code by applying the ping timeout to all pings.
Adds an experiment to allow multiple pings outstanding too (this was originally an accidental behavior change of the work, but one that I think may be useful going forward).