The health checks shouldn't create new connections to resources but utilise the existing ones

[evilpilaf]

What would you like to be added:

The health checks for dependencies which you're supposed to share an open connection though the application lifetime (e.g. Oracle, SQL, CosmosDB, Redis, etc.) should not create a new connection but take in an instance of the existing connections for the app.

Why is this needed:

Currently health checks like the ones for Redis, CosmosDB, Azure Storage and others take as a parameter a connection string , this will mean a new connection's gonna be created just for health tests which can lead to port exhaustion and lower performance, it also means that you aren't testing your systems connection status but that of a separate connection.

[mt89vein]

i am agree with you, we are written own implementation for workaround this problem with rabbitmq

[unaizorrilla]

i am agree with you, we are written own implementation for workaround this problem with rabbitmq

Hi @mt89vein

RabbitMQ healthcheck is not creating new connections and are reused!

[unaizorrilla]

What would you like to be added:

The health checks for dependencies which you're supposed to share an open connection though the application lifetime (e.g. Oracle, SQL, CosmosDB, Redis, etc.) should not create a new connection but take in an instance of the existing connections for the app.

Why is this needed:

Currently health checks like the ones for Redis, CosmosDB, Azure Storage and others take as a parameter a connection string , this will mean a new connection's gonna be created just for health tests which can lead to port exhaustion and lower performance, it also means that you aren't testing your systems connection status but that of a separate connection.

Hi @evilpilaf

Well, this depend on the healthcheck, on Rabbit we reuse connection because the Rabbit MQ connection are threadsafe and the singleton lifetime is OK, but this aproach is not valid for all kind if healtchecks, for example some as you mention, like SQL and Oracle

[mvonck]

I'm have the same issue when i make use of IConnectionFactory as dependency injection. I have register my services + healtcheck like this:

 public void ConfigureServices(IServiceCollection services)
 {
            services.AddSingleton<IConnectionFactory>((c) =>
            {
                InfrastructureSettings options = c.GetRequiredService<IOptions<InfSettings>>().Value;
                return new ConnectionFactory() { Uri = options.RabbitMQUri };
            });
            
             var builder = services.AddHealthChecks().AddRabbitMQ();
}

When i debug at the extension RabbitMQHealthCheckBuilderExtensions and put a break point at var connection = sp.GetService<IConnection>(); i see that a new instance of HealthCheckRegistration is created every time i call the health check page. That's why a connection is recreated because the HealthCheckRegistration class is not reused by .NET HealthCheck

public static class RabbitMQHealthCheckBuilderExtensions
....//
 return builder.Add(new HealthCheckRegistration(
                name ?? NAME,
                sp =>
                {
                    var connection = sp.GetService<IConnection>();
                    var connectionFactory = sp.GetService<IConnectionFactory>();

                    if (connection != null)
                    {
                        return new RabbitMQHealthCheck(connection);
                    }
                    else if (connectionFactory != null)
                    {
                        return new RabbitMQHealthCheck(connectionFactory);
                    }
                    else
                    {
                        throw new ArgumentException($"Either an IConnection or IConnectionFactory must be registered with the service provider");
                    }
                },
                failureStatus,
                tags,
                timeout));

...//

I will think about a solution now, and place it here

[unaizorrilla]

Hi @ mvonck

I don't understant the issue, in your case IConnection is null and IConnectionFactory is singleton, this create a new RabbitMQHealthCheck using the IConnectionFactoy (internally this create a new connection)

If you use this pattern

services
        .AddSingleton<IConnection>(sp=>
        {
            var factory = new ConnectionFactory()
            {
                Uri = new Uri("amqps://user:pass@host/vhost"),
                AutomaticRecoveryEnabled = true
            };

            return  factory.CreateConnection();
        })
        .AddHealthChecks()
        .AddRabbitMQ();

Connection is always the same and is reused!

You can register also the IConnectionFactory to use on AddSingleton instead of craete a new one off course

[mvonck]

