http2: make Transport work around mod_h2 bug
The http2 server implementation in Apache (mod_http2, aka mod_h2) had
a bug where it didn't reply to the initial SETTINGS frame until it had
received a request. Apache 2.4.17 was the first Apache version to
include mod_http2 and has the bug.
The bug was fixed 20 days ago
(https://github.com/apache/httpd/commit/de4e3031aeb9775d2ebb9c917ecb1117c12d05f8)
for Apache 2.5.0 and included in the out-of-tree mod_h2 1.5.12
(https://github.com/icing/mod_h2/releases/tag/v1.5.12) but most Apache
sites are running older versions making them incompatible with Go's
client. (At least Debian Stretch and Ubuntu Xenial, currently.)
Chrome, curl, Firefox et al don't wait for the initial SETTINGS
response & ACK before sending their first request, which is why
mod_http2 didn't notice their bug for some time.
This change makes Go's http2.Transport act like other common HTTP/2
clients and not wait for the peer's SETTINGS frame & ACK before
sending the first request.
As a bonus, this reduces the latency for the first request on
connections by one RTT.
The reason we waited for the initial settings is purely
historical. Andrew and I just wrote it that way when initially
developing the client on video (https://www.youtube.com/watch?v=yG-UaBJXZ80)
because we were working top-down through the protocol and didn't
get back to optimizing the RTT away. It also seemed more compliant to
wait for the peer's SETTINGS to know the frame size and such, but as
as spec says, it's permitted for clients to not wait:
http://httpwg.org/specs/rfc7540.html#rfc.section.3.5
> To avoid unnecessary latency, clients are permitted to send
> additional frames to the server immediately after sending the client
> connection preface, without waiting to receive the server connection
> preface. It is important to note, however, that the server
> connection preface SETTINGS frame might include parameters that
> necessarily alter how a client is expected to communicate with the
> server. Upon receiving the SETTINGS frame, the client is expected to
> honor any parameters established. In some configurations, it is
> possible for the server to transmit SETTINGS before the client sends
> additional frames, providing an opportunity to avoid this issue.
So, don't wait.
Fixes golang/go#16519
Change-Id: I916827e49829f7f107a51838512816916fb553fc
Reviewed-on: https://go-review.googlesource.com/25362
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
diff --git a/http2/transport_test.go b/http2/transport_test.go
index 4f3b8a1..614fa44 100644
--- a/http2/transport_test.go
+++ b/http2/transport_test.go
@@ -652,6 +652,19 @@
}
}
+func (ct *clientTester) readNonSettingsFrame() (Frame, error) {
+ for {
+ f, err := ct.fr.ReadFrame()
+ if err != nil {
+ return nil, err
+ }
+ if _, ok := f.(*SettingsFrame); ok {
+ continue
+ }
+ return f, nil
+ }
+}
+
func (ct *clientTester) cleanup() {
ct.tr.CloseIdleConnections()
}
@@ -703,8 +716,12 @@
func testTransportReqBodyAfterResponse(t *testing.T, status int) {
const bodySize = 10 << 20
+ clientDone := make(chan struct{})
ct := newClientTester(t)
ct.client = func() error {
+ defer ct.cc.(*net.TCPConn).CloseWrite()
+ defer close(clientDone)
+
var n int64 // atomic
req, err := http.NewRequest("PUT", "https://dummy.tld/", io.LimitReader(countingReader{&n}, bodySize))
if err != nil {
@@ -745,7 +762,15 @@
for {
f, err := ct.fr.ReadFrame()
if err != nil {
- return err
+ select {
+ case <-clientDone:
+ // If the client's done, it
+ // will have reported any
+ // errors on its side.
+ return nil
+ default:
+ return err
+ }
}
//println(fmt.Sprintf("server got frame: %v", f))
switch f := f.(type) {
@@ -784,7 +809,6 @@
if err := ct.fr.WriteData(f.StreamID, true, nil); err != nil {
return err
}
- return nil
}
default:
return fmt.Errorf("Unexpected client frame %v", f)
@@ -2090,7 +2114,7 @@
// the interesting parts of both.
ct.fr.WriteGoAway(5, ErrCodeNo, []byte(goAwayDebugData))
ct.fr.WriteGoAway(5, goAwayErrCode, nil)
- ct.sc.Close()
+ ct.sc.(*net.TCPConn).CloseWrite()
<-clientDone
return nil
}
@@ -2157,23 +2181,28 @@
<-clientClosed
- f, err := ct.fr.ReadFrame()
- if err != nil {
- return fmt.Errorf("ReadFrame while waiting for RSTStreamFrame: %v", err)
+ waitingFor := "RSTStreamFrame"
+ for {
+ f, err := ct.fr.ReadFrame()
+ if err != nil {
+ return fmt.Errorf("ReadFrame while waiting for %s: %v", waitingFor, err)
+ }
+ if _, ok := f.(*SettingsFrame); ok {
+ continue
+ }
+ switch waitingFor {
+ case "RSTStreamFrame":
+ if rf, ok := f.(*RSTStreamFrame); !ok || rf.ErrCode != ErrCodeCancel {
+ return fmt.Errorf("Expected a WindowUpdateFrame with code cancel; got %v", summarizeFrame(f))
+ }
+ waitingFor = "WindowUpdateFrame"
+ case "WindowUpdateFrame":
+ if wuf, ok := f.(*WindowUpdateFrame); !ok || wuf.Increment != 4999 {
+ return fmt.Errorf("Expected WindowUpdateFrame for 4999 bytes; got %v", summarizeFrame(f))
+ }
+ return nil
+ }
}
- if rf, ok := f.(*RSTStreamFrame); !ok || rf.ErrCode != ErrCodeCancel {
- return fmt.Errorf("Expected a WindowUpdateFrame with code cancel; got %v", summarizeFrame(f))
- }
-
- // And wait for our flow control tokens back:
- f, err = ct.fr.ReadFrame()
- if err != nil {
- return fmt.Errorf("ReadFrame while waiting for WindowUpdateFrame: %v", err)
- }
- if wuf, ok := f.(*WindowUpdateFrame); !ok || wuf.Increment != 4999 {
- return fmt.Errorf("Expected WindowUpdateFrame for 4999 bytes; got %v", summarizeFrame(f))
- }
- return nil
}
ct.run()
}
@@ -2228,7 +2257,7 @@
pad := []byte("12345")
ct.fr.WriteDataPadded(hf.StreamID, false, make([]byte, 5000), pad) // without ending stream
- f, err := ct.fr.ReadFrame()
+ f, err := ct.readNonSettingsFrame()
if err != nil {
return fmt.Errorf("ReadFrame while waiting for first WindowUpdateFrame: %v", err)
}
@@ -2237,7 +2266,7 @@
return fmt.Errorf("Expected conn WindowUpdateFrame for %d bytes; got %v", wantBack, summarizeFrame(f))
}
- f, err = ct.fr.ReadFrame()
+ f, err = ct.readNonSettingsFrame()
if err != nil {
return fmt.Errorf("ReadFrame while waiting for second WindowUpdateFrame: %v", err)
}
