Skip to content

refactor(engine): Improve context scope cleanup with ConcurrentBag and IAsyncDisposable (#1477) #1793

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

Merged
thomhurst merged 4 commits into from
Jan 2, 2026
Merged

refactor(engine): Improve context scope cleanup with ConcurrentBag and IAsyncDisposable (#1477) #1793

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
894a24b
fix: Properly dispose service scopes in ModuleContextProvider (#1477)
thomhurst Jan 2, 2026
a29f238
fix: Address concurrency and disposal issues in ModuleContextProvider
thomhurst Jan 2, 2026
9589281
fix: Move snapshot inside lock to prevent race condition
thomhurst Jan 2, 2026
2b9732c
fix: Move scope creation inside lock to prevent race condition
thomhurst Jan 2, 2026
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
20 changes: 18 additions & 2 deletions src/ModularPipelines/Engine/ModuleContextProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
using System.Collections.Concurrent;
using Microsoft.Extensions.DependencyInjection;
using ModularPipelines.Context;
using ModularPipelines.Interfaces;

namespace ModularPipelines.Engine;

internal class ModuleContextProvider : IPipelineContextProvider, IScopeDisposer
internal class ModuleContextProvider : IPipelineContextProvider, IScopeDisposer, IAsyncDisposable

Copilot AI Jan 2, 2026

Copy link

Choose a reason for hiding this comment

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

The IScopeDisposer interface provides a default implementation of IDisposable.Dispose() that synchronously disposes scopes. However, with this class now also implementing IAsyncDisposable, there's a potential conflict:

  1. The synchronous Dispose from IScopeDisposer will call scope.Dispose() on each scope
  2. The asynchronous DisposeAsync will call scope.DisposeAsync() on each scope
  3. If both are called, scopes could be disposed twice

Additionally, the IScopeDisposer.Dispose implementation doesn't coordinate with DisposeAsync, creating potential race conditions. Consider whether both disposal patterns are needed, or implement proper coordination between them with a disposed flag.

Copilot uses AI. Check for mistakes.
{
private readonly IServiceProvider _serviceProvider;
private readonly List<IServiceScope> _scopes = new();
private readonly ConcurrentBag<IServiceScope> _scopes = new();

public ModuleContextProvider(IServiceProvider serviceProvider)
{
Expand All @@ -27,4 +28,19 @@ public IEnumerable<IServiceScope> GetScopes()
{
return _scopes;
}

public async ValueTask DisposeAsync()
{
foreach (var scope in _scopes)
{
if (scope is IAsyncDisposable asyncDisposable)
{
await asyncDisposable.DisposeAsync().ConfigureAwait(false);
}
else
{
scope.Dispose();
}

Copilot AI Jan 2, 2026

Copy link

Choose a reason for hiding this comment

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

The type check for IAsyncDisposable is unnecessary. CreateAsyncScope returns AsyncServiceScope, which always implements IAsyncDisposable. This check adds complexity without providing any benefit and will always take the true branch.

Consider simplifying this to directly call DisposeAsync on each scope without the conditional check.

Suggested change
if (scope is IAsyncDisposable asyncDisposable)
{
await asyncDisposable.DisposeAsync().ConfigureAwait(false);
}
else
{
scope.Dispose();
}
await ((IAsyncDisposable)scope).DisposeAsync().ConfigureAwait(false);

Copilot uses AI. Check for mistakes.
}
}
Comment on lines +49 to +101

Copilot AI Jan 2, 2026

Copy link

Choose a reason for hiding this comment

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

The DisposeAsync implementation has a critical concurrency issue. While DisposeAsync iterates over the ConcurrentBag, GetModuleContext can still be adding new scopes to it. This creates a race condition where:

  1. Scopes added during disposal may not be disposed
  2. The iteration over ConcurrentBag is not thread-safe when concurrent modifications occur

Additionally, this implementation is missing several important elements of proper async disposal:

  • No mechanism to prevent concurrent disposal calls
  • Missing GC.SuppressFinalize(this) to suppress finalization
  • No coordination to prevent new scopes from being added during disposal

Consider implementing a proper disposal pattern with a disposed flag and synchronization, or ensure that disposal only occurs when no concurrent calls to GetModuleContext are possible.

Copilot uses AI. Check for mistakes.
}
Loading