Fix some NRE in work isolation and connection handling #2337

fredericDelaporte · 2020-03-22T17:58:10Z

fredericDelaporte · 2020-03-22T18:00:21Z

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));


We can choose to just exit instead of throwing. But since the case was clearly not supported, causing a NRE in the try then a second NRE in the catch, I consider the current code base is never meant to supply null to this method, so it looks better to throw.

bahusoid · 2020-03-22T18:20:23Z

src/NHibernate/Transaction/AdoNetTransactionFactory.cs

@@ -136,7 +136,7 @@ public virtual void ExecuteWorkInIsolation(ISessionImplementor session, IIsolate
 					isolaterLog.Warn(ignore, "Unable to dispose transaction");
 				}

-				if (session.Factory.Dialect is SQLiteDialect == false)
+				if (connection != null && session.Factory.Dialect is SQLiteDialect == false)


That's some messy method here.... Additional null check doesn't make it better. May be instead move connection retrieving logic outside of try block?

Well maybe not.. That would change exception..

At least this will no more swallow a connection opening failure. But yes, it is indeed better to put the connection opening out of the try. It has no sense being into it.

Well... Changing from NRE unrelated to the trouble to the bare exception is still better.
But yes, it loses the HibernateException encapsulation.

Switching back.

I've just realized that this exception never was wrapped (my main concern was that we changed existing behavior - but we didn't) and I really don't see any benefits in such wrappings (seems like some legacy practice). So maybe better again switch back? )

fredericDelaporte · 2020-03-22T18:38:29Z

src/NHibernate/Async/Transaction/AdoNetTransactionFactory.cs

 				DbTransaction trans = null;
-				// bool wasAutoCommit = false;


I have added a small cleanup commit.
Afaik, the auto-commit feature and the transaction manager are not ported into NHibernate, so those decade old comments and todo-s are unlikely to be done any time soon. They are just polluting the code in my view.

fredericDelaporte · 2020-03-22T18:40:13Z

src/NHibernate/Async/Transaction/AdoNetTransactionFactory.cs

+				// We make an exception for SQLite and use the session's connection,
+				// since SQLite only allows one connection to the database.
+				var connection = session.Factory.Dialect is SQLiteDialect
+				? session.Connection


@maca88, small indentation glitch here with async generator, that you may be interested to look into. Nothing important, it is just for polishing it if you wish.

Fixed in #2363.

fredericDelaporte · 2020-03-22T18:44:00Z

src/NHibernate/Async/Transaction/AdoNetTransactionFactory.cs

