This updates the bundled copy of x/net/http2 to git rev d2ecd08
for https://golang.org/cl/17912 (http2: send client trailers)
and enables the final Trailer test for http2.
Fixes#13557
Change-Id: Iaa15552b82bf7a2cb01b7787a2e1ec5ee680a9d3
Reviewed-on: https://go-review.googlesource.com/17935
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Document that ListenAndServe and ListenAndServeTLS also set TCP
keep-alives.
Fixes#12748
Change-Id: Iba2e8a58dd657eba326db49a6c872e2d972883a4
Reviewed-on: https://go-review.googlesource.com/17681
Reviewed-by: Russ Cox <rsc@golang.org>
I updated this in the previous commit (https://golang.org/cl/17931)
but noticed a typo. and it still wasn't great.
The Go 1.5 text was too brief to know how to use it:
// Trailer maps trailer keys to values, in the same
// format as the header.
Change-Id: I33c49b6a4a7a3596735a4cc7865ad625809da900
Reviewed-on: https://go-review.googlesource.com/17932
Reviewed-by: Russ Cox <rsc@golang.org>
This CL updates the bundled copy of x/net/http2 to include
https://golang.org/cl/17930 and enables the previously-skipped tests
TestTrailersServerToClient_h2 and TestTrailersServerToClient_Flush_h2.
It also updates the docs on http.Response.Trailer to describe how to
use it. No change in rules. Just documenting the old unwritten rules.
(there were tests locking in the behavior, and misc docs and examples
scattered about, but not on http.Response.Trailer itself)
Updates #13557
Change-Id: I6261d439f6c0d17654a1a7928790e8ffed16df6c
Reviewed-on: https://go-review.googlesource.com/17931
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Blake Mizerany <blake.mizerany@gmail.com>
The old test was in client_test.go but was a mix of four things:
- clients writing trailers
- servers reading trailers
- servers writing trailers
- clients reading trailers
It definitely wasn't just about clients.
This moves it into clientserver_test.go and separates it into two
halves:
- servers writing trailers + clients reading trailers
- clients writing trailers + servers reading trailers
Which still isn't ideal, but is much better, and easier to read.
Updates #13557
Change-Id: I8c3e58a1f974c1b10bb11ef9b588cfa0f73ff5d9
Reviewed-on: https://go-review.googlesource.com/17895
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Blake Mizerany <blake.mizerany@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The Transport had a delicate protocol between its readLoop goroutine
and the goroutine calling RoundTrip. The basic concern is that the
caller's RoundTrip goroutine wants to wait for either a
connection-level error (the conn dying) or the response. But sometimes
both happen: there's a valid response (without a body), but the conn
is also going away. Both goroutines' logic dealing with this had grown
large and complicated with hard-to-follow comments over the years.
Simplify and document. Pull some bits into functions and do all
bodyless stuff in one place (it's special enough), rather than having
a bunch of conditionals scattered everywhere. One test is no longer
even applicable since the race it tested is no longer possible (the
code doesn't exist).
The bug that this fixes is that when the Transport reads a bodyless
response from a server, it was returning that response before
returning the persistent connection to the idle pool. As a result,
~1/1000 of serial requests would end up creating a new connection
rather than re-using the just-used connection due to goroutine
scheduling chance. Instead, this now adds bodyless responses'
connections back to the idle pool first, then sends the response to
the RoundTrip goroutine, but making sure that the RoundTrip goroutine
is outside of its select on the connection dying.
There's a new buffered channel involved now, which is a minor
complication, but it's much more self-contained and well-documented
than the previous complexity. (The alternative of making the
responseAndError channel itself unbuffered is too invasive and risky
at this point; it would require a number of changes to avoid
deadlocked goroutines in error cases)
In any case, flakes look to be gone now. We'll see if trybots agree.
Fixes#13633
Change-Id: I95a22942b2aa334ae7c87331fddd751d4cdfdffc
Reviewed-on: https://go-review.googlesource.com/17890
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
This might deflake it. Or it'll at least give us more debugging clues.
Fixes#13626 maybe
Change-Id: Ie8cd0375d60dad033ec6a64830a90e7b9152a3d9
Reviewed-on: https://go-review.googlesource.com/17825
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
CloseNotifier wasn't well specified previously. This CL simplifies its
implementation, clarifies the public documentation on CloseNotifier,
clarifies internal documentation on conn, and fixes two CloseNotifier
bugs in the process.
The main change, though, is tightening the rules and expectations for using
CloseNotifier:
* the caller must consume the Request.Body first (old rule, unwritten)
* the received value is the "true" value (old rule, unwritten)
* no promises for channel sends after Handler returns (old rule, unwritten)
* a subsequent pipelined request fires the CloseNotifier (new behavior;
previously it never fired and thus effectively deadlocked as in #13165)
* advise that it should only be used without HTTP/1.1 pipelining (use HTTP/2
or non-idempotent browsers). Not that browsers actually use pipelining.
The main implementation change is that each Handler now gets its own
CloseNotifier channel value, rather than sharing one between the whole
conn. This means Handlers can't affect subsequent requests. This is
how HTTP/2's Server works too. The old docs never clarified a behavior
either way. The other side effect of each request getting its own
CloseNotifier channel is that one handler can't "poison" the
underlying conn preventing subsequent requests on the same connection
from using CloseNotifier (this is #9763).
In the old implementation, once any request on a connection used
ClosedNotifier, the conn's underlying bufio.Reader source was switched
from the TCPConn to the read side of the pipe being fed by a
never-ending copy. Since it was impossible to abort that never-ending
copy, we could never get back to a fresh state where it was possible
to return the underlying TCPConn to callers of Hijack. Now, instead of
a never-ending Copy, the background goroutine doing a Read from the
TCPConn (or *tls.Conn) only reads a single byte. That single byte
can be in the request body, a socket timeout error, io.EOF error, or
the first byte of the second body. In any case, the new *connReader
type stitches sync and async reads together like an io.MultiReader. To
clarify the flow of Read data and combat the complexity of too many
wrapper Reader types, the *connReader absorbs the io.LimitReader
previously used for bounding request header reads. The
liveSwitchReader type is removed. (an unused switchWriter type is also
removed)
Many fields on *conn are also documented more fully.
Fixes#9763 (CloseNotify + Hijack together)
Fixes#13165 (deadlock with CloseNotify + pipelined requests)
Change-Id: I40abc0a1992d05b294d627d1838c33cbccb9dd65
Reviewed-on: https://go-review.googlesource.com/17750
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Updates bundled copy of x/net/http2 to include
https://golang.org/cl/17823 (catching panics in Handlers)
Fixes#13555
Change-Id: I08e4e38e736a8d93f5ec200e8041c143fc6eafce
Reviewed-on: https://go-review.googlesource.com/17824
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This CL also updates the bundled http2 package with the h2 fix from
https://golang.org/cl/17757Fixes#13159
Change-Id: If0e3b4bd04d0dceed67d1b416ed838c9f1961576
Reviewed-on: https://go-review.googlesource.com/17758
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
New implementation of TimeoutHandler: buffer everything to memory.
All or nothing: either the handler finishes completely within the
timeout (in which case the wrapper writes it all), or it misses the
timeout and none of it gets written, in which case handler wrapper can
reliably print the error response without fear that some of the
wrapped Handler's code already wrote to the output.
Now the goroutine running the wrapped Handler has its own write buffer
and Header copy.
Document the limitations.
Fixes#9162
Change-Id: Ia058c1d62cefd11843e7a2fc1ae1609d75de2441
Reviewed-on: https://go-review.googlesource.com/17752
Reviewed-by: David Symonds <dsymonds@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Host names in URLs must not use %-escaping for ASCII bytes, per RFC 3986.
url.Parse has historically allowed spaces and < > " in the URL host.
In Go 1.5, URL's String method started escaping those,
but then Parse would rejects the escaped form.
This CL is an attempt at some consistency between Parse and String
as far as the accepted host characters and the encoding of host characters,
so that if Parse succeeds, then Parse -> String -> Parse also succeeds.
Allowing space seems like a mistake, so reject that in Parse.
(Similarly, reject \t, \x01, and so on, all of which were being allowed.)
Allowing < > " doesn't seem awful, so continue to do that,
and go back to the Go 1.4 behavior of not escaping them in String.
Fixes#11302.
Change-Id: I0bf65b874cd936598f20694574364352a5abbe5f
Reviewed-on: https://go-review.googlesource.com/17387
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Fixes some failing Google tests when run under Go tip (1.6).
Updates #12986
Change-Id: I0ca4d20f6103d10ea9464e45730085401336dada
Reviewed-on: https://go-review.googlesource.com/17698
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Nodir Turakulov <nodir@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Until recently, we always permitted an empty string to NewRequest.
Keep that property, since it broke tests within in Google when trying
out Go 1.6, and probably would've broken others too.
Change-Id: Idddab1ae7b9423d5caac00af2c897fe1065b600b
Reviewed-on: https://go-review.googlesource.com/17699
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This is an example of converting an old HTTP/1-only test to test
against both HTTP/1 and HTTP/2.
Please send more of these!
Also, for comparing the http.Transport's responses between HTTP/1 and
HTTP/2, see clientserver_test.go's h12Compare type and tests using
h12Compare. Sometimes that's the more appropriate option.
Change-Id: Iea24d844481efd5849173b60e15dcc561a32b88f
Reviewed-on: https://go-review.googlesource.com/17409
Reviewed-by: Burcu Dogan <jbd@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This makes TestTransportResponseCloseRace much faster and no longer
flaky.
In the process it also cleans up test hooks in net/http which were
inconsistent and scattered.
Change-Id: Ifd0b11dbc7e8915c24eb5bdc36731ed6751dd7ec
Reviewed-on: https://go-review.googlesource.com/17316
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
This mirrors the same behavior and API from the server code to the
client side: if TLSNextProto is nil, HTTP/2 is on by default for
both. If it's non-nil, the user was trying to do something fancy and
step out of their way.
Updates #6891
Change-Id: Ia31808b71f336a8d5b44b985591d72113429e1d4
Reviewed-on: https://go-review.googlesource.com/17300
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
If we try to reuse a connection that the server is in the process of
closing, we may end up successfully writing out our request (or a
portion of our request) only to find a connection error when we try to
read from (or finish writing to) the socket. This manifests as an EOF
returned from the Transport's RoundTrip.
The issue, among others, is described in #4677.
This change follows some of the Chromium guidelines for retrying
idempotent requests only when the connection has been already been used
successfully and no header data has yet been received for the response.
As part of this change, an unexported error was defined for
errMissingHost, which was previously defined inline. errMissingHost is
the only non-network error returned from a Request's Write() method.
Additionally, this breaks TestLinuxSendfile because its test server
explicitly triggers the type of scenario this change is meant to retry
on. Because that test server stops accepting conns on the test listener
before the retry, the test would time out. To fix this, the test was
altered to use a non-idempotent test type (POST).
Change-Id: I1ca630b944f0ed7ec1d3d46056a50fb959481a16
Reviewed-on: https://go-review.googlesource.com/3210
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
And updates h2_bundle.go with the fix from x/net/http2.
Fixes#13298
Updates #6891
Change-Id: Ia25f22fa10e2a64b9d59211269882681aa18c101
Reviewed-on: https://go-review.googlesource.com/17241
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>
This compares the behavior of server handlers and the net/http
Transport in both HTTP/1 and HTTP/2 mode and verifies they're the
same.
This also moves some client<->server tests into clientserver_test.go.
Many of them were in serve_test.go or transport_test.go but were
basically testing both.
h2_bundle.go is an update of the golang.org/x/net/http2 code
from https://golang.org/cl/17204 (x/net git rev c745c36eab10)
Fixes#13315Fixes#13316Fixes#13317
Fixes other stuff found in the process too
Updates #6891 (http2 support in general)
Change-Id: Id9c45fad44cdf70ac95d2b89e578d66e882d3cc2
Reviewed-on: https://go-review.googlesource.com/17205
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
