-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Added body close on retry for downloader round trip #10008
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uvojpkln0l
@@ -183,6 +183,11 @@ func (r *requestHandler) RoundTrip(req *http.Request) (resp *http.Response, err | |||
case http.StatusInternalServerError, http.StatusBadGateway: | |||
r.downloader.stats.WebseedServerFails.Add(1) | |||
|
|||
if resp.Body != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe also add:
defer func() {
if err == nil && resp != nil && resp.Body != nil {
resp.Body.Close()
}
}
to close at panic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an intermediate function I'm not sure it should be catching panics - I think they should propagate up to the caller ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If need propagate - can propagate by panic(rec)
. i'm only about closing body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if err == nil && delay < maxDelay {
delay = delay + (time.Duration(rand.Intn(200-75)+75)*delay)/100
}
did you mean != nil
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: you also can take a look for inspiration https://github.com/hashicorp/go-retryablehttp/blob/main/client.go#L535
Plz add comment about: why this status codes are choosen, why no other retryable codes: https://stackoverflow.com/a/74627395 |
Add missing body close method when webseed roundtrip is retried