SPIE-Worksphere / haystack-csharp

Haystack .net client
Academic Free License v3.0
16 stars 6 forks source link

Connect to several haystack server from one app #105

Closed minzdrav closed 1 year ago

minzdrav commented 1 year ago

Hi haystack-csharp dev team public HaystackClient(IAuthenticator authentication, Uri apiUri) - this constructor uses the default HTTP client. This client is static. When I try to connect to two Haystack servers at the same time, the second connection overwrites the auth header, and the first client is unable to connect to the first server. It's simple to workaround without changes to the library - I'm using the constructor with httpClient - public HaystackClient(HttpClient client, IAuthenticator authenticator, Uri apiUri). I think this is a bug, and the default client shouldn't be static or this behavior should be documented in the constructor description. I suggest using the BuildDefaultClient method instead of the static _defaultClient property.

Breederveld commented 1 year ago

Hi @minzdrav, I've looked into this and the constructor without the HttpClient is a convenience method that, as you mentioned, can be skipped in favor of providing an explicit HttpClient. In most scenario's this is helpful, as it limits the amount of HttpClients created (which is a big performance boon as you can use the default http client pool behavior). However I understand it might not be very clear form the constructor as it only mentions "using a default HttpClient" which doesn't explicitly say it's static. If you can come up with wording that makes this behavior more clear, I think this may help others in the future.

minzdrav commented 1 year ago

@Breederveld is it sounds good?

        /// <summary>
        /// Connect using a specified authenticator and base uri, using a default HttpClient.
        /// Default HttpClient is static and shared between all instances of this class.
        /// If you need to connect to several different haystack servers, you should use a different HttpClient.
        /// </summary>
Breederveld commented 1 year ago

Thank you for the documentation example. I've added it to both constructors that make use of the default HttpClient. A new NuGet version 2.0.27 is now available.