-							throw new HibernateException("error performing isolated work", t);
-						}
+						switch (t)
+					{


Another indentation glitch.

fredericDelaporte · 2020-03-22T19:06:31Z

Back to initial change, forced push to remove fixup noise.

Fixes nhibernate#2336

bahusoid

I would prefer to have connection opening be outside of try block but feel free to keep it as is

bahusoid · 2020-03-22T18:47:47Z

src/NHibernate/Transaction/AdoNetTransactionFactory.cs

@@ -16,7 +15,7 @@ namespace NHibernate.Transaction
 	/// </summary>
 	public partial class AdoNetTransactionFactory : ITransactionFactory
 	{
-		private readonly INHibernateLogger isolaterLog = NHibernateLogger.For(typeof(ITransactionFactory));
+		private readonly INHibernateLogger _isolatorLog = NHibernateLogger.For(typeof(ITransactionFactory));


Shouldn't it be static?

bahusoid · 2020-03-24T16:35:38Z

src/NHibernate/Transaction/AdoNetTransactionFactory.cs

@@ -136,7 +136,7 @@ public virtual void ExecuteWorkInIsolation(ISessionImplementor session, IIsolate
 					isolaterLog.Warn(ignore, "Unable to dispose transaction");
 				}

-				if (session.Factory.Dialect is SQLiteDialect == false)
+				if (connection != null && session.Factory.Dialect is SQLiteDialect == false)


I've just realized that this exception never was wrapped (my main concern was that we changed existing behavior - but we didn't) and I really don't see any benefits in such wrappings (seems like some legacy practice). So maybe better again switch back? )

fredericDelaporte · 2020-03-24T19:54:47Z

Yes it could not be wrapped since the finally was failing with NRE.
But this is the usual practice of NHibernate: wrap DB exceptions into NHibernateException. That is why when you have signaled the wrapping would not be done, I have reverted even though the wrapping for this case was failing previously.
For consistency, I think we should have this wrapping. So when fixing bugs in this very wrapping, we should not by-pass it.

bahusoid · 2020-03-24T20:09:24Z

For consistency, I think we should have this wrapping.

I'm very suspicious that OpenSession connection opening problems would be wrapped in HibernateException. So this consistency is not obvious to me and IMHO code looks cleaner without those wrappings but well use your judging here...

fredericDelaporte · 2020-03-24T20:09:55Z

I have amended the cleanup commit for switching the logger to static.

fredericDelaporte · 2020-03-24T20:15:43Z

OpenSession does not have to, it does not open the connection. The connection is lazily opened.
If respecting best practices, the connection will be opened by starting a transaction, by this code in AdoTransaction:

try
{
	trans = session.Factory.ConnectionProvider.Driver.BeginTransaction(isolationLevel, session.Connection);
}
catch (HibernateException)
{
	// Don't wrap HibernateExceptions
	throw;
}
catch (Exception e)
{
	log.Error(e, "Begin transaction failed");
	throw new TransactionException("Begin failed with SQL exception", e);
}

bahusoid · 2020-03-24T20:21:13Z

OpenSession does not have to, it does not open the connection

Ok.. Bad example. But I meant I'm pretty sure that many session methods wouldn't wrap such issue in HibernateException.

fredericDelaporte added the t: Fix label Mar 22, 2020

fredericDelaporte added this to the 5.3 milestone Mar 22, 2020

fredericDelaporte commented Mar 22, 2020

View reviewed changes

bahusoid reviewed Mar 22, 2020

View reviewed changes

fredericDelaporte commented Mar 22, 2020

View reviewed changes

fredericDelaporte force-pushed the FixSomeNre branch from d88025f to 8f45c08 Compare March 22, 2020 19:05

fredericDelaporte force-pushed the FixSomeNre branch from 8f45c08 to f82f953 Compare March 22, 2020 19:14

Fix some NRE in work isolation and connection handling

2b60d20

Fixes nhibernate#2336

fredericDelaporte force-pushed the FixSomeNre branch from f82f953 to 1962361 Compare March 23, 2020 20:34

bahusoid previously approved these changes Mar 24, 2020

View reviewed changes

Clean up a bit isolation work code

8d58808

fredericDelaporte dismissed bahusoid’s stale review via 8d58808 March 24, 2020 20:04

fredericDelaporte force-pushed the FixSomeNre branch from 1962361 to 8d58808 Compare March 24, 2020 20:04

bahusoid approved these changes Mar 24, 2020

View reviewed changes

fredericDelaporte merged commit e7ba6ac into nhibernate:master Mar 27, 2020

fredericDelaporte deleted the FixSomeNre branch March 27, 2020 18:20

fredericDelaporte added the r: Fixed label Mar 27, 2020

maca88 mentioned this pull request Apr 24, 2020

Upgrade AsyncGenerator to 0.18.2 #2363

Merged

Fix some NRE in work isolation and connection handling #2337

Fix some NRE in work isolation and connection handling #2337

Uh oh!

Conversation

fredericDelaporte commented Mar 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Mar 22, 2020

Uh oh!

bahusoid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Mar 24, 2020

Uh oh!

bahusoid commented Mar 24, 2020

Uh oh!

fredericDelaporte commented Mar 24, 2020

Uh oh!

fredericDelaporte commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bahusoid commented Mar 24, 2020

Uh oh!

Uh oh!

fredericDelaporte commented Mar 24, 2020 •

edited

Loading