Hi @unaizorrilla unaizorrilla
I can explain it with a functional test, if you add this test to functional test RabbitMHealthCheckTests. (+ install moq as package, because i could't find your moq framework)

 [SkipOnAppVeyor]
        public async Task be_create_one_connection_if_calling_health_multiple_times()
        {
            var factoryMock = new Mock<IConnectionFactory>();
            var connectionMock = new Mock<IConnection>();
            factoryMock.Setup(m => m.CreateConnection()).Returns(connectionMock.Object);

            var webHostBuilder = new WebHostBuilder()
            .UseStartup<DefaultStartup>()
            .ConfigureServices(services =>
            {
                services
                    .AddSingleton<IConnectionFactory>(factoryMock.Object)
                    .AddHealthChecks()
                    .AddRabbitMQ(tags: new string[] { "rabbitmq" });
            })
            .Configure(app =>
            {
                app.UseHealthChecks("/health", new HealthCheckOptions()
                {
                    Predicate = r => r.Tags.Contains("rabbitmq")
                });
            });

            var server = new TestServer(webHostBuilder);

            await server.CreateRequest($"/health").GetAsync();
            await server.CreateRequest($"/health").GetAsync();
            await server.CreateRequest($"/health").GetAsync();

            factoryMock.Verify(m => m.CreateConnection(), Times.Exactly(1), "expected one connection to be created");
        }

A possible fix is to register RabbitMQHealthCheck as singleton so the logic in that class of reusing the connection works. So
replace the function in RabbitMQHealthCheckBuilderExtensions.cs

  public static IHealthChecksBuilder AddRabbitMQ(this IHealthChecksBuilder builder, string name = default,
            HealthStatus? failureStatus = default, IEnumerable<string> tags = default, TimeSpan? timeout = default)
        {
            return builder.Add(new HealthCheckRegistration(
                name ?? NAME,
                sp =>
                {
                    var connection = sp.GetService<IConnection>();
                    var connectionFactory = sp.GetService<IConnectionFactory>();

                    if (connection != null)
                    {
                        return new RabbitMQHealthCheck(connection);
                    }
                    else if (connectionFactory != null)
                    {
                        return new RabbitMQHealthCheck(connectionFactory);
                    }
                    else
                    {
                        throw new ArgumentException($"Either an IConnection or IConnectionFactory must be registered with the service provider");
                    }
                },
                failureStatus,
                tags,
                timeout));
        }

with

 public static IHealthChecksBuilder AddRabbitMQ(this IHealthChecksBuilder builder, string name = default,
            HealthStatus? failureStatus = default, IEnumerable<string> tags = default, TimeSpan? timeout = default)
        {
            // Register the health check as singleton, so we can reuse the connection.
            builder.Services.AddSingleton<RabbitMQHealthCheck>(sp => {
                var connection = sp.GetService<IConnection>();
                var connectionFactory = sp.GetService<IConnectionFactory>();

                if (connection != null)
                {
                    return new RabbitMQHealthCheck(connection);
                }
                else if (connectionFactory != null)
                {
                    return new RabbitMQHealthCheck(connectionFactory);
                }
                else
                {
                    throw new ArgumentException($"Either an IConnection or IConnectionFactory must be registered with the service provider");
                }
            });

            // The healthcheck is registered as transient, so get the singleton one via dependency container
            return builder.Add(new HealthCheckRegistration(
                name ?? NAME,
                sp => sp.GetRequiredService<RabbitMQHealthCheck>(),
                failureStatus,
                tags,
                timeout));
        }

[unaizorrilla]

Hi

As I explained on my latest comment, if you inject IConnection (not only IConnectionFactory ), connection is reused

services
        .AddSingleton<IConnection>(sp=>
        {
            var factory = new ConnectionFactory()
            {
                Uri = new Uri("amqps://user:pass@host/vhost"),
                AutomaticRecoveryEnabled = true
            };

            return  factory.CreateConnection();
        })
        .AddHealthChecks()
        .AddRabbitMQ();

[mvonck]

This extension also supports working IConnectionFactory, so if this is not supported the else if (connectionFactory != null) in RabbitMQHealthCheckBuilderExtensions should be removed:

                var connection = sp.GetService<IConnection>();
                var connectionFactory = sp.GetService<IConnectionFactory>();

                if (connection != null)
                {
                    return new RabbitMQHealthCheck(connection);
                }
                else if (connectionFactory != null)
                {
                    return new RabbitMQHealthCheck(connectionFactory);
                }
                else
                {
                    throw new ArgumentException($"Either an IConnection or IConnectionFactory must be registered with the service provider");
                }

however i prefer to use IConnectionFactory instead of injection IConnection because if i reboot my application and rabbitMQ is not ready yet, my application will not crash. See this test:

 [SkipOnAppVeyor]
        public async Task be_not_crash_on_startup_when_rabbitmq_is_down_at_startup()
        {
            var factoryMock = new Mock<IConnectionFactory>();
            var connectionMock = new Mock<IConnection>();
            
            // Given rabbitMQ is not ready yet at the first attempt of calling /health
            factoryMock.SetupSequence(m => m.CreateConnection())
                .Throws(new Exception("RabbitMQ is not ready yet"))
                .Returns(connectionMock.Object);

            var webHostBuilder = new WebHostBuilder()
            .UseStartup<DefaultStartup>()
            .ConfigureServices(services =>
            {
                services
                    .AddSingleton<IConnectionFactory>(factoryMock.Object)
                    //.AddSingleton<IConnection>(ci => factoryMock.Object.CreateConnection()) // uncomment this and the test will fail
                    .AddHealthChecks()
                    .AddRabbitMQ(tags: new string[] { "rabbitmq" });
            })
            .Configure(app =>
            {
                app.UseHealthChecks("/health", new HealthCheckOptions()
                {
                    Predicate = r => r.Tags.Contains("rabbitmq")
                });
            });

            var server = new TestServer(webHostBuilder);

            var response1 = await server.CreateRequest($"/health").GetAsync();
            response1.StatusCode
               .Should().Be(HttpStatusCode.ServiceUnavailable);

            var response2 = await server.CreateRequest($"/health").GetAsync();
            response2.StatusCode
               .Should().Be(HttpStatusCode.OK);
        }

[unaizorrilla]

Well, true!!

But is a breaking change! Let me work on this and mark as obsolete some methods on this extensions!!

[mvonck]

Ok thx for your fast reply. I will keep using IConnectionFactory until it's removed and share my workaround here.

Current problem
Using this library via IConnectionFactory leads to new connections everytime the healthcheck url is called.

Why register via IConnectionFactory?
Because registering via IConnection leads to startup exception when rabbitMQ is not ready yet. Registering via IConnectionFactory solves this problem.

Timeline when registering via IConnection
-------- Start website> ----CRASH in DI container->----------------
---------------------------------------------------------Start RabbitMQ

Timeline when registering via IConnectionFactory
-------- Start website ---->HealthCheck 503_---->DoOtherStuff-------->HealthCheck 200----
-------------------------------------------------------Start RabbitMQ------------------------------

Workaround
Register IConnectionFactory + Add workaround method to register HealthCheck

public void ConfigureServices(IServiceCollection services)
{
    services.AddSingleton<IConnectionFactory>((c) =>
        new ConnectionFactory()
        {
            Uri = new Uri("amqps://user:pass@host/vhost"),
            AutomaticRecoveryEnabled = true
        });

    var healthCheckBuilder = services.AddHealthChecks();
    AddRabbitMQWorkaround(healthCheckBuilder);
}

/// <summary>
/// Because the rabbitMQ library creates duplicate connections, what leads to memory leak, register the healthcheck via own extension.
/// See https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/552
/// </summary>
private IHealthChecksBuilder AddRabbitMQWorkaround(IHealthChecksBuilder builder, string name = default,
    HealthStatus? failureStatus = default, IEnumerable<string> tags = default, TimeSpan? timeout = default)
{
    // Register the health check as singleton, so we can reuse the connection.
    builder.Services.AddSingleton<RabbitMQHealthCheck>(sp => {
        IConnection connection = sp.GetService<IConnection>();
        IConnectionFactory connectionFactory = sp.GetService<IConnectionFactory>();

        if (connection != null)
        {
            return new RabbitMQHealthCheck(connection);
        }
        else if (connectionFactory != null)
        {
            return new RabbitMQHealthCheck(connectionFactory);
        }
        else
        {
            throw new ArgumentException($"Either an IConnection or IConnectionFactory must be registered with the service provider");
        }
    });

    // The healthcheck is registered as transient, so get the singleton one via dependency container
    return builder.Add(new HealthCheckRegistration(
        name ?? "rabbitmq",
        sp => sp.GetRequiredService<RabbitMQHealthCheck>(),
        failureStatus,
        tags,
        timeout));
}

Functional Tests
See previous comments

[mvonck]

Is it ok if i make a Pull Request this week so we can use IConnectionFactory again @unaizorrilla? I will make sure it will only use one connection for the health check to prevent memory leaks. Because i see that more people have made a issue
#578 #563 #480

[unaizorrilla]

Hi @mvonck

Yeap I accept the PR if you have time or I can try to do it

[mvonck]

ok PR is ready

[unaizorrilla]

Closed with #587