Repository: fortio/fortio (go)

Issue:
EOF error for non keep alive HTTP 1.1 requests
**The version of the project you were using:** v1.66.2 (582d82a75a3172cc40eb0fe8e807c753b2a53770)

**Operating system you are using:** Arch Linux (6.10.5-arch1-1)

**The exact, minimal, steps needed to reproduce the issue:** Unfortunately I am not able to implement a minimal example that reliable reproduces the issue.

**Problem description:**
When i try to load test a specific application using the following command, i always get a `EOF` error resulting in a `Code  -1` statistic:
```
./fortio load -n 1 -c 1 --qps 0 -keepalive=false  https://somehost/api/infos
10:04:14.138 r1 [INF] scli.go:122> Starting, command="Φορτίο", version="dev  go1.23.0 amd64 linux", go-max-procs=8
Fortio dev running at 0 queries per second, 8->8 procs, for 1 calls: https://somehost/api/infos
10:04:14.139 r1 [INF] httprunner.go:121> Starting http test, run=0, url="https://somehost/api/infos", threads=1, qps="-1.0", warmup="parallel", conn-reuse=""
Starting at max qps with 1 thread(s) [gomax 8] for exactly 1 calls (1 per thread + 0)
10:04:14.153 r1 [ERR] http_client.go:1100> Read error, err="EOF", size=0, header_len=0, dest={"IP":"0.0.0.0","Port":443,"Zone":""}, url="https://somehost/api/infos", thread=0, run=0
10:04:14.154 r1 [INF] periodic.go:851> T000 ended after 12.940561ms : 1 calls. qps=77.27640246817738
Ended after 13.004645ms : 1 calls. qps=76.896
10:04:14.154 r1 [INF] periodic.go:581> Run ended, run=0, elapsed=13004645, calls=1, qps=76.8956015331445
Aggregated Function Time : count 1 avg 0.01293497 +/- 0 min 0.01293497 max 0.01293497 sum 0.01293497
# range, mid point, percentile, count
>= 0.012935 <= 0.012935 , 0.012935 , 100.00, 1
# target 50% 0.012935
# target 75% 0.012935
# target 90% 0.012935
# target 99% 0.012935
# target 99.9% 0.012935
Error cases : count 1 avg 0.01293497 +/- 0 min 0.01293497 max 0.01293497 sum 0.01293497
# range, mid point, percentile, count
>= 0.012935 <= 0.012935 , 0.012935 , 100.00, 1
# target 50% 0.012935
# target 75% 0.012935
# target 90% 0.012935
# target 99% 0.012935
# target 99.9% 0.012935
# Socket and IP used for each connection:
[0]   1 socket used, resolved to 0.0.0.0:443, connection timing : count 1 avg 0.007703781 +/- 0 min 0.007703781 max 0.007703781 sum 0.007703781
Connection time histogram (s) : count 1 avg 0.007703781 +/- 0 min 0.007703781 max 0.007703781 sum 0.007703781
# range, mid point, percentile, count
>= 0.00770378 <= 0.00770378 , 0.00770378 , 100.00, 1
# target 50% 0.00770378
# target 75% 0.00770378
# target 90% 0.00770378
# target 99% 0.00770378
# target 99.9% 0.00770378
Sockets used: 1 (for perfect keepalive, would be 1)
Uniform: false, Jitter: false, Catchup allowed: true
IP addresses distribution:
0.0.0.0:443: 1
Code  -1 : 1 (100.0 %)
Response Header Sizes : count 1 avg 0 +/- 0 min 0 max 0 sum 0
Response Body/Total Sizes : count 1 avg 0 +/- 0 min 0 max 0 sum 0
All done 1 calls (plus 0 warmup) 12.935 ms avg, 76.9 qps
```
Initially I debugged the client-server communication using wireshark, but did not find any abnormalities. That said, from the network perspective everything worked correctly, which means that all data was transferred completely from the server to the client and the and the connections (TLS as well as TCP) were closed normally.
I then started debugging fortio on the latest master branch (582d82a75a3172cc40eb0fe8e807c753b2a53770) and noticed that `fhttp/http_client.go:1074`  (`nI, err := conn.Read(c.buffer[c.size:])`) was actually returning the `EOF` error. When inspecting the content of `c.buffer` right after the `conn.Read` call I also noticed the the buffer already contained all the data sent by the server. `c.size` however was still 0 since this variable is increased at some later point (`fhttp/http_client.go:1104`). As far as I understand, `fhttp/http_client.go:1089` should handle the case where we get a `EOF` error as part of a normal end of stream connection termination. In this case however, the condition cannot evaluate to true since `c.size` is still `0`. As a result the request is interpreted as failed event though it was successful.
As far as I understand the issue is caused by the rather unusual behavior of the TLS socket's `Read` function which in this case reads the data completely and returns `EOF` error. Would it instead read the data and return `EOF` error on the next iteration call (just like in regular C code), then the current implementation would work correctly.

