Make the timeout configurable globally #720
Conversation
The cli package uses just HTTPClient for this.
This configures the timeout on the cli package rather than making a whole new client.
In the root command, configure a persistent timeout flag, and check it in the 'prerun' logic, overriding the default value if it isn't the zerovalue for the flag.
There was a problem hiding this comment.
@kytrinyx thanks for getting to this. I was hoping to work on it during my flight. I left some comments about the default timeout, with some context to help paint the picture.
I think the timeout flag is a great addition, but I think we can reduce things further with a higher default time out.
api/client.go
Outdated
| // DefaultHTTPClient configures a timeout to use by default. | ||
| DefaultHTTPClient = &http.Client{Timeout: 10 * time.Second} | ||
| // TimeoutInSeconds is the timeout the default HTTP client will use. | ||
| TimeoutInSeconds = 10 |
There was a problem hiding this comment.
Judging from the user comments we’ve seen pertaining to this issue, and from what I’ve seen after diving into http Client connection errors. I recommend we bump the default to 30s even 60s.
I’m 75% sure the issue is that 10s is not enough time to complete the connection (dns, tls handshake, handle response, etc), may it be a slow DNS, slow response time from backend or slow Internet connections with large roundtip times.
We can use the httptrace pkg, maybe when passed the debug flag, to gain more insight on the issue.
If you're wondering what's the 75% make up: 25% user comments (which ruled out OS, CLI), 50% my research and local testing. What's missing is the httptrace stats.
There was a problem hiding this comment.
I think that's fine. As long as we have some default.
I'll set the default to 60s everywhere.
cmd/troubleshoot.go
Outdated
| @@ -30,7 +29,7 @@ If you're running into trouble, copy and paste the output from the troubleshoot | |||
| command into a GitHub issue so we can help figure out what's going on. | |||
| `, | |||
| RunE: func(cmd *cobra.Command, args []string) error { | |||
| cli.HTTPClient = &http.Client{Timeout: 20 * time.Second} | |||
| cli.TimeoutInSeconds = 20 | |||
There was a problem hiding this comment.
What if we multiply the default value as opposed to hard coding to 20s? If a user is having a problem pining the API and setting a timeout it would appear that the timeout is not enough, but in fact it is always 20s which the user may not know or care to look at the source.
There was a problem hiding this comment.
You mean, in troubleshoot say something like cli.TimeoutInSeconds = cli.TimeoutInSeconds * 5 or something?
There was a problem hiding this comment.
Yeah, maybe with a smaller multiplier.
There was a problem hiding this comment.
Let's start with 2 and see how we fare :)
| @@ -39,6 +40,10 @@ Download exercises and submit your solutions.`, | |||
| if verbose, _ := cmd.Flags().GetBool("verbose"); verbose { | |||
| debug.Verbose = verbose | |||
| } | |||
| if timeout, _ := cmd.Flags().GetInt("timeout"); timeout > 0 { | |||
| cli.TimeoutInSeconds = timeout | |||
| api.TimeoutInSeconds = timeout | |||
There was a problem hiding this comment.
I see what you’re talking about in issue #719. I will follow up shortly.
|
Alrighty, updated the timeouts per the discussion. Let me know what you think! |
|
It is working now, thank you very much! |
This tweaks the HTTP clients that we create to use a package variable for the timeout instead of hard-coding it.
Then it adds a persistent flag to the root command, which lets the 'pre-run' logic override the package variables if the flag is populated with something other than the zero value.
Closes #701