Skip to content

EF-280: Treat dispose of an active MongoTransaction as rollback #263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
damieng wants to merge 2 commits into
base: main
Choose a base branch
from
Open

EF-280: Treat dispose of an active MongoTransaction as rollback #263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b3fe68
Treat dispose of an active MongoTransaction as rollback.
damieng Dec 11, 2025
879fa63
Recommendations from Copilot.
damieng Dec 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 35 additions & 15 deletions src/MongoDB.EntityFrameworkCore/Storage/MongoTransaction.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@

using System;
using System.Diagnostics;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.EntityFrameworkCore;
Expand Down Expand Up @@ -91,7 +90,6 @@ internal static MongoTransaction Start(

var transaction = new MongoTransaction(session, context, transactionId, transactionManager, transactionLogger);
transactionLogger.TransactionStarted(transaction, async, startTime, stopwatch.Elapsed);

return transaction;
}

Expand Down Expand Up @@ -234,22 +232,51 @@ public TransactionOptions TransactionOptions
/// <inheritdoc />
public void Dispose()
{
if (_transactionState == TransactionState.Disposed) return;
switch (_transactionState)
{
case TransactionState.Disposed:
return;

case TransactionState.Active:
Rollback();
break;

case TransactionState.Committed:
case TransactionState.RolledBack:
case TransactionState.Failed:
break;

default:
throw new InvalidOperationException($"Can not Dispose MongoTransaction {TransactionId} because it is {_transactionState}.");
}

AssertCorrectState("Dispose", TransactionState.Committed, TransactionState.RolledBack, TransactionState.Failed);
_transactionState = TransactionState.Disposed;
session.Dispose();
}

/// <inheritdoc />
public ValueTask DisposeAsync()
public async ValueTask DisposeAsync()
{
if (_transactionState == TransactionState.Disposed) return ValueTask.CompletedTask;
switch (_transactionState)
{
case TransactionState.Disposed:
return;

case TransactionState.Active:
await RollbackAsync();
break;

case TransactionState.Committed:
case TransactionState.RolledBack:
case TransactionState.Failed:
break;

default:
throw new InvalidOperationException($"Can not Dispose MongoTransaction {TransactionId} because it is {_transactionState}.");
Copy link
Collaborator

@ajcvickers ajcvickers Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we throw instead of rolling back for the "Committing" and "RollingBack" states? Also, do we want to throw by default if a new state is introduced? If we're responsible for the lifetime of the transaction, and it is disposed while in anything other than a final state (Failed, Committed, RolledBack), then I think we should attempt to roll back, since the transaction is going away.

Copy link
Member Author

@damieng damieng Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Committing and RollingBack are in-progress states. They should never get here.

Even if we do I don't think we can roll-back mid-commit and I'm sure we can't roll back mid roll back.

Copy link
Collaborator

@ajcvickers ajcvickers Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you point me to where these states are defined so I can read up on it?

Copy link
Member Author

@damieng damieng Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an enum at the start of this file.

We set it to Committing at the start of the Commit methods, and RollingBack at the start of the RollBack methods.

If it's either of these states when disposing it basically has to either be either a bug in our code or a concurrency issue where they are trying to use the non-thread-safe MongoTransaction across multiple threads and one is doing a dispose while the other is performing a commit or rollback.

Copy link
Collaborator

@ajcvickers ajcvickers Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I spent some more time looking at all the code here, rather than just the PR, and I now understand these are just states we have, not the server or driver. Given that, as you say, this is not thread-safe, then Committing and RollingBack are not observable outside the class for any thread-safe correct code. So, first, why even have them? Second, if we detect that we are in a state that can only be reached by executing in a thread-unsafe manner, then we should explicitly say that in the exception message so that people know this is why the code threw--like what System.Collections.Generic does when you try to modify a collection while it is in use.

}

AssertCorrectState("Dispose", TransactionState.Committed, TransactionState.RolledBack, TransactionState.Failed);
_transactionState = TransactionState.Disposed;
session.Dispose();
return ValueTask.CompletedTask;
}

private void AssertCorrectState(string action, TransactionState validState)
Expand All @@ -258,11 +285,4 @@ private void AssertCorrectState(string action, TransactionState validState)
throw new
InvalidOperationException($"Can not {action} MongoTransaction {TransactionId} because it is {_transactionState}.");
}

private void AssertCorrectState(string action, params TransactionState[] validStates)
{
if (!validStates.Contains(_transactionState))
throw new
InvalidOperationException($"Can not {action} MongoTransaction {TransactionId} because it is {_transactionState}.");
}
}
127 changes: 86 additions & 41 deletions tests/MongoDB.EntityFrameworkCore.FunctionalTests/Storage/TransactionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,27 +273,6 @@ await db.Entities.AddRangeAsync(
}
}