**Reproducing the problem:**
In my setup I was connecting to a nginx:1.27.1-alpine on the local lan that implemented TLS termination (TLS 1.2) and proxied the requests to a node.js application on the same lan, serving a static in-memory response. I noticed, that the problem could be reproduced more easily if the response from the node.js application was rather small (~413 bytes including headers) and the response was sent with very low latency.
NB: The same request using HTTP instead of HTTPS (still passing the nginx reverse proxy) never had these problems.

**Possible solution:**
IMO the following patch should fix the problem (at least it did in my case), however I'm not sure about the implications of such a change:
```
diff --git a/fhttp/http_client.go b/fhttp/http_client.go
index 21d5958..021a740 100644
--- a/fhttp/http_client.go
+++ b/fhttp/http_client.go
@@ -1086,7 +1086,7 @@ func (c *FastClient) readResponse(conn net.Conn, reusedSocket bool) {
                                        c.code = RetryOnce // special "retry once" code
                                        return
                                }
-                               if errors.Is(err, io.EOF) && c.size != 0 {
+                               if errors.Is(err, io.EOF) && (c.size > 0 || n > 0) {
                                        // handled below as possibly normal end of stream after we read something
                                        break
                                }
```


Pull request description:
Introduce DelayedErrorReader to avoid the case where we get both EOF and some data in a single read
fixes #972 but a bit messy way/too generic

Patch:
diff --git a/Makefile b/Makefile
index 25f123c..9fca227 100755
--- a/Makefile
+++ b/Makefile
@@ -59,7 +59,7 @@ test:
 	go test ./stats
 else
 test: dependencies
-	go test -timeout $(TEST_TIMEOUT) -race $(PACKAGES)
+	go test -tags netgo -timeout $(TEST_TIMEOUT) -race $(PACKAGES)
 endif
 
 # To debug strange linter errors, uncomment
