If the client sends a request with a custom Host header and receives
a relative redirect in response, the second request should use the
same Host header as the first request. However, if the response is
an abolute redirect, the Host header should not be preserved. See
further discussion on the issue tracker.
Fixes#22233
Change-Id: I8796e2fbc1c89b3445e651f739d5d0c82e727c14
Reviewed-on: https://go-review.googlesource.com/70792
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Change-Id: Ic3f7f575d3640706adb7d64545ed8027add6c58f
Reviewed-on: https://go-review.googlesource.com/61350
Run-TryBot: Tom Bergan <tombergan@google.com>
Reviewed-by: Tom Bergan <tombergan@google.com>
This is another attempt at the change attempted in
https://golang.org/cl/27117 and rolled back in https://golang.org/cl/34134
The difference between this and the previous attempt is that this version only
retries if the new field GetBody is set on the Request.
Additionally, this allows retries of requests with idempotent methods even if
they have bodies, as long as GetBody is defined.
This also fixes an existing bug where readLoop could make a redundant call to
setReqCanceler for DELETE/POST/PUT/etc requests with no body with zero bytes
written.
This clarifies the existing TestRetryIdempotentRequestsOnError test (and changes
it into a test with 4 subtests). When that test was written, it was in fact
testing "retry idempotent requests" logic, but the logic had changed since then,
and it was actually testing "retry requests with no body when no bytes have been
written". (You can confirm this by changing the existing test from a GET to a
DELETE; it passes without the changes in this CL.) We now test for the no-Body
and GetBody cases for both idempotent and nothing-written-non-idempotent
requests.
Fixes#18241Fixes#17844
Change-Id: I69a48691796f6dc08c31f7aa7887b7dfd67e278a
Reviewed-on: https://go-review.googlesource.com/42142
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
After merging https://go-review.googlesource.com/c/34639/,
it was pointed out to me that a lot of tests under net/http
could use the new functionality to simplify and unify testing.
Using the httptest.Server provided Client removes the need to
call CloseIdleConnections() on all Transports created, as it
is automatically called on the Transport associated with the
client when Server.Close() is called.
Change the transport used by the non-TLS
httptest.Server to a new *http.Transport rather than using
http.DefaultTransport implicitly. The TLS version already
used its own *http.Transport. This change is to prevent
concurrency problems with using DefaultTransport implicitly
across several httptest.Server's.
Add tests to ensure the httptest.Server.Client().Transport
RoundTripper interface is implemented by a *http.Transport,
as is now assumed across large parts of net/http tests.
Change-Id: I9f9d15f59d72893deead5678d314388718c91821
Reviewed-on: https://go-review.googlesource.com/37771
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
If I put a 10 millisecond sleep at testHookWaitResLoop, before the big
select in (*persistConn).roundTrip, two flakes immediately started
happening, TestTransportBodyReadError (#19231) and
TestTransportPersistConnReadLoopEOF.
The problem was that there are many ways for a RoundTrip call to fail
(errors reading from Request.Body while writing the response, errors
writing the response, errors reading the response due to server
closes, errors due to servers sending malformed responses,
cancelations, timeouts, etc.), and many of those failures then tear
down the TCP connection, causing more failures, since there are always
at least three goroutines involved (reading, writing, RoundTripping).
Because the errors were communicated over buffered channels to a giant
select, the error returned to the caller was a function of which
random select case was called, which was why a 10ms delay before the
select brought out so many bugs. (several fixed in my previous CLs the past
few days).
Instead, track the error explicitly in the transportRequest, guarded
by a mutex.
In addition, this CL now:
* differentiates between the two ways writing a request can fail: the
io.Copy reading from the Request.Body or the io.Copy writing to the
network. A new io.Reader type notes read errors from the
Request.Body. The read-from-body vs write-to-network errors are now
prioritized differently.
* unifies the two mapRoundTripErrorFromXXX methods into one
mapRoundTripError method since their logic is now the same.
* adds a (*Request).WithT(*testing.T) method in export_test.go, usable
by tests, to call t.Logf at points during RoundTrip. This is disabled
behind a constant except when debugging.
* documents and deflakes TestClientRedirectContext
I've tested this CL with high -count values, with/without -race,
with/without delays before the select, etc. So far it seems robust.
Fixes#19231 (TestTransportBodyReadError flake)
Updates #14203 (source of errors unclear; they're now tracked more)
Updates #15935 (document Transport errors more; at least understood more now)
Change-Id: I3cccc3607f369724b5344763e35ad2b7ea415738
Reviewed-on: https://go-review.googlesource.com/37495
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
In an unrelated CL I found a way to increase the likelihood of latent
flaky tests and found this one.
This is just like yesterday's https://golang.org/cl/37624 and dozens
before it (all remnants from the great net/http test parallelization
of Nov 2016 in https://golang.org/cl/32684).
Change-Id: I3fe61d1645062e5109206ff27d74f573ef6ebb2e
Reviewed-on: https://go-review.googlesource.com/37627
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
This was a t.Parallel test but it was using the global DefaultTransport
via the global Get func.
Use a private Transport that won't have its CloseIdleConnections etc
methods called by other tests.
(I hit this flake myself while testing a different change.)
Change-Id: If0665e3e8580ee198f8e5f3079bfaea55f036eca
Reviewed-on: https://go-review.googlesource.com/37624
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
The presence of Request.GetBody being set on a request was causing all
redirected requests to have a body, even if the redirect status didn't
warrant one.
This bug came from 307/308 support (https://golang.org/cl/29852) which
removed the line that set req.Body to nil after POST/PUT redirects.
Change-Id: I2a4dd5320f810ae25cfd8ea8ca7c9700e5dbd369
Reviewed-on: https://go-review.googlesource.com/35633
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
In Go1.7, a 301, 302, or 303 redirect on a HEAD method, would still
cause the following redirects to still use a HEAD.
In CL/29852 this behavior was changed such that those codes always
caused a redirect with the GET method. Fix this such that both
GET and HEAD will preserve the method.
Fixes#18570
Change-Id: I4bfe69872a30799419e3fad9178f907fe439b449
Reviewed-on: https://go-review.googlesource.com/34981
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This rolls back https://golang.org/cl/27117 partly, softening it so it
only retries POST/PUT/DELETE etc requests where there's no Body (nil
or NoBody). This is a little useless, since most idempotent requests
have a body (except maybe DELETE), but it's late in the Go 1.8 release
cycle and I want to do the proper fix.
The proper fix will look like what we did for http2 and only retrying
the request if Request.GetBody is defined, and then creating a new request
for the next attempt. See https://golang.org/cl/33971 for the http2 fix.
Updates #15723Fixes#18239
Updates #18241
Change-Id: I6ebaa1fd9b19b5ccb23c8d9e7b3b236e71cf57f3
Reviewed-on: https://go-review.googlesource.com/34134
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>
This test was only enabled by default today so it hasn't been hardened
by build.golang.org. Welcome to the ring, TestClientTimeout.
Change-Id: I1967f6c825699f13f6c659dc14d3c3c22b965272
Reviewed-on: https://go-review.googlesource.com/33101
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Fix another case of a parallel test relying on a global variable
(DefaultTransport) implicitly.
Use the private Transport already in scope instead. It's closed at the
end, instead of randomly via another test.
Change-Id: I95e51926177ad19a766cabbb306782ded1bbb59b
Reviewed-on: https://go-review.googlesource.com/32913
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
A few tests were using the global DefaultTransport implicitly.
Be explicit instead. And then make some parallel while I'm there.
Change-Id: I3c617e75429ecc8f6d23567d1470f5e5d0cb7cfd
Reviewed-on: https://go-review.googlesource.com/32758
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Before: 8.9 seconds for go test -short
After: 2.8 seconds
There are still 250 tests without t.Parallel, but I got the important
onces using a script:
$ go test -short -v 2>&1 | go run ~/slowtests.go
Where slowtests.go is https://play.golang.org/p/9mh5Wg1nLN
The remaining 250 (the output lines from slowtests.go) all have a
reported duration of 0ms, except one 50ms test which has to be serial.
Where tests can't be parallel, I left a comment at the top saying why,
so people don't add t.Parallel later and get surprised at failures.
Updates #17751
Change-Id: Icbe32cbe2b996e23c89f1af6339287fa22af5115
Reviewed-on: https://go-review.googlesource.com/32684
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
This CL tweaks the new (unreleased) 307/308 support added in
https://golang.org/cl/29852 for #10767.
Change 1: if a 307/308 response doesn't have a Location header in its
response (as observed in the wild in #17773), just do what we used to
do in Go 1.7 and earlier, and don't try to follow that redirect.
Change 2: don't follow a 307/308 if we sent a body on the first
request and the caller's Request.GetBody func is nil so we can't
"rewind" the body to send it again.
Updates #17773 (will be fixed more elsewhere)
Change-Id: I183570f7346917828a4b6f7f1773094122a30406
Reviewed-on: https://go-review.googlesource.com/32595
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Provides redirection support for 307, 308 server statuses.
Provides redirection support for DELETE method.
Updates old tests that assumed all redirects were treated
the way 301, 302 and 303 are processed.
Fixes#9348Fixes#10767Fixes#13994
Change-Id: Iffa8dbe0ff28a1afa8da59869290ec805b1dd2c4
Reviewed-on: https://go-review.googlesource.com/29852
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
In the situation where the Client.Jar is set and the Request.Header
has cookies manually inserted, the redirect logic needs to be
able to apply changes to cookies from "Set-Cookie" headers to both
the Jar and the manually inserted Header cookies.
Since Header cookies lack information about the original domain
and path, the logic in this CL simply removes cookies from the
initial Header if any subsequent "Set-Cookie" matches. Thus,
in the event of cookie conflicts, the logic preserves the behavior
prior to change made in golang.org/cl/28930.
Fixes#17494
Updates #4800
Change-Id: I645194d9f97ff4d95bd07ca36de1d6cdf2f32429
Reviewed-on: https://go-review.googlesource.com/31435
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Copy all of the original request's headers on redirect, unless they're
sensitive. Only send sensitive ones to the same origin, or subdomains
thereof.
Fixes#4800
Change-Id: Ie9fa75265c9d5e4c1012c028d31fd1fd74465712
Reviewed-on: https://go-review.googlesource.com/28930
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Reviewed-by: Francesc Campoy Flores <campoy@golang.org>
Reviewed-by: Ross Light <light@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This permits the error message to distinguish between a context that was
canceled and a context that timed out.
Updates #16381.
Change-Id: I3994b98e32952abcd7ddb5fee08fa1535999be6d
Reviewed-on: https://go-review.googlesource.com/24978
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
On HTTP redirect, the HTTP client creates a new request and don't copy
over the Cancel channel. This prevents any redirected request from being
cancelled.
Fixes#14053
Change-Id: I467cdd4aadcae8351b6e9733fc582b7985b8b9d3
Reviewed-on: https://go-review.googlesource.com/18810
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
In the beginning, there was no way to cancel an HTTP request.
We later added Transport.CancelRequest to cancel an in-flight HTTP
request by breaking its underlying TCP connection, but it was hard to
use correctly and didn't work in all cases. And its error messages
were terrible. Some of those issues were fixed over time, but the most
unfixable problem was that it didn't compose well. All RoundTripper
implementations had to choose to whether to implement CancelRequest
and both decisions had negative consequences.
In Go 1.5 we added Request.Cancel, which composed well, worked in all
phases, had nice error messages, etc. But we forgot to use it in the
implementation of Client.Timeout (a timeout which spans multiple
requests and reading request bodies).
In Go 1.6 (upcoming), we added HTTP/2 support, but now Client.Timeout
didn't work because the http2.Transport didn't have a CancelRequest
method.
Rather than add a CancelRequest method to http2, officially deprecate
it and update the only caller (Client, for Client.Cancel) to use
Request.Cancel instead.
The http2 Client timeout tests are enabled now.
For compatibility, we still use CancelRequest in Client if we don't
recognize the RoundTripper type. But documentation has been updated to
tell people that CancelRequest is deprecated.
Fixes#13540
Change-Id: I15546b90825bb8b54905e17563eca55ea2642075
Reviewed-on: https://go-review.googlesource.com/18260
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
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>
Inspect the crypto/tls error to recognize this case and give a more
helpful error.
Fixes#11111.
Change-Id: I63f6af8c375aa892326ccccbd29655d54d68df0b
Reviewed-on: https://go-review.googlesource.com/16079
Run-TryBot: Caleb Spare <cespare@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(This relands commit a4dcc69201.)
https://tools.ietf.org/html/rfc6066#section-3 states:
“Literal IPv4 and IPv6 addresses are not permitted in "HostName".”
However, if an IP literal was set as Config.ServerName (which could
happen as easily as calling Dial with an IP address) then the code would
send the IP literal as the SNI value.
This change filters out IP literals, as recognised by net.ParseIP, from
being sent as the SNI value.
Fixes#13111.
Change-Id: I6e544a78a01388f8fe98150589d073b917087f75
Reviewed-on: https://go-review.googlesource.com/16776
Run-TryBot: Adam Langley <agl@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Fixes#12866
net/http.Client returns some errors wrapped in a *url.Error. To avoid
the requirement to unwrap these errors to determine if the cause was
temporary or a timeout, make *url.Error implement net.Error directly.
Change-Id: I1ba84ecc7ad5147a40f056ff1254e60290152408
Reviewed-on: https://go-review.googlesource.com/15672
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
The one in misc/makerelease/makerelease.go is particularly bad and
probably warrants rotating our keys.
I didn't update old weekly notes, and reverted some changes involving
test code for now, since we're late in the Go 1.5 freeze. Otherwise,
the rest are all auto-generated changes, and all manually reviewed.
Change-Id: Ia2753576ab5d64826a167d259f48a2f50508792d
Reviewed-on: https://go-review.googlesource.com/12048
Reviewed-by: Rob Pike <r@golang.org>
These were found by grepping the comments from the go code and feeding
the output to aspell.
Change-Id: Id734d6c8d1938ec3c36bd94a4dbbad577e3ad395
Reviewed-on: https://go-review.googlesource.com/10941
Reviewed-by: Aamir Khan <syst3m.w0rm@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
When running the client header timeout test, there is a race between
us timing out and waiting on the remaining requests to be serviced. If
the client times out before the server blocks on the channel in the
handler, we will be simultaneously adding to a waitgroup with the
value 0 and waiting on it when we call TestServer.Close().
This is largely a theoretical race. We have to time out before we
enter the handler and the only reason we would time out if we're
blocked on the channel. Nevertheless, make the race detector happy
by turning the close into a channel send. This turns the defer call
into a synchronization point and we can be sure that we've entered
the handler before we close the server.
Fixes#10780
Change-Id: Id73b017d1eb7503e446aa51538712ef49f2f5c9e
Reviewed-on: https://go-review.googlesource.com/9905
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
