Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert Pull 1233 fix: The client write operation did not immediately return upon encountering an RST packet. #1849

Merged

Conversation

newacorn
Copy link
Contributor

The current client implementation does not immediately return when encountering an RST packet while sending a request, but instead ignores it. This behavior is inconsistent with the net/http package and does not make logical sense.

In issue #1232 and the corresponding fix in pull request #1233, an example was used to demonstrate that net/http ignores an RST packet when sending a request and continues to read the response. This was cited as a reason why the fasthttp client should behave similarly.

However, that example represents a special case where the RST packet, even if sent, may not be observed by the client. For the net/http client, even if the RST packet is observed, it won't respond to it if the response channel in the select statement wins over the error channel. But if the error channel for the write request wins priority in the select statement, the client will immediately return the error and not continue processing the response.

Next, let's modify that example to give the client a chance to observe the RST packet. The principle here is to send the request in segments. This way, the later segments will encounter the RST packet error caused by the earlier segments.

func TestRstConnResponseWhileSendingChunkedVersion(t *testing.T) {
	t.Parallel()
	const expectedStatus = http.StatusTeapot
	srv, err := net.Listen("tcp", "127.0.0.1:8089")
	if err != nil {
		t.Fatal(err.Error())
	}

	go func() {
		conn, err := srv.Accept()
		if err != nil {
			t.Errorf(err.Error())
		}

		// Read at least one byte of the header
		// Otherwise we would have an unsolicited response
		ioutil.ReadAll(io.LimitReader(conn, 1))

		// Respond
		conn.Write([]byte("HTTP/1.1 418 Teapot\r\n\r\n"))

		// Forcefully close connection
		err = conn.(*net.TCPConn).SetLinger(0)
		if err != nil {
			t.Errorf(err.Error())
		}
		conn.Close()
	}()

	svrUrl := "http://" + srv.Addr().String()

	cli := http.DefaultClient
	req := http.Request{}
	req.URL, _ = url.Parse(svrUrl)
	reqStrBody := "hello 2323 23323 2323 2323 232323 323 2323 2333333 hello 2323 23323 2323 2323 232323 323 2323 2333333 hello 2323 23323 2323 2323 232323 323 2323 2333333 hello 2323 23323 2323 2323 232323 323 2323 2333333 hello 2323 23323 2323 2323 232323 323 2323 2333333"
	req.Body = F{strings.NewReader(reqStrBody)}
	httpResp, err := cli.Do(&req)
       // On Linux platforms, the error for an RST packet is syscall.ECONNRESET, while on macOS platforms, it is syscall.EPIPE.
	if !errors.Is(err, syscall.EPIPE) && errors.Is(err, syscall.ECONNRESET) {
		t.Errorf("expected error, but got nil")
	}
	/*
		httpResp, err := http.Post(svrUrl, "application/json", strings.NewReader(payload))
		if err != nil {
			t.Fatal(err.Error())
		}
	*/
	if httpResp != nil {
		if expectedStatus == httpResp.StatusCode {
			t.Errorf("Not Expected status code %d", httpResp.StatusCode)
		}
	}
}
type F struct {
	*strings.Reader
}

func (f F) Read(p []byte) (n int, err error) {
	if len(p) > 2 {
		p = make([]byte, 2)
	}
	return f.Reader.Read(p)
}

func (f F) Close() error {
	return nil
}
=== RUN   TestRevertPull4
=== PAUSE TestRevertPull4
=== CONT  TestRevertPull4
--- PASS: TestRevertPull4 (0.00s)
PASS

Because the net/http client implementation handles both the reception of the response and the reception of the request's sending result within a single select statement, this revised test does not guarantee a 100% success rate.

Refer to the corresponding source code in the net/http package.
https://github.com/golang/go/blob/36b45bca66d86020f0b4daf1f15b02921a8dad43/src/net/http/transport.go#L2752-L2804

Since the fasthttp client handles requests and responses sequentially, it can definitely observe errors related to RST packets when sending segmented requests. Therefore, I added the corresponding test in this pull request. However, due to the randomness in the net/http package, I did not include a reference test in the submission.

…ntering an RST packet.

The current client implementation does not immediately return when encountering an RST packet while sending a request, but instead ignores it. This behavior is inconsistent with the net/http package and does not make logical sense.
@newacorn
Copy link
Contributor Author

As for whether to immediately return an error when encountering an RST packet while reading the response or to reset the error to nil, refer to the net/http approach, which was also the practice before pull request 1233:

func TestNetHTTPRespReadRSTBehaving(t *testing.T) {
	log.SetFlags(log.Lshortfile | log.Ltime)
	ln, err := net.Listen("tcp4", "127.0.0.1:8089")
	if err != nil {
		t.Fatal(err)
	}
	go func() {
		for {
			con, err := ln.Accept()
			if err != nil {
				log.Fatal(err)
			}
			con.Write([]byte("HTTP/1.1 200 OK\r\nContent-Length: 3\r\n\r\n"))
			time.Sleep(time.Millisecond * 15)
			con.Close()
		}
	}()
	time.Sleep(time.Millisecond * 10)
	resp, err := http.Get("http://127.0.0.1:8089")
	if err != nil {
		t.Fatal(err)
	}
	_, err = io.ReadAll(resp.Body)
	if !errors.Is(err, syscall.EPIPE) && !errors.Is(err, syscall.ECONNRESET) {
		t.Fatal(err)
	}

}

Result:

=== RUN   TestNetHTTPRespReadRSTBehaving
--- PASS: TestNetHTTPRespReadRSTBehaving (0.03s)
PASS

@erikdubbelboer erikdubbelboer merged commit d5c7d89 into valyala:master Aug 31, 2024
15 checks passed
@erikdubbelboer
Copy link
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants