Reduce SessionIdLoggingContext creation (#1984)

Author: bahusoid

src/NHibernate.Test/NHSpecificTest/GH1391/Fixture.cs

@@ -13,6 +13,12 @@ namespace NHibernate.Test.NHSpecificTest.GH1391
 	public class Fixture
 	{
 		private readonly Random _random = new Random();
+
+		[OneTimeSetUp]
+		public void Test()
+		{
+			SessionIdLoggingContext.SessionId = null;
+		}
 
 		[Test]
 		public void Concurrent()
@@ -56,11 +62,11 @@ async Task RunAsync(bool enabled)
 		public void Enabled()
 		{
 			var guid = Guid.NewGuid();
-			using (new SessionIdLoggingContext(guid))
+			using (SessionIdLoggingContext.CreateOrNull(guid))
 			{
 				Assert.That(SessionIdLoggingContext.SessionId, Is.EqualTo(guid));
 				var guid2 = Guid.NewGuid();
-				using (new SessionIdLoggingContext(guid2))
+				using (SessionIdLoggingContext.CreateOrNull(guid2))
 				{
 					Assert.That(SessionIdLoggingContext.SessionId, Is.EqualTo(guid2));
 				}
@@ -73,14 +79,14 @@ public void Enabled()
 		public async Task EnabledAsync()
 		{
 			var guid = Guid.NewGuid();
-			using (new SessionIdLoggingContext(guid))
+			using (SessionIdLoggingContext.CreateOrNull(guid))
 			{
 				Assert.That(SessionIdLoggingContext.SessionId, Is.EqualTo(guid));
 				await Task.Delay(1).ConfigureAwait(false);
 				Assert.That(SessionIdLoggingContext.SessionId, Is.EqualTo(guid));
 
 				var guid2 = Guid.NewGuid();
-				using (new SessionIdLoggingContext(guid2))
+				using (SessionIdLoggingContext.CreateOrNull(guid2))
 				{
 					Assert.That(SessionIdLoggingContext.SessionId, Is.EqualTo(guid2));
 					await Task.Delay(1).ConfigureAwait(false);
@@ -95,10 +101,10 @@ public async Task EnabledAsync()
 		public void Disabled()
 		{
 			var guid = Guid.Empty;
-			using (new SessionIdLoggingContext(guid))
+			using (SessionIdLoggingContext.CreateOrNull(guid))
 			{
 				Assert.That(SessionIdLoggingContext.SessionId, Is.Null);
-				using (new SessionIdLoggingContext(guid))
+				using (SessionIdLoggingContext.CreateOrNull(guid))
 				{
 					Assert.That(SessionIdLoggingContext.SessionId, Is.Null);
 				}
@@ -111,13 +117,13 @@ public void Disabled()
 		public async Task DisabledAsync()
 		{
 			var guid = Guid.Empty;
-			using (new SessionIdLoggingContext(guid))
+			using (SessionIdLoggingContext.CreateOrNull(guid))
 			{
 				Assert.That(SessionIdLoggingContext.SessionId, Is.Null);
 				await Task.Delay(1).ConfigureAwait(false);
 				Assert.That(SessionIdLoggingContext.SessionId, Is.Null);
 
-				using (new SessionIdLoggingContext(guid))
+				using (SessionIdLoggingContext.CreateOrNull(guid))
 				{
 					Assert.That(SessionIdLoggingContext.SessionId, Is.Null);
 					await Task.Delay(1).ConfigureAwait(false);

src/NHibernate.Test/NHSpecificTest/Logs/LogsFixture.cs

@@ -201,13 +201,13 @@ public void WillGetSessionIdFromSessionLogsConcurrent()
 		}
 
 		[Test]
-		public void WillCleanlyFailOnDoubleProcessDispose()
+		public void DoubleProcessDisposeIsAllowed()
 		{
 			using (var s = OpenSession())
 			{
 				var p = ((AbstractSessionImpl) s).BeginProcess();
 				p.Dispose();
-				Assert.That(() => p.Dispose(), Throws.TypeOf<ObjectDisposedException>());
+				Assert.That(() => p.Dispose(), Throws.Nothing);
 			}
 		}
 

src/NHibernate/Async/Transaction/AdoTransaction.cs

@@ -105,7 +105,7 @@ private async Task AfterTransactionCompletionAsync(bool successful, Cancellation
 		public async Task RollbackAsync(CancellationToken cancellationToken = default(CancellationToken))
 		{
 			cancellationToken.ThrowIfCancellationRequested();
-			using (new SessionIdLoggingContext(sessionId))
+			using (SessionIdLoggingContext.CreateOrNull(sessionId))
 			{
 				CheckNotDisposed();
 				CheckBegun();
@@ -157,7 +157,7 @@ private async Task AfterTransactionCompletionAsync(bool successful, Cancellation
 		protected virtual async Task DisposeAsync(bool isDisposing, CancellationToken cancellationToken)
 		{
 			cancellationToken.ThrowIfCancellationRequested();
-			using (new SessionIdLoggingContext(sessionId))
+			using (SessionIdLoggingContext.CreateOrNull(sessionId))
 			{
 				if (_isAlreadyDisposed)
 				{

src/NHibernate/Engine/ISessionImplementor.cs

@@ -25,17 +25,20 @@ internal static IDisposable BeginContext(this ISessionImplementor session)
 		{
 			if (session == null)
 				return null;
-			return (session as AbstractSessionImpl)?.BeginContext() ?? new SessionIdLoggingContext(session.SessionId);
+			return session is AbstractSessionImpl impl
+				? impl.BeginContext()
+				: SessionIdLoggingContext.CreateOrNull(session.SessionId);
 		}
 
 		internal static IDisposable BeginProcess(this ISessionImplementor session)
 		{
 			if (session == null)
 				return null;
-			return (session as AbstractSessionImpl)?.BeginProcess() ??
+			return session is AbstractSessionImpl impl
+				? impl.BeginProcess()
 				// This method has only replaced bare call to setting the id, so this fallback is enough for avoiding a
 				// breaking change in case a custom session implementation is used.
-				new SessionIdLoggingContext(session.SessionId);
+				: SessionIdLoggingContext.CreateOrNull(session.SessionId);
 		}
 
 		//6.0 TODO: Expose as ISessionImplementor.FutureBatch and replace method usages with property

src/NHibernate/Impl/AbstractSessionImpl.cs

@@ -358,7 +358,7 @@ public bool IsClosed
 		/// </returns>
 		public IDisposable BeginProcess()
 		{
-			return _processing ? null : new ProcessHelper(this);
+			return _processHelper.BeginProcess(this);
 		}
 
 		/// <summary>
@@ -370,48 +370,56 @@ public IDisposable BeginProcess()
 		/// </returns>
 		public IDisposable BeginContext()
 		{
-			return _processing ? null : new SessionIdLoggingContext(SessionId);
+			return _processHelper.Processing ? null : SessionIdLoggingContext.CreateOrNull(SessionId);
 		}
 
-		[NonSerialized]
-		private bool _processing;
+		private ProcessHelper _processHelper = new ProcessHelper();
 
+		[Serializable]
 		private sealed class ProcessHelper : IDisposable
 		{
-			private AbstractSessionImpl _session;
-			private SessionIdLoggingContext _context;
+			[NonSerialized]
+			private IDisposable _context;
+
+			[NonSerialized]
+			private bool _processing;
 
-			public ProcessHelper(AbstractSessionImpl session)
+			public ProcessHelper()
 			{
-				_session = session;
-				_context = new SessionIdLoggingContext(session.SessionId);
+			}
+
+			public bool Processing { get => _processing; }
+
+			public IDisposable BeginProcess(AbstractSessionImpl session)
+			{
+				if (_processing)
+					return null;
+
 				try
 				{
-					_session.CheckAndUpdateSessionStatus();
-					_session._processing = true;
+					_context = SessionIdLoggingContext.CreateOrNull(session.SessionId);
+					session.CheckAndUpdateSessionStatus();
+					_processing = true;
 				}
 				catch
 				{
-					_context.Dispose();
-					_context = null;
+					Dispose();
 					throw;
 				}
+				return this;
 			}
 
 			public void Dispose()
 			{
 				_context?.Dispose();
 				_context = null;
-				if (_session == null)
-					throw new ObjectDisposedException("The session process helper has been disposed already");
-				_session._processing = false;
-				_session = null;
+				_processing = false;
 			}
 		}
 
 		protected internal virtual void CheckAndUpdateSessionStatus()
 		{
-			if (_processing)
+			if (_processHelper.Processing)
 				return;
 
 			ErrorIfClosed();

src/NHibernate/Impl/SessionIdLoggingContext.cs

@@ -15,18 +15,38 @@ public class SessionIdLoggingContext : IDisposable
 #else
 		private const string LogicalCallContextVariableName = "__" + nameof(SessionIdLoggingContext) + "__";
 #endif
-		private readonly Guid? _oldSessonId;
-		private readonly bool _hasChanged;
-		
+		private readonly Guid? _oldSessionId;
+		private bool _hasChanged;
+
+		[Obsolete("Please use SessionIdLoggingContext.CreateOrNull instead.")]
 		public SessionIdLoggingContext(Guid id)
 		{
 			if (id == Guid.Empty) return;
-			_oldSessonId = SessionId;
-			if (id == _oldSessonId) return;
+			_oldSessionId = SessionId;
+			if (id == _oldSessionId) return;
 			_hasChanged = true;
 			SessionId = id;
 		}
 
+		private SessionIdLoggingContext(Guid newId, Guid? oldId)
+		{
+			SessionId = newId;
+			_oldSessionId = oldId;
+			_hasChanged = true;
+		}
+
+		public static IDisposable CreateOrNull(Guid id)
+		{
+			if (id == Guid.Empty)
+				return null;
+			var oldId = SessionId;
+
+			if (oldId == id)
+				return null;
+
+			return new SessionIdLoggingContext(id, oldId);
+		}
+
 		/// <summary>
 		/// We always set the result to use an async local variable, on the face of it,
 		/// it looks like it is not a valid choice, since ASP.Net and WCF may decide to switch
@@ -58,7 +78,8 @@ public void Dispose()
 		{
 			if (_hasChanged)
 			{
-				SessionId = _oldSessonId;
+				SessionId = _oldSessionId;
+				_hasChanged = false;
 			}
 		}
 	}

src/NHibernate/Impl/SessionImpl.cs

@@ -183,7 +183,11 @@ void IDeserializationCallback.OnDeserialization(object sender)
 		internal SessionImpl(SessionFactoryImpl factory, ISessionCreationOptions options)
 			: base(factory, options)
 		{
-			using (BeginContext())
+			// This context is disposed only on session own disposal. This greatly reduces the number of context switches
+			// for most usual session usages. It may cause an irrelevant session id to be set back on disposal, but since all
+			// session entry points are supposed to set it, it should not have any consequences.
+			_context = SessionIdLoggingContext.CreateOrNull(SessionId);
+			try
 			{
 				actionQueue = new ActionQueue(this);
 				persistenceContext = new StatefulPersistenceContext(this);
@@ -203,6 +207,11 @@ internal SessionImpl(SessionFactoryImpl factory, ISessionCreationOptions options
 
 				CheckAndUpdateSessionStatus();
 			}
+			catch
+			{
+				_context?.Dispose();
+				throw;
+			}
 		}
 
 		//Since 5.2
@@ -1473,6 +1482,7 @@ public void Reconnect(DbConnection conn)
 		#region System.IDisposable Members
 
 		private string fetchProfile;
+		private IDisposable _context;
 
 		/// <summary>
 		/// Finalizer that ensures the object is correctly disposed of.
@@ -1505,6 +1515,7 @@ public void Dispose()
 				}
 				Dispose(true);
 			}
+			_context?.Dispose();
 		}
 
 		/// <summary>

src/NHibernate/Impl/StatelessSessionImpl.cs

@@ -31,7 +31,11 @@ public partial class StatelessSessionImpl : AbstractSessionImpl, IStatelessSessi
 		internal StatelessSessionImpl(SessionFactoryImpl factory, ISessionCreationOptions options)
 			: base(factory, options)
 		{
-			using (BeginContext())
+			// This context is disposed only on session own disposal. This greatly reduces the number of context switches
+			// for most usual session usages. It may cause an irrelevant session id to be set back on disposal, but since all
+			// session entry points are supposed to set it, it should not have any consequences.
+			_context = SessionIdLoggingContext.CreateOrNull(SessionId);
+			try
 			{
 				temporaryPersistenceContext = new StatefulPersistenceContext(this);
 
@@ -43,6 +47,11 @@ internal StatelessSessionImpl(SessionFactoryImpl factory, ISessionCreationOption
 
 				CheckAndUpdateSessionStatus();
 			}
+			catch
+			{
+				_context?.Dispose();
+				throw;
+			}
 		}
 
 		public override void InitializeCollection(IPersistentCollection collection, bool writing)
@@ -748,6 +757,7 @@ public IQueryOver<T, T> QueryOver<T>(Expression<Func<T>> alias) where T : class
 		#region IDisposable Members
 
 		private bool _isAlreadyDisposed;
+		private IDisposable _context;
 
 		/// <summary>
 		/// Finalizer that ensures the object is correctly disposed of.
@@ -806,6 +816,7 @@ protected void Dispose(bool isDisposing)
 				// nothing for Finalizer to do - so tell the GC to ignore it
 				GC.SuppressFinalize(this);
 			}
+			_context?.Dispose();
 		}
 
 		#endregion

src/NHibernate/Transaction/AdoTransaction.cs

@@ -246,7 +246,7 @@ public void Commit()
 		/// </exception>
 		public void Rollback()
 		{
-			using (new SessionIdLoggingContext(sessionId))
+			using (SessionIdLoggingContext.CreateOrNull(sessionId))
 			{
 				CheckNotDisposed();
 				CheckBegun();
@@ -363,7 +363,7 @@ public void Dispose()
 		/// </remarks>
 		protected virtual void Dispose(bool isDisposing)
 		{
-			using (new SessionIdLoggingContext(sessionId))
+			using (SessionIdLoggingContext.CreateOrNull(sessionId))
 			{
 				if (_isAlreadyDisposed)
 				{