Fix kafka producer usage

[fkuehnSAG]

This pull request is fixing the usage of the producer for the kafka health check. Without this "using" the producer will create threads that never get deleted anymore every time the health check is executed. I suppose this is due to the fact that according to a gcdump that i created there were 2 producer<String, String> instances (which are only used by the health check).

It should be possible to reproduce this issue by creating a project with the health check, observing the number of threads for this process (e. g.: in the task manager) and calling the health check multiple times.

[sungam3r]

@fkuehnSAG Thanks for spotting this. It's a known problem with some health checks. #1440 fixes this problem in other way. HC is singleton now + it implements IDisposable. It's generally better to go this way than create needed resources each time health check is executed.

[fkuehnSAG]

@sungam3r okay, you are right. This really sounds like a better solution. Thank you! :)

[sungam3r]

To be honest after looking throught the code one more time I don't think it contains memory leak. HC was registered as instance, not as factory:

 return builder.Add(new HealthCheckRegistration(
                name ?? NAME,
                new KafkaHealthCheck(config, topic),  <----- NOT A FACTORY
                failureStatus,
                tags,
                timeout));

so each iteration the same instance should be used.

[sungam3r]

Without this "using" the producer will create threads that never get deleted anymore every time the health check is executed. I suppose this is due to the fact that according to a gcdump that i created there were 2 producer<String, String> instances (which are only used by the health check).

Effectively we have singleton-like health check even before fix. I suggest you investigate dumps one more time more carefully. However, now KafkaHealthCheck implements IDisposable and KafkaHealthCheckBuilderExtensions extension methods are implemented in the same way as other methods that work with health checks with disposable reources so all should be in line.

[sungam3r]

https://www.nuget.org/packages/AspNetCore.HealthChecks.Kafka/6.0.3

[fkuehnSAG]

@sungam3r i was also confused when i saw it the first time but the threads appear when calling the health check and after i swapped to my version of the fix it didn't happen anymore. Here is a Screenshot that i got from the gcdump:

and here a comparison of the spawned threads:

There is no other place in the whole application where we are using a Producer<String, String>. So it is 100 % related to the health check.

But thank you for publishing the fix! i will test it soon 👍

[sungam3r]

It can be so if you (or someone else) plug in this check in your app manually, i.e. without use of KafkaHealthCheckBuilderExtensions:

builder.Add(new HealthCheckRegistration("kafka", _ => new KafkaHealthCheck(..., ...));

I used HealthCheckRegistration ctor with factory method. In that case app should indeed suffer from memory leak.

[fkuehnSAG]

@sungam3r we built a HealthChecksBuilderExtensions class like this:

  public static IHealthChecksBuilder AddKafka(
        this IHealthChecksBuilder builder,
        string name = null,
        HealthStatus? failureStatus = null,
        IEnumerable<string> tags = null,
        TimeSpan? timeout = null)
    {
        return builder.Add(new HealthCheckRegistration(name ?? "kafka", BuildHealthCheck, failureStatus, tags, timeout));
    }

    private static KafkaHealthCheck BuildHealthCheck(IServiceProvider serviceProvider)
    {
        var options = serviceProvider.GetRequiredService<IOptions<KafkaOptions>>();
        var config = BuildProducerConfig(options);
        return new KafkaHealthCheck(config, null);
    }

So i think this should not be the issue

[sungam3r]

It really should since factory method from HealthCheckRegistration is executed each time HC request comes to your app.

[sungam3r]

Check out DefaultHealthCheckService MS sources.

[fkuehnSAG]

@sungam3r Oh yeah, i misunderstood before. You are absolutely right, this is the actual cause of the issue. I just tested it with KafkaHealthCheckBuilderExtensions and it works perfectly fine. Not sure why we implemented a own HealthCheckBuilderExtensions class with this error to be honest :/

But anyway, thank you really much and sorry for the effort ^^