Repository: go
Issue: net/http: Errors from persistConn.readLoop not returned to client still writing a request
Labels: NeedsInvestigation

### What version of Go are you using (`go version`)?

<pre>
$ go version
go version go1.21.0 darwin/arm64
</pre>

### Does this issue reproduce with the latest release?
Yes


### What operating system and processor architecture are you using (`go env`)?

<details><summary><code>go env</code> Output</summary><br><pre>
$ go env
GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/mitr/Library/Caches/go-build'
GOENV='/Users/mitr/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/mitr/Go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/mitr/Go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.21.0'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/mitr/Go/src/github.com/containers/image/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/gx/746zv1352fl7clbfh5q24grc0000gn/T/go-build66149756=/tmp/go-build -gno-record-gcc-switches -fno-common'
</pre></details>

### What did you do?

Consider a HTTP server facing a client which is slow to upload a request body, and the HTTP server times out and closes the socket.

<details><summary> <a href="https://go.dev/play/p/zu0cZuiGxDZ">https://go.dev/play/p/zu0cZuiGxDZ</a> </summary>

```go
package main

import (
	"context"
	"fmt"
	"io"
	"log"
	"net"
	"net/http"
	"time"
)

func dieOnError(err error) {
	if err != nil {
		panic(err)
	}
}

var timeStep = time.Second

func server(ln net.Listener) {
	for {
		conn, err := ln.Accept()
		if err != nil {
			return
		}
		log.Printf("%v: Accepted", conn.RemoteAddr())
		time.Sleep(1 * timeStep)
		if false {
			_, err = conn.Write([]byte("HTTP/1.1 200 OK\r\n\r\n")) // Just enough for readLoop to return a response with some body
			dieOnError(err)
		}
		err = conn.Close()
		dieOnError(err)
		log.Printf("%v: Closed", conn.RemoteAddr())
	}
}

func main() {
	ln, err := net.Listen("tcp", "127.0.0.1:0")
	dieOnError(err)
	go server(ln)

	sourceReader, sourceWriter := io.Pipe()
	go func() {
		_, err := sourceWriter.Write([]byte("line 1\n"))
		dieOnError(err)

		time.Sleep(2 * timeStep)
		_, err = sourceWriter.Write([]byte("line 2\n"))
		dieOnError(err)
		err = sourceWriter.Close()
		dieOnError(err)
		log.Printf("Producer finished")
	}()

	req, err := http.NewRequestWithContext(context.Background(), http.MethodPatch, fmt.Sprintf("http://%s/", ln.Addr().String()), io.NopCloser(sourceReader))
	dieOnError(err)
	res, err := http.DefaultClient.Do(req)
	dieOnError(err)
	body, err := io.ReadAll(res.Body)
	log.Printf("Reading body: %#v, %v", body, err)

}
```

</details>

### What did you expect to see?

On the client side, a real error cause of some kind; such as the one in https://go.dev/play/p/fKwUQwOjTDW (with `s/if false/if true`):
```
2009/11/10 23:00:00 127.0.0.1:34699: Accepted
2009/11/10 23:00:01 127.0.0.1:34699: Closed
2009/11/10 23:00:01 Reading body: []byte{}, read tcp 127.0.0.1:34699->127.0.0.1:44764: read: connection reset by peer
```

contains a useful “connection reset by peer” error.

### What did you see instead?

```
2009/11/10 23:00:00 127.0.0.1:47676: Accepted
2009/11/10 23:00:01 127.0.0.1:47676: Closed
panic: Patch "http://127.0.0.1:22356/": write tcp 127.0.0.1:47676->127.0.0.1:22356: use of closed network connection
```

contains `net.ErrClosed`, with no indication of the _actual_ underlying root cause. And I think it's possibly controversial but plausible point that `net.ErrClosed` is considered an indication of a programming bug.

The reports in https://github.com/golang/go/issues/4373 obliquely hint at net/http encountering this issue, but never quite spell it out, hence this report.

The underlying cause, at least for this specific reproducer, is that `persistConn.readLoop` and `persistConn.writeLoop` are quite independent; in this case, `readLoop` encounters a socket read error, terminates, and closes `persistConn.conn`. Meanwhile, `writeLoop` is deep in `transferWriter.doBodyCopy`, continuing to issue writes and flushes to `persistConn.conn`, and that triggers `net.ErrClosed` on the write side. Arguably that’s the primary bug, issuing a write on a closed connection.

And then, `persistConn.roundTrip` actually sees the read-side error first (`re := <-resc:`); but it proceeds to `mapRoundTripError`, and sees `req.err` set, and returns that value. Alternatively, if issuing a write on a closed connection is considered fine (or the performance cost to avoid that would be too great), the logic in `mapRoundTripError` could see that there is both a read-side and write-side error, and decide that `net.ErrClosed` is not the one that should be returned.

---

There also seems to be a dual of this issue, where `persistConn.writeLoop` can encounter an error and close the connection, causing `readLoop` to fail with `net.ErrClosed` instead, and in that case, again, an error with the “use of closed network connection” text is returned. I’m afraid I lost the reproducer I had for that one.