Fixes after CR

Tzachi009 · Tzachi009 · commit fe04d5e8a273 · 2025-01-27T09:30:51.000+02:00
diff --git a/src/DistributedLock.Postgres/PostgresAdvisoryLock.cs b/src/DistributedLock.Postgres/PostgresAdvisoryLock.cs
@@ -138,6 +138,9 @@ async ValueTask RollBackTransactionTimeoutVariablesIfNeededAsync(bool acquired)
         {
             if (needsSavePoint
                 // For transaction scoped locks, we can't roll back the save point on success because that will roll back our hold on the lock.
+                // It's ok to "leak" the savepoint because if it's an internally-owned transaction then the savepoint will be cleaned up with the disposal of the transaction.
+                // If it's an externally-owned transaction then we must "leak" it or we will lose the lock. Also, we can't avoid using a save point in this case
+                // because otherwise if an exception had occurred the extrenally-owned transaction will be aborted and become completely unusable.
                 && !(acquired && UseTransactionScopedLock(connection)))
             {
                 // attempt to clear the timeout variables we set
@@ -195,9 +198,9 @@ private DatabaseCommand CreateAcquireCommand(DatabaseConnection connection, Post
 
     private static async ValueTask<CapturedTimeoutSettings?> CaptureTimeoutSettingsIfNeededAsync(DatabaseConnection connection, CancellationToken cancellationToken)
     {
-        var shouldCaptureTimeoutSettings = UseTransactionScopedLock(connection);
+        var shouldCaptureTimeoutSettings = connection.IsExernallyOwned && UseTransactionScopedLock(connection);
 
-        // Return null in case we won't try to acquire a transaction-scoped lock.
+        // Return null in case we won't try to acquire an externally-owned transaction-scoped lock.
         if (!shouldCaptureTimeoutSettings) { return null; }
 
         var statementTimeout = await GetCurrentSetting("statement_timeout", connection, cancellationToken).ConfigureAwait(false);
@@ -247,7 +250,7 @@ private static async ValueTask<bool> ShouldDefineSavePoint(DatabaseConnection co
 
     private static async ValueTask RestoreTimeoutSettingsIfNeededAsync(CapturedTimeoutSettings? settings, DatabaseConnection connection)
     {
-        // Settings is expected to be null in case we didn't try to acquire a transaction-scoped lock.
+        // Settings is expected to be null in case we didn't try to acquire an externally-owned transaction-scoped lock.
         if (settings is null) { return; }
 
         using var restoreTimeoutSettingsCommand = connection.CreateCommand();
diff --git a/src/DistributedLock.Tests/Tests/Postgres/PostgresDistributedLockExtensionsTest.cs b/src/DistributedLock.Tests/Tests/Postgres/PostgresDistributedLockExtensionsTest.cs
@@ -109,6 +109,8 @@ public async Task TestTimeoutSettingsRestoredWithExternalTransaction()
 
     
     [Test]
+    // Each lock acquisition creates the same named savepoint; this seems like it would create a conflict
+    // but it actually works fine in Postgres (see https://www.postgresql.org/docs/current/sql-savepoint.html)
     public async Task TestWorksForMultipleLocksUnderTheSameConnectionWithExternalTransaction()
     {
         var key1 = new PostgresAdvisoryLockKey(1);

Original file line number	Diff line number	Diff line change
`@@ -138,6 +138,9 @@ async ValueTask RollBackTransactionTimeoutVariablesIfNeededAsync(bool acquired)`
`138`	`138`	`{`
`139`	`139`	`if (needsSavePoint`
`140`	`140`	`// For transaction scoped locks, we can't roll back the save point on success because that will roll back our hold on the lock.`
	`141`	`+ // It's ok to "leak" the savepoint because if it's an internally-owned transaction then the savepoint will be cleaned up with the disposal of the transaction.`
	`142`	`+ // If it's an externally-owned transaction then we must "leak" it or we will lose the lock. Also, we can't avoid using a save point in this case`
	`143`	`+ // because otherwise if an exception had occurred the extrenally-owned transaction will be aborted and become completely unusable.`
`141`	`144`	`&& !(acquired && UseTransactionScopedLock(connection)))`
`142`	`145`	`{`
`143`	`146`	`// attempt to clear the timeout variables we set`
`@@ -195,9 +198,9 @@ private DatabaseCommand CreateAcquireCommand(DatabaseConnection connection, Post`
`195`	`198`
`196`	`199`	`private static async ValueTask<CapturedTimeoutSettings?> CaptureTimeoutSettingsIfNeededAsync(DatabaseConnection connection, CancellationToken cancellationToken)`
`197`	`200`	`{`
`198`		`- var shouldCaptureTimeoutSettings = UseTransactionScopedLock(connection);`
	`201`	`+ var shouldCaptureTimeoutSettings = connection.IsExernallyOwned && UseTransactionScopedLock(connection);`
`199`	`202`
`200`		`- // Return null in case we won't try to acquire a transaction-scoped lock.`
	`203`	`+ // Return null in case we won't try to acquire an externally-owned transaction-scoped lock.`
`201`	`204`	`if (!shouldCaptureTimeoutSettings) { return null; }`
`202`	`205`
`203`	`206`	`var statementTimeout = await GetCurrentSetting("statement_timeout", connection, cancellationToken).ConfigureAwait(false);`
`@@ -247,7 +250,7 @@ private static async ValueTask<bool> ShouldDefineSavePoint(DatabaseConnection co`
`247`	`250`
`248`	`251`	`private static async ValueTask RestoreTimeoutSettingsIfNeededAsync(CapturedTimeoutSettings? settings, DatabaseConnection connection)`
`249`	`252`	`{`
`250`		`- // Settings is expected to be null in case we didn't try to acquire a transaction-scoped lock.`
	`253`	`+ // Settings is expected to be null in case we didn't try to acquire an externally-owned transaction-scoped lock.`
`251`	`254`	`if (settings is null) { return; }`
`252`	`255`
`253`	`256`	`using var restoreTimeoutSettingsCommand = connection.CreateCommand();`
Original file line number	Diff line number	Diff line change
`@@ -109,6 +109,8 @@ public async Task TestTimeoutSettingsRestoredWithExternalTransaction()`
`109`	`109`
`110`	`110`
`111`	`111`	`[Test]`
	`112`	`+ // Each lock acquisition creates the same named savepoint; this seems like it would create a conflict`
	`113`	`+ // but it actually works fine in Postgres (see https://www.postgresql.org/docs/current/sql-savepoint.html)`
`112`	`114`	`public async Task TestWorksForMultipleLocksUnderTheSameConnectionWithExternalTransaction()`
`113`	`115`	`{`
`114`	`116`	`var key1 = new PostgresAdvisoryLockKey(1);`