Fixed Connection Leak for RabbitMQ

[HesamKashefi]

What this PR does / why we need it: The connection leak in RabbitMQ Health Checks made this library unusable

Which issue(s) this PR fixes: The connection leak in RabbitMQ

Please reference the issue this PR will close: #2364

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

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
• [ ] Extended the documentation
• [ ] Provided sample for the feature

[codecov-commenter]

:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 58.21%. Comparing base (72d9abf) to head (a193a6e). Report is 35 commits behind head on master.

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

:exclamation: There is a different number of reports uploaded between BASE (72d9abf) and HEAD (a193a6e). Click for more details.

HEAD has 13 uploads less than BASE

Flag	BASE (72d9abf)	HEAD (a193a6e)
AzureFileStorage	5	0
AzureQueueStorage	4	0
Dapr	1	0
AzureEventHubs	1	0
AzureTableStorage	1	0
AzureBlobStorage	1	0

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2393      +/-   ##
==========================================
- Coverage   66.88%   58.21%   -8.68%     
==========================================
  Files         268       10     -258     
  Lines        8730      280    -8450     
  Branches      631       25     -606     
==========================================
- Hits         5839      163    -5676     
+ Misses       2723      112    -2611     
+ Partials      168        5     -163     

Flag	Coverage Δ
ApplicationStatus	?
ArangoDb	?
Aws.S3	?
Aws.SecretsManager	?
Aws.Sns	?
Aws.Sqs	?
Aws.SystemsManager	?
Azure.IoTHub	?
AzureApplicationInsights	?
AzureBlobStorage	?
AzureDigitalTwin	?
AzureEventHubs	?
AzureFileStorage	?
AzureKeyVault	?
AzureQueueStorage	?
AzureSearch	?
AzureServiceBus	?
AzureTableStorage	?
Consul	?
CosmosDb	?
Dapr	?
DocumentDb	?
DynamoDb	?
Elasticsearch	?
EventStore	?
EventStore.gRPC	?
Gcp.CloudFirestore	?
Gremlin	?
Hangfire	?
IbmMQ	?
InfluxDB	?
Kafka	?
Kubernetes	?
Milvus	?
MongoDb	?
MySql	?
Nats	?
Npgsql	?
OpenIdConnectServer	?
Oracle	?
Prometheus.Metrics	?
Publisher.ApplicationInsights	?
Publisher.CloudWatch	?
Publisher.Datadog	?
Publisher.Prometheus	?
Publisher.Seq	?
Qdrant	?
RabbitMQ	58.21% <100.00%> (+7.57%)	:arrow_up:
RavenDb	?
Redis	?
SendGrid	?
SignalR	?
SqlServer	?
Sqlite	?
System	?
UI	?
Uris	?

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[ThumbGen]

Isn't this fix disposing the singleton connection (if configured as per Readme recommendations)? Is the configuration suggested in Readme not addressing the issue? I am not near my dev machine to try it, but seems OK, and makes this PR not necessary.

[HesamKashefi]

@dotnet-policy-service agree

[HesamKashefi]

@HesamKashefi, can you ensure that the connection is not disposed of in the second check and the next one? Please add a test about that.

This class uses a factory function to get an instance of the connection. If you create an instance inside the factory function everything will be OK, but if you provide a factory function that returns a Singleton Instance, then yes it will be disposed and that would be a problem! But the main problem is that this code seems unusable!

If you take the first approach and create an instance on every call, the instances won't be disposed and the connection leak explodes the server and if you take the second approach and want to return a singleton instance, the connection MUST be created when RabbitMQ is already running and connection will be lost when RabbitMQ goes down! this is unusable!

The third option is to provide a connection (which It's not possible if RabbitMQ is not running)

Am I doing it wrong? I just want to make this work when the RabbitMQ is possibly not available!

[HesamKashefi]

I am not near my dev machine to try it, but seems OK, and makes this PR not necessar

Please check the situation when RabbitMQ is not running. Please read this issue #2393 .

p.s.

Registering a Singleton like this looks awful! Calling .GetAwaiter().GetResult(); is not a good way to code and creating a singleton connection to a server that might not exists is worse!

            builder.Services.AddSingleton(s =>
            {
                var cf = new RabbitMQ.Client.ConnectionFactory
                {
                    HostName = builder.Configuration.GetValue<string>("RabbitMq:Host") ?? "localhost",
                    UserName = builder.Configuration.GetValue<string>("RabbitMq:UserName") ?? "",
                    Password = builder.Configuration.GetValue<string>("RabbitMq:Password") ?? ""
                };
                return cf.CreateConnectionAsync().GetAwaiter().GetResult();
            });

[ThumbGen]

Just to summarize:

• using an automatically created and maintained IConnection is not wanted anymore (this is what v8.x was doing)
• creating a new connection for every health check is not following RabbitMq recommendations https://www.rabbitmq.com/docs/connections#high-connection-churn
• expecting the client application to manage the connection feels weird, as I expect a health checks library to "do what is supposed to do" without me interfering with its internals, especially forcing me to use .Result

🤔

[HesamKashefi]

Just to summarize:

* using an automatically created and maintained IConnection is not wanted anymore (this is what v8.x was doing)

* creating a new connection for every health check is not following RabbitMq recommendations https://www.rabbitmq.com/docs/connections#high-connection-churn

* expecting the client application to manage the connection feels weird, as I expect a health checks library to "do what is supposed to do" without me interfering with its internals, especially forcing me to use .Result

🤔

Yes you are right about the fact that creating a new connection is not a good way. I was just trying to don't touch any code and just fix the problem that I think it's a bug. Maybe I've a misunderstanding of something here! But I just couldn't use this code! as I mentioned the problem above! So instead I'm using this: (just a temporary workaround)

            builder.Services.AddHealthChecks()
            .AddCheck("self", () => HealthCheckResult.Healthy())
            .AddRedis("redis")
            .AddNpgSql(builder.Configuration.GetConnectionString("Default")!)
            .AddAsyncCheck("RabbitMQ", async () =>
            {
                try
                {
                    var cf = new RabbitMQ.Client.ConnectionFactory
                    {
                        HostName = builder.Configuration.GetValue<string>("RabbitMq:Host") ?? "localhost",
                        UserName = builder.Configuration.GetValue<string>("RabbitMq:UserName") ?? "",
                        Password = builder.Configuration.GetValue<string>("RabbitMq:Password") ?? ""
                    };
                    await using var connection = await cf.CreateConnectionAsync();
                    if (connection.IsOpen)
                        return HealthCheckResult.Healthy();
                }
                catch (Exception e)
                {
                    LogManager.GetCurrentClassLogger().Error(e);
                }
                return HealthCheckResult.Unhealthy();
            });

I think AddRabbitMQ must either get a fix or get completely removed!