dotnet / dotnet/efcore

DbContext pooling recycles a context whose ConcurrencyDetector is stuck in the critical section; every later rent fails with "A second operation was started on this context instance"

Closed
#38,983 2 comments 0 reactions 0 assignees View on GitHub
area-dbcontext
Dominant language
C#
Stars
14.8k
Forks
3.4k
PR merge metrics
PR metrics pending

Description

### Bug description

`ConcurrencyDetector` is not reset when a pooled `DbContext` is returned to the pool, and `EnterCriticalSection`/`ExitCriticalSection` toggle `_inCriticalSection` around an allocating `AsyncLocal` write. If that write throws (in practice: `OutOfMemoryException` under memory pressure), the flag stays set, the context is returned to the pool as usual, and every subsequent operation on that instance — for the rest of the process lifetime — throws `InvalidOperationException: A second operation was started on this context instance...`.

This turns one transient failure in one request into a permanent failure of the process. In our case a single low-traffic pod (the pool held one or two instances, so the poisoned one was rented almost every time) failed **every** database call for 45 minutes until it was redeployed: the original OOM hit one endpoint 24 times, the poisoned context produced ~1,000 errors on unrelated endpoints.

This is the mechanism behind #22802, which was closed with "OutOfMemoryException is not something that can be recovered from". Agreed that the request that hit OOM is lost; the ask here is different: **the pool should not hand a known-bad instance to the next request**, and the detector should not have a window where a failed allocation leaves it permanently locked. Both are cheap to fix and need no OOM handling.

`Interlocked.CompareExchange(ref _inCriticalSection, 1, 0)` has already run when `ThreadAcquiredLocksCount.Value++` throws, so `EnterCriticalSection` never returns a disposer and nothing ever clears the flag. `ExitCriticalSection` has the mirror-image window: `ThreadAcquiredLocksCount.Value--` (also an allocating write) runs before `_inCriticalSection = 0`.

### Your code

The pooling half is deterministic and reproduces without OOM, using the standard `AddDbContextPool` + scope-per-request setup:

```csharp
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Infrastructure;
using Microsoft.Extensions.DependencyInjection;

var services = new ServiceCollection();
services.AddDbContextPool(o => o.UseSqlite("Data Source=repro.db"));
await using var provider = services.BuildServiceProvider();

using (var scope = provider.CreateScope())
{
var ctx = scope.ServiceProvider.GetRequiredService();
ctx.Database.EnsureDeleted();
ctx.Database.EnsureCreated();
}

var first = await Request1();
await Request2(first);

// Separate async method: AsyncLocal changes made here do not flow back to the caller,
// so Request2 runs on a fresh flow like a real next request would.
async Task Request1()
{
using var scope = provider.CreateScope();
var ctx = scope.ServiceProvider.GetRequiredService();
await ctx.Blogs.ToListAsync();

// Simulates EnterCriticalSection succeeding but the matching Exit never running
// (what happens when the AsyncLocal write inside Enter/Exit throws).
ctx.GetService().EnterCriticalSection(); // disposer dropped
return ctx;
} // scope disposed -> context returned to pool; ResetState() does not touch ConcurrencyDetector

async Task Request2(BlogContext first)
{
using var scope = provider.CreateScope();
var ctx = scope.ServiceProvider.GetRequiredService();
Console.WriteLine($"same instance: {ReferenceEquals(first, ctx)}"); // True
await ctx.Blogs.ToListAsync(); // InvalidOperationException: A second operation was started on this context instance...
}

public class BlogContext(DbContextOptions options) : DbContext(options)
{
public DbSet Blogs => Set();
}
public class Blog { public int Id { get; set; } }
```

Note the two requests must run on different async flows (separate async methods, or `ExecutionContext.SuppressFlow()`); on the same flow the leaked `ThreadAcquiredLocksCount` makes the detector treat the second call as re-entrant and the bug is masked. That is probably why this was hard to reproduce in #22802.

### Stack traces

The other half (how the disposer gets dropped in production) is the stack trace from #22802 and from our incident:

```text
System.OutOfMemoryException
at System.Threading.AsyncLocalValueMap.MultiElementAsyncLocalValueMap.Set(IAsyncLocal key, Object value, Boolean treatNullValueAsNonexistent)
at System.Threading.ExecutionContext.SetLocalValue(IAsyncLocal local, Object newValue, Boolean needChangeNotifications)
at System.Threading.AsyncLocal`1.set_Value(T value)
at Microsoft.EntityFrameworkCore.Infrastructure.Internal.ConcurrencyDetector.EnterCriticalSection()
at Microsoft.EntityFrameworkCore.Query.Internal.SingleQueryingEnumerable`1.AsyncEnumerator.MoveNextAsync()
```

### Verbose output

_No response_

### EF Core version

10.0.9 (also verified against current `main`; `ConcurrencyDetector` is unchanged since 8.x)

### Database provider

Microsoft.EntityFrameworkCore.SqlServer (repro above uses Sqlite)

### Target framework

.NET 10

### Operating system

Linux (Kubernetes); #22802 reported the same on Windows

### IDE

_No response_

### Proposed fix

Two small, allocation-free changes; no behaviour change on the happy path:

1. `ConcurrencyDetector` implements `IResettableService` and `StateManager.ResetState()` resets it (same way it already resets `_changeDetector`), so a pooled context always starts its next lease clean.
2. Reorder the bookkeeping so the flag is only kept if the AsyncLocal write succeeds, and cleared before the AsyncLocal write on exit:

```csharp
public virtual ConcurrencyDetectorCriticalSectionDisposer EnterCriticalSection()
{
if (Interlocked.CompareExchange(ref _inCriticalSection, 1, 0) == 1
&& ThreadAcquiredLocksCount.Value == 0)
{
throw new InvalidOperationException(CoreStrings.ConcurrentMethodInvocation);
}

try
{
ThreadAcquiredLocksCount.Value++;
}
catch
{
if (_currentContextRefCount == 0)
{
_inCriticalSection = 0;
}

throw;
}

_currentContextRefCount++;
return new ConcurrencyDetectorCriticalSectionDisposer(this);
}

public virtual void ExitCriticalSection()
{
if (--_currentContextRefCount == 0)
{
_inCriticalSection = 0;
}

ThreadAcquiredLocksCount.Value--;
}
```

I have this implemented with tests (pooled context reusable after a leaked section — red before, green after; `EFCore.Tests` fully green) and can open a PR if the team is open to it.

Contributor guide

Open the contributing guide

Research direction

Start with ConcurrencyDetector and StateManager.ResetState, then inspect the AddDbContextPool scope-per-request repro described in the issue. Run the mentioned pooled-context test and EFCore.Tests; done means a context remains reusable after a leaked critical section and the full test suite stays green.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.