Andrew Gerrand [Wed, 6 Jan 2016 04:45:16 +0000 (15:45 +1100)]
net/http: better documentation for Transport
Mention that:
- connection pooling is enabled by default,
- the Transport is safe for concurrent use, and
- the Client type should be used for high-level stuff.
Adam Langley [Mon, 21 Dec 2015 22:40:23 +0000 (14:40 -0800)]
crypto/x509: handle ECC private keys with the wrong length.
SEC-1 says: “The component privateKey is the private key defined to be
the octet string of length ⌊log₂(n)/8⌋ (where n is the order of the
curve)”.
Previously the code for parsing ECC private keys would panic (on
non-amd64) when the private was too long. It would also pass a too-short
private key to crypto/elliptic, possibly resulting in undesirable
behaviour.
This change makes the parsing function handle both too much and too
little padding because GnuTLS does the former and OpenSSL did the latter
until 30cd4ff294252c4b6a4b69cbef6a5b4117705d22. It also causes
serialisation to pad private keys correctly.
Fixes #13699
Change-Id: If9c2faeaeb45af8a4d7770d784f3d2633e7f8290
Reviewed-on: https://go-review.googlesource.com/18094
Run-TryBot: Adam Langley <agl@golang.org> Reviewed-by: Adam Langley <agl@golang.org> Reviewed-by: Russ Cox <rsc@golang.org>
Michael Hudson-Doyle [Wed, 23 Dec 2015 02:33:55 +0000 (15:33 +1300)]
cmd/go: special case shared library name when passed "$prefix/..."
Before golang.org/cl/13921, "go install -buildmode=shared prefix/..." created a
file called "libprefix.so", which was obviously a problem when prefix was
something like "." or "../". However, now it expands the ... into all the
matched packages, joins them with -, which can clearly be a very long name
indeed. Because I plan to build shared libraries for Ubuntu by running commands
exactly like "go install -buildmode=shared prefix/...", this special cases this
to produce the old behaviour (but de-relativises prefix first).
Fixes #13714
Change-Id: I4fd8d4934279f9a18cc70a13e4ef3e23f6abcb6e
Reviewed-on: https://go-review.googlesource.com/18114 Reviewed-by: Ian Lance Taylor <iant@golang.org>
David Benjamin [Tue, 5 Jan 2016 00:16:28 +0000 (16:16 -0800)]
encoding/asn1: fix off-by-one in parseBase128Int.
parseBase128Int compares |shifted| with four, seemingly to ensure the result
fits in an int32 on 32-bit platforms where int is 32-bit. However, there is an
off-by-one in this logic, so it actually allows five shifts, making the maximum
tag number or OID component 2^35-1.
Fix this so the maximum is 2^28-1 which should be plenty for OID components and
tag numbers while not overflowing on 32-bit platforms.
Brad Fitzpatrick [Tue, 5 Jan 2016 22:19:10 +0000 (14:19 -0800)]
A+C: automated update
These are the easy, automated cases. There were some more where we
need to fight Gerrit and the CLA system to extract the appropriate
metadata.
Updates #12042
Change-Id: Id63ae635ee7efeec4cd372c7d85bb5b1f557951b
Reviewed-on: https://go-review.googlesource.com/18264 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Brad Fitzpatrick [Mon, 4 Jan 2016 17:12:31 +0000 (09:12 -0800)]
log/syslog: document that syslog is frozen
Try to reduce feature request bug reports.
Change-Id: I713bb715d25d23e084b054aea8e1c3197dde90d4
Reviewed-on: https://go-review.googlesource.com/18222 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Brad Fitzpatrick [Tue, 5 Jan 2016 04:59:05 +0000 (20:59 -0800)]
net/http: make Client use Request.Cancel for timeouts instead of CancelRequest
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.
Brad Fitzpatrick [Tue, 5 Jan 2016 19:40:25 +0000 (19:40 +0000)]
net/http: tighten protocol between Transport.roundTrip and persistConn.readLoop
In debugging the flaky test in #13825, I discovered that my previous
change to tighten and simplify the communication protocol between
Transport.roundTrip and persistConn.readLoop in
https://golang.org/cl/17890 wasn't complete.
This change simplifies it further: the buffered-vs-unbuffered
complexity goes away, and we no longer need to re-try channel reads in
the select case. It was trying to prioritize channels in the case that
two were readable in the select. (it was only failing in the race builder
because the race builds randomize select scheduling)
The problem was that in the bodyless response case we had to return
the idle connection before replying to roundTrip. But putIdleConn
previously both added it to the free list (which we wanted), but also
closed the connection, which made the caller goroutine
(Transport.roundTrip) have two readable cases: pc.closech, and the
response. We guarded against similar conditions in the caller's select
for two readable channels, but such a fix wasn't possible here, and would
be overly complicated.
Instead, switch to unbuffered channels. The unbuffered channels were only
to prevent goroutine leaks, so address that differently: add a "callerGone"
channel closed by the caller on exit, and select on that during any unbuffered
sends.
As part of the fix, split putIdleConn into two halves: a part that
just returns to the freelist, and a part that also closes. Update the
four callers to the variants each wanted.
Incidentally, the connections were closing on return to the pool due
to MaxIdleConnsPerHost (somewhat related: #13801), but this bug
could've manifested for plenty of other reasons.
Austin Clements [Tue, 5 Jan 2016 16:33:25 +0000 (11:33 -0500)]
runtime/pprof: skip TestStackBarrierProfiling
This test triggers a large number of usleep(100)s. linux/arm, openbsd,
and solaris have very poor timer resolution on the builders, so
usleep(100) actually gives up the whole scheduling quantum. On Linux
and OpenBSD (and probably Solaris), profiling signals are only
generated when a process completes a whole scheduling quantum, so this
test often gets zero profiling signals and fails.
Until we figure out what to do about this, skip this test on these
platforms.
Ian Lance Taylor [Fri, 1 Jan 2016 23:44:12 +0000 (15:44 -0800)]
runtime: fix exit status when killed by signal
Previously, when a program died because of a SIGHUP, SIGINT, or SIGTERM
signal it would exit with status 2. This CL fixes the runtime to exit
with a status indicating that the program was killed by a signal.
Change-Id: Ic2982a2562857edfdccaf68856e0e4df532af136
Reviewed-on: https://go-review.googlesource.com/18156 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Russ Cox [Mon, 4 Jan 2016 20:08:03 +0000 (15:08 -0500)]
misc/cgo/testcshared: print a little more about GOROOT on failure
For #13789.
Change-Id: I83973298a35afcf55627f0a72223098306a51f4b
Reviewed-on: https://go-review.googlesource.com/18233 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Russ Cox [Mon, 4 Jan 2016 20:03:45 +0000 (15:03 -0500)]
cmd/dist: wait for pending tests before exiting
When 'go tool dist test' stops, it was intended that it first wait for
pending background tests, like a failed compilation waits for pending
background compiles. But these three lines prevented that.
Fix by deleting them. (The actual loop already contains the correct
logic to avoid running the others and to wait for what's left.)
Change-Id: I4e945495ada903fb0af567910626241bc1c52ba6
Reviewed-on: https://go-review.googlesource.com/18232 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Brad Fitzpatrick [Mon, 4 Jan 2016 20:38:20 +0000 (20:38 +0000)]
net/http: relax recently-updated rules and behavior of CloseNotifier
The CloseNotifier implementation and documentation was
substantially changed in https://golang.org/cl/17750 but it was a bit
too aggressive.
Issue #13666 highlighted that in addition to breaking external
projects, even the standard library (httputil.ReverseProxy) didn't
obey the new rules about not using CloseNotifier until the
Request.Body is fully consumed.
So, instead of fixing httputil.ReverseProxy, dial back the rules a
bit. It's now okay to call CloseNotify before consuming the request
body. The docs now say CloseNotifier may wait to fire before the
request body is fully consumed, but doesn't say that the behavior is
undefined anymore. Instead, we just wait until the request body is
consumed and start watching for EOF from the client then.
This CL also adds a test to ReverseProxy (using a POST request) that
would've caught this earlier.
Ian Lance Taylor [Tue, 5 Jan 2016 01:21:05 +0000 (17:21 -0800)]
os: remove unused import to fix build
Change-Id: Ia8c1c77590115a5ffda144962436d489ed77a423
Reviewed-on: https://go-review.googlesource.com/18227 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Ian Lance Taylor [Mon, 28 Dec 2015 19:29:22 +0000 (11:29 -0800)]
os, runtime: better EPIPE behavior for command line programs
Old behavior: 10 consecutive EPIPE errors on any descriptor cause the
program to exit with a SIGPIPE signal.
New behavior: an EPIPE error on file descriptors 1 or 2 cause the
program to raise a SIGPIPE signal. If os/signal.Notify was not used to
catch SIGPIPE signals, this will cause the program to exit with SIGPIPE.
An EPIPE error on a file descriptor other than 1 or 2 will simply be
returned from Write.
Russ Cox [Tue, 29 Dec 2015 15:16:40 +0000 (10:16 -0500)]
cmd/link: use current GOROOT for source file paths for standard library
This CL changes the source file information in the
standard library's .a files to say "$GOROOT/src/runtime/chan.go"
(with a literal "$GOROOT") instead of spelling out the actual directory.
The linker then substitutes the actual $GOROOT (or $GOROOT_FINAL)
as appropriate.
If people download a binary distribution to an alternate location,
following the instructions at https://golang.org/doc/install#install,
the code before this CL would end up with source paths pointing to
/usr/local/go no matter where the actual sources were.
Now the source paths for built binaries will point to the actual sources
(hopefully).
The source line information in distributed binaries is not affected:
those will still say /usr/local/go. But binaries people build themselves
(their own programs, not the go distribution programs) will be correct.
Fixing this path also fixes the lookup of the runtime-gdb.py file.
Fixes #5533.
Change-Id: I03729baae3fbd8cd636e016275ee5ad2606e4663
Reviewed-on: https://go-review.googlesource.com/18200
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Didier Spezia [Sat, 2 Jan 2016 14:26:13 +0000 (14:26 +0000)]
build: prevent the entire repo to be wiped out by cgo test scripts
Following the parallelization of some tests, a race condition can
occur in testcarchive, testshared and testcshared.
In some cases, it can result in the go env GOROOT command returning
corrupted data, which are then passed to a rm command.
Make the shell script more robust by not trusting the result of
the go env GOROOT command. It does not really fix the issue, but
at least prevent the entire repository to be deleted.
Brad Fitzpatrick [Mon, 4 Jan 2016 18:27:51 +0000 (18:27 +0000)]
net/http: deflake tests in full mode after t.Parallel additions
https://golang.org/cl/18087 added a bunch of t.Parallel calls, which
aren't compatible with the afterTest func. But in short mode, afterTest
is a no-op. To keep all.bash (short mode) fast, conditionally set
t.Parallel when in short mode, but keep it unset for compatibility with
afterFunc otherwise.
Fixes #13804
Change-Id: Ie841fbc2544e1ffbee43ba1afbe895774e290da0
Reviewed-on: https://go-review.googlesource.com/18143 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Alex Brainman [Wed, 30 Dec 2015 01:13:21 +0000 (12:13 +1100)]
os: change Open(`C:`) to open current directory on C:
Open(`C:`) currently opens root directory on C:. Change that to open
current directory on C:. Just like cmd.exe's "dir C:" command does.
Just like FindFirstFile("C:*") Windows API does. It is also consistent
with what filepath.Join("C:", "a") currently does.
Fixes #13763
Change-Id: I60b6e7d80215d110bbbb6265c9f32717401638c6
Reviewed-on: https://go-review.googlesource.com/18184 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
Russ Cox [Mon, 21 Dec 2015 15:29:21 +0000 (10:29 -0500)]
runtime: move test programs out of source code, coalesce
Now there are just three programs to compile instead of many,
and repeated tests can reuse the compilation result instead of
rebuilding it.
Combined, these changes reduce the time spent testing runtime
during all.bash on my laptop from about 60 to about 30 seconds.
(All.bash itself runs in 5½ minutes.)
Brad Fitzpatrick [Tue, 29 Dec 2015 17:54:16 +0000 (09:54 -0800)]
net/http: update docs on Request.Proto, ProtoMajor, ProtoMinor
Change-Id: I4a6928b4674b6aaab3611cad7526347923a0015f
Reviewed-on: https://go-review.googlesource.com/18153 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Russ Cox [Mon, 21 Dec 2015 21:08:57 +0000 (16:08 -0500)]
cmd/dist: run various one-off tests in parallel
Takes 15% off my all.bash run time
(after this and earlier CLs, now down to 3½ from 5½ minutes).
For #10571.
Change-Id: Iac316ffb730c9ff0a0faa7cc3b82ed4f7e6d4361
Reviewed-on: https://go-review.googlesource.com/18088 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Austin Clements [Mon, 21 Dec 2015 19:26:33 +0000 (11:26 -0800)]
runtime: fix, simplify, and improve scan state in goroutine header
Currently goroutineheader goes through some convolutions to *almost*
print the scan state of a G. However, the code path that would print
the scan state of the G refers to gStatusStrings where it almost
certainly meant to refer to gScanStatusStrings (which is unused), so
it winds up printing the regular status string without the scan state
either way. Furthermore, if the G is in _Gwaiting, we override the
status string and lose where this would indicate the scan state if it
worked.
This commit fixes this so the runtime prints the scan state. However,
rather than using a parallel list of status strings, this simply adds
a conditional print if the scan bit is set. This lets us remove the
string list, prints the scan state even in _Gwaiting, and lets us
strip off the scan bit at the beginning of the function, which
simplifies the rest of it.
Change-Id: Ic0adbe5c05abf4adda93da59f93b578172b28e3d
Reviewed-on: https://go-review.googlesource.com/18092 Reviewed-by: Keith Randall <khr@golang.org>
Ian Lance Taylor [Tue, 22 Dec 2015 06:27:01 +0000 (22:27 -0800)]
runtime: adjust gsignal stack to current signal stack
If non-Go code calls sigaltstack before a signal is received, use
sigaltstack to determine the current signal stack and set the gsignal
stack to use it. This makes the Go runtime more robust in the face of
non-Go code. We still can't handle a disabled signal stack or a signal
triggered with SA_ONSTACK clear, but we now give clear errors for those
cases.
Fixes #7227.
Update #9896.
Change-Id: Icb1607e01fd6461019b6d77d940e59b3aed4d258
Reviewed-on: https://go-review.googlesource.com/18102
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Mikio Hara <mikioh.mikioh@gmail.com>
Mikio Hara [Tue, 22 Dec 2015 00:35:27 +0000 (09:35 +0900)]
net: fix race in TestTCPStress
Fixes #13704.
Change-Id: I7afef5058fa88b0de41213cf46219b684369f47f
Reviewed-on: https://go-review.googlesource.com/18111 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Mikio Hara [Mon, 21 Dec 2015 07:35:05 +0000 (16:35 +0900)]
net/internal/socktest: simplify log message format
This change replaces the existing log format separated by commas and
spaces with space-separated one.
Change-Id: I9a4b38669025430190c9a1a6b5c82b862866559d
Reviewed-on: https://go-review.googlesource.com/17999 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Mikio Hara [Fri, 18 Dec 2015 03:39:57 +0000 (12:39 +0900)]
net: make use of IPv4 for parsing routing information on windows
In general the package net deals IPv4 addresses as IPv6 IPv4-mapped
addresses internally for the dual stack era, when we need to support
various techniques on IPv4/IPv6 translation.
This change makes windows implementation follow the same pattern which
BSD variants and Linux do.
Updates #13544.
Also fixes an unintentionally formatted line by accident by gofmt.
Change-Id: I4953796e751fd8050c73094468a0d7b0d33f5516
Reviewed-on: https://go-review.googlesource.com/17992 Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Ian Lance Taylor [Thu, 17 Dec 2015 22:49:34 +0000 (14:49 -0800)]
runtime: write sigsetstack for Darwin, fix sigaction arg
It turns out that the second argument for sigaction on Darwin has a
different type than the first argument. The second argument is the user
visible sigaction struct, and does not have the sa_tramp field.
I base this on
http://www.opensource.apple.com/source/Libc/Libc-1081.1.3/sys/sigaction.c
not to mention actual testing.
While I was at it I removed a useless memclr in setsig, a relic of the C
code.
This CL is Darwin-specific changes. The tests for this CL are in
https://golang.org/cl/17903 .
Change-Id: I61fe305c72311df6a589b49ad7b6e49b6960ca24
Reviewed-on: https://go-review.googlesource.com/18015 Reviewed-by: David Crawshaw <crawshaw@golang.org>
Russ Cox [Fri, 18 Dec 2015 16:29:15 +0000 (11:29 -0500)]
doc: add mention of debug.SetTraceback
Change-Id: I59829029769ae08c6c54208a1e38a0794868c5db
Reviewed-on: https://go-review.googlesource.com/18045 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Austin Clements [Fri, 18 Dec 2015 17:53:25 +0000 (09:53 -0800)]
debug/elf: rename Chdr64.Reserved to _
This future-proofs the Chdr64 structure against later versions of ELF
defining this field and declutters the documentation without changing
the layout of the struct.
This structure does not exist in the current release, so this change
is safe.
Change-Id: I239aad7243ddaf063a1f8cd521d8a50b30413281
Reviewed-on: https://go-review.googlesource.com/18028 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Russ Cox [Thu, 17 Dec 2015 18:16:25 +0000 (13:16 -0500)]
cmd/go: do not skip dirs with syntax errors in wildcard matching (like ./...)
Fixes #11407.
Change-Id: If35a8e04a3abf8acf955250c909dde57131b6bb8
Reviewed-on: https://go-review.googlesource.com/17971 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Austin Clements [Fri, 18 Dec 2015 00:09:38 +0000 (16:09 -0800)]
runtime: require the stack barrier lock to traceback cgo and libcalls
Currently, if sigprof determines that the G is in user code (not cgo
or libcall code), it will only traceback the G stack if it can acquire
the stack barrier lock. However, it has no such restriction if the G
is in cgo or libcall code. Because cgo calls count as syscalls, stack
scanning and stack barrier installation can occur during a cgo call,
which means sigprof could attempt to traceback a G in a cgo call while
scanstack is installing stack barriers in that G's stack. As a result,
the following sequence of events can cause the sigprof traceback to
panic with "missed stack barrier":
1. M1: G1 performs a Cgo call (which, on Windows, is any system call,
which could explain why this is easier to reproduce on Windows).
2. M1: The Cgo call puts G1 into _Gsyscall state.
3. M2: GC starts a scan of G1's stack. It puts G1 in to _Gscansyscall
and acquires the stack barrier lock.
4. M3: A profiling signal comes in. On Windows this is a global
(though I don't think this matters), so the runtime stops M1 and
calls sigprof for G1.
5. M3: sigprof fails to acquire the stack barrier lock (because the
GC's stack scan holds it).
6. M3: sigprof observes that G1 is in a Cgo call, so it calls
gentraceback on G1 with its Cgo transition point.
7. M3: gentraceback on G1 grabs the currently empty g.stkbar slice.
9. M3: gentraceback encounters one of the just-installed stack
barriers and panics.
This commit fixes this by only allowing cgo tracebacks if sigprof can
acquire the stack barrier lock, just like in the regular user
traceback case.
For good measure, we put the same constraint on libcall tracebacks.
This case is probably already safe because, unlike cgo calls, libcalls
leave the G in _Grunning and prevent reaching a safe point, so
scanstack cannot run during a libcall. However, this also means that
sigprof will always acquire the stack barrier lock without contention,
so there's no cost to adding this constraint to libcall tracebacks.
Fixes #12528. For 1.5.3 (will require some backporting).
Austin Clements [Thu, 17 Dec 2015 23:35:49 +0000 (15:35 -0800)]
runtime: prevent race between setNextBarrierPC and sigprof
Currently, setNextBarrierPC manipulates the stack barriers without
acquiring the stack barrier lock. This is mostly okay because
setNextBarrierPC also runs synchronously on the G and prevents safe
points, but this doesn't prevent a sigprof from occurring during a
setNextBarrierPC and performing a traceback.
Given that setNextBarrierPC simply sets one entry in the stack barrier
array, this is almost certainly safe in reality. However, given that
this depends on a subtle argument, which may not hold in the future,
and that setNextBarrierPC almost never happens, making it nowhere near
performance-critical, we can simply acquire the stack barrier lock and
be sure that the synchronization will work.
Shenghou Ma [Thu, 17 Dec 2015 21:37:30 +0000 (16:37 -0500)]
test: fix linkmain test
Change-Id: Ie8ec4cfc68abef51e52090a75245f96af874c74a
Reviewed-on: https://go-review.googlesource.com/18000 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Russ Cox [Thu, 17 Dec 2015 20:10:25 +0000 (15:10 -0500)]
cmd/link: reject non-package main toplevel.a file, remove dead code
The test for non-package main top-level inputs is done while parsing
the export data. Issue #13468 happened because we were not parsing
the export data when using compiler-generated archives
(that is, when using go tool compile -pack).
Fix this by parsing the export data even for archives.
However, that turns up a different problem: the export data check
reports (one assumes spurious) skew errors now, because it has
not been run since Go 1.2.
(Go 1.3 was the first release to use go tool compile -pack.)
Since the code hasn't run since Go 1.2, it can't be that important.
Since it doesn't work today, just delete it.
Figuring out how to make this code work with Robert's export
format was one of the largest remaining TODOs for that format.
Now we don't have to.
Fixes #13468 and makes the world a better place.
Change-Id: I40a4b284cf140d49d48b714bd80762d6889acdb9
Reviewed-on: https://go-review.googlesource.com/17976 Reviewed-by: Robert Griesemer <gri@golang.org>
Russ Cox [Thu, 17 Dec 2015 05:21:13 +0000 (00:21 -0500)]
cmd/go: fix processing of HTTPS 404 without -insecure
The change here is to move the closeBody call into the if block.
The logging adjustments are just arranging to tell the truth:
in particular if we're not in insecure mode and we get a non-200
error then we do not actually ignore the response
(except as caused by closing the body incorrectly).
As the comment below the change indicates, it is intentional that
we process non-200 pages. The code does process them, because
the if err != nil || status != 200 block does not return.
But that block does close the body, which depending on timing
can apparently poison the later read from the body.
See #13037's initial report:
$ go get -v bosun.org/cmd/bosun/cache
Fetching https://bosun.org/cmd/bosun/cache?go-get=1
ignoring https fetch with status code 404
Parsing meta tags from https://bosun.org/cmd/bosun/cache?go-get=1 (status code 404)
import "bosun.org/cmd/bosun/cache": parsing bosun.org/cmd/bosun/cache: http: read on closed response body
package bosun.org/cmd/bosun/cache: unrecognized import path "bosun.org/cmd/bosun/cache"
The log print about ignoring the https fetch is not strictly true,
since the next thing that happened was parsing the body of that fetch.
But the read on the closed response body failed during parsing.
Moving the closeBody to happen only when we're about to discard the
result and start over (that is, only in -insecure mode) fixes the parse.
At least it should fix the parse. I can't seem to break the parse anymore,
because of #13648 (close not barring future reads anymore),
but this way is clearly better than the old way. If nothing else the old code
closed the body twice when err != nil and -insecure was not given.