Get rid of IHealthChecksBuilder.AddTypeActivatedCheck<T> in Microsoft.Extensions.DependencyInjection.HealthChecksBuilderAddCheckExtensions

[RyanLiu99]

Background and motivation

AddTypeActivatedCheck support additional arguments to concert type constructor which implements IHealthCheck.
This sounds good idea, but it brought few issues.

First, I feel it is not really needed.

More important, it might cause memory leak.

It resolves (some) HealthCheck constructor arguments from IoC container, and not resolve HealthCheck itself from Ioc container ( to support additional constructor arguments). This bring few issues:

The runtime create particular HealthCheck instance each time Health end point is hit. Since itself is not managed by Ioc container, lifetime does not apply to it. Ioc container cannot dispose it even it is a IDisposable. Heath check Runtime just create it but does not dispose it either. Application cannot dispose it since it was not created by application itself.

So it was created each time when health check is performed and never disposed. That can cause resource/memory leak.
I call it "Memory leak introduced by Ioc container leak" -- HealthCheck instances are not managed by Ioc container, but their dependencies are. Those dependencies are referred by HealthCheck but their lifetime can out of sync, this can cause another type issue.

API Proposal

//Get rid of AddTypeActivatedCheck method.

### API Usage

Get rid of extension method  AddTypeActivatedCheck<T> of IHealthChecksBuilder.


### Alternative Designs

_No response_

### Risks

Existing code depends on it need rewritten.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[RyanLiu99]

I have no permission to add label, I cannot find heath check area, seems area-Extensions-DependencyInjection is the closest.

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

AddTypeActivatedCheck support additional arguments to concert type constructor which implements IHealthCheck.
This sounds good idea, but it brought few issues.

First, I feel it is not really needed.

More important, it might cause memory leak.

It resolves (some) HealthCheck constructor arguments from IoC container, and not resolve HealthCheck itself from Ioc container ( to support additional constructor arguments). This bring few issues:

The runtime create particular HealthCheck instance each time Health end point is hit. Since itself is not managed by Ioc container, lifetime does not apply to it. Ioc container cannot dispose it even it is a IDisposable. Heath check Runtime just create it but does not dispose it either. Application cannot dispose it since it was not created by application itself.

So it was created each time when health check is performed and never disposed. That can cause resource/memory leak.
I call it memory leak introduced by Ioc container leak -- HealthCheck instances are not managed by Ioc container, but their dependencies are. Those dependencies are referred by HealthCheck but their lifetime can out of sync, this can cause another type issue.

API Proposal

namespace System.Collections.Generic
{
    public class MyFancyCollection<T> : IEnumerable<T>
    {
        public void Fancy(T item);
    }
}

API Usage

Get rid of extension method AddTypeActivatedCheck of IHealthChecksBuilder.

Alternative Designs

No response

Risks

Existing code depends on it need rewritten.

Author:	RyanLiu99
Assignees:	-
Labels:	api-suggestion, untriaged, area-Extensions-DependencyInjection
Milestone:	-

[davidfowl]

You're saying we should delete the API and break people that were using it? How about we add a remarks to the API or potentially add an analyzer that puts out a message about this API.

This API is still useful if your health check itself isn't disposable but has disposable dependencies...

[RyanLiu99]

David, you are right, adding a remark or mark as [Obsolete] is better idea, and/or maybe a warning it will be removed in few version down the road. I was just trying to make my point. Thanks .

And I think "Health check itself isn't disposed but dependencies are disposed" is a dangerous state to be.

[RyanLiu99]

While we are keeping the API, the improvement can be done is for health check runtime to detect if it is a IDisposable/IAsyncDisposable, and dispose it at the end of health check if so. Health check runtime creates instance, it is its responsibility to clean it up.

But still some health checks might be too expensive to be created and disposed each time. So this approach is just a comprise, still better is to warn people stay away from AddTypeActivatedCheck( ) .

[BrennanConroy]

Triage: We think it would be a possible workaround to see if we can try to resolve T from DI first before falling back to ActivatorUtilities so the lifetime will be managed by the container if the user puts their health check in DI.

Alternative: Try to as-cast to IDisposable when we are done with the health check.