[Fact]
public void Explicit_transaction_dispose_without_commit_or_rollback_throws_and_changes_not_visible()
{
var collection = database.CreateCollection<SimpleEntity>();

// Use `using` to force Dispose without commit/rollback
var ex = Assert.Throws<InvalidOperationException>(() =>
{
using var db = SingleEntityDbContext.Create(collection);
using var transaction = db.Database.BeginTransaction();
db.Entities.Add(new SimpleEntity { name = "pending" });
db.SaveChanges();
// Intentionally not committing or rolling back; disposing transaction should throw
});

Assert.Contains("Dispose", ex.Message);

using var dbCheck = SingleEntityDbContext.Create(collection);
Assert.Empty(dbCheck.Entities); // nothing committed
}

[Fact]
public void Ambient_TransactionScope_blocks_begin_explicit_transaction()
{
Expand Down Expand Up @@ -526,39 +505,29 @@ public async Task Same_context_reads_its_own_writes_inside_explicit_transaction_
}

[Fact]
public void Disposing_explicit_transaction_without_commit_or_rollback_throws_and_does_not_commit()
public void Disposing_explicit_transaction_without_commit_or_rollback_does_not_commit()
{
var collection = database.CreateCollection<SimpleEntity>();

var ex = Assert.Throws<InvalidOperationException>(() =>
{
using var db = SingleEntityDbContext.Create(collection);
using var transaction = db.Database.BeginTransaction();
db.Entities.Add(new SimpleEntity { name = "pending" });
db.SaveChanges();
// Transaction goes out of scope without Commit/Rollback -> Dispose should throw
});
using var db = SingleEntityDbContext.Create(collection);
using var transaction = db.Database.BeginTransaction();
db.Entities.Add(new SimpleEntity { name = "pending" });
db.SaveChanges();

Assert.Contains("Dispose", ex.Message);
using var check = SingleEntityDbContext.Create(collection);
Assert.Empty(check.Entities);
}

[Fact]
public async Task Disposing_explicit_transaction_without_commit_or_rollback_throws_and_does_not_commit_async()
public async Task Disposing_explicit_transaction_without_commit_or_rollback_does_not_commit_async()
{
var collection = database.CreateCollection<SimpleEntity>();

var ex = await Assert.ThrowsAsync<InvalidOperationException>(async () =>
{
await using var db = SingleEntityDbContext.Create(collection);
await using var transaction = await db.Database.BeginTransactionAsync();
await db.Entities.AddAsync(new SimpleEntity { name = "pending" });
await db.SaveChangesAsync();
// Transaction goes out of scope without Commit/Rollback -> DisposeAsync should throw
});
await using var db = SingleEntityDbContext.Create(collection);
await using var transaction = await db.Database.BeginTransactionAsync();
await db.Entities.AddAsync(new SimpleEntity { name = "pending" });
await db.SaveChangesAsync();

Assert.Contains("Dispose", ex.Message);
await using var check = SingleEntityDbContext.Create(collection);
Assert.Empty(await check.Entities.ToListAsync());
}
Expand Down Expand Up @@ -667,4 +636,80 @@ public void Can_start_new_explicit_transaction_on_same_context_after_rollback()
var names = check.Entities.Select(e => e.name).OrderBy(n => n).ToList();
Assert.Equal(["transaction2-committed"], names);
}

[Theory]
[InlineData(false)]
[InlineData(true)]
public void Can_execute_transaction_via_execution_strategy(bool succeeds)
{
var expectedCount = succeeds ? 2 : 0;
var collection = database.CreateCollection<SimpleEntity>(values: [succeeds]);

using (var db = SingleEntityDbContext.Create(collection))
{
var strategy = db.Database.CreateExecutionStrategy();

try
{
strategy.ExecuteInTransaction(
db,
operation: context =>
{
Assert.NotNull(context.Entities);
context.Entities.Add(new SimpleEntity { name = "First" });
context.Entities.Add(new SimpleEntity { name = "Second" });
context.SaveChanges();
if (!succeeds) throw new Exception("Expected failure");
},
verifySucceeded: _ => true);
}
catch (Exception ex) when (ex.Message == "Expected failure")
{
// We expect this to fail
}
}

using (var db = SingleEntityDbContext.Create(collection))
{
Assert.Equal(expectedCount, db.Entities.Count());
}
}

[Theory]
[InlineData(false)]
[InlineData(true)]
public async Task Can_execute_transaction_via_execution_strategy_async(bool succeeds)
{
var expectedCount = succeeds ? 2 : 0;
var collection = database.CreateCollection<SimpleEntity>(values: [succeeds]);

await using (var db = SingleEntityDbContext.Create(collection))
{
var strategy = db.Database.CreateExecutionStrategy();

try
{
await strategy.ExecuteInTransactionAsync(
db,
operation: async (context, cancellationToken) =>
{
Assert.NotNull(context.Entities);
context.Entities.Add(new SimpleEntity { name = "First" });
context.Entities.Add(new SimpleEntity { name = "Second" });
await context.SaveChangesAsync(cancellationToken);
if (!succeeds) throw new Exception("Expected failure");
},
verifySucceeded: (_, _) => Task.FromResult(true));
}
catch (Exception ex) when (ex.Message == "Expected failure")
{
// We expect this to fail
}
}

await using (var db = SingleEntityDbContext.Create(collection))
{
Assert.Equal(expectedCount, db.Entities.Count());
}
}
}