-
Notifications
You must be signed in to change notification settings - Fork 803
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove private static cache from RabbitMQHealthCheck #2343
Conversation
RabbitMQHealthCheck is maintaining it's private static cache of instances. It's wrong, as it can cause a memory leak (it won't ever be freed). Moreover, it's can create instances of IConnection. This is wrong, as it can lead into a situation when we have multiple instances of IConnection that are connected to the same server: one used by the app and another created and used by the health check. And we should have only one. We should do the same as in Xabaril#2040, Xabaril#2116 and Xabaril#2096 I left RabbitMQ.v6 as-is to limit the breaking change. Only when an existing user moves from RabbitMQ.Client v6 to v7 will they need to update their health check (along with other breaking changes in RabbitMQ.Client).
Copilot
AI
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 6 out of 14 changed files in this pull request and generated 1 suggestion.
Files not reviewed (8)
- src/HealthChecks.Rabbitmq.v6/HealthChecks.Rabbitmq.v6.csproj: Language not supported
- src/HealthChecks.Rabbitmq/HealthChecks.Rabbitmq.csproj: Language not supported
- test/HealthChecks.RabbitMQ.Tests/HealthChecks.Rabbitmq.approved.txt: Language not supported
- test/HealthChecks.RabbitMQ.v6.Tests/HealthChecks.RabbitMQ.v6.Tests.csproj: Language not supported
- test/HealthChecks.RabbitMQ.Tests/Functional/RabbitHealthCheckTests.cs: Evaluated as low risk
- test/HealthChecks.RabbitMQ.v6.Tests/DependencyInjection/RegistrationTests.cs: Evaluated as low risk
- test/HealthChecks.RabbitMQ.v6.Tests/Functional/RabbitHealthCheckTests.cs: Evaluated as low risk
- src/HealthChecks.Rabbitmq/RabbitMQHealthCheck.cs: Evaluated as low risk
Comments skipped due to low confidence (1)
test/HealthChecks.RabbitMQ.Tests/DependencyInjection/RegistrationTests.cs:16
- The test should verify that the health check fails gracefully if no IConnection or IConnectionFactory is registered. Add a test case to ensure proper error handling in this scenario.
.AddRabbitMQ();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks a lot for your help @eerhardt !
src/HealthChecks.Rabbitmq/DependencyInjection/RabbitMQHealthCheckBuilderExtensions.cs
Show resolved
Hide resolved
src/HealthChecks.Rabbitmq/DependencyInjection/RabbitMQHealthCheckBuilderExtensions.cs
Show resolved
Hide resolved
src/HealthChecks.Rabbitmq/DependencyInjection/RabbitMQHealthCheckBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #2343 +/- ##
==========================================
- Coverage 66.88% 66.47% -0.41%
==========================================
Files 268 254 -14
Lines 8730 8514 -216
Branches 631 606 -25
==========================================
- Hits 5839 5660 -179
+ Misses 2723 2693 -30
+ Partials 168 161 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
What this PR does / why we need it:
RabbitMQHealthCheck is maintaining it's private static cache of instances. It's wrong, as it can cause a memory leak (it won't ever be freed).
Moreover, it's can create instances of IConnection. This is wrong, as it can lead into a situation when we have multiple instances of IConnection that are connected to the same server: one used by the app and another created and used by the health check. And we should have only one.
We should do the same as in #2040, #2116 and #2096
I left RabbitMQ.v6 as-is to limit the breaking change. Only when an existing user moves from RabbitMQ.Client v6 to v7 will they need to update their health check (along with other breaking changes in RabbitMQ.Client).
Does this PR introduce a user-facing change?:
Yes, this is a breaking change to the RabbitMQ health check.
Please make sure you've completed the relevant tasks for this PR, out of the following list: