Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add HTTP/2 keep alive pings #22565

Merged
merged 17 commits into from
Jun 15, 2020
Merged

Add HTTP/2 keep alive pings #22565

merged 17 commits into from
Jun 15, 2020

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jun 5, 2020

Fixes #15104

@ghost ghost added the area-servers label Jun 5, 2020
@JamesNK JamesNK marked this pull request as draft June 5, 2020 05:00
@JamesNK JamesNK requested a review from davidfowl June 5, 2020 05:12
@JamesNK JamesNK added api-suggestion Early API idea and discussion, it is NOT ready for implementation HTTP2 labels Jun 5, 2020
@JamesNK JamesNK added this to the 5.0.0-preview7 milestone Jun 5, 2020
@JamesNK JamesNK force-pushed the jamesnk/http2keepalive branch from 4cda602 to 631f04d Compare June 6, 2020 10:33
@JamesNK JamesNK marked this pull request as ready for review June 6, 2020 10:36
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the right idea.

src/Servers/Kestrel/Core/src/Http2Limits.cs Outdated Show resolved Hide resolved
src/Servers/Kestrel/Core/src/Http2Limits.cs Outdated Show resolved Hide resolved
src/Servers/Kestrel/Core/src/Http2Limits.cs Outdated Show resolved Hide resolved
src/Servers/Kestrel/Core/src/Http2Limits.cs Show resolved Hide resolved
Co-authored-by: Chris Ross <Tratcher@Outlook.com>
@halter73
Copy link
Member

halter73 commented Jun 8, 2020

I have a high level question. If we never initiate pings as long as we are receiving frames faster than the KeepAlivePingInterval, why are we waiting specifically for the ping ack to cancel the ping timeout? Why not cancel the timeout whenever we receive any frame?

I get that cancelling the timeout whenever we receive any frame does nothing to measure latency, but isn't activity the real thing we want to check? If we want to check latency instead of activity, we should send pings regardless of recent activity.

@JamesNK
Copy link
Member Author

JamesNK commented Jun 8, 2020

That is something I thought about.

golang appears to be ok with any data:

// After having pinged for keepalive check, the server waits for a duration
// of Timeout and if no activity is seen even after that the connection is
// closed.

I think Java waits on the ping ACK

https://grpc.github.io/grpc-java/javadoc/io/grpc/netty/NettyServerBuilder.html#keepAliveTimeout-long-java.util.concurrent.TimeUnit-

Sets a custom keepalive timeout, the timeout for keepalive ping requests. An unreasonably small value might be increased.

@JamesNK
Copy link
Member Author

JamesNK commented Jun 8, 2020

Updated.

Open questions:

  • GOAWAY error code. I like NO_ERROR
  • Is activity any incoming data, or completed frames?

@Tratcher
Copy link
Member

Tratcher commented Jun 8, 2020

No_error is probably fine. All it means is that the server has decided to close the connection for an arbitrary reason.

I don't know if any data vs completed frames is a meaningful distinction in reality. Will the socket even notify us that data is available before it's finished reading a TCP frame? These HTTP/2 frames should fit into a TCP frame most of the time.

@JamesNK
Copy link
Member Author

JamesNK commented Jun 9, 2020

@yashykt Hi Yash

I'm adding keep alive support to the ASP.NET Core server. It will be useful when hosting gRPC on ASP.NET Core. A couple of questions for you:

  1. What is considered activity that will reset the keep alive interval (timer that will send a ping) and keep alive timeout (timer that will close the connection)? Is it any bytes being received by the connection, or is it a completed frame?
  2. If the keep alive timeout has been exceeded and the connection is being ended by the server, what should it do?
    • Close the connection without sending any frames, or
    • Send a GOAWAY. If a GOAWAY is sent, what is its error code?

@yashykt
Copy link

yashykt commented Jun 10, 2020

Hi, the source of truth for keepalives in gRPC is https://github.com/grpc/proposal/blob/master/A8-client-side-keepalive.md

  1. Bytes received
  2. The proposal does not add any details around this, but gRPC Core does not send any GOAWAY frame. It simply closes down the connection.

@JamesNK JamesNK added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jun 11, 2020
@ghost
Copy link

ghost commented Jun 11, 2020

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@JamesNK JamesNK removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 11, 2020
@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jun 15, 2020
@JamesNK JamesNK merged commit a861d18 into master Jun 15, 2020
@JamesNK JamesNK deleted the jamesnk/http2keepalive branch June 15, 2020 22:13
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions HTTP2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

configurable HTTP/2 PING timeouts on kestrel
5 participants