[RabbitMQ] Reusing healthy connection #352
Conversation
The `ConnectionFactory` doesn't handle a null SslOption field for amqp connections. It must be a created instance with Enabled=false The constructor for a new `ConnectionFactory` will create a new SslOption already so only overwriting that default if something was passed in to the constructor.
Removing Lazy that was only created to return an already created instance
|
Not sure how the tests were passing before the change either. It has always tried to create a |
|
ConnectionFactory used to be Lazy so it was never getting to the attempt to parse the Uri. Updating tests. |
|
@unaizorrilla This PR already addressed the null sslOption handling. Resolving conflicts with 6adb624 |
|
Adding unit tests on the actual health check since there are now many entry points that need to be verified as working. @unaizorrilla Any guidance on a Mocking framework to use (Moq?)? I don't see any other tests that mock anything out right now. |
|
@unaizorrilla - Is there someone else I should ping if you're not able to get to reviewing this? |
|
Hi @rwkarg I try to review this a after xmas holidays! :-) |
|
👍 enjoy the rest of your holiday! |
| @@ -28,7 +28,7 @@ public static IHealthChecksBuilder AddRabbitMQ(this IHealthChecksBuilder builder | |||
| { | |||
| return builder.Add(new HealthCheckRegistration( | |||
| name ?? NAME, | |||
| sp => new RabbitMQHealthCheck(rabbitMQConnectionString, sslOption), | |||
| new RabbitMQHealthCheck(rabbitMQConnectionString, sslOption), | |||
There was a problem hiding this comment.
When passing in a Func to HealthCheckRegistration, that Func gets called every time that a health check is evaluated.
This resulted in a new RabbitMQHealthCheck instance being created for every health check evaluation. This further lead to a new ConnectionFactory and Connection being created for each health check evaluation which eliminated reuse (a RabbitMQHealthCheck will reuse an already provided/created ConnectionFactory/Connection but if a new RabbitMQHealthCheck is created each time then that reuse can't happen since it's a fresh instance each time).
There was a problem hiding this comment.
Hi @rwkarg
True, I try to merge asap!
test/UnitTests/DependencyInjection/RabbitMQ/RabbitMQUnitTests.cs
Outdated
Show resolved
Hide resolved
test/UnitTests/DependencyInjection/RabbitMQ/RabbitMQUnitTests.cs
Outdated
Show resolved
Hide resolved
Functional Tests already cover these use cases
| @@ -28,7 +28,7 @@ public static IHealthChecksBuilder AddRabbitMQ(this IHealthChecksBuilder builder | |||
| { | |||
| return builder.Add(new HealthCheckRegistration( | |||
| name ?? NAME, | |||
| sp => new RabbitMQHealthCheck(rabbitMQConnectionString, sslOption), | |||
| new RabbitMQHealthCheck(rabbitMQConnectionString, sslOption), | |||
There was a problem hiding this comment.
When passing in a Func to HealthCheckRegistration, that Func gets called every time that a health check is evaluated.
This resulted in a new RabbitMQHealthCheck instance being created for every health check evaluation. This further lead to a new ConnectionFactory and Connection being created for each health check evaluation which eliminated reuse (a RabbitMQHealthCheck will reuse an already provided/created ConnectionFactory/Connection but if a new RabbitMQHealthCheck is created each time then that reuse can't happen since it's a fresh instance each time).
This ensures that a connection that remains open will be reused within a RabbitMQHealthCheck instance