fix: don't timeout a slow version download #910
Conversation
| @@ -220,7 +220,12 @@ async function download(ver: RunnableVersion): Promise<string> { | |||
| downloadOptions: { | |||
| quiet: true, | |||
| getProgressCallback, | |||
| timeout: 10000, | |||
| timeout: { | |||
| lookup: 15000, | |||
There was a problem hiding this comment.
question: The lookup option docs mention that:
It is preferred to not use any greater value than 100.
I don't really understand the reasoning behind it. Is there a reason you chose 15000 here?
There was a problem hiding this comment.
I'm not too familiar with Got either! Didn't spot that part in the docs.
I used larger values to err on the side of ensuring these download requests don't time out. Currently the UX isn't ideal (under Preferences -> Electron at least) since you don't get feedback if a time out happens. Actually the first two times I ran into it (and why I dug in) I clicked the "Download" button, looked away at something else, and when I looked back it was back on saying "Download", and I didn't know what had happened. 😄
So, one goal with this PR is just to try to use values which ensure actually working requests don't time out, even if they're slow. I understand having the time out so the requests don't hang indefinitely, but I think it needs to be balanced with ensuring requests which are in-progress but on a bad network don't get timed out prematurely, as it is not a good UX at the moment.
There's the high-level thought process. So I used "Network Link Conditioner" to try various weak/bad networks, and found I needed a bit higher time out values (like for connect) to ensure it met my goals of not timing out valid requests.
I'd be curious to know more about their reasoning for a max value of 100ms there. Some of the network profiles I tried for testing include high latency (500ms) and packet loss, so 100ms for the DNS lookup seems like an awfully low time out value which would make that step time out on a lot of network conditions.
There was a problem hiding this comment.
The documentation was revamped in v12 and the v11 docs actually don't mention this, from what I can tell: https://github.com/sindresorhus/got/tree/v11#timeout
I'm fine with just keeping it this way, TBH!
There was a problem hiding this comment.
Cool! These could definitely be tweaked in the future, I just opted to keep it more relaxed by default to err on the side of not timing out bad requests. It looks like the timeout was only added to Fiddle a couple months ago anyway, so this just pulls back that change a little bit, and I don't think it will affect the original goal of adding the timeout, which was to handle offline situations from hanging indefinitely.
Fixes #909.
This fixes the current timeout behavior, which will cause a timeout for any request lasting over 10 seconds, even if it is an in-progress download which is simply a bit slow. Change bumps time outs to 15s, and breaks out individual
gottimeout values sincesocketis the timeout that makes the most sense to set (resets time out each time data is received), but once you break out one timeout you need to give the others values as well or they will have no timeout.I tested these changes on macOS with the "Network Link Conditioner" so that I could try different network conditions. With these changes it could successfully download with the "3G" profile, and it could start downloading with the "Edge" and "Very Bad Network" profiles as well, although I did not wait for those to complete as it would have taken hours.
cc @erickzhao