From e19c482dc96539531d624f11c6b0514faebb352f Mon Sep 17 00:00:00 2001 From: "florian.gouze" Date: Tue, 21 Dec 2021 17:23:58 +0100 Subject: [PATCH 1/2] Prevent rollback when connection is Broken or closed. Otherwise, Dispose called by the GC were throwing an exception when trying to rollback on a broken Connection Resulting in a error thrown during GC and fatal on all DS --- src/AdoNetCore.AseClient/AseTransaction.cs | 2 +- .../Unit/AseTransactionTests.cs | 65 +++++++++++++++++-- 2 files changed, 62 insertions(+), 5 deletions(-) diff --git a/src/AdoNetCore.AseClient/AseTransaction.cs b/src/AdoNetCore.AseClient/AseTransaction.cs index e01fa86c..a4ffc79e 100644 --- a/src/AdoNetCore.AseClient/AseTransaction.cs +++ b/src/AdoNetCore.AseClient/AseTransaction.cs @@ -165,7 +165,7 @@ public override void Rollback() throw new ObjectDisposedException(nameof(AseTransaction)); } - if (_complete) + if (_complete || _connection.State == ConnectionState.Closed || _connection.State == ConnectionState.Broken) { return; } diff --git a/test/AdoNetCore.AseClient.Tests/Unit/AseTransactionTests.cs b/test/AdoNetCore.AseClient.Tests/Unit/AseTransactionTests.cs index 6f9df0c0..419e4082 100644 --- a/test/AdoNetCore.AseClient.Tests/Unit/AseTransactionTests.cs +++ b/test/AdoNetCore.AseClient.Tests/Unit/AseTransactionTests.cs @@ -1,4 +1,5 @@ -using System.Data; +using System; +using System.Data; using Moq; using NUnit.Framework; @@ -124,8 +125,8 @@ public void ExplicitRollback_WithValidTransaction_InteractsWithTheDbCommandCorre var t = new AseTransaction(mockConnection.Object, isolationLevel); t.Begin(); return t; - }); - + }); + mockConnection.Setup(x => x.State).Returns(() => { return ConnectionState.Open; }); mockConnection .SetupSequence(x => x.CreateCommand()) .Returns(mockCommandIsolationLevel.Object) @@ -193,6 +194,7 @@ public void ImplicitRollback_WithValidTransaction_InteractsWithTheDbCommandCorre return t; }); + mockConnection.Setup(x => x.State).Returns(() => { return ConnectionState.Open; }); mockConnection .SetupSequence(x => x.CreateCommand()) .Returns(mockCommandIsolationLevel.Object) @@ -222,11 +224,67 @@ public void ImplicitRollback_WithValidTransaction_InteractsWithTheDbCommandCorre mockCommandRollbackTransaction.Verify(); } + [Test] + public void ImplicitRollback_WithBrokenConnection_DoesNotThrowExceptionDuringDispose() + { + // Arrange + var mockConnection = new Mock(); + var isolationLevel = IsolationLevel.Serializable; + + var mockCommandIsolationLevel = new Mock(); + var mockCommandBeginTransaction = new Mock(); + var mockCommandRollbackTransaction = new Mock(); + + mockCommandIsolationLevel + .SetupAllProperties() + .Setup(x => x.ExecuteNonQuery()) + .Returns(0); + + mockCommandBeginTransaction + .SetupAllProperties() + .Setup(x => x.ExecuteNonQuery()) + .Returns(0); + + mockCommandRollbackTransaction + .SetupAllProperties() + .Setup(x => x.ExecuteNonQuery()) + .Throws( new InvalidOperationException("Cannot execute on a connection which is not open")); + + + mockConnection.Setup(x => x.State).Returns(() => { return ConnectionState.Broken; }); + mockConnection + .Setup(x => x.BeginTransaction(isolationLevel)) + .Returns(() => + { + // Simulate what AseConnection.BeginTransaction() does. + var t = new AseTransaction(mockConnection.Object, isolationLevel); + t.Begin(); + return t; + }); + + mockConnection + .SetupSequence(x => x.CreateCommand()) + .Returns(mockCommandIsolationLevel.Object) + .Returns(mockCommandBeginTransaction.Object) + .Returns(mockCommandRollbackTransaction.Object); + + + // Act + var connection = mockConnection.Object; + var transaction = connection.BeginTransaction(isolationLevel); + + + transaction.Dispose(); // Implicit rollback + } + + + [Test] public void RepeatedDisposal_DoesNotThrow() { // Arrange var mockConnection = new Mock(); + var isolationLevel = IsolationLevel.Serializable; var mockCommandIsolationLevel = new Mock(); @@ -247,7 +305,6 @@ public void RepeatedDisposal_DoesNotThrow() .SetupAllProperties() .Setup(x => x.ExecuteNonQuery()) .Returns(0); - mockConnection .Setup(x => x.BeginTransaction(isolationLevel)) .Returns(() => From da9e6c911656059421edf215cc035653d94a2d0c Mon Sep 17 00:00:00 2001 From: florian gouze Date: Thu, 12 Jan 2023 16:24:54 +0100 Subject: [PATCH 2/2] Switch to correct dispose pattern --- src/AdoNetCore.AseClient/AseTransaction.cs | 11 ++++++++--- .../Unit/AseTransactionTests.cs | 16 ++++++++++++++-- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/AdoNetCore.AseClient/AseTransaction.cs b/src/AdoNetCore.AseClient/AseTransaction.cs index a4ffc79e..fd43483b 100644 --- a/src/AdoNetCore.AseClient/AseTransaction.cs +++ b/src/AdoNetCore.AseClient/AseTransaction.cs @@ -141,16 +141,21 @@ public override void Commit() /// protected override void Dispose(bool disposing) { - base.Dispose(disposing); - if (_isDisposed) { return; } - Rollback(); + if (disposing) + { + _connection?.Dispose(); + } + Rollback(); _isDisposed = true; + + base.Dispose(disposing); + } internal bool IsDisposed => _isDisposed; diff --git a/test/AdoNetCore.AseClient.Tests/Unit/AseTransactionTests.cs b/test/AdoNetCore.AseClient.Tests/Unit/AseTransactionTests.cs index 419e4082..9197f225 100644 --- a/test/AdoNetCore.AseClient.Tests/Unit/AseTransactionTests.cs +++ b/test/AdoNetCore.AseClient.Tests/Unit/AseTransactionTests.cs @@ -273,8 +273,8 @@ public void ImplicitRollback_WithBrokenConnection_DoesNotThrowExceptionDuringDis var connection = mockConnection.Object; var transaction = connection.BeginTransaction(isolationLevel); - - transaction.Dispose(); // Implicit rollback + Assert.DoesNotThrow(() => { transaction.Dispose(); }); + // Implicit rollback } @@ -290,6 +290,7 @@ public void RepeatedDisposal_DoesNotThrow() var mockCommandIsolationLevel = new Mock(); var mockCommandBeginTransaction = new Mock(); var mockCommandRollbackTransaction = new Mock(); + mockCommandIsolationLevel .SetupAllProperties() @@ -305,6 +306,7 @@ public void RepeatedDisposal_DoesNotThrow() .SetupAllProperties() .Setup(x => x.ExecuteNonQuery()) .Returns(0); + mockConnection .Setup(x => x.BeginTransaction(isolationLevel)) .Returns(() => @@ -328,6 +330,16 @@ public void RepeatedDisposal_DoesNotThrow() transaction.Dispose(); // Implicit rollback transaction.Dispose(); // Should do nothing + + mockConnection.Verify(t => t.Dispose(), Times.Once, "we called multiple time the dispose on the master"); + + //mockAseTransaction.Object. + //we tried to make this work but cannot work it as long as the class is sealed and it's sealed for performance constraint. + //if you want to verify it still, use var mockAseTransaction = new Mock(MockBehavior.Loose,mockConnection.Object, isolationLevel) { CallBase = true } + //mockAseTransaction.Verify(t => t.Rollback(), Times.Once, "we should have only called once the rollback"); + + + } } }