-
Notifications
You must be signed in to change notification settings - Fork 18k
x/net/http2: delay sending request body in Transport if 100-continue is set #13851
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
CL https://golang.org/cl/18370 mentions this issue. |
…e tests This makes the Transport ignore 100-continue responses from servers, rather than get confused by them. This is good enough for golang/go#13659. I filed golang/go#13851 to do better later, but that's less important. This CL also adds comprehensive tests for the 36 different ways that frames can be arranged from servers when reading a response. That exposed some bugs (now fixed), and even affected the http2 API: I'd added a END_STREAM accessor on CONTINUATION frames, but it's not even valid there. I also renamed some confusing variables which sounded too similar. Updates golang/go#13659 Updates golang/go#13851 Change-Id: I58868a27258981267f1b2043f711f50a42ec744a Reviewed-on: https://go-review.googlesource.com/18370 Reviewed-by: Andrew Gerrand <adg@golang.org>
CL https://golang.org/cl/19424 mentions this issue. |
For now, don't enable http2 when Transport.TLSConfig != nil. See background in #14275. Also don't enable http2 when ExpectContinueTimeout is specified for now, in case somebody depends on that functionality. (It is not yet implemented in http2, and was only just added to net/http too in Go 1.6, so nobody would be setting it yet). Updates #14275 Updates #13851 Change-Id: I192d555f5fb0a567bd89b6ad87175bbdd7891ae3 Reviewed-on: https://go-review.googlesource.com/19424 Reviewed-by: Russ Cox <rsc@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
@mdevan, in #14391 (comment) you mentioned that you didn't think this was relevant for HTTP/2. Why not? |
I assumed that was the case from a previous comment in the same thread. I also said "if it is not relevant for HTTP/2" :-) |
CL https://golang.org/cl/23235 mentions this issue. |
CL https://golang.org/cl/23254 mentions this issue. |
In Go 1.6, the HTTP/1 client got Transport.ExpectContinueTimeout. This makes the HTTP/2 client respect a Request's "Expect: 100-continue" field and the Transport.ExpectContinueTimeout configuration. This also makes sure to call the traceWroteRequest hook if the server replied while we're still writing the request, since that code was in the same spot and it couldn't be trivially separated. Updates golang/go#13851 (fixed after integrating it into std) Updates golang/go#15744 Change-Id: I67dfd68532daa6c4a0c026549c6e5cbfce50e1ea Reviewed-on: https://go-review.googlesource.com/23235 Reviewed-by: Andrew Gerrand <adg@golang.org>
In Go 1.6, the HTTP/1 client got Transport.ExpectContinueTimeout.
The http2 Transport should do the same. And ideally it should use the configuration value from the *http1.Transport, at least when used via the standard library.
Issue #13659 is about at least not getting confused by the 100-continue header responses from servers. This bug is about doing it properly.
/cc @bmizerany
The text was updated successfully, but these errors were encountered: