-
Notifications
You must be signed in to change notification settings - Fork 18k
x/net/http2: Transport should handle 421 #18341
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
Triage, @bradfitz? |
Thanks. Yeah, we could still do this sometime. It's just lowish priority, since I've never seen one in the wild and the spec only says we "MAY" retry the request. http://httpwg.org/specs/rfc7540.html#rfc.section.9.1.2 /cc @tombergan |
The title of this issue puts the retry responsibility in the hands of Transport. I don't see how the transport code could handle a retry. According to the (There's also the language of "should" vs "must". The quote above uses should, whereas a prior sentence in that godoc RoundTripper paragraph, uses "must" regarding error responses - "...RoundTrip must return err == nil..." Therefore, the restriction is not an absolute requirement.) I'm new here so please let me know if I'm missing something. |
There is more detail in RFC 7838. I believe what can happen is:
RFC 7838 Section 6 says that the Alt-Svc cache entry MUST be cleared, then the request MAY be retried:
Since this is a MAY, I tend to agree with @meirf that RoundTrip should not automatically retry. Instead, the implementation can remove foo.cdn.com from the Alt-Svc cache to guarantee that a subsequent retry will use foo.com. However, RFC 7540 Section 9.1.2 says:
I really don't understand why this would happen -- isn't that what GOAWAY is for? In any case, I think the http2 client library could handle this by treating 421 as GOAWAY(NOERROR). |
Here's an example of 421 because of connection reuse.
I think GOAWAY is too extreme for reuse-caused 421s. Specifically, 6.8 says:
GOAWAY prevents any additional steams, but 421 (at least for connection reuse) indicates that the connection can be used again - just not for requests destined for the problematic host.
Indeed, we want the request to be retried on a different connection just as we do for GOAWAY(NOERROR). We agree that I'd love to submit some code for this, but I don't see how it would work. I'm open to any ideas/corrections on what I've written here or above. |
@meirf, thanks for the virtual hosts example. I knew about 421 with Alt-Svc, but not virtual hosts. Before diving into a solution, I want to make sure we completely understand the problem. Specifically, I want to understand all the ways a client may decide to send a request to a server that will return 421. I believe there are (at least) three ways this can happen:
I believe 1 and 2 cannot happen in the current Go library because Go does not implement Alt-Svc or connection sharing when advertised by TLS certs (@bradfitz, correct me if I'm wrong). The third case can happen, but unfortunately, this is trickier to fix because we'd need to blacklist specific IPs in the dialer so we don't end up with the same IP when redialing a new connection. Does that sound right? |
Thanks for bringing up the status quo: "I believe... connection." I think it's worthwhile to go back to 9.1.2. The (unfortunately vague) gist is
It then lists 3 categories of cases when this might happen. Case A
Case B
Case C
Cases B & C are the only cases referred to in the context of retrying. (For A, we would never want to retry the request because it would certainly fail.) For examples of B, the spec describes one with a middlebox. As another example, I referenced one above with virtual hosts that had different ssl configurations. For C, 7838 says that this happens when "an alternative service is not authoritative". One way to be authoritative is to present a TLS certificate that’s valid for the origin . 7838 also says "they MAY retry the request, either at another alternative server, or at the origin." Going back to the 3 cases you listed.
If it does not implement Alt-Svc/there is no plan to, then we can rule out Case C.
I think this is correct - connection sharing is done only on the basis of authority (host:port). Though there is a currently a "TODO: add support for sharing conns based on cert names". The spec's only example of 421 in case B and the only cases I've seen involve different authorities that are covered by a single cert. Because of that, I assume there are currently no cases of 421 occurring in Case B with the current implementation.
I don't see how the third case could happen. The vhosts have different authorities. Additionally, it requires connection reuse based on cert because it's https. If Cases A, B and C are exhaustive and my assumption ("...there are currently no cases...") is correct, then it looks like retrying would accomplish nothing right now. It would only be valuable once alt-svc or sharing conns based on cert names was implemented. Even at that point, we would still have to address the issue I brought up way earlier - "retry responsibility". |
Having just experienced a problem related to this in real life - I do think Go should handle it in the transport. However, it specifically should handle it when:
And then the request should then be made on a new connection - not a shared one. If the connection was received when sent on its own connection, pass it up to the client. The author of the server may have been creatively using http codes that the client may know how to handle. |
Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (
go version
)?What operating system and processor architecture are you using (
go env
)?What did you do?
If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.
What did you expect to see?
What did you see instead?
The text was updated successfully, but these errors were encountered: