-
Notifications
You must be signed in to change notification settings - Fork 18k
x/net/http2: make Transport send GOAWAY? #18340
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
Comments
Is this client side, server side or both? |
Client. The server already sends GOAWAY on shutdown and the client already respects the server's GOAWAY so it can retry requests on new connections. This issue is about whether there's any reason for the client to send GOAWAY when it's closed. |
Hm. GOAWAY is roughly "Don't initiate any more streams." In vanilla H2, the only practical protocol-visible application of a client->server GOAWAY would be to stop the server initiating any more pushes, I think. However, protocol extensions or other ALPN tokens might change that. Inside the implementation, I suppose it would also give the server an opportunity to release resources in a slightly more ordered fashion (depending on how you close the transport connection). So, it's polite, and it's a SHOULD in the spec (it says "endpoints," not just "servers"). What are the downsides of sending it? Anything to add, @martinthomson? |
That about sums it up. Though I would add that if a server is tracking whether individual requests are getting through, GOAWAY helps with that. The server never really had any certainty here, but I am aware of cases where there are optimizations that apply when you know that a client had gotten the message. |
Decision: let's send it. But not targeting any particular release. If somebody wants to send the change we can review it. |
We previously debated whether the Transport should send GOAWAY frames on shutdown, but couldn't think of a useful reason why.
The spec does say:
Or code is like:
But I'm still not quite sure why a server would care.
/cc @mnot @tombergan
The text was updated successfully, but these errors were encountered: