- Notifications
You must be signed in to change notification settings - Fork554
Increase AndroidClientHandler timeouts#3328
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Merged
Uh oh!
There was an error while loading.Please reload this page.
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
`AndroidClientHandler` has no way of accessing the `HttpClient.Timeout` propertyin order to set the timeout value of *two* native http clientproperties (connect and read timeouts) and so it uses two custom properties toprovide these values. So far, the values were set to 100s for the read timeoutand 120s for the connect timeout, which seemed to be a reasonable value fortheir purposes. However, if a developer sets `HttpClient.Timeout` to avalue *larger* than our defaults, `AndroidClientHandler` values "win" and theconnection/read time out earlier. The workaround is to set "our" timeouts alongwith the `HttpClient` one, but if the developer cannot do it, for any kind ofreasons (i.e. to avoid platform-specific code) then they are faced with anannoying situation.The real fix would be to improve `HttpClient` API so that its associated clienthandler can access `HttpClient` properties, but since it's not a quick fix wecan implement now, this commit bumps the default timeout values tothe (unreasonable) value of 24h to make sure we use values higher than the mostlikely figures assigned to `HttpClient.Timeout` and to match Xamarin.iOSNSUrlSessionHandler defaults.
The Azure statuses are off - the builds have finished and are green |
jonathanpeppers approved these changesJul 5, 2019
In the final commit message, it might be good to link to:https://github.com/xamarin/xamarin-macios/blob/4a704e448e117b46f1636565e0e43f0b81500bc2/src/Foundation/NSUrlSessionHandler.cs#L138-L141 |
jonathanpeppers pushed a commit that referenced this pull requestJul 9, 2019
Context:http://work.devdiv.io/911705Context:dotnet/macios@30d60bf`AndroidClientHandler` has no way of accessing the`HttpClient.Timeout` property in order to set the timeout value of*two* native http client properties (connect and read timeouts), soit uses two custom properties to provide these values. So far, thevalues were set to 100 seconds for the read timeout and 120 secondsfor the connect timeout, which seemed to be a reasonable value fortheir purposes.However, if a developer sets `HttpClient.Timeout` to a value *larger*than our defaults, `AndroidClientHandler` values "win" and theconnection/read times out earlier. The workaround is to set "our"timeouts along with the `HttpClient` one, but if the developer cannotdo it, for any kind of reasons (i.e. to avoid platform-specific code),then they are faced with an annoying situation.The real fix would be to improve `HttpClient` API so that itsassociated client handler can access `HttpClient` properties, but asthat's not a quick fix we can implement now, we instead bump thedefault timeout values to the (unreasonable) value of 24 hours tomake sure we use values higher than the most likely figures assignedto `HttpClient.Timeout`, and to match the Xamarin.iOS`NSUrlSessionHandler` defaults.
jonathanpeppers pushed a commit that referenced this pull requestJul 9, 2019
Context:http://work.devdiv.io/911705Context:dotnet/macios@30d60bf`AndroidClientHandler` has no way of accessing the`HttpClient.Timeout` property in order to set the timeout value of*two* native http client properties (connect and read timeouts), soit uses two custom properties to provide these values. So far, thevalues were set to 100 seconds for the read timeout and 120 secondsfor the connect timeout, which seemed to be a reasonable value fortheir purposes.However, if a developer sets `HttpClient.Timeout` to a value *larger*than our defaults, `AndroidClientHandler` values "win" and theconnection/read times out earlier. The workaround is to set "our"timeouts along with the `HttpClient` one, but if the developer cannotdo it, for any kind of reasons (i.e. to avoid platform-specific code),then they are faced with an annoying situation.The real fix would be to improve `HttpClient` API so that itsassociated client handler can access `HttpClient` properties, but asthat's not a quick fix we can implement now, we instead bump thedefault timeout values to the (unreasonable) value of 24 hours tomake sure we use values higher than the most likely figures assignedto `HttpClient.Timeout`, and to match the Xamarin.iOS`NSUrlSessionHandler` defaults.
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Labels
None yet
3 participants
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.
AndroidClientHandlerhas no way of accessing theHttpClient.Timeoutpropertyin order to set the timeout value oftwo native http client
properties (connect and read timeouts) and so it uses two custom properties to
provide these values. So far, the values were set to 100s for the read timeout
and 120s for the connect timeout, which seemed to be a reasonable value for
their purposes. However, if a developer sets
HttpClient.Timeoutto avaluelarger than our defaults,
AndroidClientHandlervalues "win" and theconnection/read time out earlier. The workaround is to set "our" timeouts along
with the
HttpClientone, but if the developer cannot do it, for any kind ofreasons (i.e. to avoid platform-specific code) then they are faced with an
annoying situation.
The real fix would be to improve
HttpClientAPI so that its associated clienthandler can access
HttpClientproperties, but since it's not a quick fix wecan implement now, this commit bumps the default timeout values to
the (unreasonable) value of 24h to make sure we use values higher than the most
likely figures assigned to
HttpClient.Timeoutand to match Xamarin.iOSNSUrlSessionHandler defaults.