Skip to content

Revert PR 9 changes #11

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

Merged
merged 1 commit into from
Apr 21, 2021
Merged

Conversation

mgoodfellow
Copy link
Contributor

@mgoodfellow mgoodfellow commented Apr 16, 2021

This PR is to revert the changes made in #9

If a timeout is required on the HttpClient it should be set on an HttpClient instance passed in via the new SocketLabsClient(x, y, httpClient) ctor call.

Why?

A HttpClient timeout property cannot be modified after using it.

https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclient.timeout?view=net-5.0

As the timeout is set during the SendAsync call, it will throw an error next time this code runs unless a new HttpClient instance is created for each send call. This has broken HttpClient reuse.

The same timeout will apply for all requests using this HttpClient instance. You may also set different timeouts for individual requests using a CancellationTokenSource on a task. Note that only the shorter of the two timeouts will apply.

Please note the above - from the MSDN docs. If per-request timeouts are required, then CTS should be used on the task, causing the task to cancel.

If SendAsync is called for a second time on the same SocketLabsClient the following will be thrown:

System.InvalidOperationException: This instance has already started one or more requests. Properties can only be modified before sending the first request.
   at System.Net.Http.HttpClient.CheckDisposedOrStarted()
   at System.Net.Http.HttpClient.set_Timeout(TimeSpan value)
   at SocketLabs.InjectionApi.SocketLabsClient.SendAsync(IBasicMessage message, CancellationToken cancellationToken)

I recommend reverting as a user can simply create their own HttpClient with their use case specific RequestTimeout, and then pass that configured client in via the constructor which allows this.

However, if per-request timeouts were required, and the SocketLabsClient wanted to support that, you could consider using CTS on the Task to cancel in-flight requests, however, this change is out of scope of this fix. I would also argue that the caller should configure this as required.

@david-schrenker
Copy link
Member

Thank you for catching this issue. You are correct that by exposing the HttpClient in the constructor we already have the option to set your own timeout.

@david-schrenker david-schrenker merged commit 078590d into socketlabs:main Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants