DI: avoid duplicate disposal for shared singleton instances#128768
DI: avoid duplicate disposal for shared singleton instances#128768Copilot wants to merge 4 commits into
Conversation
Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>
|
@copilot , the brute force deduplication isn't an option here since services might be resolved in hot-path, gating the list with a set to deduplicate would mean higher memory consumption (8 bytes vs 28 bytes per item) so the best compromise seems to be lazy deduplication when we dispose the scope, going 0->count with a set and nullify all already seen objects and then do count->0 disposing and skipping nulls. |
Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>
Updated in 2fe33aa. I removed capture-time duplicate checks and switched to lazy deduplication in |
There was a problem hiding this comment.
Pull request overview
This PR changes DI scope disposal to avoid disposing the same instance multiple times when it’s captured repeatedly via factory indirection (e.g., multiple singleton registrations returning the same underlying singleton). It does this by collapsing duplicate entries in the captured-disposables list at dispose time, while keeping reverse-disposal ordering for dependents.
Changes:
- Add reference-based deduplication of
_disposablesentries duringBeginDispose, nulling duplicate list slots and skipping nulls during disposal. - Update sync and async disposal loops to tolerate null entries introduced by deduplication.
- Add regression tests ensuring a shared singleton instance is disposed once and disposed after dependents.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs | Adds dispose-time deduplication for captured disposables and updates dispose loops to skip null entries. |
| src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs | Adds tests for singleton aliasing scenarios to validate single-dispose behavior and dependent teardown ordering. |
| if (_disposables is { Count: > 0 } disposables) | ||
| { | ||
| var seen = new HashSet<object>(ReferenceEqualityComparer.Instance); | ||
| for (int i = 0; i < disposables.Count; i++) | ||
| { | ||
| if (!seen.Add(disposables[i])) | ||
| { | ||
| disposables[i] = null!; | ||
| } | ||
| } | ||
| } |
| [Fact] | ||
| public void Dispose_DoesNotDisposeSameSingletonInstanceResolvedAsMultipleServices() | ||
| { | ||
| var services = new ServiceCollection(); | ||
| services.AddSingleton<MultipleServiceImpl>(); | ||
| services.AddSingleton<IMultipleService1>(sp => sp.GetRequiredService<MultipleServiceImpl>()); | ||
| services.AddSingleton<IMultipleService2>(sp => sp.GetRequiredService<MultipleServiceImpl>()); | ||
|
|
||
| var serviceProvider = services.BuildServiceProvider(); | ||
| var service = serviceProvider.GetRequiredService<MultipleServiceImpl>(); | ||
|
|
||
| Assert.Same(service, serviceProvider.GetRequiredService<IMultipleService1>()); | ||
| Assert.Same(service, serviceProvider.GetRequiredService<IMultipleService2>()); | ||
|
|
||
| serviceProvider.Dispose(); | ||
|
|
||
| Assert.Equal(1, service.DisposeCount); | ||
| } |
| [Fact] | ||
| public void Dispose_DisposesSharedSingletonAfterDependents() | ||
| { | ||
| var services = new ServiceCollection(); | ||
| services.AddSingleton<MultipleServiceImpl>(); | ||
| services.AddSingleton<IMultipleService1>(sp => sp.GetRequiredService<MultipleServiceImpl>()); | ||
| services.AddSingleton<IMultipleService2>(sp => sp.GetRequiredService<MultipleServiceImpl>()); | ||
| services.AddSingleton<DependsOnMultipleService>(); | ||
|
|
||
| var serviceProvider = services.BuildServiceProvider(); | ||
|
|
||
| _ = serviceProvider.GetRequiredService<DependsOnMultipleService>(); | ||
| _ = serviceProvider.GetRequiredService<IMultipleService2>(); | ||
| var service = serviceProvider.GetRequiredService<MultipleServiceImpl>(); | ||
|
|
||
| serviceProvider.Dispose(); | ||
|
|
||
| Assert.Equal(1, service.DisposeCount); | ||
| } |
When multiple service registrations resolve to the same singleton instance via factory indirection (
sp => sp.GetRequiredService<TImpl>()), MEDI could capture that object multiple times for scope disposal. This can lead to repeated disposal and incorrect teardown ordering for dependents.Scope capture/disposal behavior
ServiceProviderEngineScope.CaptureDisposableallocation-light on hot paths by appending disposables without capture-time deduplication.BeginDispose: scan0 -> count, null duplicate entries, then disposecount -> 0while skipping nulls.Behavioral coverage
ServiceProviderEngineScopeTestsfor:Representative scenario
After this change, duplicate captures are lazily collapsed at dispose time so
TImplis disposed once, and dependent teardown order remains correct.