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

x/net/http2: make Transport send GOAWAY? #18340

Open
bradfitz opened this issue Dec 16, 2016 · 5 comments
Open

x/net/http2: make Transport send GOAWAY? #18340

bradfitz opened this issue Dec 16, 2016 · 5 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bradfitz
Copy link
Contributor

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:

When either endpoint chooses to close the transport-layer TCP connection, the terminating endpoint SHOULD first send a GOAWAY (Section 6.8) frame so that both endpoints can reliably determine whether previously sent frames have been processed and gracefully complete or terminate any necessary remaining tasks.

Or code is like:

        // TODO: do clients send GOAWAY too? maybe? Just Close:                                                                                           
        cc.tconn.Close()

But I'm still not quite sure why a server would care.

/cc @mnot @tombergan

@bradfitz bradfitz added this to the Go1.9Maybe milestone Dec 16, 2016
@bradfitz bradfitz changed the title x/net/http2: make Transport send GOAWAY x/net/http2: make Transport send GOAWAY? Dec 16, 2016
@mnot
Copy link

mnot commented Dec 25, 2016

Is this client side, server side or both?

@bradfitz
Copy link
Contributor Author

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.

@mnot
Copy link

mnot commented Dec 25, 2016

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?

@martinthomson
Copy link

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.

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9Maybe Jun 28, 2017
@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jun 28, 2017
@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Nov 29, 2017
@bradfitz bradfitz modified the milestones: Go1.11, Unplanned Jul 9, 2018
@bradfitz bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Jul 9, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 9, 2018
@bradfitz
Copy link
Contributor Author

bradfitz commented Jul 9, 2018

Decision: let's send it. But not targeting any particular release. If somebody wants to send the change we can review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants