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.

Sign up for GitHub

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

Jump to bottom

Increase upup http response header timeout #12694

Merged
merged 1 commit into from
Nov 8, 2021
Merged

Increase upup http response header timeout #12694

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion upup/pkg/fi/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func downloadURLAlways(url string, destPath string, dirMode os.FileMode) error {
KeepAlive: 30 * time.Second,
}).DialContext,
TLSHandshakeTimeout: 10 * time.Second,
ResponseHeaderTimeout: 10 * time.Second,
ResponseHeaderTimeout: 30 * time.Second,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should use the default transport directly

https://cs.opensource.google/go/go/+/refs/tags/go1.17.3:src/net/http/transport.go;bpv=1;bpt=1;l=43?gsn=DefaultTransport&gs=kythe%3A%2F%2Fgolang.org%3Flang%3Dgo%3Fpath%3Dnet%2Fhttp%23var%2520DefaultTransport
httpClient := &http.Client{
Transport: http.DefaultTransport
},

I can't remember where these values are coming, if there are no special reasons and the default are more conservative we should use them

Copy link
Contributor Author

@AlexLast AlexLast Nov 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this was implemented to support slow downloads of large files - 19f47d4

Although I'm unsure of the history around this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it, but I can't remember where did I get those values, most probably from some kubernetes code, but they don't seem to apply here so I think that better to use the more conservative values from the default client, that in this case doesn't have any timeout at all for response header.

	httpClient := &http.Client{
		Transport: &http.Transport{
	Proxy: ProxyFromEnvironment,
	DialContext: (&net.Dialer{
		Timeout:   30 * time.Second,
		KeepAlive: 30 * time.Second,
	}).DialContext,
	ForceAttemptHTTP2:     true,
	MaxIdleConns:          100,
	IdleConnTimeout:       90 * time.Second,
	TLSHandshakeTimeout:   10 * time.Second,
	ExpectContinueTimeout: 1 * time.Second,
		},
	}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the diff is removing the ResponseHeaderTimeout: 30 * time.Second, and increasing the IdleConnTimeout: 90 * time.Second, and set ForceAttemptHTTP2: true,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the goal was to reduce the IdleConnTimeout mostly. Not sure if ForceAttemptHTTP2 affected things in any way in the past, but don't see a real need for in in the case of downloading various binaries.
This change looks pretty good to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you know better than me the network requirements here, the change lgtm too, just wanted to add more context in case you think some other change will needed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate. Thanks @aojea! 😄

IdleConnTimeout: 30 * time.Second,
},
}
Expand Down