net/http: clarify whether Requests can be reused in HTTP/2 #45311
Comments
seankhliao
added
the
NeedsInvestigation
|
cc @fraenkel |
|
Requests are not safe to be reused for http or http/2. |
|
The rules for HTTP/2 are the same as the rules for HTTP/1, as documented previously in 3039bff for #19653. Your test code is wrong; it's the response is still active, as you didn't close the body: client := http.DefaultClient
headResp, err := client.Do(req)
if err == nil {
if headResp.StatusCode == 200 {
req.Header.Set("Range", "bytes=0-")
}
} |
|
I still see races after adding a |
|
@bradfitz I am not so sure about previous statements given both contexts that are added or trailing headers that could be added. These are two items I noticed at a glance., One would have to look at all the changes that are made to a request when it is not copied. |
|
@bradfitz You are right that the race is no longer reproducible with the same HTTP/2 host & method when the body is closed. However I did manage to reproduce it even with body closure: package main
import (
"net/http"
"testing"
)
func TestRepro(t *testing.T) {
req, err := http.NewRequest("HEAD", "https://releases.hashicorp.com/", nil)
if err != nil {
t.Fatal(err)
}
client := http.DefaultClient
headResp, err := client.Do(req)
headResp.Body.Close() // <--- closing body here
if err == nil {
if headResp.StatusCode == 200 {
req.Header.Set("Range", "bytes=0-")
}
}
}I do not know what difference between the two servers/requests is triggering it, but it is getting triggered reliably. |
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?
What did you expect to see?
Either implementation that makes it safe to reuse the
http.Requestin the context outlined in the above snippet where aHEADrequest is sent to check whether serveraccepts-rangeand thenGETis sent with relevantrangeheader to effectively resume previously interrupted download.Or docs clarifying that
http.Requestis not safe to reuse in this context.The current wording doesn't make it clear unfortunately:
go/src/net/http/client.go
Lines 130 to 132 in 135c9f4
which was added as part of #19653
Even if the body is closed in the above example, the race condition doesn't go away because it seems to have a different root cause. In HTTP/2 a goroutine stays around and accesses request headers as can be seen below from the stack trace.
go/src/net/http/h2_bundle.go
Lines 9189 to 9193 in 135c9f4
What did you see instead?
The text was updated successfully, but these errors were encountered: