From db64055acb1beb7df569be3a239e20abd76d811f Mon Sep 17 00:00:00 2001 From: Rhys Ickeringill Date: Wed, 7 Jan 2026 10:53:40 +1100 Subject: [PATCH] Improve exception/error handling --- ...greSQLNotificationConfigurationReloader.cs | 88 ++++++++++-------- .../README.md | 10 +- .../README.md | 11 ++- ...ServerNotificationConfigurationReloader.cs | 93 ++++++++++++------- 4 files changed, 120 insertions(+), 82 deletions(-) diff --git a/RAIC.Extensions.Configuration.EntityFrameworkCore.PostgreSQL/PostgreSQLNotificationConfigurationReloader.cs b/RAIC.Extensions.Configuration.EntityFrameworkCore.PostgreSQL/PostgreSQLNotificationConfigurationReloader.cs index 1f30924..8953435 100644 --- a/RAIC.Extensions.Configuration.EntityFrameworkCore.PostgreSQL/PostgreSQLNotificationConfigurationReloader.cs +++ b/RAIC.Extensions.Configuration.EntityFrameworkCore.PostgreSQL/PostgreSQLNotificationConfigurationReloader.cs @@ -52,22 +52,29 @@ internal class PostgreSQLNotificationConfigurationReloader : Microsoft.Extension { var dbConnection = Initialise(); - do + try { await ListenForNotifications(stoppingToken); - try + do { - await dbConnection.WaitAsync(stoppingToken); + try + { + await dbConnection.WaitAsync(stoppingToken); + } + catch (Exception e) when (e is not OperationCanceledException) + { + _logger?.LogWarning(e, "Exception while waiting for notifications on channel '{channel}'", _options.ChannelName); + } } - catch (Exception e) when (e is not TaskCanceledException) - { - _logger?.LogWarning(e, "Exception while waiting for notifications on channel '{channel}'", _options.ChannelName); - } - } - while (ConnectionState.Open == dbConnection.State || await IsReconnectionPossible(stoppingToken)); + while (ConnectionState.Open == dbConnection.State || await TryReconnect(stoppingToken)); - _logger?.LogWarning("Giving up listening for notifications on channel '{channel}' because reconnection attempts exhausted. Configuration updates from database will no longer occur", _options.ChannelName); + _logger?.LogWarning("Giving up listening for notifications on channel '{channel}' because reconnection attempts exhausted. Configuration updates from database will no longer occur", _options.ChannelName); + } + catch (OperationCanceledException e) + { + _logger?.LogInformation(e, "Exiting due to signal from stopping token"); + } } private Npgsql.NpgsqlConnection Initialise() @@ -76,8 +83,37 @@ internal class PostgreSQLNotificationConfigurationReloader : Microsoft.Extension dbConnection.Notification += OnNotification; return dbConnection; + + void OnNotification(object sender, Npgsql.NpgsqlNotificationEventArgs args) + { + _logger?.LogTrace("Received notification '{payload}' on channel '{channel}'", args.Payload, args.Channel); + if (args.Channel != _options.ChannelName) + return; + + using (var previousCts = Interlocked.Exchange(ref _cts, new())) + { + previousCts.Cancel(); + } + + ReadOnlySpan keyValue = args.Payload.Split('=', 2); // DEBT: Do split without heap allocation + switch (keyValue.Length) + { + case 2: + _configProvider.Set(keyValue[0], keyValue[1]); + break; + case 1: + _configProvider.Remove(keyValue[0]); + break; + default: + _logger?.LogWarning("Invalid '{channel}' payload '{payload}'", args.Channel, args.Payload); + return; // Don't proceed to debounced reload + } + + _ = DebouncedProviderReload(); // Intentionally running without awaiting, may be cancelled + } } + private async Task ListenForNotifications(CancellationToken stoppingToken) { var listenStatement = $"LISTEN {_options.ChannelName}"; @@ -86,40 +122,12 @@ internal class PostgreSQLNotificationConfigurationReloader : Microsoft.Extension _logger?.LogInformation("Listening for notifications on channel '{channel}'", _options.ChannelName); } - private void OnNotification(object sender, Npgsql.NpgsqlNotificationEventArgs args) - { - _logger?.LogTrace("Received notification '{payload}' on channel '{channel}'", args.Payload, args.Channel); - if (args.Channel != _options.ChannelName) - return; - - using (var previousCts = Interlocked.Exchange(ref _cts, new())) - { - previousCts.Cancel(); - } - - ReadOnlySpan keyValue = args.Payload.Split('=', 2); // DEBT: Do split without heap allocation - switch (keyValue.Length) - { - case 2: - _configProvider.Set(keyValue[0], keyValue[1]); - break; - case 1: - _configProvider.Remove(keyValue[0]); - break; - default: - _logger?.LogWarning("Invalid '{channel}' payload '{payload}'", args.Channel, args.Payload); - return; // Don't proceed to debounced reload - } - - var _ = DebouncedProviderReload(); // Intentionally running without awaiting, may be cancelled - } - /// /// Trigger reload of the after waiting for a [cancelable] debounce period /// /// Does not log each time debounce task is cancelled, unlike - private async ValueTask DebouncedProviderReload() + private async Task DebouncedProviderReload() { var delayTask = Task.Delay(_options.DebounceInterval); var cancelableTask = Task.Delay(Timeout.Infinite, _cts.Token); @@ -154,7 +162,7 @@ internal class PostgreSQLNotificationConfigurationReloader : Microsoft.Extension } - private async Task IsReconnectionPossible(CancellationToken stoppingToken) + private async Task TryReconnect(CancellationToken stoppingToken) { await Task.Delay(_options.InitialReconnectionDelay, stoppingToken); diff --git a/RAIC.Extensions.Configuration.EntityFrameworkCore.PostgreSQL/README.md b/RAIC.Extensions.Configuration.EntityFrameworkCore.PostgreSQL/README.md index 28db980..323dc7d 100644 --- a/RAIC.Extensions.Configuration.EntityFrameworkCore.PostgreSQL/README.md +++ b/RAIC.Extensions.Configuration.EntityFrameworkCore.PostgreSQL/README.md @@ -5,8 +5,8 @@ This library enhances `RAIC.Extensions.Configuration.EntityFrameworkCore` with s ## Goals 1. No polling! -1. Updates happen in background via worker service (`IHostedService`) -1. Only update settings which change rather than reloading all of them +1. Updates happen in background via a [hosted service](https://learn.microsoft.com/en-us/aspnet/core/fundamentals/host/hosted-services) implementation. +1. Only update settings which change rather than updating them all ## Requirements * .NET 8 @@ -14,8 +14,8 @@ This library enhances `RAIC.Extensions.Configuration.EntityFrameworkCore` with s ## Gotchas * Setting values cannot be `null` (as signified by the `RequiredAttribute` on `ISetting.Value`) * Setting keys must not contain the `=` character (similar to `CommandLineConfigurationProvider` & `EnvironmentVariablesConfigurationProvider`) -* Small window of opportunity for updates to be missed during reconnection process -* Consider adding `Keepalive` to your conenction string (https://www.npgsql.org/doc/keepalive.html) if its not already present +* Small window of opportunity for updates to be missed during reconnection process after any network dropouts or other connectivity flakiness +* Consider adding `Keepalive` [parameter](https://www.npgsql.org/doc/keepalive.html) to your connection string if its not already present ## Known Issues * Not tested under load @@ -65,7 +65,7 @@ AFTER DELETE ON settings FOR EACH ROW EXECUTE FUNCTION notify_setting_remove(); ``` -Reccommend adding your SQL to the migration which adds the `Settings` table/view (or a new migration if that table/view already exists). +Recommend adding your SQL to the migration which adds the `Settings` table/view (or a new migration if that table/view already exists). ## Usage Example diff --git a/RAIC.Extensions.Configuration.EntityFrameworkCore.SqlServer/README.md b/RAIC.Extensions.Configuration.EntityFrameworkCore.SqlServer/README.md index 32ee0a9..c8403c4 100644 --- a/RAIC.Extensions.Configuration.EntityFrameworkCore.SqlServer/README.md +++ b/RAIC.Extensions.Configuration.EntityFrameworkCore.SqlServer/README.md @@ -7,8 +7,8 @@ This library enhances `RAIC.Extensions.Configuration.EntityFrameworkCore` with s ## Goals 1. No polling! -1. Updates happen in background via worker service (`IHostedService`) -1. Only update settings which change rather than reloading all of them +1. Updates happen in background via a [hosted service](https://learn.microsoft.com/en-us/aspnet/core/fundamentals/host/hosted-services) implementation +1. Only update settings which change rather than updating them all ## Requirements @@ -17,13 +17,16 @@ This library enhances `RAIC.Extensions.Configuration.EntityFrameworkCore` with s ## Gotchas * Won't work with Azure SQL until Microsoft adds/enables Service Broker support * Setting values cannot be `null` (as signified by the `RequiredAttribute` on `ISetting.Value`) +* Consider adding `ConnectRetryCount` and `ConnectRetryInterval` [parameters](https://learn.microsoft.com/en-us/dotnet/framework/data/adonet/whats-new#sqlclient-data-provider) to your connection string if not already present ## Known Issues * Not tested under load +* Transient failure detection logic is not well tested given the challenges in reproducing these issues ## Configuration Options -There is a single property which can be configured (cf. the `SqlServerNotificationConfigurationReloaderOptions` POCO) +There are two properties which can be configured (cf. the `SqlServerNotificationConfigurationReloaderOptions` POCO) 1. `ConnectionString` - the full connection string for the SQL Server instance +1. `TransientErrors` - the list of `SqlError.Number` values that will be treated as transient if present in a thrown `SqlException`'s `Errors` collection while listening for or processing notifications (ie. reconnection will be attempted if any are present) ## Setup For `SqlServerNotificationConfigurationReloader` to work it requires Change Tracking to be enable on the `Settings` table (and therefore also on the database itself) @@ -37,7 +40,7 @@ ALTER TABLE dbo.Settings ENABLE CHANGE_TRACKING; ``` -Reccommend adding your SQL to the migration which adds the `Settings` table/view (or a new migration if that table/view already exists). +Recommend adding your SQL to the migration which adds the `Settings` table/view (or a new migration if that table/view already exists). ## Usage Example diff --git a/RAIC.Extensions.Configuration.EntityFrameworkCore.SqlServer/SqlServerNotificationConfigurationReloader.cs b/RAIC.Extensions.Configuration.EntityFrameworkCore.SqlServer/SqlServerNotificationConfigurationReloader.cs index 8063914..87a72e0 100644 --- a/RAIC.Extensions.Configuration.EntityFrameworkCore.SqlServer/SqlServerNotificationConfigurationReloader.cs +++ b/RAIC.Extensions.Configuration.EntityFrameworkCore.SqlServer/SqlServerNotificationConfigurationReloader.cs @@ -12,6 +12,9 @@ public class SqlServerNotificationConfigurationReloaderOptions // must be public { [Required] public required string ConnectionString { get; set; } + + [Required] + public required HashSet TransientErrors { get; set; } = [2, 53, 121, 10060, 11001]; } @@ -40,35 +43,50 @@ internal class SqlServerNotificationConfigurationReloader protected override async Task ExecuteAsync(CancellationToken stoppingToken) { - var settingsQuery = await Initialise(stoppingToken); - do + try { + var settingsQuery = await Initialise(stoppingToken); + do { - var (taskCompletionSource, cancelTokenRegistration) = await ListenForNotifications(settingsQuery, stoppingToken); - using (cancelTokenRegistration) + try { - var lastVersion = await GetChangeTrackingVersion(stoppingToken); - - try + do { - await taskCompletionSource.Task; + var notificationCompletion = await ListenForNotifications(settingsQuery, stoppingToken); + using var tcsCancelRegistration = stoppingToken.Register(() => notificationCompletion.TrySetCanceled()); + + var lastVersion = await GetChangeTrackingVersion(stoppingToken); + + await notificationCompletion.Task; UpdateConfiguration(lastVersion); } - catch (Exception e) when (e is not TaskCanceledException) - { - _logger?.LogWarning(e, "Exception while listening for notifications on query '{query}'", settingsQuery); - break; - } + while (true); // each notification is one-and-done, so must keep re-regesitering for notifications indefinitely + } + catch (SqlException e) when (e.InnerException is SqlException inner && _options.TransientErrors.Overlaps(inner.Errors.OfType().Select(e => e.Number))) + { + _logger?.LogWarning(e, "Transient exception during notification setup process or the processing of notifications"); + } + catch (SqlNotificationException e) // only ever thown while waiting for notification task to complete, must (?) be dropped connection of some sort + { + _logger?.LogWarning(e, "Transient exception while listening for notifications"); + } + catch (Exception e) when (e is not OperationCanceledException) + { + _logger?.LogError(e, "Exception while setting up, listening for or processing notifications"); + throw; } } - while (true); // each notification is one-and-done, so must keep re-regesitering indefinitely - } - while (await ReconnectOnDependencyException(stoppingToken)); + while (await TryReconnect(stoppingToken)); - _logger?.LogWarning("Giving up listening for notifications on query '{query}' because reconnection failed. Configuration updates from database will no longer occur", settingsQuery); - SqlDependency.Stop(_options.ConnectionString); + _logger?.LogWarning("Giving up listening for notifications on query '{query}' because reconnection failed. Configuration updates from database will no longer occur", settingsQuery); + SqlDependency.Stop(_options.ConnectionString); + } + catch (OperationCanceledException e) + { + _logger?.LogInformation(e, "Exiting due to signal from stopping token"); + } } @@ -88,7 +106,7 @@ internal class SqlServerNotificationConfigurationReloader var settingsQuery = _dbContext.Settings.ToQueryString(); - _logger?.LogInformation("Listening for notifications on query '{query}'", settingsQuery); + _logger?.LogInformation(@"Query upon which notifications will be listened for: ""{query}""", settingsQuery); await openTask; @@ -96,8 +114,10 @@ internal class SqlServerNotificationConfigurationReloader } - private async Task<(TaskCompletionSource, CancellationTokenRegistration)> ListenForNotifications(string settingsQuery, CancellationToken stoppingToken) + private async Task ListenForNotifications(string settingsQuery, CancellationToken stoppingToken) { + // Can't reuse the command built here, as once it gets associated with a SqlDependency it can't be used with another, + // and SqlDependencies can't be used more than once either... hence there's no point "preparing" the command either using var command = new SqlCommand() { Connection = (SqlConnection)_dbContext.Database.GetDbConnection(), @@ -108,22 +128,22 @@ internal class SqlServerNotificationConfigurationReloader var dependency = new SqlDependency(command); var tcs = new TaskCompletionSource(); dependency.OnChange += OnChange; - var tcsCancelRegistration = stoppingToken.Register(() => tcs.TrySetCanceled()); try { await command.ExecuteNonQueryAsync(stoppingToken); } - catch (Exception e) when (e is not TaskCanceledException) + catch (Exception e) when (e is not OperationCanceledException) { _logger?.LogError(e, "Exception while attempting to register query dependency"); throw; } - return (tcs, tcsCancelRegistration); + return tcs; void OnChange(object sender, SqlNotificationEventArgs args) { + dependency.OnChange -= OnChange; switch (args.Info) { case SqlNotificationInfo.Insert: @@ -135,7 +155,7 @@ internal class SqlServerNotificationConfigurationReloader return; case SqlNotificationInfo.Error: _logger?.LogWarning("SqlDependency '{info}' from {type}@{source}", args.Info, args.Type, args.Source); - tcs.TrySetException(new Exception($"SqlDependency {args.Info} from {args.Type}@{args.Source}")); + tcs.TrySetException(new SqlNotificationException($"SqlDependency {args.Info} from {args.Type}@{args.Source}")); return; default: _logger?.LogWarning("Ignoring '{info}' from {type}@{source} received from SqlDependency", args.Info, args.Type, args.Source); @@ -148,7 +168,7 @@ internal class SqlServerNotificationConfigurationReloader private async Task GetChangeTrackingVersion(CancellationToken stoppingToken) { var query = "SELECT CHANGE_TRACKING_CURRENT_VERSION() as value"; - return await _dbContext.Database.SqlQueryRaw(query).FirstOrDefaultAsync(stoppingToken); + return await _dbContext.Database.SqlQueryRaw(query).SingleAsync(stoppingToken); } @@ -198,19 +218,15 @@ internal class SqlServerNotificationConfigurationReloader } - private async Task ReconnectOnDependencyException(CancellationToken stoppingToken) + private async Task TryReconnect(CancellationToken stoppingToken) { - // explicitly close connection to force subsequent OpenConnectionAsync() to actually try to open the connection instead of it possibly being a no-op - await _dbContext.Database.CloseConnectionAsync(); - try + // Unlike the postgres version of this there's no retry logic here - SqlClient does its own retries internally so it is not needed (?) + if (await _dbContext.Database.CanConnectAsync(stoppingToken)) { await _dbContext.Database.OpenConnectionAsync(stoppingToken); return true; } - catch (Exception e) - { - _logger?.LogWarning(e, "Exception while attempting to reconnect to database. Configuration updates from database will no longer occur"); - } + return false; } @@ -263,4 +279,15 @@ internal class SqlServerNotificationConfigurationReloader base.Dispose(); _onChangeHandler?.Dispose(); } + + private class SqlNotificationException : Exception + { + public SqlNotificationException(string? message) : base(message) + { + } + + public SqlNotificationException(string? message, Exception? innerException) : base(message, innerException) + { + } + } } \ No newline at end of file