Merge pull request #2337 from fredericDelaporte/FixSomeNre

fredericDelaporte · web-flow · commit e7ba6ac83996 · 2020-03-27T19:19:59.000+01:00
Fix some NRE in work isolation and connection handling
diff --git a/src/NHibernate/Async/Transaction/AdoNetTransactionFactory.cs b/src/NHibernate/Async/Transaction/AdoNetTransactionFactory.cs
@@ -16,7 +16,6 @@
 using NHibernate.Engine;
 using NHibernate.Engine.Transaction;
 using NHibernate.Exceptions;
-using NHibernate.Impl;
 
 namespace NHibernate.Transaction
 {
@@ -42,33 +41,24 @@ async Task InternalExecuteWorkInIsolationAsync()
 
 				DbConnection connection = null;
 				DbTransaction trans = null;
-				// bool wasAutoCommit = false;
 				try
 				{
 					// We make an exception for SQLite and use the session's connection,
 					// since SQLite only allows one connection to the database.
-					if (session.Factory.Dialect is SQLiteDialect)
-						connection = session.Connection;
-					else
-						connection = await (session.Factory.ConnectionProvider.GetConnectionAsync(cancellationToken)).ConfigureAwait(false);
+					connection = session.Factory.Dialect is SQLiteDialect
+					? session.Connection
+					: await (session.Factory.ConnectionProvider.GetConnectionAsync(cancellationToken)).ConfigureAwait(false);
 
 					if (transacted)
 					{
 						trans = connection.BeginTransaction();
-						// TODO NH: a way to read the autocommit state is needed
-						//if (TransactionManager.GetAutoCommit(connection))
-						//{
-						//  wasAutoCommit = true;
-						//  TransactionManager.SetAutoCommit(connection, false);
-						//}
 					}
 
 					await (work.DoWorkAsync(connection, trans, cancellationToken)).ConfigureAwait(false);
 
 					if (transacted)
 					{
 						trans.Commit();
-						//TransactionManager.Commit(connection);
 					}
 				}
 				catch (Exception t)
@@ -84,49 +74,33 @@ async Task InternalExecuteWorkInIsolationAsync()
 						}
 						catch (Exception ignore)
 						{
-							isolaterLog.Debug(ignore, "Unable to rollback transaction");
+							_isolatorLog.Debug(ignore, "Unable to rollback transaction");
 						}
 
-						if (t is HibernateException)
-						{
-							throw;
-						}
-						else if (t is DbException)
-						{
-							throw ADOExceptionHelper.Convert(session.Factory.SQLExceptionConverter, t,
-						                                 "error performing isolated work");
-						}
-						else
-						{
-							throw new HibernateException("error performing isolated work", t);
-						}
+						switch (t)
+					{
+						case HibernateException _:
+								throw;
+						case DbException _:
+								throw ADOExceptionHelper.Convert(session.Factory.SQLExceptionConverter, t,
+							                                 "error performing isolated work");
+						default:
+								throw new HibernateException("error performing isolated work", t);
+					}
 					}
 				}
 				finally
 				{
-					//if (transacted && wasAutoCommit)
-					//{
-					//  try
-					//  {
-					//    // TODO NH: reset autocommit
-					//    // TransactionManager.SetAutoCommit(connection, true);
-					//  }
-					//  catch (Exception)
-					//  {
-					//    log.Debug("was unable to reset connection back to auto-commit");
-					//  }
-					//}
-
 					try
 					{
 						trans?.Dispose();
 					}
 					catch (Exception ignore)
 					{
-						isolaterLog.Warn(ignore, "Unable to dispose transaction");
+						_isolatorLog.Warn(ignore, "Unable to dispose transaction");
 					}
 
-					if (session.Factory.Dialect is SQLiteDialect == false)
+					if (connection != null && session.Factory.Dialect is SQLiteDialect == false)
 						session.Factory.ConnectionProvider.CloseConnection(connection);
 				}
 			}
