diff --git a/src/HealthChecks.NpgSql/DependencyInjection/NpgSqlHealthCheckBuilderExtensions.cs b/src/HealthChecks.NpgSql/DependencyInjection/NpgSqlHealthCheckBuilderExtensions.cs index fb0dd34653..b2faa9c55d 100644 --- a/src/HealthChecks.NpgSql/DependencyInjection/NpgSqlHealthCheckBuilderExtensions.cs +++ b/src/HealthChecks.NpgSql/DependencyInjection/NpgSqlHealthCheckBuilderExtensions.cs @@ -37,7 +37,13 @@ public static IHealthChecksBuilder AddNpgSql( IEnumerable? tags = default, TimeSpan? timeout = default) { - return builder.AddNpgSql(_ => connectionString, healthQuery, configure, name, failureStatus, tags, timeout); + Guard.ThrowIfNull(connectionString, throwOnEmptyString: true); + + return builder.AddNpgSql(new NpgSqlHealthCheckOptions(connectionString) + { + CommandText = healthQuery, + Configure = configure + }, name, failureStatus, tags, timeout); } /// @@ -65,14 +71,36 @@ public static IHealthChecksBuilder AddNpgSql( IEnumerable? tags = default, TimeSpan? timeout = default) { - return builder.AddNpgSql(sp => new NpgsqlDataSourceBuilder(connectionStringFactory(sp)).Build(), healthQuery, configure, name, failureStatus, tags, timeout); + // 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)); + + ResolveDataSourceIfPossible(options, sp); + + return new NpgSqlHealthCheck(options); + }, + failureStatus, + tags, + timeout)); } /// /// Add a health check for Postgres databases. /// /// The . - /// A factory to build the NpgsqlDataSource to use. + /// + /// An optional factory to obtain instance. + /// When not provided, is simply resolved from . + /// /// The query to be used in check. /// An optional action to allow additional Npgsql specific configuration. /// The health check name. Optional. If null the type name 'npgsql' will be used for the name. @@ -85,7 +113,7 @@ public static IHealthChecksBuilder AddNpgSql( /// The specified . public static IHealthChecksBuilder AddNpgSql( this IHealthChecksBuilder builder, - Func dbDataSourceFactory, + Func? dbDataSourceFactory = null, string healthQuery = HEALTH_QUERY, Action? configure = null, string? name = default, @@ -93,9 +121,7 @@ public static IHealthChecksBuilder AddNpgSql( IEnumerable? tags = default, TimeSpan? timeout = default) { - Guard.ThrowIfNull(dbDataSourceFactory); - - NpgsqlDataSource? dataSource = null; + // This instance is captured in lambda closure, so it can be reused (perf) NpgSqlHealthCheckOptions options = new() { CommandText = healthQuery, @@ -106,47 +132,15 @@ public static IHealthChecksBuilder AddNpgSql( name ?? NAME, sp => { - // The Data Source needs to be created only once, - // as each instance has it's own connection pool. - // See https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/1993 for more details. - - // Perform an atomic read of the current value. - NpgsqlDataSource? existingDataSource = Volatile.Read(ref dataSource); - if (existingDataSource is null) + if (options.DataSource is null) { - // Create a new Data Source - NpgsqlDataSource fromFactory = dbDataSourceFactory(sp); - // Try to resolve the Data Source from DI. - NpgsqlDataSource? fromDI = sp.GetService(); - - if (fromDI is not null && fromDI.ConnectionString.Equals(fromFactory.ConnectionString)) + // this lock will most likely be obtained once, with no contention so the cost should be low and acceptable + lock (options) { - // If they are using the same ConnectionString, we can reuse the instance from DI. - // So there is only ONE NpgsqlDataSource per the whole app and ONE connection pool. - - if (!ReferenceEquals(fromDI, fromFactory)) - { - // Dispose it, as long as it's not the same instance. - fromFactory.Dispose(); - } - Interlocked.Exchange(ref dataSource, fromDI); - options.DataSource = fromDI; - } - else - { - // Perform an atomic exchange, but only if the value is still null. - existingDataSource = Interlocked.CompareExchange(ref dataSource, fromFactory, null); - if (existingDataSource is not null) - { - // Some other thread has created the data source in the meantime, - // we dispose our own copy, and use the existing instance. - fromFactory.Dispose(); - options.DataSource = existingDataSource; - } - else - { - options.DataSource = fromFactory; - } + // The Data Source needs to be created only once, + // as each instance has it's own connection pool. + // See https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/1993 for more details. + options.DataSource ??= dbDataSourceFactory?.Invoke(sp) ?? sp.GetRequiredService(); } } @@ -161,7 +155,7 @@ public static IHealthChecksBuilder AddNpgSql( /// Add a health check for Postgres databases. /// /// The . - /// Options for health check. + /// Options for health check. It's mandatory to provide . /// The health check name. Optional. If null the type name 'npgsql' will be used for the name. /// /// The that should be reported when the health check fails. Optional. If null then @@ -179,12 +173,32 @@ public static IHealthChecksBuilder AddNpgSql( TimeSpan? timeout = default) { Guard.ThrowIfNull(options); + Guard.ThrowIfNull(options.ConnectionString, throwOnEmptyString: true, paramName: "ConnectionString"); return builder.Add(new HealthCheckRegistration( name ?? NAME, - _ => new NpgSqlHealthCheck(options), + sp => + { + ResolveDataSourceIfPossible(options, sp); + + return new NpgSqlHealthCheck(options); + }, failureStatus, tags, timeout)); } + + private static void ResolveDataSourceIfPossible(NpgSqlHealthCheckOptions options, IServiceProvider sp) + { + if (options.DataSource is null && !options.TriedToResolveFromDI) + { + NpgsqlDataSource? fromDi = sp.GetService(); + if (fromDi?.ConnectionString == options.ConnectionString) + { + // When it's possible, we reuse the DataSource registered in the DI + options.DataSource = fromDi; + } + options.TriedToResolveFromDI = true; // save the answer, so we don't do it more than once + } + } } diff --git a/src/HealthChecks.NpgSql/NpgSqlHealthCheck.cs b/src/HealthChecks.NpgSql/NpgSqlHealthCheck.cs index 37a72c681e..379ab32357 100644 --- a/src/HealthChecks.NpgSql/NpgSqlHealthCheck.cs +++ b/src/HealthChecks.NpgSql/NpgSqlHealthCheck.cs @@ -1,4 +1,6 @@ +using System.Diagnostics; using Microsoft.Extensions.Diagnostics.HealthChecks; +using Npgsql; namespace HealthChecks.NpgSql; @@ -11,7 +13,7 @@ public class NpgSqlHealthCheck : IHealthCheck public NpgSqlHealthCheck(NpgSqlHealthCheckOptions options) { - Guard.ThrowIfNull(options.DataSource); + Debug.Assert(options.ConnectionString is not null || options.DataSource is not null); Guard.ThrowIfNull(options.CommandText, true); _options = options; } @@ -21,7 +23,9 @@ public async Task CheckHealthAsync(HealthCheckContext context { try { - await using var connection = _options.DataSource!.CreateConnection(); + await using var connection = _options.DataSource is not null + ? _options.DataSource.CreateConnection() + : new NpgsqlConnection(_options.ConnectionString); _options.Configure?.Invoke(connection); await connection.OpenAsync(cancellationToken).ConfigureAwait(false); @@ -33,7 +37,6 @@ public async Task CheckHealthAsync(HealthCheckContext context return _options.HealthCheckResultBuilder == null ? HealthCheckResult.Healthy() : _options.HealthCheckResultBuilder(result); - } catch (Exception ex) { diff --git a/src/HealthChecks.NpgSql/NpgSqlHealthCheckOptions.cs b/src/HealthChecks.NpgSql/NpgSqlHealthCheckOptions.cs index 0138c30ea8..39f0724e7a 100644 --- a/src/HealthChecks.NpgSql/NpgSqlHealthCheckOptions.cs +++ b/src/HealthChecks.NpgSql/NpgSqlHealthCheckOptions.cs @@ -9,20 +9,34 @@ namespace HealthChecks.NpgSql; /// public class NpgSqlHealthCheckOptions { + internal NpgSqlHealthCheckOptions() + { + // this ctor is internal on purpose: + // those who want to use DataSource need to use the extension methods + // that take care of creating the right thing + } + /// - /// The Postgres connection string to be used. - /// Use property for advanced configuration. + /// Creates an instance of . /// - public string? ConnectionString + /// The PostgreSQL connection string to be used. + public NpgSqlHealthCheckOptions(string connectionString) { - get => DataSource?.ConnectionString; - set => DataSource = value is not null ? NpgsqlDataSource.Create(value) : null; + ConnectionString = Guard.ThrowIfNull(connectionString, throwOnEmptyString: true); } + /// + /// The Postgres connection string to be used. + /// Use property for advanced configuration. + /// + public string? ConnectionString { get; set; } + /// /// The Postgres data source to be used. /// - public NpgsqlDataSource? DataSource { get; set; } + internal NpgsqlDataSource? DataSource { get; set; } + + internal bool TriedToResolveFromDI { get; set; } /// /// The query to be executed. diff --git a/test/HealthChecks.Npgsql.Tests/DependencyInjection/RegistrationTests.cs b/test/HealthChecks.Npgsql.Tests/DependencyInjection/RegistrationTests.cs index 1d3252e4cd..9161f2f2f4 100644 --- a/test/HealthChecks.Npgsql.Tests/DependencyInjection/RegistrationTests.cs +++ b/test/HealthChecks.Npgsql.Tests/DependencyInjection/RegistrationTests.cs @@ -96,7 +96,7 @@ public void factory_is_called_only_once() [InlineData(false)] public void factory_reuses_pre_registered_datasource_when_possible(bool sameConnectionString) { - const string connectionString = "Server=localhost"; + const string connectionString = "Host=localhost"; ServiceCollection services = new(); services.AddSingleton(serviceProvider => @@ -122,10 +122,13 @@ public void factory_reuses_pre_registered_datasource_when_possible(bool sameConn for (int i = 0; i < 10; i++) { var healthCheck = (NpgSqlHealthCheck)registration.Factory(serviceProvider); - var fieldInfo = typeof(NpgSqlHealthCheck).GetField("_options", BindingFlags.Instance | BindingFlags.NonPublic); - var npgSqlHealthCheckOptions = (NpgSqlHealthCheckOptions)fieldInfo!.GetValue(healthCheck)!; + var optionsField = typeof(NpgSqlHealthCheck).GetField("_options", BindingFlags.Instance | BindingFlags.NonPublic); + var npgSqlHealthCheckOptions = (NpgSqlHealthCheckOptions)optionsField!.GetValue(healthCheck)!; - Assert.Equal(sameConnectionString, ReferenceEquals(serviceProvider.GetRequiredService(), npgSqlHealthCheckOptions.DataSource)); + var dataSourceProperty = typeof(NpgSqlHealthCheckOptions).GetProperty("DataSource", BindingFlags.Instance | BindingFlags.NonPublic); + var dataSource = (NpgsqlDataSource)dataSourceProperty!.GetValue(npgSqlHealthCheckOptions)!; + + Assert.Equal(sameConnectionString, ReferenceEquals(serviceProvider.GetRequiredService(), dataSource)); } factoryCalls.ShouldBe(1); diff --git a/test/HealthChecks.Npgsql.Tests/HealthChecks.NpgSql.approved.txt b/test/HealthChecks.Npgsql.Tests/HealthChecks.NpgSql.approved.txt index d84990c4dd..0b4427c67b 100644 --- a/test/HealthChecks.Npgsql.Tests/HealthChecks.NpgSql.approved.txt +++ b/test/HealthChecks.Npgsql.Tests/HealthChecks.NpgSql.approved.txt @@ -7,11 +7,10 @@ namespace HealthChecks.NpgSql } public class NpgSqlHealthCheckOptions { - public NpgSqlHealthCheckOptions() { } + public NpgSqlHealthCheckOptions(string connectionString) { } public string CommandText { get; set; } public System.Action? Configure { get; set; } public string? ConnectionString { get; set; } - public Npgsql.NpgsqlDataSource? DataSource { get; set; } public System.Func? HealthCheckResultBuilder { get; set; } } } @@ -20,8 +19,8 @@ namespace Microsoft.Extensions.DependencyInjection public static class NpgSqlHealthCheckBuilderExtensions { public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddNpgSql(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, HealthChecks.NpgSql.NpgSqlHealthCheckOptions options, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable? tags = null, System.TimeSpan? timeout = default) { } - public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddNpgSql(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, System.Func dbDataSourceFactory, string healthQuery = "SELECT 1;", System.Action? configure = null, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable? tags = null, System.TimeSpan? timeout = default) { } public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddNpgSql(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, System.Func connectionStringFactory, string healthQuery = "SELECT 1;", System.Action? configure = null, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable? tags = null, System.TimeSpan? timeout = default) { } + public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddNpgSql(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, System.Func? dbDataSourceFactory = null, string healthQuery = "SELECT 1;", System.Action? configure = null, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable? tags = null, System.TimeSpan? timeout = default) { } public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddNpgSql(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, string connectionString, string healthQuery = "SELECT 1;", System.Action? configure = null, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable? tags = null, System.TimeSpan? timeout = default) { } } } \ No newline at end of file