Change RestClientOptions to MaxTimeout which is what it really is #1804
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
… resulting actual timeout is going to be the shorter of the two. By default HttpClient has a timeout of 100 seconds built in, so that means if someone is expecting to be able to send a request and have it wait longer than 100 seconds, it won't work unless the client is also changed. So I propose we change the property in RestClientOptions which is then applied to HttpClient to be called MaxTimeout so it is a lot more clear what the connection is? You cannot have a longer timeout for any request unless you change this value. It might also make sense to set this to a default value of something (maybe 100 seconds to make it clear) rather than leaving it null, so the user of the library will be more aware of how the client timeout and request timeout interact with each other? The other option would be to always set the timeout in the client to infinite, and then always use a request timeout that is the Max() of either the client timeout or the request one if the request one was provided?
kendallb
force-pushed
the
feature/timeout-cleanups
branch
from
895877a to
5bbdcf3
Compare
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
If you have a timeout on the client AND a timeout on the request, the resulting actual timeout is going to be the shorter of the two. By default HttpClient has a timeout of 100 seconds built in, so that means if someone is expecting to be able to send a request and have it wait longer than 100 seconds, it won't work unless the client is also changed.
So I propose we change the property in RestClientOptions which is then applied to HttpClient to be called MaxTimeout so it is a lot more clear what the connection is? You cannot have a longer timeout for any request unless you change this value.
It might also make sense to set this to a default value of something (maybe 100 seconds to make it clear) rather than leaving it null, so the user of the library will be more aware of how the client timeout and request timeout interact with each other?
The other option would be to always set the timeout in the client to infinite, and then always use a request timeout that is the Max() of either the client timeout or the request one if the request one was provided?
Purpose
This pull request is a: