Dynamic Connection String Not Refreshed in Subsequent Health Checks

[MGithubber]

What happened: The AddNpgSql extension method for health checks caches the first NpgsqlDataSource created by the dbDataSourceFactory, which prevents the factory from being invoked on subsequent health checks. This is problematic for scenarios where the database connection string changes over time.

What you expected to happen: I expected the dbDataSourceFactory to be called on every health check invocation to ensure that the most current connection string is used.

How to reproduce it: 1) Configure the health checks using the AddNpgSql method with a connectionStringFactory that provides a dynamic connection string. 2) Call the health check endpoint once. 3) Change the underlying connection string that the connectionStringFactory provides. 4) Run the health check again and notice that the change in connection string is not reflected in the health check.

Source code sample:

using HealthChecks.UI.Client;

using Microsoft.AspNetCore.Diagnostics.HealthChecks;
using Microsoft.Extensions.Options;

var builder = WebApplication.CreateBuilder(args);

builder.Services
    .AddOptions<ConnectionStringOptions>()
    .BindConfiguration(ConnectionStringOptions.SectionName);

builder.Services.AddHealthChecks() // The lambda retrieves the current DefaultDb connection string from the configuration.
    .AddNpgSql(services => services.GetRequiredService<IOptionsSnapshot<ConnectionStringOptions>>().Value.DefaultDb);

var app = builder.Build();

app.UseHttpsRedirection();

app.MapHealthChecks("_health", new HealthCheckOptions
{
    ResponseWriter = UIResponseWriter.WriteHealthCheckUIResponse
});

app.Run();

public class ConnectionStringOptions
{
    public const string SectionName = "ConnectionStrings";

    public string DefaultDb { get; set; } = default!;
}

Environment:

• .NET Core version: .NET 7
• Healthchecks version: 7.1.0
• Operative system: Windows 11 Pro

[sungam3r]

@adamsitnik could you please give a comment? I know you've refactored npgsql HC package.

[adamsitnik]

I've introduced this bug in 7.1.0 and I am afraid that it's still a thing with 8.0.0.

The options instance is cached by capturing it in a closure;

https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/blob/73abc7a094f9ef625b83455898a21d22d2fa6a82/src/HealthChecks.NpgSql/DependencyInjection/NpgSqlHealthCheckBuilderExtensions.cs#L74-L79

the connection string is assigned once:

https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/blob/73abc7a094f9ef625b83455898a21d22d2fa6a82/src/HealthChecks.NpgSql/DependencyInjection/NpgSqlHealthCheckBuilderExtensions.cs#L85

and then a new NpgsqlConnection is created every time the health check is used (from the cached connection string):

https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/blob/73abc7a094f9ef625b83455898a21d22d2fa6a82/src/HealthChecks.NpgSql/NpgSqlHealthCheck.cs#L26-L28

@MGithubber would you be interested in sending a PR with a fix? The fix should be simple:

- // This instance is captured in lambda closure, so it can be reused (perf)
- NpgSqlHealthCheckOptions options = new()
- {
-     CommandText = healthQuery,
-     Configure = configure,
- };

return builder.Add(new HealthCheckRegistration(
    name ?? NAME,
    sp =>
    {
        - options.ConnectionString ??= Guard.ThrowIfNull(connectionStringFactory.Invoke(sp), throwOnEmptyString: true, paramName: nameof(connectionStringFactory));
        + // This instance is NOT captured in lambda closure, as reusing it can leads into bugs (https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/2087)
        + NpgSqlHealthCheckOptions options = new(connectionStringFactory.Invoke(sp))
        + {
        +     CommandText = healthQuery,
        +     Configure = configure,
        + };

        return new NpgSqlHealthCheck(options);
    },

Writing the test will be more demanding, but since you know how the bug affects end users it should be easy for you.

[MGithubber]

Hi, I'm pretty busy but I'll send the PR this week. Most probably during the weekend.

On Mon, Feb 5, 2024 at 2:04 PM Adam Sitnik @.***> wrote:

I've introduced this bug in 7.1.0 and I am afraid that it's still a thing with 8.0.0.

The options instance is cached by capturing it in a closure;

https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/blob/73abc7a094f9ef625b83455898a21d22d2fa6a82/src/HealthChecks.NpgSql/DependencyInjection/NpgSqlHealthCheckBuilderExtensions.cs#L74-L79

the connection string is assigned once:

https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/blob/73abc7a094f9ef625b83455898a21d22d2fa6a82/src/HealthChecks.NpgSql/DependencyInjection/NpgSqlHealthCheckBuilderExtensions.cs#L85

and then a new NpgsqlConnection is created every time the health check is used (from the cached connection string):

https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/blob/73abc7a094f9ef625b83455898a21d22d2fa6a82/src/HealthChecks.NpgSql/NpgSqlHealthCheck.cs#L26-L28

@MGithubber https://github.com/MGithubber would you be interested in sending a PR with a fix? The fix should be simple:

• // This instance is captured in lambda closure, so it can be reused (perf)- NpgSqlHealthCheckOptions options = new()- {- CommandText = healthQuery,- Configure = configure,- };

return builder.Add(new HealthCheckRegistration( name ?? NAME, sp => {

• options.ConnectionString ??= Guard.ThrowIfNull(connectionStringFactory.Invoke(sp), throwOnEmptyString: true, paramName: nameof(connectionStringFactory));
• // This instance is NOT captured in lambda closure, as reusing it can leads into bugs (https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/2087)
• NpgSqlHealthCheckOptions options = new(connectionStringFactory.Invoke(sp))
• {
• CommandText = healthQuery,
• Configure = configure,

• };

  return new NpgSqlHealthCheck(options);

  },

Writing the test will be more demanding, but since you know how the bug affects end users it should be easy for you.

— Reply to this email directly, view it on GitHub https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/2087#issuecomment-1926839919, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYJTMUGF6MJGSIH2JNPCGS3YSDDGNAVCNFSM6AAAAAA647U7SCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRWHAZTSOJRHE . You are receiving this because you were mentioned.Message ID: @.*** .com>