Improve exception/error handling

RAIC.Extensions.Configuration.EntityFrameworkCore.PostgreSQL/PostgreSQLNotificationConfigurationReloader.cs

@@ -52,23 +52,30 @@ internal class PostgreSQLNotificationConfigurationReloader : Microsoft.Extension
     {
         var dbConnection = Initialise();
 
-        do
+        try
         {
             await ListenForNotifications(stoppingToken);
 
+            do
+            {
                 try
                 {
                     await dbConnection.WaitAsync(stoppingToken);
                 }
-            catch (Exception e) when (e is not TaskCanceledException)
+                catch (Exception e) when (e is not OperationCanceledException)
                 {
                     _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);
         }
+        catch (OperationCanceledException e)
+        {
+            _logger?.LogInformation(e, "Exiting due to signal from stopping token");
+        }
+    }
 
     private Npgsql.NpgsqlConnection Initialise()
     {
@@ -76,17 +83,8 @@ internal class PostgreSQLNotificationConfigurationReloader : Microsoft.Extension
         dbConnection.Notification += OnNotification;
 
         return dbConnection;
-    }
 
-    private async Task ListenForNotifications(CancellationToken stoppingToken)
+        void OnNotification(object sender, Npgsql.NpgsqlNotificationEventArgs args)
-    {
-        var listenStatement = $"LISTEN {_options.ChannelName}";
-        await _db.OpenConnectionAsync(stoppingToken);
-        await _db.ExecuteSqlRawAsync(listenStatement, stoppingToken);
-        _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)
@@ -111,7 +109,17 @@ internal class PostgreSQLNotificationConfigurationReloader : Microsoft.Extension
                     return; // Don't proceed to debounced reload
             }
 
-        var _ = DebouncedProviderReload(); // Intentionally running without awaiting, may be cancelled
+            _ = DebouncedProviderReload(); // Intentionally running without awaiting, may be cancelled
+        }
+    }
+
+
+    private async Task ListenForNotifications(CancellationToken stoppingToken)
+    {
+        var listenStatement = $"LISTEN {_options.ChannelName}";
+        await _db.OpenConnectionAsync(stoppingToken);
+        await _db.ExecuteSqlRawAsync(listenStatement, stoppingToken);
+        _logger?.LogInformation("Listening for notifications on channel '{channel}'", _options.ChannelName);
     }
 
 
@@ -119,7 +127,7 @@ internal class PostgreSQLNotificationConfigurationReloader : Microsoft.Extension
     /// Trigger reload of the <see cref="IEntityFrameworkCoreDbSetConfigurationProvider"/> after waiting for a [cancelable] debounce period
     /// </summary>
     /// <remarks>Does not log <see cref="TaskCanceledException "/> each time debounce task is cancelled, unlike <see cref="SimpleDebouncedProviderReload"/></remarks>
-    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<bool> IsReconnectionPossible(CancellationToken stoppingToken)
+    private async Task<bool> TryReconnect(CancellationToken stoppingToken)
     {
         await Task.Delay(_options.InitialReconnectionDelay, stoppingToken);
 

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. 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 reloading all of them
+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
+* Small window of opportunity for updates to be missed during reconnection process after any network dropouts or other connectivity flakiness
-* Consider adding `Keepalive` to your conenction string (https://www.npgsql.org/doc/keepalive.html) if its not already present
+* 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
 

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. 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 reloading all of them
+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
 

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<int> TransientErrors { get; set; } = [2, 53, 121, 10060, 11001];
 }
 
 
@@ -39,37 +42,52 @@ internal class SqlServerNotificationConfigurationReloader<TDbContext, TSetting>
 
 
     protected override async Task ExecuteAsync(CancellationToken stoppingToken)
+    {
+
+        try
         {
             var settingsQuery = await Initialise(stoppingToken);
 
             do
             {
-            do
-            {
-                var (taskCompletionSource, cancelTokenRegistration) = await ListenForNotifications(settingsQuery, stoppingToken);
-                using (cancelTokenRegistration)
-                {
-                    var lastVersion = await GetChangeTrackingVersion(stoppingToken);
-
                 try
                 {
-                        await taskCompletionSource.Task;
+                    do
+                    {
+                        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)
+                    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<SqlError>().Select(e => e.Number)))
                 {
-                        _logger?.LogWarning(e, "Exception while listening for notifications on query '{query}'", settingsQuery);
+                    _logger?.LogWarning(e, "Transient exception during notification setup process or the processing of notifications");
-                        break;
+                }
+                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 (await TryReconnect(stoppingToken));
-            while (true); // each notification is one-and-done, so must keep re-regesitering indefinitely
-        }
-        while (await ReconnectOnDependencyException(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);
         }
+        catch (OperationCanceledException e)
+        {
+            _logger?.LogInformation(e, "Exiting due to signal from stopping token");
+        }
+    }
 
 
     private async Task<string> Initialise(CancellationToken stoppingToken)
@@ -88,7 +106,7 @@ internal class SqlServerNotificationConfigurationReloader<TDbContext, TSetting>
 
         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<TDbContext, TSetting>
     }
 
 
-    private async Task<(TaskCompletionSource, CancellationTokenRegistration)> ListenForNotifications(string settingsQuery, CancellationToken stoppingToken)
+    private async Task<TaskCompletionSource> 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<TDbContext, TSetting>
         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<TDbContext, TSetting>
                     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<TDbContext, TSetting>
     private async Task<long> GetChangeTrackingVersion(CancellationToken stoppingToken)
     {
         var query = "SELECT CHANGE_TRACKING_CURRENT_VERSION() as value";
-        return await _dbContext.Database.SqlQueryRaw<long>(query).FirstOrDefaultAsync(stoppingToken);
+        return await _dbContext.Database.SqlQueryRaw<long>(query).SingleAsync(stoppingToken);
     }
 
 
@@ -198,19 +218,15 @@ internal class SqlServerNotificationConfigurationReloader<TDbContext, TSetting>
     }
 
 
-    private async Task<bool> ReconnectOnDependencyException(CancellationToken stoppingToken)
+    private async Task<bool> 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
+        // Unlike the postgres version of this there's no retry logic here - SqlClient does its own retries internally so it is not needed (?)
-        await _dbContext.Database.CloseConnectionAsync();
+        if (await _dbContext.Database.CanConnectAsync(stoppingToken))
-        try
         {
             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<TDbContext, TSetting>
         base.Dispose();
         _onChangeHandler?.Dispose();
     }
+
+    private class SqlNotificationException : Exception
+    {
+        public SqlNotificationException(string? message) : base(message)
+        {
+        }
+
+        public SqlNotificationException(string? message, Exception? innerException) : base(message, innerException)
+        {
+        }
+    }
 }