net/http: request.Body.Read() can be called after client.Do() returns and resp.Body is drained/closed #51907
Comments
|
shorter reproducer , EDIT, it seems different but it happens only in 1.18 too func TestRequestWriteSeek(t *testing.T) {
testServer := httptest.NewServer(HandlerFunc(func(w ResponseWriter, req *Request) {
w.Write([]byte("200"))
}))
defer testServer.Close()
client := &Client{}
body := bytes.NewReader([]byte(strings.Repeat("abcd", 1000)))
req, err := NewRequest("POST", testServer.URL, body)
if err != nil {
t.Fatal(err)
}
resp, err := client.Do(req)
if err != nil {
t.Fatal(err)
}
defer resp.Body.Close()
body.Seek(0, 0)
io.Copy(io.Discard, resp.Body)
}race detected removing |
mknyszek
added
the
NeedsInvestigation
|
CC @neild |
TestRequestWriteSeek is not the same race we're seeing in kubernetes/kubernetes#108906. I don't see how the response writer would race with a Seek on the request body from the client side |
|
The HTTP client transport can continue to read from a request body after This is probably underdocumented in the The fix here is to close the response body before reusing the request body; e.g.: |
|
hrmm... that took longer to fail but still hit the same race between Read and Seek package mytest
import (
"bytes"
"net/http"
"net/http/httptest"
"strings"
"testing"
)
func TestSeek(t *testing.T) {
testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
defer req.Body.Close()
w.Write([]byte("ok"))
}))
defer testServer.Close()
client := &http.Client{}
body := bytes.NewReader([]byte(strings.Repeat("abcd", 1000)))
req, err := http.NewRequest("POST", testServer.URL, body)
if err != nil {
t.Fatal(err)
}
resp, err := client.Do(req)
if err != nil {
t.Fatal(err)
}
resp.Body.Close() // close the response body to complete the request before reusing the request body
body.Seek(0, 0)
} |
|
Interesting. That does look like a bug; nothing should continue reading from the request body after the response body has been closed. |
liggitt
changed the title
|
Updated the issue description with a reproducer for the race occurring after client.Do() returns and resp.Body is drained/closed. It ~quickly fails with race issues when run with |
|
This is interesting, enabling the we can see that the write finish before the recv in a working test however, in a test with a race, the writer returns later than the reader, and races with the @neild can this be related to any changes with the buffers or with some of the optimizations like or is a legit race? |
|
this race happens in 1.17 too $ go version
go version go1.17.6 linux/amd64
$ go test http_test.go -v -race
=== RUN TestSeek
==================
WARNING: DATA RACE
Write at 0x00c0001a2020 by goroutine 7:, if we increase the request body package mytest
import (
"bytes"
"net/http"
"net/http/httptest"
"strings"
"testing"
)
func TestSeek(t *testing.T) {
testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
defer req.Body.Close()
w.Write([]byte("ok"))
}))
defer testServer.Close()
client := &http.Client{}
body := bytes.NewReader([]byte(strings.Repeat("abcd", 10000000))) // <----
req, err := http.NewRequest("POST", testServer.URL, body)
if err != nil {
t.Fatal(err)
}
resp, err := client.Do(req)
if err != nil {
t.Fatal(err)
}
resp.Body.Close() // close the response body to complete the request before reusing the request body
body.Seek(0, 0)
}it seems that the difference is go1.18 is faster to read the response and exit |
ah, that's helpful to know (though disconcerting) |
|
the question is if the writers, to the body of the request or the response, should be cancellable. |
@neild that is not what is happening from my understanding of the code, I can see that there are 2 independent goroutines for processing the request and the response, and the one that reads the request body can not be cancelled by closing the response body, actually I can't see how it can be cancelled by any means. It is easy to test by providing a big buffer in the request #51907 (comment) |
liggitt
changed the title
|
This issue makes it unclear as to whether it is safe to reuse buffers (like Applications may want to reuse buffers (using a The current comments on |
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
Make a request with a resettable body, if the response is a 429, reset the body and repeat the request.
Reported in kubernetes/kubernetes#108906, standalone reproducer here:
What did you expect to see?
Success, as in go1.17 and earlier releases
What did you see instead?
Data race between the body reset and reads from the request body called from
net/http.(*transferWriter).writeBody()The text was updated successfully, but these errors were encountered: