From 6256557ec43e8c8df5d402cae3ad1524b197fbcd Mon Sep 17 00:00:00 2001 From: Jeff Kleber Date: Mon, 13 Jan 2025 08:29:46 -0500 Subject: [PATCH 1/6] WIP Rollback can throw therefore AseTransaction.Dispose must be able to handle/tolerate them. Add tests for exception thrown during dispose. --- src/AdoNetCore.AseClient/AseTransaction.cs | 35 +++-- .../Unit/AseTransactionTests.cs | 132 +++++++++++++++++- 2 files changed, 156 insertions(+), 11 deletions(-) diff --git a/src/AdoNetCore.AseClient/AseTransaction.cs b/src/AdoNetCore.AseClient/AseTransaction.cs index e01fa86c..cda54eb1 100644 --- a/src/AdoNetCore.AseClient/AseTransaction.cs +++ b/src/AdoNetCore.AseClient/AseTransaction.cs @@ -141,16 +141,29 @@ public override void Commit() /// protected override void Dispose(bool disposing) { - base.Dispose(disposing); - - if (_isDisposed) - { + if (_isDisposed) { return; } - - Rollback(); - - _isDisposed = true; + try { + if (!(_complete || _connection.State == ConnectionState.Closed || _connection.State == ConnectionState.Broken)) { + ExecuteRollback(); + } + } + catch (Exception) { + if (disposing) { + throw; + } + else { + // I don't know what if anything we could do here. + } + } + finally { + _isDisposed = true; + if (disposing) { + _connection?.Dispose(); + } + base.Dispose(disposing); + } } internal bool IsDisposed => _isDisposed; @@ -170,8 +183,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; diff --git a/test/AdoNetCore.AseClient.Tests/Unit/AseTransactionTests.cs b/test/AdoNetCore.AseClient.Tests/Unit/AseTransactionTests.cs index 6f9df0c0..ed4744ac 100644 --- a/test/AdoNetCore.AseClient.Tests/Unit/AseTransactionTests.cs +++ b/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(); + 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()) + .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(); + 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()) + .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() { From c75d9e5b5c61c39571405bae179c9c588928109c Mon Sep 17 00:00:00 2001 From: Jeff Kleber Date: Tue, 14 Jan 2025 08:49:11 -0500 Subject: [PATCH 2/6] Return Broken if the internal connection is in the doomed state. Add a test to verify broken is returned when doomed. --- src/AdoNetCore.AseClient/AseConnection.cs | 2 +- src/AdoNetCore.AseClient/AseTransaction.cs | 6 +++++- .../Unit/AseConnectionTests.cs | 17 +++++++++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/AdoNetCore.AseClient/AseConnection.cs b/src/AdoNetCore.AseClient/AseConnection.cs index 2c5283ee..73e77d3a 100644 --- a/src/AdoNetCore.AseClient/AseConnection.cs +++ b/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) diff --git a/src/AdoNetCore.AseClient/AseTransaction.cs b/src/AdoNetCore.AseClient/AseTransaction.cs index cda54eb1..a868e170 100644 --- a/src/AdoNetCore.AseClient/AseTransaction.cs +++ b/src/AdoNetCore.AseClient/AseTransaction.cs @@ -145,7 +145,11 @@ protected override void Dispose(bool disposing) return; } try { - if (!(_complete || _connection.State == ConnectionState.Closed || _connection.State == ConnectionState.Broken)) { + // 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(); } } diff --git a/test/AdoNetCore.AseClient.Tests/Unit/AseConnectionTests.cs b/test/AdoNetCore.AseClient.Tests/Unit/AseConnectionTests.cs index 934d64ef..14941a11 100644 --- a/test/AdoNetCore.AseClient.Tests/Unit/AseConnectionTests.cs +++ b/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(); + var mockConnectionPoolManager = new Mock(); + + mockConnectionPoolManager + .Setup(x => x.Reserve(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .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(); From e69b97cd11402073d7b78e212fadd3189fab1b7c Mon Sep 17 00:00:00 2001 From: Jeff Kleber Date: Tue, 14 Jan 2025 08:54:22 -0500 Subject: [PATCH 3/6] Add a test for no rollback if the connection is in the broken state --- .../Unit/AseTransactionTests.cs | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/test/AdoNetCore.AseClient.Tests/Unit/AseTransactionTests.cs b/test/AdoNetCore.AseClient.Tests/Unit/AseTransactionTests.cs index ed4744ac..1bac4fed 100644 --- a/test/AdoNetCore.AseClient.Tests/Unit/AseTransactionTests.cs +++ b/test/AdoNetCore.AseClient.Tests/Unit/AseTransactionTests.cs @@ -402,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(); + 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()) + .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); + } } } From a02744610dd1c06ada9b7e70a0b6b36d7ad3d3a4 Mon Sep 17 00:00:00 2001 From: Jeff Kleber Date: Tue, 14 Jan 2025 12:40:47 -0500 Subject: [PATCH 4/6] Improve AseTransaction Dispose method readability Added System.Diagnostics for debugging. Improved readability by adjusting brace style. Simplified error handling with Debug.Assert and Debug.WriteLine. Updated finally block for proper disposal sequence. --- src/AdoNetCore.AseClient/AseTransaction.cs | 29 +++++++++++----------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/AdoNetCore.AseClient/AseTransaction.cs b/src/AdoNetCore.AseClient/AseTransaction.cs index a868e170..f1afc5cb 100644 --- a/src/AdoNetCore.AseClient/AseTransaction.cs +++ b/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,32 +142,30 @@ public override void Commit() /// protected override void Dispose(bool disposing) { - if (_isDisposed) { + if (_isDisposed) + { return; } - try { + 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) { + if (!_complete && _connection.State == ConnectionState.Open) + { ExecuteRollback(); } } - catch (Exception) { - if (disposing) { - throw; - } - else { - // I don't know what if anything we could do here. - } + catch + { + Debug.Assert(false, "Failed to rollback transaction during dispose"); + Debug.WriteLine("Failed to rollback transaction during dispose"); } - finally { - _isDisposed = true; - if (disposing) { - _connection?.Dispose(); - } + finally + { base.Dispose(disposing); + _isDisposed = true; } } From 6e0dc9580a02fc724d963b70d6d9a03df617719d Mon Sep 17 00:00:00 2001 From: Jeff Kleber Date: Tue, 14 Jan 2025 14:27:11 -0500 Subject: [PATCH 5/6] Improve error handling in AseTransaction rollback Enhanced error handling in the AseTransaction class: - Catch exceptions and assign to variable `ex` - Add assertion for rollback failure during disposal - Replace Debug.WriteLine with conditional logging: - Use Trace.TraceError for NETFRAMEWORK or NETSTANDARD2_0 --- src/AdoNetCore.AseClient/AseTransaction.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/AdoNetCore.AseClient/AseTransaction.cs b/src/AdoNetCore.AseClient/AseTransaction.cs index f1afc5cb..66716df5 100644 --- a/src/AdoNetCore.AseClient/AseTransaction.cs +++ b/src/AdoNetCore.AseClient/AseTransaction.cs @@ -157,10 +157,12 @@ protected override void Dispose(bool disposing) ExecuteRollback(); } } - catch + catch (Exception ex) { Debug.Assert(false, "Failed to rollback transaction during dispose"); - Debug.WriteLine("Failed to rollback transaction during dispose"); +#if NETFRAMEWORK || NETSTANDARD2_0 + Trace.TraceError(ex.ToString()); +#endif } finally { From 6159f1b7db7a7f8d847758fb4e0f37c93b68109e Mon Sep 17 00:00:00 2001 From: Jeff Kleber Date: Fri, 17 Jan 2025 19:23:25 -0500 Subject: [PATCH 6/6] update version number --- .../AdoNetCore.AseClient.csproj | 17 +++ .../Properties/Resources.Designer.cs | 64 +++++++++++ .../Properties/Resources.resx | 101 ++++++++++++++++++ 3 files changed, 182 insertions(+) create mode 100644 src/AdoNetCore.AseClient/Properties/Resources.Designer.cs create mode 100644 src/AdoNetCore.AseClient/Properties/Resources.resx diff --git a/src/AdoNetCore.AseClient/AdoNetCore.AseClient.csproj b/src/AdoNetCore.AseClient/AdoNetCore.AseClient.csproj index 5a726503..6967ff44 100644 --- a/src/AdoNetCore.AseClient/AdoNetCore.AseClient.csproj +++ b/src/AdoNetCore.AseClient/AdoNetCore.AseClient.csproj @@ -5,5 +5,22 @@ 7 + 0.19.3 + 0.19.3 + True + True + + + True + True + Resources.resx + + + + + ResXFileCodeGenerator + Resources.Designer.cs + + diff --git a/src/AdoNetCore.AseClient/Properties/Resources.Designer.cs b/src/AdoNetCore.AseClient/Properties/Resources.Designer.cs new file mode 100644 index 00000000..d704e9b4 --- /dev/null +++ b/src/AdoNetCore.AseClient/Properties/Resources.Designer.cs @@ -0,0 +1,64 @@ +//------------------------------------------------------------------------------ +// +// This code was generated by a tool. +// Runtime Version:4.0.30319.42000 +// +// Changes to this file may cause incorrect behavior and will be lost if +// the code is regenerated. +// +//------------------------------------------------------------------------------ + +namespace AdoNetCore.AseClient.Properties { + using System; + using System.Reflection; + + + /// + /// A strongly-typed resource class, for looking up localized strings, etc. + /// + // This class was auto-generated by the StronglyTypedResourceBuilder + // class via a tool like ResGen or Visual Studio. + // To add or remove a member, edit your .ResX file then rerun ResGen + // with the /str option, or rebuild your VS project. + [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "17.0.0.0")] + [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] + [global::System.Runtime.CompilerServices.CompilerGeneratedAttribute()] + internal class Resources { + + private static global::System.Resources.ResourceManager resourceMan; + + private static global::System.Globalization.CultureInfo resourceCulture; + + [global::System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("Microsoft.Performance", "CA1811:AvoidUncalledPrivateCode")] + internal Resources() { + } + + /// + /// Returns the cached ResourceManager instance used by this class. + /// + [global::System.ComponentModel.EditorBrowsableAttribute(global::System.ComponentModel.EditorBrowsableState.Advanced)] + internal static global::System.Resources.ResourceManager ResourceManager { + get { + if (object.ReferenceEquals(resourceMan, null)) { + global::System.Resources.ResourceManager temp = new global::System.Resources.ResourceManager("AdoNetCore.AseClient.Properties.Resources", typeof(Resources).GetTypeInfo().Assembly); + resourceMan = temp; + } + return resourceMan; + } + } + + /// + /// Overrides the current thread's CurrentUICulture property for all + /// resource lookups using this strongly typed resource class. + /// + [global::System.ComponentModel.EditorBrowsableAttribute(global::System.ComponentModel.EditorBrowsableState.Advanced)] + internal static global::System.Globalization.CultureInfo Culture { + get { + return resourceCulture; + } + set { + resourceCulture = value; + } + } + } +} diff --git a/src/AdoNetCore.AseClient/Properties/Resources.resx b/src/AdoNetCore.AseClient/Properties/Resources.resx new file mode 100644 index 00000000..4fdb1b6a --- /dev/null +++ b/src/AdoNetCore.AseClient/Properties/Resources.resx @@ -0,0 +1,101 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + text/microsoft-resx + + + 1.3 + + + System.Resources.ResXResourceReader, System.Windows.Forms, Version=2.0.3500.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + + System.Resources.ResXResourceWriter, System.Windows.Forms, Version=2.0.3500.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + \ No newline at end of file