HttpClient observations

[AleRoe]

Hi Michiel,

I'd like to share some observations regarding the way HttpClient is used in LocalHueApi, especially in regards to the possibility of passing in your own instance of HttpClient both for the constructor as well as when calling StartEventStream(), presumably in order to reuse an existing (shared) HttpClient instance (which to my knowledge is considered good practice anyway).

So I gave it a try - and ran into some findings that I wanted to share:

1) Passing a HttpClient instance in the constructor By default, GetConfiguredHttpClient() creates a new HttpClient instance if none is passed as a parameter to the method, and then goes on to set some properties (Timeout and BaseAddress) as well as setting DefaultRequestHeaders, and then stores the object for further use.

But this fails if you pass in a HttpClient instance that has previously been used to send a request, because these properties can only be modified before sending the first request.

Example:

var handler = new HttpClientHandler() 
{ ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator };
var client = new HttpClient(handler);

client.BaseAddress = new Uri("https://api.publicapis.org");
_= await client.GetAsync("/entries");

var localHueClient = new LocalHueApi(config["ip"], key: config["key"], client);
var result = await localHueClient.GetLightsAsync();

-> System.InvalidOperationException: This instance has already started one or more requests. Properties can only be modified before sending the first request.

Instead of setting the BaseAddress property and Headers on the instance, the complete Uri as well as the RequestHeaders should be passed using a HttpRequestMessage object and the request then sent using HttpClient.SendAsync(). As you are making extensive use of the HttpClientJsonExtensions, this would quite a rewrite though....

Side notes: a) Setting the DefaultRequestHeader on a shared HttpClient instance would add that header to any other subsequent requests sent using that instance. b) Server certificate validation is supressed by using a custom HttpClientHandler which can only be set in the HttpClient constructor. So any shared HttpCLient instance would have to have that handler set as well, which could be a potential risk.

2) Passing the same HttpClient instance in the constructor and to StartEventStream() This results in a similar situation (InvalidOperationException) when GetConfiguredHttpClient() tries to set the timeout. For some reason, the expection is only thrown in Debug mode though.

3) StopEvenStream() throws unhandled TaskCanceledException (not related to HttpClient) Last but not least - and not related to HttpClient() - I noticed that using StopEventStream() to cancel the EventStream throws an unhandled exception on var streamReader = new StreamReader(await infiniteHttpClient.GetStreamAsync(EventStreamUrl, cancelToken)), which bubbles up further down the line in my async calls. I would suggest wrapping the outer while-loop in a try-catch block to gracefully terminate the StreamReader.

Please consider my findings as improvement suggestions. Assuming that the intent of adding HttpClient as parameters was to enable reuse of a shared instance, I do think they should be addressed though.

Kind regards Alexander

[michielpost]

Thanks, great feedback. I will need to create some testcases to investigate and then see if I can check if the HttpClient is already used and then maybe throw an exception.

I do think most people understand to use one HttpClient for their Hue calls and another for their other http calls. If they don't and mix them, they will soon encounter the things you mention and use a dedicated HttpClient for Hue anyway. But documentation/code comments can also be improved there.

[michielpost]

I added a try/catch block for the TaskCanceledException, I think this should fix your third point. As for the first two, I created test cases and see that they fail. However I won't be doing a rewrite to solve this, as using your own HttpClient is meant as a pro feature, so you should already know what you are doing and test your setup.

[AleRoe]

Hi Michiel, thanks for addressing the TaskCanceledException issue!

Regarding the HttpClient issues, I understand your point. As said, these were just observations, albeit imho they are important in situations where consumers want to reuse an existing HttpClient instance (which is recommended). But I can live with the situation :-)

Kind regards Alexander

[AleRoe]

Hi Michiel, I can confirm that version 0.10.0 resolves the unhandled TaskCanceledException when using StopEventStream(). The library now shuts down gracefully, so I'm closing this issue. Thanks! P.S. I might take up the HttpClient refactoring on a separate fork.