Skip to content

Commit 5b3ed34

Browse files
fredericDelaportefredericDelaporte
committed
Fix transaction leak
Fixes #2144
1 parent cc6baef commit 5b3ed34

File tree

14 files changed

+305
-43
lines changed

14 files changed

+305
-43
lines changed

‎src/NHibernate.Test/Async/TransactionTest/TransactionFixture.cs

Lines changed: 109 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,8 @@ public async Task FlushFromTransactionAppliesToSharingSessionAsync()
145145

146146
using (var s1 = builder.Interceptor(new TestInterceptor(1, flushOrder)).OpenSession())
147147
using (var s2 = builder.Interceptor(new TestInterceptor(2, flushOrder)).OpenSession())
148-
using (var s3 = s1.SessionWithOptions().Connection().Interceptor(new TestInterceptor(3, flushOrder)).OpenSession())
148+
using (var s3 = s1.SessionWithOptions().Connection().Interceptor(new TestInterceptor(3, flushOrder))
149+
.OpenSession())
149150
using (var t = s.BeginTransaction())
150151
{
151152
var p1 = new Person();
@@ -234,9 +235,115 @@ public async Task WhenCommittingItemsWillAddThemTo2ndLevelCacheAsync()
234235
using (var s = Sfi.WithOptions().Connection(connection).OpenSession())
235236
{
236237
CacheablePerson person = null;
237-
Assert.DoesNotThrowAsync(async () => person = await (s.LoadAsync<CacheablePerson>(id)), "Failed loading entity from second level cache.");
238+
Assert.DoesNotThrowAsync(
239+
async () => person = await (s.LoadAsync<CacheablePerson>(id)),
240+
"Failed loading entity from second level cache.");
238241
Assert.That(person.NotNullData, Is.EqualTo(notNullData));
239242
}
240243
}
244+
245+
[Test]
246+
public async Task CanCommitFromSessionTransactionAsync()
247+
{
248+
int id;
249+
using (var s = OpenSession())
250+
{
251+
Assert.That(s.GetCurrentTransaction(), Is.Null);
252+
using (s.BeginTransaction())
253+
{
254+
var person = new Person();
255+
await (s.SaveAsync(person));
256+
id = person.Id;
257+
258+
await (s.GetCurrentTransaction().CommitAsync());
259+
}
260+
Assert.That(s.GetCurrentTransaction(), Is.Null);
261+
}
262+
263+
using (var s = OpenSession())
264+
using (var t = s.BeginTransaction())
265+
{
266+
var person = await (s.GetAsync<Person>(id));
267+
Assert.That(person, Is.Not.Null);
268+
await (t.CommitAsync());
269+
}
270+
}
271+
272+
[Test]
273+
public async Task CanRollbackFromSessionTransactionAsync()
274+
{
275+
int id;
276+
using (var s = OpenSession())
277+
{
278+
Assert.That(s.GetCurrentTransaction(), Is.Null);
279+
using (s.BeginTransaction())
280+
{
281+
var person = new Person();
282+
await (s.SaveAsync(person));
283+
id = person.Id;
284+
285+
await (s.GetCurrentTransaction().RollbackAsync());
286+
287+
// Need to check before leaving the current using, otherwise the rollback could be the result of the
288+
// disposing.
289+
using (var s2 = OpenSession())
290+
using (var t2 = s2.BeginTransaction())
291+
{
292+
person = await (s2.GetAsync<Person>(id));
293+
Assert.That(person, Is.Null);
294+
await (t2.CommitAsync());
295+
}
296+
}
297+
Assert.That(s.GetCurrentTransaction(), Is.Null);
298+
}
299+
}
300+
301+
[Test, Obsolete]
302+
public async Task CanCommitFromSessionObsoleteTransactionAsync()
303+
{
304+
int id;
305+
using (var s = OpenSession())
306+
using (s.BeginTransaction())
307+
{
308+
var person = new Person();
309+
await (s.SaveAsync(person));
310+
id = person.Id;
311+
312+
await (s.Transaction.CommitAsync());
313+
}
314+
315+
using (var s = OpenSession())
316+
using (var t = s.BeginTransaction())
317+
{
318+
var person = await (s.GetAsync<Person>(id));
319+
Assert.That(person, Is.Not.Null);
320+
await (t.CommitAsync());
321+
}
322+
}
323+
324+
[Test, Obsolete]
325+
public async Task CanRollbackFromSessionObsoleteTransactionAsync()
326+
{
327+
int id;
328+
using (var s = OpenSession())
329+
using (s.BeginTransaction())
330+
{
331+
var person = new Person();
332+
await (s.SaveAsync(person));
333+
id = person.Id;
334+
335+
await (s.Transaction.RollbackAsync());
336+
337+
// Need to check before leaving the current using, otherwise the rollback could be the result of the
338+
// disposing.
339+
using (var s2 = OpenSession())
340+
using (var t2 = s2.BeginTransaction())
341+
{
342+
person = await (s2.GetAsync<Person>(id));
343+
Assert.That(person, Is.Null);
344+
await (t2.CommitAsync());
345+
}
346+
}
347+
}
241348
}
242349
}