diff --git a/README.md b/README.md
index fa47765..7f6b47d 100644
--- a/README.md
+++ b/README.md
@@ -1,4 +1,4 @@
-<!-- 1.66.2 -->
+<!-- 1.66.3 -->
 # Fortio
 
 [![Awesome Go](https://fortio.org/mentioned-badge.svg)](https://github.com/avelino/awesome-go#networking)
@@ -60,13 +60,13 @@ You can install from source:
 The [releases](https://github.com/fortio/fortio/releases) page has binaries for many OS/architecture combinations (see assets):
 
 ```shell
-curl -L https://github.com/fortio/fortio/releases/download/v1.66.2/fortio-linux_amd64-1.66.2.tgz \
+curl -L https://github.com/fortio/fortio/releases/download/v1.66.3/fortio-linux_amd64-1.66.3.tgz \
  | sudo tar -C / -xvzpf -
 # or the debian package
-wget https://github.com/fortio/fortio/releases/download/v1.66.2/fortio_1.66.2_amd64.deb
-dpkg -i fortio_1.66.2_amd64.deb
+wget https://github.com/fortio/fortio/releases/download/v1.66.3/fortio_1.66.3_amd64.deb
+dpkg -i fortio_1.66.3_amd64.deb
 # or the rpm
-rpm -i https://github.com/fortio/fortio/releases/download/v1.66.2/fortio-1.66.2-1.x86_64.rpm
+rpm -i https://github.com/fortio/fortio/releases/download/v1.66.3/fortio-1.66.3-1.x86_64.rpm
 # and more, see assets in release page
 ```
 
@@ -76,7 +76,7 @@ On macOS you can also install Fortio using [Homebrew](https://brew.sh/):
 brew install fortio
 ```
 
-On Windows, download https://github.com/fortio/fortio/releases/download/v1.66.2/fortio_win_1.66.2.zip and extract `fortio.exe` to any location, then using the Windows Command Prompt:
+On Windows, download https://github.com/fortio/fortio/releases/download/v1.66.3/fortio_win_1.66.3.zip and extract `fortio.exe` to any location, then using the Windows Command Prompt:
 ```
 fortio.exe server
 ```
@@ -130,7 +130,7 @@ Full list of command line flags (`fortio help`):
 <!-- use release/updateFlags.sh to update this section -->
 <pre>
 <!-- USAGE_START -->
-Φορτίο 1.66.2 usage:
+Φορτίο 1.66.3 usage:
         fortio command [flags] target
 where command is one of: load (load testing), server (starts ui, rest api,
  http-echo, redirect, proxies, tcp-echo, udp-echo and grpc ping servers),
diff --git a/fhttp/http_client.go b/fhttp/http_client.go
index 21d5958..621febc 100644
--- a/fhttp/http_client.go
+++ b/fhttp/http_client.go
@@ -727,6 +727,7 @@ type FastClient struct {
 	req          []byte
 	dest         net.Addr
 	socket       net.Conn
+	reader       *DelayedErrorReader
 	socketCount  int // number of sockets attempts, same as the new connectStats.Count() + DNS errors if any.
 	size         int64
 	code         int
@@ -772,10 +773,11 @@ func (c *FastClient) HasBuffer() bool {
 func (c *FastClient) Close() {
 	log.Debugf("[%d] Closing %p %s socket count %d", c.id, c, c.url, c.socketCount)
 	if c.socket != nil {
-		if err := c.socket.Close(); err != nil {
+		if err := c.reader.Close(); err != nil {
 			log.S(log.Warning, "Error closing fast client's socket",
 				log.Attr("err", err), log.Attr("thread", c.id), log.Attr("run", c.runID))
 		}
+		c.reader = nil
 		c.socket = nil
 	}
 }
@@ -918,7 +920,7 @@ func (c *FastClient) returnRes() (int, int64, uint) {
 }
 
 // connect to destination.
-func (c *FastClient) connect(ctx context.Context) net.Conn {
+func (c *FastClient) connect(ctx context.Context) (net.Conn, *DelayedErrorReader) {
 	c.socketCount++
 	var socket net.Conn
 	var err error
@@ -930,7 +932,7 @@ func (c *FastClient) connect(ctx context.Context) net.Conn {
 		if err != nil {
 			log.S(log.Error, "Unable to resolve hostname", log.Str("hostname", c.hostname), log.Attr("err", err),
 				log.Attr("thread", c.id), log.Attr("run", c.runID))
-			return nil
+			return nil, nil
 		}
 	}
 
@@ -942,7 +944,7 @@ func (c *FastClient) connect(ctx context.Context) net.Conn {
 		if err != nil {
 			log.S(log.Error, "Unable to TLS connect", log.Attr("dest", c.dest), log.Attr("err", err),
 				log.Attr("thread", c.id), log.Attr("run", c.runID))
-			return nil
+			return nil, nil
 		}
 	} else {
 		socket, err = d.Dial(c.dest.Network(), c.dest.String())
@@ -951,11 +953,11 @@ func (c *FastClient) connect(ctx context.Context) net.Conn {
 			log.S(log.Error, "Unable to connect", log.Attr("dest", c.dest), log.Attr("err", err),
 				log.Attr("numfd", scli.NumFD()),
 				log.Attr("thread", c.id), log.Attr("run", c.runID))
-			return nil
+			return nil, nil
 		}
 	}
 	fnet.SetSocketBuffers(socket, len(c.buffer), len(c.req))
-	return socket
+	return socket, &DelayedErrorReader{r: socket}
 }
 
 // Extra error codes outside of the HTTP Status code ranges. ie negative.
@@ -983,6 +985,7 @@ func (c *FastClient) StreamFetch(ctx context.Context) (int, int64, uint) {
 	c.headerLen = 0
 	// Connect or reuse existing socket:
 	conn := c.socket
+	reader := c.reader
 	canReuse := conn != nil
 	if c.reachedReuseThreshold() {
 		c.connReuse = generateReuseThreshold(c.connReuseRange[0], c.connReuseRange[1])
@@ -991,7 +994,7 @@ func (c *FastClient) StreamFetch(ctx context.Context) (int, int64, uint) {
 	}
 
 	if conn == nil {
-		conn = c.connect(ctx)
+		conn, reader = c.connect(ctx)
 		c.reuseCount = 1
 		if conn == nil {
 			return c.returnRes()
@@ -1040,7 +1043,7 @@ func (c *FastClient) StreamFetch(ctx context.Context) (int, int64, uint) {
 		}
 	}
 	// Read the response:
-	c.readResponse(conn, canReuse)
+	c.readResponse(reader, conn, canReuse)
 	if c.code == RetryOnce {
 		// Special "eof on reused socket" code
 		return c.StreamFetch(ctx) // recurse once
@@ -1054,10 +1057,38 @@ func codeIsOK(code int) bool {
 	return (code >= 200 && code <= 299) || code == http.StatusTeapot
 }
 
+// DelayedErrorReader is a reader that never returns an error along with a non 0 read.
+// It will return the error on the next read. In other words n > 0 is always ok and
+// n is always 0 if there is an error. This is useful to simplify code and deal
+// with readers that can return both an error and data (like go 1.23+ TLS conn.Read).
+type DelayedErrorReader struct {
+	r   io.Reader
+	err error
+}
+
+func (d *DelayedErrorReader) Read(p []byte) (int, error) {
+	if d.err != nil {
+		return 0, d.err
+	}
+	n, err := d.r.Read(p)
+	if n > 0 {
+		d.err = err
+		return n, nil
+	}
+	return n, err
+}
+
+func (d *DelayedErrorReader) Close() error {
+	if closer, ok := d.r.(io.Closer); ok {
+		return closer.Close()
+	}
+	return d.err
+}
+
 // Response reading:
 //
 //nolint:nestif,funlen,gocognit,gocyclo,maintidx,gosec // TODO: refactor - unwiedly/ugly atm.
-func (c *FastClient) readResponse(conn net.Conn, reusedSocket bool) {
+func (c *FastClient) readResponse(conn *DelayedErrorReader, socket net.Conn, reusedSocket bool) {
 	maxV := int64(len(c.buffer))
 	parsedHeaders := false
 	// TODO: safer to start with -1 / SocketError and fix ok for HTTP/1.0
@@ -1259,7 +1290,8 @@ func (c *FastClient) readResponse(conn net.Conn, reusedSocket bool) {
 	} // end of big for loop
 	// Figure out whether to keep or close the socket:
 	if keepAlive && codeIsOK(c.code) && !c.reachedReuseThreshold() {
-		c.socket = conn // keep the open socket
+		c.socket = socket // keep the open socket
+		c.reader = conn
 	} else {
 		if err := conn.Close(); err != nil {
 			log.S(log.Error, "Close error", log.Attr("err", err), log.Attr("size", c.size),


Test patch:
diff --git a/fhttp/http_test.go b/fhttp/http_test.go
index 267bfa5..aec3774 100644
--- a/fhttp/http_test.go
+++ b/fhttp/http_test.go
@@ -763,7 +763,7 @@ func TestDefaultPort(t *testing.T) {
 	if code != expectedRedirect {
 		t.Errorf("unexpected code for %s: %d (expecting %d redirect to https)", url, code, expectedRedirect)
 	}
-	conn := cli.(*FastClient).connect(ctx)
+	conn, _ := cli.(*FastClient).connect(ctx)
 	if conn != nil {
 		p := conn.RemoteAddr().(*net.TCPAddr).Port
 		if p != 80 {