Fix connections leak in RabbitMQHealthCheck

[Vladimirezh]

What this PR does / why we need it:
Every call of healthcheck create new connection. Connection doesn't close.

Which issue(s) this PR fixes:

Please reference the issue this PR will close: #[issue number]

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
No

Please make sure you've completed the relevant tasks for this PR, out of the following list:

• Code compiles correctly
• Created/updated tests
• Unit tests passing
• End-to-end tests passing

[unaizorrilla]

Hi @Vladimirezh

Let me some days to review.

[codecov-commenter]

Codecov Report

Merging #2048 (6c7b693) into master (7060bdd) will decrease coverage by 3.49%.
Report is 7 commits behind head on master.
The diff coverage is 87.50%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master    #2048      +/-   ##
==========================================
- Coverage   69.06%   65.57%   -3.49%     
==========================================
  Files         179      174       -5     
  Lines        4613     4099     -514     
  Branches      447      400      -47     
==========================================
- Hits         3186     2688     -498     
+ Misses       1323     1321       -2     
+ Partials      104       90      -14     

Flag	Coverage Δ
RabbitMQ	48.53% <87.50%> (+0.63%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...eyVault.Secrets/AzureKeyVaultSecretsHealthCheck.cs	63.15% <ø> (-3.51%)	⬇️
...tion/AzureKeyVaultHealthChecksBuilderExtensions.cs	0.00% <ø> (ø)
...tion/AzureServiceBusQueueMessagesCountThreshold.cs	100.00% <ø> (ø)
...yInjection/CosmosDbHealthCheckBuilderExtensions.cs	100.00% <ø> (+4.22%)	⬆️
...yInjection/RabbitMQHealthCheckBuilderExtensions.cs	94.02% <87.50%> (+1.60%)	⬆️

... and 8 files with indirect coverage changes

[unaizorrilla]

Hi @Vladimirezh

Thanks for submit this PR, just a few notes

• Just tested the current code on sample app HealthChecks.Sample with Rabbit and just one connection is created and the health check is reused always using the uri and options overloads
• You can register your connection and connection factory on the container and use the proper extension method to alternate registration method

[Vladimirezh]

To reproduce leak modify ConfigureServices of HealthChecks.Sample to:

  //Custom rabbitmq factory
        services.AddSingleton<Func<IConnection>>(() =>
        {
            var factory = new ConnectionFactory
            {
                Uri = new Uri("amqp://localhost:5672"),
                AutomaticRecoveryEnabled = true,
                UseBackgroundThreadsForIO = true
            };

            return factory.CreateConnection();
        });


        services
            .AddApplicationInsightsTelemetry()
            .AddHealthChecks()
            // .AddRabbitMQ(rabbitConnectionString: "amqp://localhost:5672", name: "rabbit1"); //<- Work fine
            .AddRabbitMQ((sp, options) =>
                options.Connection = sp.GetRequiredService<Func<IConnection>>()()); //<- Leak

After call route /healthz many times look to Rabbit Management UI

[unaizorrilla]

Hi @Vladimirezh

Just curious about your code.. some notes...

If you registered the IConnection as a Singleton and use the overload you mention like this

       
        var connectionFactory = new ConnectionFactory
        {
            Uri = new Uri("amqp://localhost:5672")
        };

        services.AddSingleton<IConnection>(connectionFactory.CreateConnection());

        services
            .AddHealthChecks()
            .AddRabbitMQ((sp, options) =>
                options.Connection = sp.GetRequiredService<IConnection>(), name: "rabbit1");

just one connection is used

Why you need to register a Func of IConnection?

[Vladimirezh]

I have custom connection factory. Factory doesn't implement IConnectionFactory. In example it's Func, in my project it's library abstraction. I use overload with setup and set connection to options.

[sungam3r]

@Vladimirezh Proposed changes will not work for multiple registrations. You register RabbitMQHealthCheck as singleton in method accepting user defined Action<RabbitMQHealthCheckOptions>? setup/Action<IServiceProvider, RabbitMQHealthCheckOptions>? setup, name, etc. Second and all other registrations will fetch the latest registered RabbitMQHealthCheck from ServiceProvider. This is a well-known design flaw and codebase tends to avoid such usages although you still can find such code in some health checks (alas).

If you look in the code in master then you'll see that RabbitMQHealthCheck caches connections in static dictionary by options instance as as key. I think that the real problem here - calling Action<IServiceProvider, RabbitMQHealthCheckOptions>? setup inside HealthCheckRegistration. Other overload with Action<RabbitMQHealthCheckOptions>? setup calls it outside - one time. Why so? Because ServiceProvider is available only inside HealthCheckRegistration delegate so user-defined delegate is called each time. I agree that options should be changed (initialized) only once. In our overload it's initialized deferred and not once. Please check #2153. It should fix that issue. If OK I would like you to make subsequent PR with all your tests.

ping @adamsitnik It seems to me that we have similar problems with some other checks. Did you fix this with Npgsql or was there something else? I still have not returned to the full support of the project, so I have not studied your changes.