‎src/NHibernate.Test/TransactionTest/TransactionFixture.cs

Lines changed: 109 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,8 @@ public void FlushFromTransactionAppliesToSharingSession()
148148

149149
using (var s1 = builder.Interceptor(new TestInterceptor(1, flushOrder)).OpenSession())
150150
using (var s2 = builder.Interceptor(new TestInterceptor(2, flushOrder)).OpenSession())
151-
using (var s3 = s1.SessionWithOptions().Connection().Interceptor(new TestInterceptor(3, flushOrder)).OpenSession())
151+
using (var s3 = s1.SessionWithOptions().Connection().Interceptor(new TestInterceptor(3, flushOrder))
152+
.OpenSession())
152153
using (var t = s.BeginTransaction())
153154
{
154155
var p1 = new Person();
@@ -237,9 +238,115 @@ public void WhenCommittingItemsWillAddThemTo2ndLevelCache()
237238
using (var s = Sfi.WithOptions().Connection(connection).OpenSession())
238239
{
239240
CacheablePerson person = null;
240-
Assert.DoesNotThrow(() => person = s.Load<CacheablePerson>(id), "Failed loading entity from second level cache.");
241+
Assert.DoesNotThrow(
242+
() => person = s.Load<CacheablePerson>(id),
243+
"Failed loading entity from second level cache.");
241244
Assert.That(person.NotNullData, Is.EqualTo(notNullData));
242245
}
243246
}
247+
248+
[Test]
249+
public void CanCommitFromSessionTransaction()
250+
{
251+
int id;
252+
using (var s = OpenSession())
253+
{
254+
Assert.That(s.GetCurrentTransaction(), Is.Null);
255+
using (s.BeginTransaction())
256+
{
257+
var person = new Person();
258+
s.Save(person);
259+
id = person.Id;
260+
261+
s.GetCurrentTransaction().Commit();
262+
}
263+
Assert.That(s.GetCurrentTransaction(), Is.Null);
264+
}
265+
266+
using (var s = OpenSession())
267+
using (var t = s.BeginTransaction())
268+
{
269+
var person = s.Get<Person>(id);
270+
Assert.That(person, Is.Not.Null);
271+
t.Commit();
272+
}
273+
}
274+
275+
[Test]
276+
public void CanRollbackFromSessionTransaction()
277+
{
278+
int id;
279+
using (var s = OpenSession())
280+
{
281+
Assert.That(s.GetCurrentTransaction(), Is.Null);
282+
using (s.BeginTransaction())
283+
{
284+
var person = new Person();
285+
s.Save(person);
286+
id = person.Id;
287+
288+
s.GetCurrentTransaction().Rollback();
289+
290+
// Need to check before leaving the current using, otherwise the rollback could be the result of the
291+
// disposing.
292+
using (var s2 = OpenSession())
293+
using (var t2 = s2.BeginTransaction())
294+
{
295+
person = s2.Get<Person>(id);
296+
Assert.That(person, Is.Null);
297+
t2.Commit();
298+
}
299+
}
300+
Assert.That(s.GetCurrentTransaction(), Is.Null);
301+
}
302+
}
303+
304+
[Test, Obsolete]
305+
public void CanCommitFromSessionObsoleteTransaction()
306+
{
307+
int id;
308+
using (var s = OpenSession())
309+
using (s.BeginTransaction())
310+
{
311+
var person = new Person();
312+
s.Save(person);
313+
id = person.Id;
314+
315+
s.Transaction.Commit();
316+
}
317+
318+
using (var s = OpenSession())
319+
using (var t = s.BeginTransaction())
320+
{
321+
var person = s.Get<Person>(id);
322+
Assert.That(person, Is.Not.Null);
323+
t.Commit();
324+
}
325+
}
326+
327+
[Test, Obsolete]
328+
public void CanRollbackFromSessionObsoleteTransaction()
329+
{
330+
int id;
331+
using (var s = OpenSession())
332+
using (s.BeginTransaction())
333+
{
334+
var person = new Person();
335+
s.Save(person);
336+
id = person.Id;
337+
338+
s.Transaction.Rollback();
339+
340+
// Need to check before leaving the current using, otherwise the rollback could be the result of the
341+
// disposing.
342+
using (var s2 = OpenSession())
343+
using (var t2 = s2.BeginTransaction())
344+
{
345+
person = s2.Get<Person>(id);
346+
Assert.That(person, Is.Null);
347+
t2.Commit();
348+
}
349+
}
350+
}
244351
}
245352
}

‎src/NHibernate/AdoNet/AbstractBatcher.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ protected void Prepare(DbCommand cmd)
130130
cmd.Connection = sessionConnection;
131131
}
132132

