Skip to content

Commit 3dcb91b

Browse files
bahusoidfredericDelaporte
bahusoid
authored and
fredericDelaporte
committed
Reduce SessionIdLoggingContext creation (#1984)
1 parent 5144a57 commit 3dcb91b

File tree

9 files changed

+103
-43
lines changed

9 files changed

+103
-43
lines changed

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

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,12 @@ namespace NHibernate.Test.NHSpecificTest.GH1391
1313
public class Fixture
1414
{
1515
private readonly Random _random = new Random();
16+
17+
[OneTimeSetUp]
18+
public void Test()
19+
{
20+
SessionIdLoggingContext.SessionId = null;
21+
}
1622

1723
[Test]
1824
public void Concurrent()
@@ -56,11 +62,11 @@ async Task RunAsync(bool enabled)
5662
public void Enabled()
5763
{
5864
var guid = Guid.NewGuid();
59-
using (new SessionIdLoggingContext(guid))
65+
using (SessionIdLoggingContext.CreateOrNull(guid))
6066
{
6167
Assert.That(SessionIdLoggingContext.SessionId, Is.EqualTo(guid));
6268
var guid2 = Guid.NewGuid();
63-
using (new SessionIdLoggingContext(guid2))
69+
using (SessionIdLoggingContext.CreateOrNull(guid2))
6470
{
6571
Assert.That(SessionIdLoggingContext.SessionId, Is.EqualTo(guid2));
6672
}
@@ -73,14 +79,14 @@ public void Enabled()
7379
public async Task EnabledAsync()
7480
{
7581
var guid = Guid.NewGuid();
76-
using (new SessionIdLoggingContext(guid))
82+
using (SessionIdLoggingContext.CreateOrNull(guid))
7783
{
7884
Assert.That(SessionIdLoggingContext.SessionId, Is.EqualTo(guid));
7985
await Task.Delay(1).ConfigureAwait(false);
8086
Assert.That(SessionIdLoggingContext.SessionId, Is.EqualTo(guid));
8187

8288
var guid2 = Guid.NewGuid();
83-
using (new SessionIdLoggingContext(guid2))
89+
using (SessionIdLoggingContext.CreateOrNull(guid2))
8490
{
8591
Assert.That(SessionIdLoggingContext.SessionId, Is.EqualTo(guid2));
8692
await Task.Delay(1).ConfigureAwait(false);
@@ -95,10 +101,10 @@ public async Task EnabledAsync()
95101
public void Disabled()
96102
{
97103
var guid = Guid.Empty;
98-
using (new SessionIdLoggingContext(guid))
104+
using (SessionIdLoggingContext.CreateOrNull(guid))
99105
{
100106
Assert.That(SessionIdLoggingContext.SessionId, Is.Null);
101-
using (new SessionIdLoggingContext(guid))
107+
using (SessionIdLoggingContext.CreateOrNull(guid))
102108
{
103109
Assert.That(SessionIdLoggingContext.SessionId, Is.Null);
104110
}
@@ -111,13 +117,13 @@ public void Disabled()
111117
public async Task DisabledAsync()
112118
{
113119
var guid = Guid.Empty;
114-
using (new SessionIdLoggingContext(guid))
120+
using (SessionIdLoggingContext.CreateOrNull(guid))
115121
{
116122
Assert.That(SessionIdLoggingContext.SessionId, Is.Null);
117123
await Task.Delay(1).ConfigureAwait(false);
118124
Assert.That(SessionIdLoggingContext.SessionId, Is.Null);
119125

120-
using (new SessionIdLoggingContext(guid))
126+
using (SessionIdLoggingContext.CreateOrNull(guid))
121127
{
122128
Assert.That(SessionIdLoggingContext.SessionId, Is.Null);
123129
await Task.Delay(1).ConfigureAwait(false);

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -201,13 +201,13 @@ public void WillGetSessionIdFromSessionLogsConcurrent()
201201
}
202202

203203
[Test]
204-
public void WillCleanlyFailOnDoubleProcessDispose()
204+
public void DoubleProcessDisposeIsAllowed()
205205
{
206206
using (var s = OpenSession())
207207
{
208208
var p = ((AbstractSessionImpl) s).BeginProcess();
209209
p.Dispose();
210-
Assert.That(() => p.Dispose(), Throws.TypeOf<ObjectDisposedException>());
210+
Assert.That(() => p.Dispose(), Throws.Nothing);
211211
}
212212
}
213213

‎src/NHibernate/Async/Transaction/AdoTransaction.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ private async Task AfterTransactionCompletionAsync(bool successful, Cancellation
105105
public async Task RollbackAsync(CancellationToken cancellationToken = default(CancellationToken))
106106
{
107107
cancellationToken.ThrowIfCancellationRequested();
108-
using (new SessionIdLoggingContext(sessionId))
108+
using (SessionIdLoggingContext.CreateOrNull(sessionId))
109109
{
110110
CheckNotDisposed();
111111
CheckBegun();
@@ -157,7 +157,7 @@ private async Task AfterTransactionCompletionAsync(bool successful, Cancellation
157157
protected virtual async Task DisposeAsync(bool isDisposing, CancellationToken cancellationToken)
158158
{
159159
cancellationToken.ThrowIfCancellationRequested();
160-
using (new SessionIdLoggingContext(sessionId))
160+
using (SessionIdLoggingContext.CreateOrNull(sessionId))
161161
{
162162
if (_isAlreadyDisposed)
163163
{

‎src/NHibernate/Engine/ISessionImplementor.cs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,17 +25,20 @@ internal static IDisposable BeginContext(this ISessionImplementor session)
2525
{
2626
if (session == null)
2727
return null;
28-
return (session as AbstractSessionImpl)?.BeginContext() ?? new SessionIdLoggingContext(session.SessionId);
28+
return session is AbstractSessionImpl impl
29+
? impl.BeginContext()
30+
: SessionIdLoggingContext.CreateOrNull(session.SessionId);
2931
}
3032

3133
internal static IDisposable BeginProcess(this ISessionImplementor session)
3234
{
3335
if (session == null)
3436
return null;
35-
return (session as AbstractSessionImpl)?.BeginProcess() ??
37+
return session is AbstractSessionImpl impl
38+
? impl.BeginProcess()
3639
// This method has only replaced bare call to setting the id, so this fallback is enough for avoiding a
3740
// breaking change in case a custom session implementation is used.
38-
new SessionIdLoggingContext(session.SessionId);
41+
: SessionIdLoggingContext.CreateOrNull(session.SessionId);
3942
}
4043

4144
//6.0 TODO: Expose as ISessionImplementor.FutureBatch and replace method usages with property

‎src/NHibernate/Impl/AbstractSessionImpl.cs

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,7 @@ public bool IsClosed
358358
/// </returns>
359359
public IDisposable BeginProcess()
360360
{
361-
return _processing ? null : new ProcessHelper(this);
361+
return _processHelper.BeginProcess(this);
362362
}
363363

364364
/// <summary>
@@ -370,48 +370,56 @@ public IDisposable BeginProcess()
370370
/// </returns>
371371
public IDisposable BeginContext()
372372
{
373-
return _processing ? null : new SessionIdLoggingContext(SessionId);
373+
return _processHelper.Processing ? null : SessionIdLoggingContext.CreateOrNull(SessionId);
374374
}
375375

376-
[NonSerialized]
377-
private bool _processing;
376+
private ProcessHelper _processHelper = new ProcessHelper();
378377

378+
[Serializable]
379379
private sealed class ProcessHelper : IDisposable
380380
{
381-
private AbstractSessionImpl _session;
382-
private SessionIdLoggingContext _context;
381+
[NonSerialized]
382+
private IDisposable _context;
383+
384+
[NonSerialized]
385+
private bool _processing;
383386

384-
public ProcessHelper(AbstractSessionImpl session)
387+
public ProcessHelper()
385388
{
386-
_session = session;
387-
_context = new SessionIdLoggingContext(session.SessionId);
389+
}
390+
391+
public bool Processing { get => _processing; }
392+
393+
public IDisposable BeginProcess(AbstractSessionImpl session)
394+
{
395+
if (_processing)
396+
return null;
397+
388398
try
389399
{
390-
_session.CheckAndUpdateSessionStatus();
391-
_session._processing = true;
400+
_context = SessionIdLoggingContext.CreateOrNull(session.SessionId);
401+
session.CheckAndUpdateSessionStatus();
402+
_processing = true;
392403
}
393404
catch
394405
{
395-
_context.Dispose();
396-
_context = null;
406+
Dispose();
397407
throw;
398408
}
409+
return this;
399410
}
400411

401412
public void Dispose()
402413
{
403414
_context?.Dispose();
404415
_context = null;
405-
if (_session == null)
406-
throw new ObjectDisposedException("The session process helper has been disposed already");
407-
_session._processing = false;
408-
_session = null;
416+
_processing = false;
409417
}
410418
}
411419

412420
protected internal virtual void CheckAndUpdateSessionStatus()
413421
{
414-
if (_processing)
422+
if (_processHelper.Processing)
415423
return;
416424

417425
ErrorIfClosed();

‎src/NHibernate/Impl/SessionIdLoggingContext.cs

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,18 +15,38 @@ public class SessionIdLoggingContext : IDisposable
1515
#else
1616
private const string LogicalCallContextVariableName = "__" + nameof(SessionIdLoggingContext) + "__";
1717
#endif
18-
private readonly Guid? _oldSessonId;
19-
private readonly bool _hasChanged;
20-
18+
private readonly Guid? _oldSessionId;
19+
private bool _hasChanged;
20+
21+
[Obsolete("Please use SessionIdLoggingContext.CreateOrNull instead.")]
2122
public SessionIdLoggingContext(Guid id)
2223
{
2324
if (id == Guid.Empty) return;
24-
_oldSessonId = SessionId;
25-
if (id == _oldSessonId) return;
25+
_oldSessionId = SessionId;
26+
if (id == _oldSessionId) return;
2627
_hasChanged = true;
2728
SessionId = id;
2829
}
2930

31+
private SessionIdLoggingContext(Guid newId, Guid? oldId)
32+
{
33+
SessionId = newId;
34+
_oldSessionId = oldId;
35+
_hasChanged = true;
36+
}
37+
38+
public static IDisposable CreateOrNull(Guid id)
39+
{
40+
if (id == Guid.Empty)
41+
return null;
42+
var oldId = SessionId;
43+
44+
if (oldId == id)
45+
return null;
46+
47+
return new SessionIdLoggingContext(id, oldId);
48+
}
49+
3050
/// <summary>
3151
/// We always set the result to use an async local variable, on the face of it,
3252
/// 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()
5878
{
5979
if (_hasChanged)
6080
{
61-
SessionId = _oldSessonId;
81+
SessionId = _oldSessionId;
82+
_hasChanged = false;
6283
}
6384
}
6485
}

‎src/NHibernate/Impl/SessionImpl.cs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,11 @@ void IDeserializationCallback.OnDeserialization(object sender)
183183
internal SessionImpl(SessionFactoryImpl factory, ISessionCreationOptions options)
184184
: base(factory, options)
185185
{
186-
using (BeginContext())
186+
// This context is disposed only on session own disposal. This greatly reduces the number of context switches
187+
// for most usual session usages. It may cause an irrelevant session id to be set back on disposal, but since all
188+
// session entry points are supposed to set it, it should not have any consequences.
189+
_context = SessionIdLoggingContext.CreateOrNull(SessionId);
190+
try
187191
{
188192
actionQueue = new ActionQueue(this);
189193
persistenceContext = new StatefulPersistenceContext(this);
@@ -203,6 +207,11 @@ internal SessionImpl(SessionFactoryImpl factory, ISessionCreationOptions options
203207

204208
CheckAndUpdateSessionStatus();
205209
}
210+
catch
211+
{
212+
_context?.Dispose();
213+
throw;
214+
}
206215
}
207216

208217
//Since 5.2
@@ -1473,6 +1482,7 @@ public void Reconnect(DbConnection conn)
14731482
#region System.IDisposable Members
14741483

14751484
private string fetchProfile;
1485+
private IDisposable _context;
14761486

14771487
/// <summary>
14781488
/// Finalizer that ensures the object is correctly disposed of.
@@ -1505,6 +1515,7 @@ public void Dispose()
15051515
}
15061516
Dispose(true);
15071517
}
1518+
_context?.Dispose();
15081519
}
15091520

15101521
/// <summary>

‎src/NHibernate/Impl/StatelessSessionImpl.cs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,11 @@ public partial class StatelessSessionImpl : AbstractSessionImpl, IStatelessSessi
3131
internal StatelessSessionImpl(SessionFactoryImpl factory, ISessionCreationOptions options)
3232
: base(factory, options)
3333
{
34-
using (BeginContext())
34+
// This context is disposed only on session own disposal. This greatly reduces the number of context switches
35+
// for most usual session usages. It may cause an irrelevant session id to be set back on disposal, but since all
36+
// session entry points are supposed to set it, it should not have any consequences.
37+
_context = SessionIdLoggingContext.CreateOrNull(SessionId);
38+
try
3539
{
3640
temporaryPersistenceContext = new StatefulPersistenceContext(this);
3741

@@ -43,6 +47,11 @@ internal StatelessSessionImpl(SessionFactoryImpl factory, ISessionCreationOption
4347

4448
CheckAndUpdateSessionStatus();
4549
}
50+
catch
51+
{
52+
_context?.Dispose();
53+
throw;
54+
}
4655
}
4756

4857
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
748757
#region IDisposable Members
749758

750759
private bool _isAlreadyDisposed;
760+
private IDisposable _context;
751761

752762
/// <summary>
753763
/// Finalizer that ensures the object is correctly disposed of.
@@ -806,6 +816,7 @@ protected void Dispose(bool isDisposing)
806816
// nothing for Finalizer to do - so tell the GC to ignore it
807817
GC.SuppressFinalize(this);
808818
}
819+
_context?.Dispose();
809820
}
810821

811822
#endregion

‎src/NHibernate/Transaction/AdoTransaction.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,7 @@ public void Commit()
246246
/// </exception>
247247
public void Rollback()
248248
{
249-
using (new SessionIdLoggingContext(sessionId))
249+
using (SessionIdLoggingContext.CreateOrNull(sessionId))
250250
{
251251
CheckNotDisposed();
252252
CheckBegun();
@@ -363,7 +363,7 @@ public void Dispose()
363363
/// </remarks>
364364
protected virtual void Dispose(bool isDisposing)
365365
{
366-
using (new SessionIdLoggingContext(sessionId))
366+
using (SessionIdLoggingContext.CreateOrNull(sessionId))
367367
{
368368
if (_isAlreadyDisposed)
369369
{

0 commit comments

Comments
 (0)