Make the default timout 5 minutes when timeout is 0 #1056
Conversation
|
I am a robot that works on MagicModules PRs! I built this PR into one or more PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
| @@ -41,6 +41,10 @@ func sendRequestWithTimeout(config *Config, method, rawurl string, body map[stri | |||
| reqHeaders.Set("User-Agent", config.userAgent) | |||
| reqHeaders.Set("Content-Type", "application/json") | |||
|
|
|||
| if timeout == 0 { | |||
There was a problem hiding this comment.
Why not just have sendRequest send 5 instead of 0?
There was a problem hiding this comment.
I had considered that, but if somebody called sendRequestWithTimeout and passed in a 0 timeout (the nil value for timeout) then it will timeout immediately which doesn't feel like a good behavior for handling a nil value. I would have preferred if a nil value caused there to be no timeout at all but that would have made the method a bit more complex so I took this middle route.
I don't feel super strongly about any of the options so let me know if you have a preference.
There was a problem hiding this comment.
Drive-by but personally I would prefer passing in the default value in sendRequest instead of replacing actual timeout values of 0 with that default
There was a problem hiding this comment.
Out of curiosity, what's your opinion on expected behavior when passing in a nil value into the callWithTimeout method?
There was a problem hiding this comment.
Either an explicit error or something terrible happening! I could also see one of 0 or nil meaning no timeout at all if time.Duration doesn't express that in another way.
There was a problem hiding this comment.
wouldn't that be a compile-time error?
There was a problem hiding this comment.
Passing actual nil would be compile time, but it's recommended that you code should handle zero value inputs in a responsible manner. So I would think that if you passed in a zero value for a timeout that the code would handle it with no timeout, since a timeout of 0 isn't really a thing.
Kind of like how methods that are on pointer receivers shouldn't throw errors or panic when called with a nil receiver.
There was a problem hiding this comment.
That makes sense. I don't think there's a way to do infinity, but you can certainly just do a very large number.
|
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit 9e08a51) have been included in your existing downstream PRs. |
| @@ -41,6 +41,10 @@ func sendRequestWithTimeout(config *Config, method, rawurl string, body map[stri | |||
| reqHeaders.Set("User-Agent", config.userAgent) | |||
| reqHeaders.Set("Content-Type", "application/json") | |||
|
|
|||
| if timeout == 0 { | |||
There was a problem hiding this comment.
That makes sense. I don't think there's a way to do infinity, but you can certainly just do a very large number.
|
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit 0a211a8) have been included in your existing downstream PRs. |
Tracked submodules are build/terraform-beta build/terraform build/ansible build/inspec.
modular-magician
force-pushed
the
default-timeouts
branch
from
23cc9de to
cb0a212
Compare
[all]
Make the default timout 5 minutes when timeout is 0
[terraform]
[terraform-beta]
[ansible]
[inspec]