diff --git a/src/NHibernate/Connection/ConnectionProvider.cs b/src/NHibernate/Connection/ConnectionProvider.cs
@@ -24,6 +24,9 @@ public abstract partial class ConnectionProvider : IConnectionProvider
 		/// <param name="conn">The <see cref="DbConnection"/> to clean up.</param>
 		public virtual void CloseConnection(DbConnection conn)
 		{
+			if (conn == null)
+				throw new ArgumentNullException(nameof(conn));
+
 			log.Debug("Closing connection");
 			try
 			{
diff --git a/src/NHibernate/Transaction/AdoNetTransactionFactory.cs b/src/NHibernate/Transaction/AdoNetTransactionFactory.cs
@@ -6,7 +6,6 @@
 using NHibernate.Engine;
 using NHibernate.Engine.Transaction;
 using NHibernate.Exceptions;
-using NHibernate.Impl;
 
 namespace NHibernate.Transaction
 {
@@ -16,7 +15,7 @@ namespace NHibernate.Transaction
 	/// </summary>
 	public partial class AdoNetTransactionFactory : ITransactionFactory
 	{
-		private readonly INHibernateLogger isolaterLog = NHibernateLogger.For(typeof(ITransactionFactory));
+		private static readonly INHibernateLogger _isolatorLog = NHibernateLogger.For(typeof(ITransactionFactory));
 
 		/// <inheritdoc />
 		public virtual ITransaction CreateTransaction(ISessionImplementor session)
@@ -52,33 +51,24 @@ public virtual void ExecuteWorkInIsolation(ISessionImplementor session, IIsolate
 
 			DbConnection connection = null;
 			DbTransaction trans = null;
-			// bool wasAutoCommit = false;
 			try
 			{
 				// We make an exception for SQLite and use the session's connection,
 				// since SQLite only allows one connection to the database.
-				if (session.Factory.Dialect is SQLiteDialect)
-					connection = session.Connection;
-				else
-					connection = session.Factory.ConnectionProvider.GetConnection();
+				connection = session.Factory.Dialect is SQLiteDialect
+					? session.Connection
+					: session.Factory.ConnectionProvider.GetConnection();
 
 				if (transacted)
 				{
 					trans = connection.BeginTransaction();
-					// TODO NH: a way to read the autocommit state is needed
-					//if (TransactionManager.GetAutoCommit(connection))
-					//{
-					//  wasAutoCommit = true;
-					//  TransactionManager.SetAutoCommit(connection, false);
-					//}
 				}
 
 				work.DoWork(connection, trans);
 
 				if (transacted)
 				{
 					trans.Commit();
-					//TransactionManager.Commit(connection);
 				}
 			}
 			catch (Exception t)
@@ -94,49 +84,33 @@ public virtual void ExecuteWorkInIsolation(ISessionImplementor session, IIsolate
 					}
 					catch (Exception ignore)
 					{
-						isolaterLog.Debug(ignore, "Unable to rollback transaction");
+						_isolatorLog.Debug(ignore, "Unable to rollback transaction");
 					}
 
-					if (t is HibernateException)
+					switch (t)
 					{
-						throw;
-					}
-					else if (t is DbException)
-					{
-						throw ADOExceptionHelper.Convert(session.Factory.SQLExceptionConverter, t,
-						                                 "error performing isolated work");
-					}
-					else
-					{
-						throw new HibernateException("error performing isolated work", t);
+						case HibernateException _:
+							throw;
+						case DbException _:
+							throw ADOExceptionHelper.Convert(session.Factory.SQLExceptionConverter, t,
+							                                 "error performing isolated work");
+						default:
+							throw new HibernateException("error performing isolated work", t);
 					}
 				}
 			}
 			finally
 			{
-				//if (transacted && wasAutoCommit)
-				//{
-				//  try
-				//  {
-				//    // TODO NH: reset autocommit
-				//    // TransactionManager.SetAutoCommit(connection, true);
-				//  }
-				//  catch (Exception)
-				//  {
-				//    log.Debug("was unable to reset connection back to auto-commit");
-				//  }
-				//}
-
 				try
 				{
 					trans?.Dispose();
 				}
 				catch (Exception ignore)
 				{
-					isolaterLog.Warn(ignore, "Unable to dispose transaction");
+					_isolatorLog.Warn(ignore, "Unable to dispose transaction");
 				}
 
-				if (session.Factory.Dialect is SQLiteDialect == false)
+				if (connection != null && session.Factory.Dialect is SQLiteDialect == false)
 					session.Factory.ConnectionProvider.CloseConnection(connection);
 			}
 		}

Original file line number	Diff line number	Diff line change
`@@ -16,7 +16,6 @@`
`16`	`16`	`using NHibernate.Engine;`
`17`	`17`	`using NHibernate.Engine.Transaction;`
`18`	`18`	`using NHibernate.Exceptions;`
`19`		`-using NHibernate.Impl;`
`20`	`19`
`21`	`20`	`namespace NHibernate.Transaction`
`22`	`21`	`{`
`@@ -42,33 +41,24 @@ async Task InternalExecuteWorkInIsolationAsync()`
`42`	`41`
`43`	`42`	`DbConnection connection = null;`
`44`	`43`	`DbTransaction trans = null;`
`45`		`- // bool wasAutoCommit = false;`
`46`	`44`	`try`
`47`	`45`	`{`
`48`	`46`	`// We make an exception for SQLite and use the session's connection,`
`49`	`47`	`// since SQLite only allows one connection to the database.`
`50`		`- if (session.Factory.Dialect is SQLiteDialect)`
`51`		`- connection = session.Connection;`
`52`		`- else`
`53`		`- connection = await (session.Factory.ConnectionProvider.GetConnectionAsync(cancellationToken)).ConfigureAwait(false);`
	`48`	`+ connection = session.Factory.Dialect is SQLiteDialect`
	`49`	`+ ? session.Connection`
	`50`	`+ : await (session.Factory.ConnectionProvider.GetConnectionAsync(cancellationToken)).ConfigureAwait(false);`
`54`	`51`
`55`	`52`	`if (transacted)`
`56`	`53`	`{`
`57`	`54`	`trans = connection.BeginTransaction();`
`58`		`- // TODO NH: a way to read the autocommit state is needed`
`59`		`- //if (TransactionManager.GetAutoCommit(connection))`
`60`		`- //{`
`61`		`- // wasAutoCommit = true;`
`62`		`- // TransactionManager.SetAutoCommit(connection, false);`
`63`		`- //}`
`64`	`55`	`}`
`65`	`56`
`66`	`57`	`await (work.DoWorkAsync(connection, trans, cancellationToken)).ConfigureAwait(false);`
`67`	`58`
`68`	`59`	`if (transacted)`
`69`	`60`	`{`
`70`	`61`	`trans.Commit();`
`71`		`- //TransactionManager.Commit(connection);`
`72`	`62`	`}`
`73`	`63`	`}`
`74`	`64`	`catch (Exception t)`
`@@ -84,49 +74,33 @@ async Task InternalExecuteWorkInIsolationAsync()`
`84`	`74`	`}`
`85`	`75`	`catch (Exception ignore)`
`86`	`76`	`{`
`87`		`- isolaterLog.Debug(ignore, "Unable to rollback transaction");`
	`77`	`+ _isolatorLog.Debug(ignore, "Unable to rollback transaction");`
`88`	`78`	`}`
`89`	`79`
`90`		`- if (t is HibernateException)`
`91`		`- {`
`92`		`- throw;`
`93`		`- }`
`94`		`- else if (t is DbException)`
`95`		`- {`
`96`		`- throw ADOExceptionHelper.Convert(session.Factory.SQLExceptionConverter, t,`
`97`		`- "error performing isolated work");`
`98`		`- }`
`99`		`- else`
`100`		`- {`
`101`		`- throw new HibernateException("error performing isolated work", t);`
`102`		`- }`
	`80`	`+ switch (t)`
	`81`	`+ {`
	`82`	`+ case HibernateException _:`
	`83`	`+ throw;`
	`84`	`+ case DbException _:`
	`85`	`+ throw ADOExceptionHelper.Convert(session.Factory.SQLExceptionConverter, t,`
	`86`	`+ "error performing isolated work");`
	`87`	`+ default:`
	`88`	`+ throw new HibernateException("error performing isolated work", t);`
	`89`	`+ }`
`103`	`90`	`}`
`104`	`91`	`}`
`105`	`92`	`finally`
`106`	`93`	`{`
`107`		`- //if (transacted && wasAutoCommit)`
`108`		`- //{`
`109`		`- // try`
`110`		`- // {`
`111`		`- // // TODO NH: reset autocommit`
`112`		`- // // TransactionManager.SetAutoCommit(connection, true);`
`113`		`- // }`
`114`		`- // catch (Exception)`
`115`		`- // {`
`116`		`- // log.Debug("was unable to reset connection back to auto-commit");`
`117`		`- // }`
`118`		`- //}`
`119`		`-`
`120`	`94`	`try`
`121`	`95`	`{`
`122`	`96`	`trans?.Dispose();`
`123`	`97`	`}`
`124`	`98`	`catch (Exception ignore)`
`125`	`99`	`{`
`126`		`- isolaterLog.Warn(ignore, "Unable to dispose transaction");`
	`100`	`+ _isolatorLog.Warn(ignore, "Unable to dispose transaction");`
`127`	`101`	`}`
`128`	`102`
`129`		`- if (session.Factory.Dialect is SQLiteDialect == false)`
	`103`	`+ if (connection != null && session.Factory.Dialect is SQLiteDialect == false)`
`130`	`104`	`session.Factory.ConnectionProvider.CloseConnection(connection);`
`131`	`105`	`}`
`132`	`106`	`}`
Original file line number	Diff line number	Diff line change
`@@ -24,6 +24,9 @@ public abstract partial class ConnectionProvider : IConnectionProvider`
`24`	`24`	`/// <param name="conn">The <see cref="DbConnection"/> to clean up.</param>`
`25`	`25`	`public virtual void CloseConnection(DbConnection conn)`
`26`	`26`	`{`
	`27`	`+ if (conn == null)`
	`28`	`+ throw new ArgumentNullException(nameof(conn));`
	`29`	`+`
`27`	`30`	`log.Debug("Closing connection");`
`28`	`31`	`try`
`29`	`32`	`{`
Original file line number	Diff line number	Diff line change
`@@ -6,7 +6,6 @@`
`6`	`6`	`using NHibernate.Engine;`
`7`	`7`	`using NHibernate.Engine.Transaction;`
`8`	`8`	`using NHibernate.Exceptions;`
`9`		`-using NHibernate.Impl;`
`10`	`9`
`11`	`10`	`namespace NHibernate.Transaction`
`12`	`11`	`{`
`@@ -16,7 +15,7 @@ namespace NHibernate.Transaction`
`16`	`15`	`/// </summary>`
`17`	`16`	`public partial class AdoNetTransactionFactory : ITransactionFactory`
`18`	`17`	`{`
`19`		`- private readonly INHibernateLogger isolaterLog = NHibernateLogger.For(typeof(ITransactionFactory));`
	`18`	`+ private static readonly INHibernateLogger _isolatorLog = NHibernateLogger.For(typeof(ITransactionFactory));`
`20`	`19`
`21`	`20`	`/// <inheritdoc />`
`22`	`21`	`public virtual ITransaction CreateTransaction(ISessionImplementor session)`
`@@ -52,33 +51,24 @@ public virtual void ExecuteWorkInIsolation(ISessionImplementor session, IIsolate`
`52`	`51`
`53`	`52`	`DbConnection connection = null;`
`54`	`53`	`DbTransaction trans = null;`
`55`		`- // bool wasAutoCommit = false;`
`56`	`54`	`try`
`57`	`55`	`{`
`58`	`56`	`// We make an exception for SQLite and use the session's connection,`
`59`	`57`	`// since SQLite only allows one connection to the database.`
`60`		`- if (session.Factory.Dialect is SQLiteDialect)`
`61`		`- connection = session.Connection;`
`62`		`- else`
`63`		`- connection = session.Factory.ConnectionProvider.GetConnection();`
	`58`	`+ connection = session.Factory.Dialect is SQLiteDialect`
	`59`	`+ ? session.Connection`
	`60`	`+ : session.Factory.ConnectionProvider.GetConnection();`
`64`	`61`
`65`	`62`	`if (transacted)`
`66`	`63`	`{`
`67`	`64`	`trans = connection.BeginTransaction();`
`68`		`- // TODO NH: a way to read the autocommit state is needed`
`69`		`- //if (TransactionManager.GetAutoCommit(connection))`
`70`		`- //{`
`71`		`- // wasAutoCommit = true;`
`72`		`- // TransactionManager.SetAutoCommit(connection, false);`
`73`		`- //}`
`74`	`65`	`}`
`75`	`66`
`76`	`67`	`work.DoWork(connection, trans);`
`77`	`68`
`78`	`69`	`if (transacted)`
`79`	`70`	`{`
`80`	`71`	`trans.Commit();`
`81`		`- //TransactionManager.Commit(connection);`
`82`	`72`	`}`
`83`	`73`	`}`
`84`	`74`	`catch (Exception t)`
`@@ -94,49 +84,33 @@ public virtual void ExecuteWorkInIsolation(ISessionImplementor session, IIsolate`
`94`	`84`	`}`
`95`	`85`	`catch (Exception ignore)`
`96`	`86`	`{`
`97`		`- isolaterLog.Debug(ignore, "Unable to rollback transaction");`
	`87`	`+ _isolatorLog.Debug(ignore, "Unable to rollback transaction");`
`98`	`88`	`}`
`99`	`89`
`100`		`- if (t is HibernateException)`
	`90`	`+ switch (t)`
`101`	`91`	`{`
`102`		`- throw;`
`103`		`- }`
`104`		`- else if (t is DbException)`
`105`		`- {`
`106`		`- throw ADOExceptionHelper.Convert(session.Factory.SQLExceptionConverter, t,`
`107`		`- "error performing isolated work");`
`108`		`- }`
`109`		`- else`
`110`		`- {`
`111`		`- throw new HibernateException("error performing isolated work", t);`
	`92`	`+ case HibernateException _:`
	`93`	`+ throw;`
	`94`	`+ case DbException _:`
	`95`	`+ throw ADOExceptionHelper.Convert(session.Factory.SQLExceptionConverter, t,`
	`96`	`+ "error performing isolated work");`
	`97`	`+ default:`
	`98`	`+ throw new HibernateException("error performing isolated work", t);`
`112`	`99`	`}`
`113`	`100`	`}`
`114`	`101`	`}`
`115`	`102`	`finally`
`116`	`103`	`{`
`117`		`- //if (transacted && wasAutoCommit)`
`118`		`- //{`
`119`		`- // try`
`120`		`- // {`
`121`		`- // // TODO NH: reset autocommit`
`122`		`- // // TransactionManager.SetAutoCommit(connection, true);`
`123`		`- // }`
`124`		`- // catch (Exception)`
`125`		`- // {`
`126`		`- // log.Debug("was unable to reset connection back to auto-commit");`
`127`		`- // }`
`128`		`- //}`
`129`		`-`
`130`	`104`	`try`
`131`	`105`	`{`
`132`	`106`	`trans?.Dispose();`
`133`	`107`	`}`
`134`	`108`	`catch (Exception ignore)`
`135`	`109`	`{`
`136`		`- isolaterLog.Warn(ignore, "Unable to dispose transaction");`
	`110`	`+ _isolatorLog.Warn(ignore, "Unable to dispose transaction");`
`137`	`111`	`}`
`138`	`112`
`139`		`- if (session.Factory.Dialect is SQLiteDialect == false)`
	`113`	`+ if (connection != null && session.Factory.Dialect is SQLiteDialect == false)`
`140`	`114`	`session.Factory.ConnectionProvider.CloseConnection(connection);`
`141`	`115`	`}`
`142`	`116`	`}`