reactiveui
[Bug]: HttpClients not disposed
Describe the bug 🐞
It appears that the source-generated clients receive ownership of an injected HttpClient, which they do not dispose. In fact, these clients do not implement IDisposable at all. HttpClient is IDisposable, however.
Should HttpClient always be disposed? No, not if it originated from the DI container, which is then responsible for its lifetime. In that case, it must not be disposed. But in the scenario where the code just created a new HttpClient and gave it to the source-generated client to own, then that client becomes responsible for the disposal.
Especially with the generated clients currently being registered with transient lifetime, a lot of undisposed HttpClient instances can be accumulated.
Step to reproduce
- Call
services.AddRefitClient<IWhatever>(). - Use Go To Implementation (usually Ctrl+F12) to view the source-generated implementation.
- Observe how the type is not
IDisposable, and yet it injects a disposable resource, theHttpClient. - See also how the generated client is registered with transient lifetime and is fed a brand new
HttpClientthat is otherwise abandoned.
Reproduction repository
https://github.com/reactiveui/refit
Expected behavior
Preferably, an HttpClient not owned by the container should not be injected (see #1646).
Alternatively, an injected HttpClient not owned by anything else should become owned by the generated client, requiring it to implement IDisposable to dispose of the resource. This would solve the problem when parent services or the generated client have scoped lifetime, but not for transient parents! A transient parent that is IDisposable needs to be disposed by the caller. Not only is this often overlooked, but it is also hard to do if the interface being implemented itself does not implement IDisposable. This shows why the former solution is far, far preferable.
As such, solving #1646 appropriately may make the current issue obsolete. However, the issue seemed significant and complex enough to warrant a separate ticket.
Workaround
Edit: @derekm has found that if your client interface implements IDisposable, then the source-generated client will implement it and use it to dispose the HttpClient.
Screenshots 🖼️
No response
IDE
No response
Operating system
No response
Version
No response
Device
No response
Refit Version
No response
Additional information ℹ️
No response
My understanding is that HttpClients created by IHttpClientFactory don't need to be disposed explicitly:
Disposal of the client isn't required. Disposal cancels outgoing requests and guarantees the given HttpClient instance can't be used after calling Dispose. IHttpClientFactory tracks and disposes resources used by HttpClient instances. The HttpClient instances can generally be treated as .NET objects not requiring disposal.
Source: https://learn.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-2.1#httpclient-and-lifetime-management-3
Learn about using the IHttpClientFactory interface to manage logical HttpClient instances in ASP.NET Core.
As @Timovzl wrote:
Should HttpClient always be disposed? No, not if it originated from the DI container, which is then responsible for its lifetime. In that case, it must not be disposed. But in the scenario where the code just created a new HttpClient and gave it to the source-generated client to own, then that client becomes responsible for the disposal.
Especially with the generated clients currently being registered with transient lifetime, a lot of undisposed HttpClient instances can be accumulated.
This is very true.
Yes, if Refit did new HttpClient(), I totally agree it would need explicit disposal, but Refit uses IHttpClientFactory and as such is not required to dispose the client. Please correct me if I'm wrong, but I haven't seen any documentation that claims otherwise.
@velvolue Based on the docs you linked, it turns out disposal is not strictly necessary in the current implementation.
Note that is still the most sensible thing to do: ensuring that something takes responsibility for a disposable resource.
More importantly, the client that gets the injected client cannot know how that client was obtained. That is an implementation detail of another class. The reasons based on which we might currently say “it’s okay” may not hold in the future, and then it would be very, very easy to overlook the lack of ownership being taken over the injected client.
Yes, if Refit did
new HttpClient(), I totally agree it would need explicit disposal, but Refit usesIHttpClientFactoryand as such is not required to dispose the client. Please correct me if I'm wrong, but I haven't seen any documentation that claims otherwise.
But Refit does new HttpClient:() https://github.com/reactiveui/refit/blob/f8bf4bb72a19800f0968dce608d8c62935f02b6c/Refit/RestService.cs#L172
@Timovzl Sure, I get your point - it wouldn't hurt. But still, it's not a bug per se in the current implementation.
@mikhail-barg That is something else. The Refit client registered as a transient service is fed an HttpClient from IHttpClientFactory and thus does not create a new instance itself. It's a different overload.
@Timovzl -- If your client interface extends IDisposable, then InterfaceStubGenerator & Parser will detected the Dispose method interface (Parser.cs lines 290, 343 & 364) and Emitter will generate the Dispose() method to dispose the generated class's Client property (Emitter.cs lines 136 & 239).
The fix to close this bug: Docs should be updated to say that RestService users SHOULD have their client interfaces extend IDisposable.
@derekm While updating the docs, it wouldn't hurt with a big fat warning in the "Using HttpClientFactory" section about injecting Refit clients (or any typed HttpClient for that matter) into singleton services, as that's an anti-pattern to be aware of.
[...] it wouldn't hurt with a big fat warning in the "Using HttpClientFactory" section about injecting Refit clients (or any typed HttpClient for that matter) into singleton services, as that's an anti-pattern to be aware of.
The fix I've suggested here would resolve that.
Basically, unless Refit chooses to provide and recommend an IRefitClientFactory, injecting an IRefitClient into a singleton service shouldn't be an antipattern. (I've worked around the issue by feeding Refit my own long-lived HttpClient, around a SocketsHttpHandler with a relatively short PooledConnectionLifetime. This is one of the proper options from the recommended practices.)