Fix transaction dispose

src/AdoNetCore.AseClient/AseConnection.cs

@@ -361,7 +361,7 @@ public override string ConnectionString
         public override ConnectionState State => InternalState;
         private ConnectionState InternalState
         {
-            get => _state;
+            get => _internal != null && _internal.IsDoomed ? ConnectionState.Broken : _state;
             set
             {
                 if (_isDisposed)

src/AdoNetCore.AseClient/AseTransaction.cs

@@ -2,6 +2,7 @@
 using System.Collections.Generic;
 using System.Data;
 using System.Data.Common;
+using System.Diagnostics;
 
 namespace AdoNetCore.AseClient
 {
@@ -141,16 +142,33 @@ public override void Commit()
         /// </summary>
         protected override void Dispose(bool disposing)
         {
-            base.Dispose(disposing);
-
             if (_isDisposed)
             {
                 return;
             }
-
-            Rollback();
-
-            _isDisposed = true;
+            try
+            {
+                // Only rollback if the transaction is still open and the connection is open. For sure do not want to
+                // attempt to rollback a transaction on a closed or broken connection. The only other state in the
+                // ConnectionState that's currently used is Connecting and it doesn't seem appropriate to attempt a
+                // rollback from the Connecting state.
+                if (!_complete && _connection.State == ConnectionState.Open)
+                {
+                    ExecuteRollback();
+                }
+            }
+            catch (Exception ex)
+            {
+                Debug.Assert(false, "Failed to rollback transaction during dispose");
+#if NETFRAMEWORK || NETSTANDARD2_0
+                Trace.TraceError(ex.ToString());
+#endif
+            }
+            finally
+            {
+                base.Dispose(disposing);
+                _isDisposed = true;
+            }
         }
 
         internal bool IsDisposed => _isDisposed;
@@ -170,8 +188,10 @@ public override void Rollback()
                 return;
             }
 
-            using (var command = _connection.CreateCommand())
-            {
+            ExecuteRollback();
+        }
+        private void ExecuteRollback() {
+            using (var command = _connection.CreateCommand()) {
                 command.CommandText = "ROLLBACK TRANSACTION";
                 command.CommandType = CommandType.Text;
                 command.Transaction = this;

test/AdoNetCore.AseClient.Tests/Unit/AseConnectionTests.cs

@@ -267,6 +267,23 @@ public void RepeatedDisposal_DoesNotThrow()
             connection.Dispose();
         }
 
+        [Test]
+        public void DoomedReturnsBroken() {
+            var mockConnection = new Mock<IInternalConnection>();
+            var mockConnectionPoolManager = new Mock<IConnectionPoolManager>();
+
+            mockConnectionPoolManager
+                .Setup(x => x.Reserve(It.IsAny<string>(), It.IsAny<ConnectionParameters>(), It.IsAny<IInfoMessageEventNotifier>(), It.IsAny<RemoteCertificateValidationCallback>()))
+                .Returns(mockConnection.Object);
+
+            mockConnection.SetupGet(x => x.IsDoomed).Returns(true);
+
+            using (var connection = new AseConnection("Data Source=myASEserver;Port=5000;Database=foo;Uid=myUsername;Pwd=myPassword;", mockConnectionPoolManager.Object)) {
+                connection.Open();
+                Assert.AreEqual(ConnectionState.Broken, connection.State);
+            }
+        }
+
         private static IConnectionPoolManager InitMockConnectionPoolManager()
         {
             var mockConnection = new Mock<IInternalConnection>();

test/AdoNetCore.AseClient.Tests/Unit/AseTransactionTests.cs

@@ -1,4 +1,4 @@
-using System.Data;
+using System.Data;
 using Moq;
 using NUnit.Framework;
 
@@ -199,6 +199,8 @@ public void ImplicitRollback_WithValidTransaction_InteractsWithTheDbCommandCorre
                 .Returns(mockCommandBeginTransaction.Object)
                 .Returns(mockCommandRollbackTransaction.Object);
 
+            mockConnection.Setup(x => x.State).Returns(ConnectionState.Open);
+
 
             // Act
             var connection = mockConnection.Object;
@@ -222,6 +224,134 @@ public void ImplicitRollback_WithValidTransaction_InteractsWithTheDbCommandCorre
             mockCommandRollbackTransaction.Verify();
         }
 
+        [Test]
+        public void ImplicitRollback_WithValidTransaction_DisposeFromUsing()
+        {
+            // Arrange
+            var mockConnection = new Mock<IDbConnection>();
+            var isolationLevel = IsolationLevel.Serializable;
+
+            var mockCommandIsolationLevel = new Mock<IDbCommand>();
+            var mockCommandBeginTransaction = new Mock<IDbCommand>();
+            var mockCommandRollbackTransaction = new Mock<IDbCommand>();
+
+            mockCommandIsolationLevel
+                .SetupAllProperties()
+                .Setup(x => x.ExecuteNonQuery())
+                .Returns(0);
+
+            mockCommandBeginTransaction
+                .SetupAllProperties()
+                .Setup(x => x.ExecuteNonQuery())
+                .Returns(0);
+
+            mockCommandRollbackTransaction
+                .SetupAllProperties()
+                .Setup(x => x.ExecuteNonQuery())
+                .Returns(0);
+
+            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);
+
+            mockConnection.Setup(x => x.State).Returns(ConnectionState.Open);
+
+
+            // Act
+            using (var connection = mockConnection.Object) {
+                using (var transaction = connection.BeginTransaction(isolationLevel)) {
+                    // Do nothing
+                }
+            }
+
+            // Assert
+            mockCommandIsolationLevel.VerifySet(x => { x.CommandText = "SET TRANSACTION ISOLATION LEVEL 3"; });
+            mockCommandIsolationLevel.VerifySet(x => { x.CommandType = CommandType.Text; });
+            mockCommandIsolationLevel.Verify();
+
+            mockCommandBeginTransaction.VerifySet(x => { x.CommandText = "BEGIN TRANSACTION"; });
+            mockCommandBeginTransaction.VerifySet(x => { x.CommandType = CommandType.Text; });
+            mockCommandBeginTransaction.Verify();
+
+            mockCommandRollbackTransaction.VerifySet(x => { x.CommandText = "ROLLBACK TRANSACTION"; });
+            mockCommandRollbackTransaction.VerifySet(x => { x.CommandType = CommandType.Text; });
+            mockCommandRollbackTransaction.Verify();
+        }
+        [Test]
+        public void ImplicitRollback_ClosedConnection_DisposeFromUsing() {
+            // Arrange
+            var mockConnection = new Mock<IDbConnection>();
+            var isolationLevel = IsolationLevel.Serializable;
+
+            var mockCommandIsolationLevel = new Mock<IDbCommand>();
+            var mockCommandBeginTransaction = new Mock<IDbCommand>();
+            var mockCommandRollbackTransaction = new Mock<IDbCommand>();
+
+            mockCommandIsolationLevel
+                .SetupAllProperties()
+                .Setup(x => x.ExecuteNonQuery())
+                .Returns(0);
+
+            mockCommandBeginTransaction
+                .SetupAllProperties()
+                .Setup(x => x.ExecuteNonQuery())
+                .Returns(0);
+
+            mockCommandRollbackTransaction
+                .SetupAllProperties()
+                .Setup(x => x.ExecuteNonQuery())
+                .Returns(0);
+
+            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);
+
+            mockConnection.Setup(x => x.State).Returns(ConnectionState.Closed);
+
+
+            // Act
+            using (var connection = mockConnection.Object) {
+                using (var transaction = connection.BeginTransaction(isolationLevel)) {
+                    // Do nothing
+                }
+            }
+
+            // Assert
+            mockCommandIsolationLevel.VerifySet(x => { x.CommandText = "SET TRANSACTION ISOLATION LEVEL 3"; });
+            mockCommandIsolationLevel.VerifySet(x => { x.CommandType = CommandType.Text; });
+            mockCommandIsolationLevel.Verify();
+
+            mockCommandBeginTransaction.VerifySet(x => { x.CommandText = "BEGIN TRANSACTION"; });
+            mockCommandBeginTransaction.VerifySet(x => { x.CommandType = CommandType.Text; });
+            mockCommandBeginTransaction.Verify();
+
+            mockCommandRollbackTransaction.VerifySet(x => x.CommandText = "ROLLBACK TRANSACTION", Times.Never);
+            mockCommandRollbackTransaction.VerifySet(x => x.CommandType = CommandType.Text, Times.Never);
+            mockCommandRollbackTransaction.Verify(x => x.ExecuteNonQuery(), Times.Never);
+        }
+
         [Test]
         public void RepeatedDisposal_DoesNotThrow()
         {
@@ -272,5 +402,68 @@ public void RepeatedDisposal_DoesNotThrow()
             transaction.Dispose(); // Implicit rollback
             transaction.Dispose(); // Should do nothing
         }
+        [Test]
+        public void ImplicitRollback_BrokenConnection_DisposeFromUsing() {
+            // Arrange
+            var mockConnection = new Mock<IDbConnection>();
+            var isolationLevel = IsolationLevel.Serializable;
+
+            var mockCommandIsolationLevel = new Mock<IDbCommand>();
+            var mockCommandBeginTransaction = new Mock<IDbCommand>();
+            var mockCommandRollbackTransaction = new Mock<IDbCommand>();
+
+            mockCommandIsolationLevel
+                .SetupAllProperties()
+                .Setup(x => x.ExecuteNonQuery())
+                .Returns(0);
+
+            mockCommandBeginTransaction
+                .SetupAllProperties()
+                .Setup(x => x.ExecuteNonQuery())
+                .Returns(0);
+
+            mockCommandRollbackTransaction
+                .SetupAllProperties()
+                .Setup(x => x.ExecuteNonQuery())
+                .Returns(0);
+
+            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);
+
+            mockConnection.Setup(x => x.State).Returns(ConnectionState.Broken);
+
+
+            // Act
+            using (var connection = mockConnection.Object) {
+                using (var transaction = connection.BeginTransaction(isolationLevel)) {
+                    // Do nothing
+                }
+            }
+
+            // Assert
+            mockCommandIsolationLevel.VerifySet(x => { x.CommandText = "SET TRANSACTION ISOLATION LEVEL 3"; });
+            mockCommandIsolationLevel.VerifySet(x => { x.CommandType = CommandType.Text; });
+            mockCommandIsolationLevel.Verify();
+
+            mockCommandBeginTransaction.VerifySet(x => { x.CommandText = "BEGIN TRANSACTION"; });
+            mockCommandBeginTransaction.VerifySet(x => { x.CommandType = CommandType.Text; });
+            mockCommandBeginTransaction.Verify();
+
+            mockCommandRollbackTransaction.VerifySet(x => x.CommandText = "ROLLBACK TRANSACTION", Times.Never);
+            mockCommandRollbackTransaction.VerifySet(x => x.CommandType = CommandType.Text, Times.Never);
+            mockCommandRollbackTransaction.Verify(x => x.ExecuteNonQuery(), Times.Never);
+        }
     }
 }