From 325453e3910ea821a5a3718d207937c542ee19ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= Date: Sat, 20 May 2017 18:21:19 +0200 Subject: [PATCH 1/2] NH-4013 - Test case and fix for SQL-Server batcher failure after CloseCommands. --- .../ConnectionTest/BatcherFixture.cs | 45 +++++++++++++++++++ .../ConnectionTest/Silly.hbm.xml | 7 +++ .../ConnectionTest/YetAnother.cs | 12 +++++ src/NHibernate.Test/NHibernate.Test.csproj | 2 + .../AdoNet/MySqlClientBatchingBatcher.cs | 36 ++++++++++++--- .../AdoNet/SqlClientBatchingBatcher.cs | 36 ++++++++++++--- 6 files changed, 126 insertions(+), 12 deletions(-) create mode 100644 src/NHibernate.Test/ConnectionTest/BatcherFixture.cs create mode 100644 src/NHibernate.Test/ConnectionTest/YetAnother.cs diff --git a/src/NHibernate.Test/ConnectionTest/BatcherFixture.cs b/src/NHibernate.Test/ConnectionTest/BatcherFixture.cs new file mode 100644 index 00000000000..d96a7c205d5 --- /dev/null +++ b/src/NHibernate.Test/ConnectionTest/BatcherFixture.cs @@ -0,0 +1,45 @@ +using System; +using System.Collections; +using System.Collections.Generic; +using NHibernate.Cfg; +using NHibernate.Util; +using NUnit.Framework; +using Environment = NHibernate.Cfg.Environment; + +namespace NHibernate.Test.ConnectionTest +{ + [TestFixture] + public class BatcherFixture : ConnectionManagementTestCase + { + protected override void Configure(Configuration config) + { + base.Configure(config); + config.SetProperty(Environment.BatchSize, "10"); + } + + protected override ISession GetSessionUnderTest() + => OpenSession(); + + protected override void OnTearDown() + { + using (var s = OpenSession()) + { + s.CreateQuery("delete from System.Object").ExecuteUpdate(); + } + } + + [Test] + public void CanCloseCommandsAndUseBatcher() + { + using (var s = OpenSession()) + { + // Need a generator strategy not causing insert at save. + var silly = new YetAnother { Name = "Silly" }; + s.Save(silly); + s.GetSessionImplementation().ConnectionManager.Batcher.CloseCommands(); + + Assert.DoesNotThrow(s.Flush, "Flush failure after closing commands."); + } + } + } +} \ No newline at end of file diff --git a/src/NHibernate.Test/ConnectionTest/Silly.hbm.xml b/src/NHibernate.Test/ConnectionTest/Silly.hbm.xml index bc233c62ead..b36c66f911a 100644 --- a/src/NHibernate.Test/ConnectionTest/Silly.hbm.xml +++ b/src/NHibernate.Test/ConnectionTest/Silly.hbm.xml @@ -16,4 +16,11 @@ + + + + + + + \ No newline at end of file diff --git a/src/NHibernate.Test/ConnectionTest/YetAnother.cs b/src/NHibernate.Test/ConnectionTest/YetAnother.cs new file mode 100644 index 00000000000..398a9a39542 --- /dev/null +++ b/src/NHibernate.Test/ConnectionTest/YetAnother.cs @@ -0,0 +1,12 @@ +using System; + +namespace NHibernate.Test.ConnectionTest +{ + [Serializable] + public class YetAnother + { + public virtual long Id { get; set; } + + public virtual string Name { get; set; } + } +} \ No newline at end of file diff --git a/src/NHibernate.Test/NHibernate.Test.csproj b/src/NHibernate.Test/NHibernate.Test.csproj index fbd40cd73ec..1ec3a9086c4 100644 --- a/src/NHibernate.Test/NHibernate.Test.csproj +++ b/src/NHibernate.Test/NHibernate.Test.csproj @@ -202,8 +202,10 @@ + + diff --git a/src/NHibernate/AdoNet/MySqlClientBatchingBatcher.cs b/src/NHibernate/AdoNet/MySqlClientBatchingBatcher.cs index 59904a1b74e..06fff280dbe 100644 --- a/src/NHibernate/AdoNet/MySqlClientBatchingBatcher.cs +++ b/src/NHibernate/AdoNet/MySqlClientBatchingBatcher.cs @@ -74,7 +74,6 @@ protected override void DoExecuteBatch(DbCommand ps) if (Factory.Settings.SqlStatementLogger.IsDebugEnabled) { Factory.Settings.SqlStatementLogger.LogBatchCommand(currentBatchCommandsLog.ToString()); - currentBatchCommandsLog = new StringBuilder().AppendLine("Batch commands:"); } int rowsAffected; @@ -89,9 +88,7 @@ protected override void DoExecuteBatch(DbCommand ps) Expectations.VerifyOutcomeBatched(totalExpectedRowsAffected, rowsAffected); - currentBatch.Dispose(); - totalExpectedRowsAffected = 0; - currentBatch = CreateConfiguredBatch(); + ClearCurrentBatch(); } private MySqlClientSqlCommandSet CreateConfiguredBatch() @@ -99,18 +96,45 @@ private MySqlClientSqlCommandSet CreateConfiguredBatch() return new MySqlClientSqlCommandSet(batchSize); } + private void ClearCurrentBatch() + { + currentBatch.Dispose(); + totalExpectedRowsAffected = 0; + currentBatch = CreateConfiguredBatch(); + + if (Factory.Settings.SqlStatementLogger.IsDebugEnabled) + { + currentBatchCommandsLog = new StringBuilder().AppendLine("Batch commands:"); + } + } + public override void CloseCommands() { base.CloseCommands(); try { - currentBatch.Dispose(); + ClearCurrentBatch(); } catch (Exception e) { - // Prevent exceptions when closing the batch from hiding any original exception + // Prevent exceptions when clearing the batch from hiding any original exception // (We do not know here if this batch closing occurs after a failure or not.) + Log.Warn("Exception clearing batch", e); + } + } + + protected override void Dispose(bool isDisposing) + { + base.Dispose(isDisposing); + // Prevent exceptions when closing the batch from hiding any original exception + // (We do not know here if this batch closing occurs after a failure or not.) + try + { + currentBatch.Dispose(); + } + catch (Exception e) + { Log.Warn("Exception closing batcher", e); } } diff --git a/src/NHibernate/AdoNet/SqlClientBatchingBatcher.cs b/src/NHibernate/AdoNet/SqlClientBatchingBatcher.cs index 9c8196a97d7..a822a34d98d 100644 --- a/src/NHibernate/AdoNet/SqlClientBatchingBatcher.cs +++ b/src/NHibernate/AdoNet/SqlClientBatchingBatcher.cs @@ -77,7 +77,6 @@ protected override void DoExecuteBatch(DbCommand ps) if (Factory.Settings.SqlStatementLogger.IsDebugEnabled) { Factory.Settings.SqlStatementLogger.LogBatchCommand(_currentBatchCommandsLog.ToString()); - _currentBatchCommandsLog = new StringBuilder().AppendLine("Batch commands:"); } int rowsAffected; @@ -92,9 +91,7 @@ protected override void DoExecuteBatch(DbCommand ps) Expectations.VerifyOutcomeBatched(_totalExpectedRowsAffected, rowsAffected); - _currentBatch.Dispose(); - _totalExpectedRowsAffected = 0; - _currentBatch = CreateConfiguredBatch(); + ClearCurrentBatch(); } private SqlClientSqlCommandSet CreateConfiguredBatch() @@ -118,18 +115,45 @@ private SqlClientSqlCommandSet CreateConfiguredBatch() return result; } + private void ClearCurrentBatch() + { + _currentBatch.Dispose(); + _totalExpectedRowsAffected = 0; + _currentBatch = CreateConfiguredBatch(); + + if (Factory.Settings.SqlStatementLogger.IsDebugEnabled) + { + _currentBatchCommandsLog = new StringBuilder().AppendLine("Batch commands:"); + } + } + public override void CloseCommands() { base.CloseCommands(); + // Prevent exceptions when closing the batch from hiding any original exception + // (We do not know here if this batch closing occurs after a failure or not.) + try + { + ClearCurrentBatch(); + } + catch (Exception e) + { + Log.Warn("Exception clearing batch", e); + } + } + + protected override void Dispose(bool isDisposing) + { + base.Dispose(isDisposing); + // Prevent exceptions when closing the batch from hiding any original exception + // (We do not know here if this batch closing occurs after a failure or not.) try { _currentBatch.Dispose(); } catch (Exception e) { - // Prevent exceptions when closing the batch from hiding any original exception - // (We do not know here if this batch closing occurs after a failure or not.) Log.Warn("Exception closing batcher", e); } } From 747dc7aad60ff50db6b260a7c14131b34e859bd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= Date: Tue, 6 Jun 2017 13:57:28 +0200 Subject: [PATCH 2/2] NH-4013 - Guarantee cleanup on failed sql execution. --- .../AdoNet/MySqlClientBatchingBatcher.cs | 35 +++++++++-------- .../AdoNet/SqlClientBatchingBatcher.cs | 38 ++++++++++--------- 2 files changed, 41 insertions(+), 32 deletions(-) diff --git a/src/NHibernate/AdoNet/MySqlClientBatchingBatcher.cs b/src/NHibernate/AdoNet/MySqlClientBatchingBatcher.cs index 06fff280dbe..0e8dc1ff1b2 100644 --- a/src/NHibernate/AdoNet/MySqlClientBatchingBatcher.cs +++ b/src/NHibernate/AdoNet/MySqlClientBatchingBatcher.cs @@ -69,26 +69,31 @@ public override void AddToBatch(IExpectation expectation) protected override void DoExecuteBatch(DbCommand ps) { - Log.DebugFormat("Executing batch"); - CheckReaders(); - if (Factory.Settings.SqlStatementLogger.IsDebugEnabled) - { - Factory.Settings.SqlStatementLogger.LogBatchCommand(currentBatchCommandsLog.ToString()); - } - - int rowsAffected; try { - rowsAffected = currentBatch.ExecuteNonQuery(); + Log.DebugFormat("Executing batch"); + CheckReaders(); + if (Factory.Settings.SqlStatementLogger.IsDebugEnabled) + { + Factory.Settings.SqlStatementLogger.LogBatchCommand(currentBatchCommandsLog.ToString()); + } + + int rowsAffected; + try + { + rowsAffected = currentBatch.ExecuteNonQuery(); + } + catch (DbException e) + { + throw ADOExceptionHelper.Convert(Factory.SQLExceptionConverter, e, "could not execute batch command."); + } + + Expectations.VerifyOutcomeBatched(totalExpectedRowsAffected, rowsAffected); } - catch (DbException e) + finally { - throw ADOExceptionHelper.Convert(Factory.SQLExceptionConverter, e, "could not execute batch command."); + ClearCurrentBatch(); } - - Expectations.VerifyOutcomeBatched(totalExpectedRowsAffected, rowsAffected); - - ClearCurrentBatch(); } private MySqlClientSqlCommandSet CreateConfiguredBatch() diff --git a/src/NHibernate/AdoNet/SqlClientBatchingBatcher.cs b/src/NHibernate/AdoNet/SqlClientBatchingBatcher.cs index a822a34d98d..94cd00112c7 100644 --- a/src/NHibernate/AdoNet/SqlClientBatchingBatcher.cs +++ b/src/NHibernate/AdoNet/SqlClientBatchingBatcher.cs @@ -61,7 +61,7 @@ public override void AddToBatch(IExpectation expectation) { Log.Debug("Adding to batch:" + lineWithParameters); } - _currentBatch.Append((System.Data.SqlClient.SqlCommand) batchUpdate); + _currentBatch.Append((System.Data.SqlClient.SqlCommand)batchUpdate); if (_currentBatch.CountOfCommands >= _batchSize) { @@ -71,27 +71,31 @@ public override void AddToBatch(IExpectation expectation) protected override void DoExecuteBatch(DbCommand ps) { - Log.DebugFormat("Executing batch"); - CheckReaders(); - Prepare(_currentBatch.BatchCommand); - if (Factory.Settings.SqlStatementLogger.IsDebugEnabled) - { - Factory.Settings.SqlStatementLogger.LogBatchCommand(_currentBatchCommandsLog.ToString()); - } - - int rowsAffected; try { - rowsAffected = _currentBatch.ExecuteNonQuery(); + Log.DebugFormat("Executing batch"); + CheckReaders(); + Prepare(_currentBatch.BatchCommand); + if (Factory.Settings.SqlStatementLogger.IsDebugEnabled) + { + Factory.Settings.SqlStatementLogger.LogBatchCommand(_currentBatchCommandsLog.ToString()); + } + int rowsAffected; + try + { + rowsAffected = _currentBatch.ExecuteNonQuery(); + } + catch (DbException e) + { + throw ADOExceptionHelper.Convert(Factory.SQLExceptionConverter, e, "could not execute batch command."); + } + + Expectations.VerifyOutcomeBatched(_totalExpectedRowsAffected, rowsAffected); } - catch (DbException e) + finally { - throw ADOExceptionHelper.Convert(Factory.SQLExceptionConverter, e, "could not execute batch command."); + ClearCurrentBatch(); } - - Expectations.VerifyOutcomeBatched(_totalExpectedRowsAffected, rowsAffected); - - ClearCurrentBatch(); } private SqlClientSqlCommandSet CreateConfiguredBatch()