Merge pull request #255 from Plasma/fix-issue-251

Fix transient Collection was Modified exception (Issue #251)

Author: frankhommers

src/Hangfire.PostgreSql/IConnectionFactory.cs

@@ -21,15 +21,16 @@
 
 using Npgsql;
 
-namespace Hangfire.PostgreSql;
-
-/// <summary>
-/// Connection factory for runtime create connection 
-/// </summary>
-public interface IConnectionFactory
+namespace Hangfire.PostgreSql
 {
   /// <summary>
-  /// Get or create NpgsqlConnection
+  /// Connection factory for runtime create connection 
   /// </summary>
-  NpgsqlConnection GetOrCreateConnection();
-}
+  public interface IConnectionFactory
+  {
+    /// <summary>
+    /// Get or create NpgsqlConnection
+    /// </summary>
+    NpgsqlConnection GetOrCreateConnection();
+  }
+}

src/Hangfire.PostgreSql/PostgreSqlStorage.cs

@@ -87,7 +87,7 @@ public PostgreSqlStorage(
         throw new ArgumentException($"Connection string [{connectionString}] is not valid", nameof(connectionString));
       }
 
-      _connectionStringBuilder = builder;
+      _connectionStringBuilder = SetupConnectionStringBuilderParameters(builder);
       _connectionSetup = connectionSetup;
 
       if (options.PrepareSchemaIfNecessary)
@@ -238,15 +238,6 @@ internal NpgsqlConnection CreateAndOpenConnection()
         connection = _existingConnection;
         if (connection == null)
         {
-          // The connection string must not be modified when transaction enlistment is enabled, otherwise it will cause
-          // prepared transactions and probably fail when other statements (outside of hangfire) ran within the same
-          // transaction. Also see #248.
-          if (!Options.EnableTransactionScopeEnlistment)
-          {
-            _connectionStringBuilder.Enlist = false;
-            SetTimezoneToUtcForNpgsqlCompatibility(_connectionStringBuilder);
-          }
-
           connection = new NpgsqlConnection(_connectionStringBuilder.ToString());
           _connectionSetup?.Invoke(connection);
         }
@@ -441,5 +432,19 @@ private bool TryCreateConnectionStringBuilder(string connectionString, out Npgsq
         return false;
       }
     }
+
+    private NpgsqlConnectionStringBuilder SetupConnectionStringBuilderParameters(NpgsqlConnectionStringBuilder builder)
+    {
+      // The connection string must not be modified when transaction enlistment is enabled, otherwise it will cause
+      // prepared transactions and probably fail when other statements (outside of hangfire) ran within the same
+      // transaction. Also see #248.
+      if (!Options.EnableTransactionScopeEnlistment)
+      {
+        builder.Enlist = false;
+        SetTimezoneToUtcForNpgsqlCompatibility(builder);
+      }
+
+      return builder;
+    }
   }
 }

tests/Hangfire.PostgreSql.Tests/PostgreSqlStorageFacts.cs

@@ -18,13 +18,27 @@ public PostgreSqlStorageFacts()
       _options = new PostgreSqlStorageOptions { PrepareSchemaIfNecessary = false, EnableTransactionScopeEnlistment = true };
     }
 
+    /// <summary>
+    /// Ideally we could enforce UTC time-zone for all connections to Hangfire Postgres (to ensure proper DateTime handling) however if the connection is being enlisted
+    /// then we cannot touch the ConnectionString in any way because otherwise this creates a new prepared transaction, which becomes aborted
+    ///
+    /// The best we can do at the moment is enforce UTC on connections initialized (and not enlisted) by Hangfire Postgres
+    /// </summary>
     [Fact]
-    public void Connection_Timezone_Is_Set_To_UTC_For_Npgsql6_Compatibility()
+    public void Connection_Timezone_Is_Set_To_UTC_For_Npgsql6_Compatibility_When_Enlistment_Not_Set()
     {
-      PostgreSqlStorage storage = CreateStorage();
+      // Arrange
+
+      // Turn off transaction enlistment for this test
+      // We won't be performing any queries so there is no side-effect for having this disabled
+      _options.EnableTransactionScopeEnlistment = false;
 
+      PostgreSqlStorage storage = CreateStorage();
+      
+      // Act
       using (var connection = storage.CreateAndOpenConnection())
       {
+        // Assert
         Assert.Equal("UTC", connection.Timezone);
       }
     }

tests/Hangfire.PostgreSql.Tests/Utils/DefaultConnectionFactory.cs

@@ -1,14 +1,15 @@
 using Npgsql;
 
-namespace Hangfire.PostgreSql.Tests.Utils;
-
-public class DefaultConnectionFactory: IConnectionFactory
+namespace Hangfire.PostgreSql.Tests.Utils
 {
-  /// <summary>
-  /// Get or create NpgsqlConnection
-  /// </summary>
-  public NpgsqlConnection GetOrCreateConnection()
+  public class DefaultConnectionFactory : IConnectionFactory
   {
-    return ConnectionUtils.CreateConnection();
+    /// <summary>
+    /// Get or create NpgsqlConnection
+    /// </summary>
+    public NpgsqlConnection GetOrCreateConnection()
+    {
+      return ConnectionUtils.CreateConnection();
+    }
   }
-}
+}