Add option to specify a client timeout #2191
Merged
+79
−22
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
needs triage
maraino
force-pushed
the
mariano/timeout
branch
2 times, most recently
from
f5425ff to
1a05f48
Compare
hslatman
reviewed
Feb 27, 2025
Comment on lines
584
to
592
| return &Client{ | ||
| client: newClient(tr), | ||
| client: newClient(tr, o.timeout), | ||
| endpoint: u, | ||
| retryFunc: o.retryFunc, | ||
| timeout: o.timeout, | ||
| opts: opts, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Can you also set a default timeout? I believe without a default timeout set, it will just wait until the server disconnects based on its timeouts (if any). See the output for different cases below. I know this client will be used with our CA, and we're in control over those timeouts, but it's generally a good practice to set a timeout in any http.Client that is created.
The WithContext methods can also be used, and they seem to behave as expected. However, we're not setting timeouts through the context in places where we use those methods.
# timeout on connection triggered by server; no (default) timeout in the HTTP client or context:
$ go run cmd/step/main.go ca health
client GET https://127.0.0.1:8443/health failed: stream error: stream ID 1; INTERNAL_ERROR; received from peer
# timeout on the context from the client side:
$ go run cmd/step/main.go ca health
client GET https://127.0.0.1:8443/health failed: context deadline exceeded
exit status 1
# timeout on the client:
$ go run cmd/step/main.go ca health
client GET https://127.0.0.1:8443/health failed: context deadline exceeded (Client.Timeout exceeded while awaiting headers)
exit status 1
This commit adds to the ca Client a new option to specify the client timeout. Fixes #2176
hslatman
force-pushed
the
mariano/timeout
branch
from
1a05f48 to
17af05d
Compare
hslatman
force-pushed
the
mariano/timeout
branch
from
17af05d to
3135a2c
Compare
hslatman
approved these changes
Apr 1, 2025
dopey
approved these changes
Apr 1, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This commit adds a new option to specify the client timeout to the CA client.
Fixes #2176