-
Notifications
You must be signed in to change notification settings - Fork 2
fix(di): open one DI scope per session so Scoped services match the documented lifetime #74
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| namespace Repl; | ||
|
|
||
| /// <summary> | ||
| /// How a Run* call manages the session's dependency-injection scope. | ||
| /// </summary> | ||
| public enum SessionScopeBehavior | ||
| { | ||
| /// <summary> | ||
| /// The run opens one DI scope for the session (the default): Scoped services resolve | ||
| /// per session and scoped disposables are released when the session ends. | ||
| /// </summary> | ||
| PerRun = 0, | ||
|
|
||
| /// <summary> | ||
| /// The caller's service provider already represents the session's scope (for example a | ||
| /// Blazor circuit or per-request scope, or a session owner spanning several one-shot | ||
| /// runs): the run resolves from it directly and never opens a nested scope. | ||
| /// </summary> | ||
| CallerOwned = 1, | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,142 @@ | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Repl.Testing; | ||
|
|
||
| namespace Repl.IntegrationTests; | ||
|
|
||
| [TestClass] | ||
| [DoNotParallelize] | ||
| public sealed class Given_SessionScopedServices | ||
| { | ||
| private sealed class ScopedProbe | ||
| { | ||
| public Guid Id { get; } = Guid.NewGuid(); | ||
| } | ||
|
|
||
| private sealed class TransientProbe | ||
| { | ||
| public Guid Id { get; } = Guid.NewGuid(); | ||
| } | ||
|
|
||
| private sealed class DisposableProbe(List<Guid> disposed) : IDisposable | ||
| { | ||
| public Guid Id { get; } = Guid.NewGuid(); | ||
|
|
||
| public void Dispose() => disposed.Add(Id); | ||
| } | ||
|
|
||
| private static ReplTestHost CreateProbeHost(List<Guid>? disposedTracker = null) => | ||
| ReplTestHost.Create(() => | ||
| { | ||
| var app = ReplApp.Create(services => | ||
| { | ||
| services.AddScoped<ScopedProbe>(); | ||
| services.AddTransient<TransientProbe>(); | ||
| services.AddScoped(_ => new DisposableProbe(disposedTracker ?? [])); | ||
| }).UseDefaultInteractive(); | ||
| app.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); | ||
| app.Map("transient", (TransientProbe probe) => probe.Id.ToString()); | ||
| app.Map("disposable", (DisposableProbe probe) => probe.Id.ToString()); | ||
| return app; | ||
| }); | ||
|
|
||
| private static async Task<string> RunAndCaptureAsync(ReplSessionHandle session, string command) | ||
| { | ||
| var result = await session.RunCommandAsync($"{command} --no-logo").ConfigureAwait(false); | ||
| result.ExitCode.Should().Be(0, "command output was: {0}", result.OutputText); | ||
| return result.OutputText.Trim(); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| [Description("Guards the core per-run session scope on ONE shared app: two Run* calls against the same ReplApp (the exact shape of two hosted connections sharing one app) must resolve two distinct Scoped instances from the app's own root provider — this exercises the scope-creation branch itself, independently of Repl.Testing's session plumbing.")] | ||
| public void When_TwoRunsShareOneApp_Then_ScopedInstancesDiffer() | ||
| { | ||
| var sut = ReplApp.Create(services => services.AddScoped<ScopedProbe>()); | ||
| sut.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); | ||
|
|
||
| var first = ConsoleCaptureHelper.Capture(() => sut.Run(["scoped", "--no-logo"])); | ||
| var second = ConsoleCaptureHelper.Capture(() => sut.Run(["scoped", "--no-logo"])); | ||
|
|
||
| first.ExitCode.Should().Be(0); | ||
| second.ExitCode.Should().Be(0); | ||
| first.Text.Trim().Should().NotBe(second.Text.Trim()); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| [Description("Guards the CallerOwned opt-out: a caller whose provider already represents the session scope (Blazor circuit, per-request scope, multi-run session owners) must keep its own Scoped instances across runs — the entry points must not open a nested sibling scope that would hide them.")] | ||
| public async Task When_CallerOwnsTheSessionScope_Then_RunsShareTheCallerScopedInstances() | ||
| { | ||
| var sut = ReplApp.Create(services => services.AddScoped<ScopedProbe>()); | ||
| sut.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); | ||
| var scopeFactory = (IServiceScopeFactory)sut.Services.GetService(typeof(IServiceScopeFactory))!; | ||
| var callerScope = scopeFactory.CreateAsyncScope(); | ||
| await using (callerScope.ConfigureAwait(false)) | ||
| { | ||
| var options = new ReplRunOptions { SessionScope = SessionScopeBehavior.CallerOwned }; | ||
|
|
||
| var first = ConsoleCaptureHelper.Capture( | ||
| () => sut.Run(["scoped", "--no-logo"], callerScope.ServiceProvider, options)); | ||
| var second = ConsoleCaptureHelper.Capture( | ||
| () => sut.Run(["scoped", "--no-logo"], callerScope.ServiceProvider, options)); | ||
|
|
||
| first.ExitCode.Should().Be(0); | ||
| first.Text.Trim().Should().Be(second.Text.Trim()); | ||
| } | ||
| } | ||
|
|
||
| [TestMethod] | ||
| [Description("Guards the documented Scoped lifetime for hosted sessions: two distinct hosted sessions must resolve two distinct Scoped instances — sharing one instance across sessions leaks per-user state (auth context, carts) between concurrent clients.")] | ||
| public async Task When_TwoHostedSessionsResolveScopedService_Then_InstancesDiffer() | ||
| { | ||
| await using var host = CreateProbeHost(); | ||
| await using var sessionA = await host.OpenSessionAsync(); | ||
| await using var sessionB = await host.OpenSessionAsync(); | ||
|
|
||
| var idA = await RunAndCaptureAsync(sessionA, "scoped"); | ||
| var idB = await RunAndCaptureAsync(sessionB, "scoped"); | ||
|
|
||
| idA.Should().NotBe(idB); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| [Description("Guards the session-scope boundary from the other side: two commands within the SAME hosted session share the same Scoped instance — the scope is per session, not per command invocation.")] | ||
| public async Task When_SameSessionRunsTwoCommands_Then_ScopedInstanceIsShared() | ||
| { | ||
| await using var host = CreateProbeHost(); | ||
| await using var session = await host.OpenSessionAsync(); | ||
|
|
||
| var first = await RunAndCaptureAsync(session, "scoped"); | ||
| var second = await RunAndCaptureAsync(session, "scoped"); | ||
|
|
||
| first.Should().Be(second); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| [Description("Guards Transient semantics under the session scope: every resolution yields a fresh instance, including across commands of the same session.")] | ||
| public async Task When_TransientResolvedInTwoCommands_Then_InstancesDiffer() | ||
| { | ||
| await using var host = CreateProbeHost(); | ||
| await using var session = await host.OpenSessionAsync(); | ||
|
|
||
| var first = await RunAndCaptureAsync(session, "transient"); | ||
| var second = await RunAndCaptureAsync(session, "transient"); | ||
|
|
||
| first.Should().NotBe(second); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| [Description("Guards scoped-disposable lifetime: a Scoped IDisposable resolved during a hosted session is disposed when that session ends, not deferred to app shutdown — session resources (connections, per-user stores) must not accumulate for the host process lifetime.")] | ||
| public async Task When_SessionEnds_Then_ScopedDisposablesAreDisposed() | ||
| { | ||
| var disposed = new List<Guid>(); | ||
| await using var host = CreateProbeHost(disposed); | ||
|
|
||
| string id; | ||
| { | ||
| await using var session = await host.OpenSessionAsync(); | ||
| id = await RunAndCaptureAsync(session, "disposable"); | ||
| disposed.Should().NotContain(Guid.Parse(id)); | ||
| } | ||
|
|
||
| disposed.Should().Contain(Guid.Parse(id)); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -140,23 +140,31 @@ public SessionSnapshot GetSnapshot() | |
| return SessionSnapshot.Empty(SessionId); | ||
| } | ||
|
|
||
| public ValueTask DisposeAsync() | ||
| public async ValueTask DisposeAsync() | ||
| { | ||
| if (_disposed) | ||
| { | ||
| return ValueTask.CompletedTask; | ||
| return; | ||
| } | ||
|
|
||
| _disposed = true; | ||
| _owner.RemoveSession(SessionId); | ||
| ReplSessionIO.RemoveSession(SessionId); | ||
| if (_services is IDisposable disposable) | ||
| // Take the command gate before disposing the session scope so an in-flight command | ||
| // (e.g. an orphaned timed-out run) cannot race scoped-service disposal. | ||
| await _commandGate.WaitAsync().ConfigureAwait(false); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If Useful? React with 👍 / 👎. |
||
| switch (_services) | ||
| { | ||
| disposable.Dispose(); | ||
| case IAsyncDisposable asyncDisposable: | ||
| // Releases the session's DI scope, disposing its Scoped services. | ||
| await asyncDisposable.DisposeAsync().ConfigureAwait(false); | ||
| break; | ||
| case IDisposable disposable: | ||
| disposable.Dispose(); | ||
| break; | ||
| } | ||
|
|
||
| _commandGate.Dispose(); | ||
| return ValueTask.CompletedTask; | ||
| } | ||
|
|
||
| internal static ValueTask<ReplSessionHandle> StartAsync( | ||
|
|
@@ -175,14 +183,52 @@ internal static ValueTask<ReplSessionHandle> StartAsync( | |
| var sessionId = $"session-{Guid.NewGuid():N}"; | ||
| ReplSessionIO.EnsureSession(sessionId); | ||
| var app = appFactory(); | ||
| var runOptions = descriptor.BuildRunOptions(options); | ||
| var serviceCollection = new ServiceCollection(); | ||
| serviceCollection.AddSingleton<IReplSessionState, InMemorySessionState>(); | ||
| var services = serviceCollection.BuildServiceProvider(); | ||
| // The handle owns the session's DI scope (each command is a separate one-shot run), | ||
| // so every run opts out of per-run scoping. | ||
| var runOptions = descriptor.BuildRunOptions(options) with { SessionScope = SessionScopeBehavior.CallerOwned }; | ||
| var services = SessionScopedTestServices.Create(app); | ||
| var handle = new ReplSessionHandle(owner, app, options, services, runOptions, descriptor.Answers, sessionId); | ||
| return ValueTask.FromResult(handle); | ||
| } | ||
|
|
||
| // One logical test session spans several one-shot Run* calls, so the handle owns the | ||
| // session's DI scope itself: the provider is built over a scope of the APP's services | ||
| // (user registrations become visible to session commands, Scoped instances are shared | ||
| // across the session's commands) and every run uses SessionScopeBehavior.CallerOwned so | ||
| // the Run* entry points do not open a fresh scope — and fresh Scoped instances — per | ||
| // command. Note: the IReplSessionState mask below only intercepts direct GetService | ||
| // lookups; a user service constructor-injecting IReplSessionState inside the scope gets | ||
| // the app-registered implementation instead (pre-existing test-host limitation). | ||
| private sealed class SessionScopedTestServices : IServiceProvider, IAsyncDisposable | ||
| { | ||
| private readonly AsyncServiceScope? _scope; | ||
| private readonly IServiceProvider _inner; | ||
| private readonly InMemorySessionState _sessionState = new(); | ||
|
|
||
| private SessionScopedTestServices(AsyncServiceScope? scope, IServiceProvider inner) | ||
| { | ||
| _scope = scope; | ||
| _inner = inner; | ||
| } | ||
|
|
||
| public static SessionScopedTestServices Create(ReplApp app) | ||
| { | ||
| var root = app.Services; | ||
| if (root.GetService(typeof(IServiceScopeFactory)) is IServiceScopeFactory scopeFactory) | ||
| { | ||
| var scope = scopeFactory.CreateAsyncScope(); | ||
| return new SessionScopedTestServices(scope, scope.ServiceProvider); | ||
|
Comment on lines
+216
to
+220
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| return new SessionScopedTestServices(scope: null, root); | ||
| } | ||
|
|
||
| public object? GetService(Type serviceType) => | ||
| serviceType == typeof(IReplSessionState) ? _sessionState : _inner.GetService(serviceType); | ||
|
Comment on lines
+226
to
+227
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a command resolves an app-registered service whose constructor depends on Useful? React with 👍 / 👎. |
||
|
|
||
| public ValueTask DisposeAsync() => _scope?.DisposeAsync() ?? ValueTask.CompletedTask; | ||
| } | ||
|
|
||
| private static string[] BuildArgsWithAnswers( | ||
| List<string> baseTokens, | ||
| IReadOnlyDictionary<string, string>? sessionAnswers, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a caller passes an already-created
IServiceScope.ServiceProviderto the externally managedRunAsyncoverloads (for example,HttpContext.RequestServices), that provider still exposes the rootIServiceScopeFactory, so this creates a separate scope rather than using the supplied one. Scoped services resolved by the command will then differ from the caller's request/session-scoped instances, and the caller cannot opt out becauseISessionScopedServiceProvideris internal. Keep a supplied scope intact or expose an explicit opt-out mechanism.Useful? React with 👍 / 👎.