-
Notifications
You must be signed in to change notification settings - Fork 48
Description
Problem
HttpClientRequestAdapter implements IDisposable, forcing consumers to always dispose of it even when HttpClient is passed as a parameter, making it more difficult to use in those scenarios.
Disposal only exists for when the adapter creates the client itself, meaning calling Dispose does absolutely nothing otherwise:
kiota-dotnet/src/http/httpClient/HttpClientRequestAdapter.cs
Lines 701 to 708 in f71b96c
| protected virtual void Dispose(bool disposing) | |
| { | |
| // Cleanup | |
| if(createdClient) | |
| { | |
| client?.Dispose(); | |
| } | |
| } |
Context
In all of our kiota usage scenarios, we leverage HttpClientRequestAdapter and pass it a HttpClient instance managed by DI, so we can customize that client using standard extensions and have its lifecycle managed correctly via IHttpClientFactory.
However, HttpClientRequestAdapter implements IDisposable to be able to manually dispose of HttpClient instance it creates itself, when not provided one externally.
Because of that, we get CA2000 violations when manually instantiating it, such as here:
internal sealed class MediaApiClientFactory(HttpClient httpClient, CustomTokenProvider customTokenProvider)
{
public MediaApiClient Create()
{
HttpClientRequestAdapter requestAdapter = new(
new BaseBearerTokenAuthenticationProvider(customTokenProvider),
httpClient: httpClient);
return new MediaApiClient(requestAdapter);
}
}Since request adapters are tightly coupled to each API client, it makes sense to create a factory like this to create both the adapter and the client in the same place, but this now creates a problem in that we never dispose requestAdapter since we can't dispose of it in the scope of the Create method.
It would be nice if there was an implementation that did not make any assumptions about HttpClient: it would always take it from outside, and not implement IDisposable itself.
Workarounds
I thought about switching to registering the HttpClientRequestAdapter creation on the container, but that creates problems for other clients that could need other adapter implementations. So we would be forced to use something like keyed registrations here.
Alternatively, we could create the adapter on the factory's constructor and store it in the factory instance. Then, implement IDisposable on the factory itself and let the container take care of it.
As a last option, we could also suppress the warning on this particular scenario since we know disposal will not do anything, but that feels like a really bad practice to me.
I don't like any of these options :(
Metadata
Metadata
Assignees
Labels
Type
Projects
Status