133-
_connectionManager.Transaction.Enlist(cmd);
133+
_connectionManager.EnlistInTransaction(cmd);
134134
Driver.PrepareCommand(cmd);
135135
}
136136
catch (InvalidOperationException ioe)

‎src/NHibernate/AdoNet/ConnectionManager.cs

Lines changed: 41 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -386,28 +386,40 @@ void IDeserializationCallback.OnDeserialization(object sender)
386386

387387
public ITransaction BeginTransaction(IsolationLevel isolationLevel)
388388
{
389-
Transaction.Begin(isolationLevel);
389+
EnsureTransactionIsCreated();
390+
_transaction.Begin(isolationLevel);
390391
return _transaction;
391392
}
392393

393394
public ITransaction BeginTransaction()
394395
{
395-
Transaction.Begin();
396+
EnsureTransactionIsCreated();
397+
_transaction.Begin();
396398
return _transaction;
397399
}
398400

401+
private void EnsureTransactionIsCreated()
402+
{
403+
if (_transaction == null)
404+
{
405+
_transaction = Factory.TransactionFactory.CreateTransaction(Session);
406+
}
407+
}
408+
399409
public ITransaction Transaction
400410
{
401411
get
402412
{
403-
if (_transaction == null)
404-
{
405-
_transaction = Factory.TransactionFactory.CreateTransaction(Session);
406-
}
413+
EnsureTransactionIsCreated();
407414
return _transaction;
408415
}
409416
}
410417

418+
/// <summary>
419+
/// The current transaction if any is ongoing, else <see langword="null" />.
420+
/// </summary>
421+
public ITransaction CurrentTransaction => _transaction;
422+
411423
public void AfterNonTransactionalQuery(bool success)
412424
{
413425
_log.Debug("After autocommit");
@@ -444,10 +456,32 @@ public IBatcher Batcher
444456
public DbCommand CreateCommand()
445457
{
446458
var result = GetConnection().CreateCommand();
447-
Transaction.Enlist(result);
459+
EnlistInTransaction(result);
448460
return result;
449461
}
450462

463+
/// <summary>
464+
/// Enlist a command in the current transaction, if any.
465+
/// </summary>
466+
/// <param name="command">The command to enlist.</param>
467+
public void EnlistInTransaction(DbCommand command)
468+
{
469+
if (command == null)
470+
throw new ArgumentNullException(nameof(command));
471+
472+
if (_transaction != null)
473+
{
474+
_transaction.Enlist(command);
475+
return;
476+
}
477+
478+
if (command.Transaction != null)
479+
{
480+
_log.Warn("set a nonnull DbCommand.Transaction to null because the Session had no Transaction");
481+
command.Transaction = null;
482+
}
483+
}
484+
451485
/// <summary>
452486
/// Enlist the connection into provided transaction if the connection should be enlisted.
453487
/// Do nothing in case an explicit transaction is ongoing.

‎src/NHibernate/Async/AdoNet/AbstractBatcher.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ protected async Task PrepareAsync(DbCommand cmd, CancellationToken cancellationT
5858
cmd.Connection = sessionConnection;
5959
}
6060

61-
_connectionManager.Transaction.Enlist(cmd);
61+
_connectionManager.EnlistInTransaction(cmd);
6262
Driver.PrepareCommand(cmd);
6363
}
6464
catch (InvalidOperationException ioe)

‎src/NHibernate/Async/AdoNet/ConnectionManager.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ public async Task<DbCommand> CreateCommandAsync(CancellationToken cancellationTo
9090
{
9191
cancellationToken.ThrowIfCancellationRequested();
9292
var result = (await (GetConnectionAsync(cancellationToken)).ConfigureAwait(false)).CreateCommand();
93-
Transaction.Enlist(result);
93+
EnlistInTransaction(result);
9494
return result;
9595
}
9696
}

‎src/NHibernate/Async/Context/ThreadLocalSessionContext.cs

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -32,17 +32,16 @@ private static async Task CleanupAnyOrphanedSessionAsync(ISessionFactory factory
3232

3333
try
3434
{
35-
if (orphan.Transaction != null && orphan.Transaction.IsActive)
35+
try
3636
{
37-
try
38-
{
39-
await (orphan.Transaction.RollbackAsync(cancellationToken)).ConfigureAwait(false);
40-
}
41-
catch (OperationCanceledException) { throw; }
42-
catch (Exception ex)
43-
{
44-
log.Debug(ex, "Unable to rollback transaction for orphaned session");
45-
}
37+
var transaction = orphan.GetCurrentTransaction();
38+
if (transaction?.IsActive == true)
39+
await (transaction.RollbackAsync(cancellationToken)).ConfigureAwait(false);
40+
}
41+
catch (OperationCanceledException) { throw; }
42+
catch (Exception ex)
43+
{
44+
log.Debug(ex, "Unable to rollback transaction for orphaned session");
4645
}
4746
orphan.Close();
4847
}

0 commit comments

Comments
 (0)