Unlike request headers, where we are allowed to leniently accept
a bare LF in place of a CRLF, chunked bodies must always use CRLF
line terminators. We were already enforcing this for chunk-data lines;
do so for chunk-size lines as well. Also reject bare CRs anywhere
other than as part of the CRLF terminator.
Fixes CVE-2025-22871
Fixes#72011
For #71988
Change-Id: Ib0e21af5a8ba28c2a1ca52b72af8e2265ec79e4a
Reviewed-on: https://go-review.googlesource.com/c/go/+/652998
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit d31c805535f3fde95646ee4d87636aaaea66847b)
Reviewed-on: https://go-review.googlesource.com/c/go/+/657056
Clone the input slice before adjusting NextProtos
to add or remove "http/1.1" and "h2" entries,
so as not to modify a slice that the caller might be using.
(We clone the tls.Config that contains the slice, but
that's a shallow clone.)
For #72100Fixes#72103
Change-Id: I9f228b8fb6f6f2ca5023179ec114929c002dbda9
Reviewed-on: https://go-review.googlesource.com/c/go/+/654875
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/657215
Responses to HEAD requests don't have a body.
The ResponseWriter automatically discards writes to the response body
when responding to a HEAD request. ResponseWriter.ReadFrom was failing
to discard writes under some circumstances; fix it to do so.
Fixes#68609
Change-Id: I912f6b2b2a535df28ae37b875fcf15b10da1af2b
Reviewed-on: https://go-review.googlesource.com/c/go/+/601475
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Auto-Submit: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Replace reflect.DeepEqual with slices.Equal/maps.Equal, which is
much faster.
Change-Id: I54600fb63a56460c11d3d5af9072da585e31b1a2
GitHub-Last-Rev: 08c1445ad5be94d071e8ceb4b060b8f4ab0d77ba
GitHub-Pull-Request: golang/go#67606
Reviewed-on: https://go-review.googlesource.com/c/go/+/587816
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
This CL is similar to CL 562557, and it takes over CL 594175.
While here, unrelatedly remove mapKeys function, use slices.Sorted(maps.Keys(ms))
to simplify code.
Fixes#67657
Change-Id: Id8b99216f87a6dcfd6d5fa61407b515324c79112
Reviewed-on: https://go-review.googlesource.com/c/go/+/594737
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Joedian Reid <joedian@google.com>
This reverts the changes to Error from CL 571995, and adds a
GODEBUG controlling the changes to ServeContent/ServeFile/ServeFS.
The change to remove the Content-Encoding header when serving an error
breaks middleware which sets Content-Encoding: gzip and wraps a
ResponseWriter in one which compresses the response body.
This middleware already breaks when ServeContent handles a Range request.
Correct uses of ServeContent which serve pre-compressed content with
a Content-Encoding: gzip header break if we don't remove that header
when serving errors. Therefore, we keep the change to ServeContent/
ServeFile/ServeFS, but we add the ability to disable the new behavior
by setting GODEBUG=httpservecontentkeepheaders=1.
We revert the change to Error, because users who don't want to include
a Content-Encoding header in errors can simply remove the header
themselves, or not add it in the first place.
Fixes#66343
Change-Id: Ic19a24b73624a5ac1a258ed7a8fe7d9bf86c6a38
Reviewed-on: https://go-review.googlesource.com/c/go/+/593157
Reviewed-by: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Just like for tls.Config.GetCertificate the http.Server.ServeTLS method
should be checking tls.Config.GetConfigForClient before trying top open
the specified certFile/keyFile.
This was previously fixed for crypto/tls when using tls.Listen in
CL205059, but the same change for net/http was missed. I've added a
comment src/crypto/tls/tls.go in the relevant section in the hope that
any future changes of a similar nature consider will consider updating
net/http as needed as well.
Change-Id: I312303bc497d92aa2f4627fe2620c70779cbcc99
GitHub-Last-Rev: 6ed29a900816a13690a9f3e26476d9bc1055a6f7
GitHub-Pull-Request: golang/go#66795
Reviewed-on: https://go-review.googlesource.com/c/go/+/578396
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
In all cases the intent was not to interpret s as a format string.
In one case (go/types), this was a latent bug in production.
(These were uncovered by a new check in vet's printf analyzer.)
Updates #60529
Change-Id: I3e17af7e589be9aec1580783a1b1011c52ec494b
Reviewed-on: https://go-review.googlesource.com/c/go/+/587855
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Russ Cox <rsc@golang.org>
Disable three 100-continue tests that aren't exercising the
intended behavior because they don't set ExpectContinueTimeout.
The tests are flaky right now; setting ExpectContinueTimeout
makes them consistently fail.
Set ExpectContinueTimeout and t.Skip the tests for now.
Fixes#67382
For #67555
Change-Id: I459a19a927e14af03881e89c73d20c93cf0da43e
Reviewed-on: https://go-review.googlesource.com/c/go/+/587155
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
When a request contains an "Expect: 100-continue" header,
the first read from the request body causes the server to
write a 100-continue status.
This write caused a panic when performed after the server handler
has exited. Disable the write when cleaning up after a handler
exits.
This also fixes a bug where an implicit 100-continue could be
sent after a call to WriteHeader has sent a non-1xx header.
This change drops tracking of whether we've written a
100-continue or not in response.wroteContinue. This tracking
was used to determine whether we should consume the remaining
request body in chunkWriter.writeHeader, but the discard-the-body
path was only taken when the body was already consumed.
(If the body is not consumed, we set closeAfterReply, and we
don't consume the remaining body when closeAfterReply is set.
If the body is consumed, then we may attempt to discard the
remaining body, but there is obviously no body remaining.)
Fixes#53808
Change-Id: I3542df26ad6cdfe93b50a45ae2d6e7ef031e46fa
Reviewed-on: https://go-review.googlesource.com/c/go/+/585395
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
This is a reapply of CL 544019 and CL 569815, but with
less aggressive semantics as discussed in proposal #66343.
Error deletes Content-Encoding, since it is writing the response
and any preset encoding may not be correct.
On the error-serving path in ServeContent/ServeFile/ServeFS,
these functions delete additional headers: Etag, Last-Modified,
and Cache-Control. The caller may have set these intending
them for the success response, and they may well not be correct
for error responses.
Fixes#50905.
Fixes#66343.
Change-Id: I873d33edde1805990ca16d85ea8d7735b7448626
Reviewed-on: https://go-review.googlesource.com/c/go/+/571995
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Surprisingly, newClientServerTest doesn't ensure that server handlers
are done in its t.Cleanup function. This test's handler can outlive
the test and attempt to log after the test has completed, causing
race detector failures.
Add an explicit call to Server.Shutdown to ensure the handler
has completed.
We should also probably add a Shutdown to clientServerTest.close,
but that's a larger change; this fixes the immediate problem.
Change-Id: Ibe81b4b382c9c8a920b0ff5f76dea6afe69b10f5
Reviewed-on: https://go-review.googlesource.com/c/go/+/573895
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Damien Neil <dneil@google.com>
Change-Id: I38ebd280c200b30692eb35640327034a5e898bd0
Reviewed-on: https://go-review.googlesource.com/c/go/+/570376
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: David Chase <drchase@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This reverts commit 2b58355ef624239dbe32185dc8dfc9d1074615c6.
Reason for revert: This breaks tons of tests for no real reason.
Change-Id: I89773f48cf983c0b6346e46c37a0ebbe2620e3b4
Reviewed-on: https://go-review.googlesource.com/c/go/+/571675
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Error replies to a request with an error message and HTTP code.
Delete any preexisting Content-Length header before writing the header;
if a Content-Length is present, it's probably for content that the
caller has given up on writing.
For #50905
Change-Id: Ia3d4ca008be46fa5d41afadf29ca5cacb1c47660
Reviewed-on: https://go-review.googlesource.com/c/go/+/554216
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
This breaks an unbounded client-side retry loop if the server's
timeout happens to fire during its final read of the TLS handshake.
The retry loop was observed on wasm platforms at CL 557437.
I was also able to reproduce chains of dozens of retries on my
linux/amd64 workstation by adjusting some timeouts and adding a couple
of sleeps, as in this patch:
https://gist.github.com/bcmills/d0a0a57e5f64eebc24e8211d8ea502b3
However, on linux/amd64 on my workstation the test always eventually
breaks out of the retry loop due to timing jitter.
I couldn't find a retry-specific hook in the http.Client,
http.Transport, or tls.Config structs, so I have instead abused the
Transport.Proxy hook for this purpose. Separately, we may want to
consider adding a retry-specific hook, or changing the net/http
implementation to avoid transparently retrying in this case.
Fixes#65410.
Updates #65178.
Change-Id: I0e43c039615fe815f0a4ba99a8813c48b1fdc7e6
Reviewed-on: https://go-review.googlesource.com/c/go/+/559835
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
According to RFC 9110 and RFC 9112, invalid "Content-Length" headers
might involve request smuggling or response splitting, which could
also cause security failures. Currently, `net/http` ignores all
"Content-Length" headers when there is a "Transfer-Encoding" header and
forward the message anyway while other mainstream HTTP implementations
such as Apache Tomcat, Nginx, HAProxy, Node.js, Deno, Tornado, etc. reject
invalid Content-Length headers regardless of the presence of a
"Transfer-Encoding" header and only forward chunked-encoding messages
with either valid "Content-Length" headers or no "Content-Length" headers.
Fixes#65505
Change-Id: I73af2ee0785137e56c7546a4cce4a5c5c348dbc5
Reviewed-on: https://go-review.googlesource.com/c/go/+/561075
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Do not add a trailing slash and redirect if the path already
ends in a slash.
Also, and unrelatedly, add a test for cleanPath.
Fixes#65624.
Change-Id: Ifcf9edc929d2eb6db88132c09d2bade85c5dda3d
Reviewed-on: https://go-review.googlesource.com/c/go/+/562557
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
According to RFC 7230, empty field names in HTTP header are invalid.
However, there are no specific instructions for developers to deal
with that kind of case in the specification. CL 11242 chose to skip
it and do nothing about it, which now seems like a bad idea because
it has led `net/http` to behave inconsistently with the most widely-used
HTTP implementations: Apache, Nginx, Node with llhttp, H2O, Lighttpd, etc.
in the case of empty header keys.
There is a very small chance that this CL will break a few existing HTTP clients.
Fixes#65244
Change-Id: Ie01e9a6693d27caea4d81d1539345cf42b225535
Reviewed-on: https://go-review.googlesource.com/c/go/+/558095
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This also allows us to remove the chanWriter helper from the test,
using a simpler strings.Builder instead, relying on
clientServerTest.close for synchronization.
(I don't think this runs afoul of #38370, because the handler
functions themselves in these tests should never be executed,
let alone result in an asynchronous write to the error log.)
Fixes#57599.
Change-Id: I45c6cefca0bb218f6f9a9659de6bde454547f704
Reviewed-on: https://go-review.googlesource.com/c/go/+/539436
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This race was reported in
https://build.golang.org/log/6f043170946b665edb85b50804a62db68348c52f.
As best as I can tell, it is another instance of #38370. The deferred
call to backend.close() ought to be enough to ensure that the t.Logf
happens before the end of the test, but in practice it is not, and
with enough scheduling delay we can manage to trip the race detector
on a call to Logf after the test function has returned.
Updates #38370.
Change-Id: I5ee45df45c6bfad3239d665df65a138f1c4573a3
Reviewed-on: https://go-review.googlesource.com/c/go/+/531195
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Previously the test used an unbuffered channel, but testConn.Close
sends to it with a select-with-default, so the send would be dropped
if the test goroutine happened not to have parked on the receive yet.
To make this kind of bug less likely in future tests, use a
newTestConn helper function instead of constructing testConn channel
literals in each test individually.
Fixes#62622.
Change-Id: I016cd0a89cf8a2a748ed57a4cdbd01a178f04dab
Reviewed-on: https://go-review.googlesource.com/c/go/+/529475
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
If the call to ReadString returns an error, the closure in
testServerGracefulClose will return an error and retry the test with a
longer timeout. If that happens, we need to wait for the conn.Write
goroutine to complete so that we don't leak connections across tests.
Updates #57084.
Fixes#62643.
Change-Id: Ia86c1bbd0a5e5d0aeccf4dfeb994c19d1fb10b00
Reviewed-on: https://go-review.googlesource.com/c/go/+/528398
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
This suppresses the race reported in #62638.
I am not 100% certain how that race happens, but here is my theory.
The increment of reqNum happens before the server writes the response
headers, and the server necessarily writes the headers before the
client receives them. However, that write/read pair occurs through I/O
syscalls rather than Go synchronization primitives, so it doesn't
necessarily create a “happens before” relationship as defined by the
Go memory model: although we can establish a sequence of events, that
sequence is not visible to the race detector, nor to the compiler.
Fixes#62638.
Change-Id: I90d66ec3fc32b9b8e1f9bbf0bc2eb289b964b99b
Reviewed-on: https://go-review.googlesource.com/c/go/+/528475
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
As far as I can tell, some flakiness is unavoidable in tests
that race a large client request write against a server's response
when the server doesn't read the full request.
It does not appear to be possible to simultaneously ensure that
well-behaved clients see EOF instead of ECONNRESET and also prevent
misbehaving clients from consuming arbitrary server resources.
(See RFC 7230 §6.6 for more detail.)
Since there doesn't appear to be a way to cleanly eliminate
this source of flakiness, we can instead work around it:
we can allow the test to adjust the hard-coded delay if it
sees a plausibly-related failure, so that the test can retry
with a longer delay.
As a nice side benefit, this also allows the tests to run more quickly
in the typical case: since the test will retry in case of spurious
failures, we can start with an aggressively short delay, and only back
off to a longer one if it is really needed on the specific machine
running the test.
Fixes#57084.
Fixes#51104.
For #58398.
Change-Id: Ia4050679f0777e5eeba7670307a77d93cfce856f
Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest-race,gotip-linux-amd64-race,gotip-windows-amd64-race
Reviewed-on: https://go-review.googlesource.com/c/go/+/527196
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
It's shorter and can't accidentally match unlikely test names.
Change-Id: I96dd9da018cad1acf604f266819470278f54c128
Reviewed-on: https://go-review.googlesource.com/c/go/+/524949
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Use ^ and $ in the -run flag regular expression value when the intention
is to invoke a single named test. This removes the reliance on there not
being another similarly named test to achieve the intended result.
In particular, package syscall has tests named TestUnshareMountNameSpace
and TestUnshareMountNameSpaceChroot that both trigger themselves setting
GO_WANT_HELPER_PROCESS=1 to run alternate code in a helper process. As a
consequence of overlap in their test names, the former was inadvertently
triggering one too many helpers.
Spotted while reviewing CL 525196. Apply the same change in other places
to make it easier for code readers to see that said tests aren't running
extraneous tests. The unlikely cases of -run=TestSomething intentionally
being used to run all tests that have the TestSomething substring in the
name can be better written as -run=^.*TestSomething.*$ or with a comment
so it is clear it wasn't an oversight.
Change-Id: Iba208aba3998acdbf8c6708e5d23ab88938bfc1e
Reviewed-on: https://go-review.googlesource.com/c/go/+/524948
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Kirill Kolyshkin <kolyshkin@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This test can return with a Transport still processing
an in-flight request, resulting in a test failure due
to the leaked Transport.
Avoid this by waiting for the Transport to close the
request body before returning.
Fixes#60264
Change-Id: I8d8b54f633c2e28da2b1bf1bc01ce09dd77769de
Reviewed-on: https://go-review.googlesource.com/c/go/+/522695
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
On Unix platforms, testenv.Command sends SIGQUIT to stuck commands
before the test times out. For subprocesses that are written in Go,
that causes the runtime to dump running goroutines, and in other
languages it triggers similar behavior (such as a core dump).
If the subprocess is stuck due to a bug (such as #57999), that may
help to diagnose it.
For #57999.
Change-Id: Ia2e9d14718a26001e030e162c69892497a8ebb21
Reviewed-on: https://go-review.googlesource.com/c/go/+/521816
Reviewed-by: Damien Neil <dneil@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Failing to close the response body before returning leaks
the in-progress request past the test lifetime.
Fixes#60264
Change-Id: Ic327d9f8e02e87ed656324aaa042f833d9ea18ca
Reviewed-on: https://go-review.googlesource.com/c/go/+/501309
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
This is a second round of CL 469095 which has been fixed after
the issue discovered in the revert CL 495017.
The issue was a missing res.Body.Close() in the newly added test.
Change-Id: Ifd9d8458022e59f4486397443a2862d06383e990
Reviewed-on: https://go-review.googlesource.com/c/go/+/495115
Reviewed-by: Damien Neil <dneil@google.com>
Run-TryBot: Jorropo <jorropo.pgm@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Prior to Go 1.19 adding support for sending 1xx informational headers
with ResponseWriter.WriteHeader, WriteHeader(101) would send a 101
status and disable further writes to the response. This behavior
was not documented, but is intentional: Writing to the response
body explicitly checks to see if a 101 status has been sent before
writing.
Restore the pre-1.19 behavior when writing a 101 Switching Protocols
header: The header is sent, no subsequent headers are sent, and
subsequent writes to the response body fail.
For #59564
Change-Id: I72c116f88405b1ef5067b510f8c7cff0b36951ee
Reviewed-on: https://go-review.googlesource.com/c/go/+/485775
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
According to the ResponseWriter documentation:
To suppress automatic response headers (such as "Date"), set
their value to nil.
In some cases, this documentation is incorrect: chunkWriter writes
a Content-Length header even if the value was set to nil. Meaning
there is no way to suppress this header.
This patch replaces the empty string comparison with a call to
`header.has` which takes into account nil values as expected.
This is similar to the way we handle the "Date" header.
Change-Id: Ie10d54ab0bb7d41270bc944ff867e035fe2bd0c5
GitHub-Last-Rev: e0616dd46388a724df7c6ea821b3808ed1663cab
GitHub-Pull-Request: golang/go#58578
Reviewed-on: https://go-review.googlesource.com/c/go/+/469095
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Damien Neil <dneil@google.com>
Run-TryBot: Jorropo <jorropo.pgm@gmail.com>
Reviewed-by: Damien Neil <dneil@google.com>
The test will wait for all goroutines.
A race can occur if the writing goroutine uses the Log after the test exits.
For #58264
For #59883
For #59884
Change-Id: I9b8ec7c9d024ff74b922b69efa438be5a4fa3483
Reviewed-on: https://go-review.googlesource.com/c/go/+/490255
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
In TestTransportPrefersResponseOverWriteError and TestMaxBytesHandler,
the server may respond to an incoming request without ever reading the
request body. The client's Do method will return as soon as the
server's response headers are read, but the Transport will remain
active until it notices that the server has closed the connection,
which may be arbitrarily later.
When the server has closed the connection, it will call the Close
method on the request body (if it has such a method). So we can use
that method to find out when the Transport is close enough to done for
the test to complete without interfering too much with other tests.
For #57612.
For #59526.
Change-Id: Iddc7a3b7b09429113ad76ccc1c090ebc9e1835a1
Reviewed-on: https://go-review.googlesource.com/c/go/+/483895
Run-TryBot: Bryan Mills <bcmills@google.com>
Commit-Queue: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
We don't have a way to terminate the leaked goroutines, and we can't
wait forever for them to exit (or else we would risk timing out the
test and losing the log line describing what exactly leaked).
So we have reason to believe that they will remain leaked while we run
the next test, and we don't want the goroutines from the first leak to
generate a spurious error when the second test completes.
This also removes a racy Parallel call I added in CL 476036, which was
flagged by the race detector in the duplicate-suppression check.
(I hadn't considered the potential interaction with the leak checker.)
For #59526.
Updates #56421.
Change-Id: Ib1f759f102fb41ece114401680cd728343e58545
Reviewed-on: https://go-review.googlesource.com/c/go/+/483896
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
- Log the actual addresses reported, in case that information is relevant.
- Keep going after the first error, so that we report more information
about the idle connections after they have been used. (Was the first
connection dropped completely, or did it later show up as idle?)
- Remove the third request at the end of the test. It had been
assuming that the address for a new connection would always be
different from the address for the just-closed connection; however,
that assumption does not hold in general.
Removing the third request addresses one of the two failure modes seen
in #55195. It may help in investigating the other failure mode, but I
do not expect it to fix the failures entirely. (I suspect that the
other failure mode is a synchronization bug in returning the idle
connection from the first request.)
For #55195.
Change-Id: If9604ea68db0697268288ce9812dd57633e83fbd
Reviewed-on: https://go-review.googlesource.com/c/go/+/478515
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
As soon as the test server closes its listener, its port may be reused
for another test server. On some platforms port reuse takes a long
time, because they cycle through the available ports before reusing an
old one. However, other platforms reuse ports much more aggressively.
net/http shouldn't know or care which kind of platform it is on —
dialing wild connections is risky and can interfere with other tests
no matter what platform we do it on.
Instead of making the second request after the server has completely
shut down, we can start (and finish!) the entire request while we are
certain that the listener has been closed but the port is still open
serving an existing request. If everything synchronizes as we expect,
that should guarantee that the second request fails.
Fixes#56421.
Change-Id: I56add243bb9f76ee04ead8f643118f9448fd1280
Reviewed-on: https://go-review.googlesource.com/c/go/+/476036
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
This change eliminates the easy, arbitrary timouts that should
never happen. It leaves in place a couple of more complicated ones
that will probably need retry loops for robustness.
For #49336.
For #36179.
Change-Id: I657ef223a66461413a915da5ce9150f49acec04a
Reviewed-on: https://go-review.googlesource.com/c/go/+/476035
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
If the test actually deadlocks, we probably want a goroutine dump to
debug it anyway. Otherwise, the arbitrary timeout can only cause
spurious failures.
Fixes#36179.
Change-Id: Ic2037496959a38d3231eefdbc1dd5d45eebdf306
Reviewed-on: https://go-review.googlesource.com/c/go/+/474582
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
It's been years since the behavior here was changed, and there's
little point in continuing to warn users of it.
Fixes#49399
Change-Id: I95f64ca14cacb64ebe78296593b1cc3d837e6b77
Reviewed-on: https://go-review.googlesource.com/c/go/+/470315
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Update net/http to enable tests that pass with the latest update
to the vendored x/net.
Update a few tests:
Windows apparently doesn't guarantee that time.Since(time.Now())
is >=0, so to set a definitely-expired write deadline, use a time
firmly in the past rather than now.
Put a backoff loop on TestServerReadTimeout to avoid failures
when the timeout expires mid-TLS-handshake. (The TLS handshake
timeout is set to min(ReadTimeout, WriteTimeout, ReadHeaderTimeout);
there's no way to set a long TLS handshake timeout and a short
read timeout.)
Don't close the http.Server in TestServerWriteTimeout while the
handler may still be executing, since this can result in us
getting the wrong error.
Change the GOOS=js fake net implementation to properly return
ErrDeadlineExceeded when a read/write deadline is exceeded,
rather than EAGAIN.
For #49837
For #54136
Change-Id: Id8a4ff6ac58336ff212dda3c8799b320cd6b9c19
Reviewed-on: https://go-review.googlesource.com/c/go/+/449935
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Set a logger in newClientServerTest that directs the server
log output to the testing.T's log, so log output gets properly
associated with the test that caused it.
Change-Id: I13686ca35c3e21adae16b2fc37ce36daea3df9d5
Reviewed-on: https://go-review.googlesource.com/c/go/+/452075
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
