-
Notifications
You must be signed in to change notification settings - Fork 41
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
feat: add timeouts when default client is in use #51
Conversation
b07067a
to
a60ab2b
Compare
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.
We probably want instrumentation around this before setting a default, at least for the application-level timeout. Anecdotally, the DB-less config post can be fairly slow to respond with larger config blobs, and I'm fairly certain that 10s would be short enough to break it for a non-insignificant number of users.
That said, the controller does already instantiate its own HTTP client for use with the Kong client, with no timeouts, so that wouldn't actually be an issue for it.
Would you be open to a separate transport+TLS and HTTP timeout, with the latter being much less aggressive to start, maybe 120s? I'm not aware of issues with the current long (infinite) HTTP timeout, so while I agree that we should set one to follow general good practice, it seems reasonable that we may want to err on the longer side absent data on typical response times.
That's the reason for keeping the timeout so low.
I think a single endpoint behaving uniquely shouldn't be a reason to change the default timeout. For majority of use-cases of this library, Kong should be able to respond within few seconds, barring network/infra hiccups. I'd like to start with a lower timeout as it would help point out problems in the user's stack but I do see how the potential for (unwelcomed) surprises. |
Ideally yes, but as you can't override it per request (searching suggests you can use contexts and NewRequestWithContext to work around lack of dedicated request-level timeouts, but you'd have to then set the timeout on all requests individually), we're kinda stuck with something that's long enough for the slowest endpoint. 60s is better, though I'd still want the controller version to be configurable before releasing it. |
To be fair here, the timeout we are adding here is only a default. |
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.
On finite vs infinite timeout: There is a class of problems (such as network failure or reconnection) that will cause clients of go-kong to wait forever, which is not a good failure mode. Therefore my preference is for a finite timeout.
On whether the default timeout should be 10, 60, 120s or anything else: There are at least 2 components to be considered: network RTT, and remote processing duration.
Network RTT can easily take 10 seconds in corner cases such as a droopy internet connection (think: tethered phone when in movement on a highway, roaming between cells) or network incidents such as packet loss or routing changes.
Also, as @rainest pointed out, writing DB-less /config
can trip the 10s timeout.
Therefore my educated guess is that 60s might be a good choice here, as a compromise between:
- not breaking automations suffering from an intermittent lag longer than 10s (which seems likely for a variety of rasons),
- providing a decent failure mode in case of a filtered connection, a permanent network failure or other similar issues.
Therefore my suggestion is a 60s default timeout, + the attached code comment.
@@ -87,7 +92,16 @@ type Status struct { | |||
// NewClient returns a Client which talks to Admin API of Kong | |||
func NewClient(baseURL *string, client *http.Client) (*Client, error) { |
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.
I second Travis's point about making it possible to adjust the timeout as the client, and recommend the solution below.
Expecting the user to adjust the timeout in a returned client isn't perfect either because it's at odds with the open-closed principle.
What I'd recommend would be to provide an "overloaded" NewClient
:
func NewClient(...) (*Client, error) {
return NewClientWithTimeout(defaultTimeout, ...)
}
func NewClientWithTimeout(timeout time.Duration, ...) (*Client, error) {
// actual implementation goes here
}
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 seems reasonable at first glance but on a closer inspection, it seems an unnecessary addition to the API surface.
An *http.Client
has numerous knobs for tweaking network behavior and addition a method for each to this API seems unnecessary burden.
The reason we accept an *http.Client
in the constructor is so that the user of the API can define all client-specific behavior as they see fit. This is how this client is used in all downstream repositories I know.
We may add WithTimeout
function today but then that opens the door for WithXs for future and adding those here doesn't really make things better for us or the end user in my view.
For reference, here is another API that does this the same way: https://pkg.go.dev/github.com/google/go-github/v35/github#NewClient
(go-kong is inspired heavily from that API)
Based on the above comments, I updated the default timeout to |
kong/client.go
Outdated
const defaultBaseURL = "http://localhost:8001" | ||
const ( | ||
defaultBaseURL = "http://localhost:8001" | ||
defaultTimeout = 60 * time.Second |
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.
Consider exporting the default timeout and adding a godoc comment, for sake of self-documenting code.
This will make a difference for readers of autogenerated docs.
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.
Done
Go's default client has no timeouts and relies on the kernel timeouts, which goes against the recommended practice of setting timeouts at application-level whenever possible. This change ensures that if the default client is used, it is used with proper timeouts for TCP handshake, TLS handshake and HTTP request-response lifecycle. There is no emperical data for the value of 10 seconds. It is a good starting point and a sane default. Users are encouraged to create their own http.Client and many users already do so to control TLSConfig.
Go's default client has no timeouts and relies on the kernel timeouts,
which goes against the recommended practice of setting timeouts at
application-level whenever possible.
This change ensures that if the default client is used, it is used with
proper timeouts for TCP handshake, TLS handshake and HTTP
request-response lifecycle.
There is no emperical data for the value of 10 seconds.
It is a good starting point and a sane default. Users are encouraged to
create their own http.Client and many users already do so to control
TLSConfig.