Skip to content

Commit 350951d

Browse files
Copilotrosebyte
andauthored
docs: document that ServiceProvider.Dispose() throws for async-only disposables, recommend DisposeAsync() (#124966)
`ServiceProvider.Dispose()` throws `InvalidOperationException` when any resolved service implements only `IAsyncDisposable`. This behavior is undocumented, making it a surprising pitfall — especially when a new dependency introduces an async-only disposable service into an existing sync disposal flow. ## Changes - **`ServiceProvider.Dispose()`** — replaced `/// <inheritdoc />` with explicit docs noting it throws `InvalidOperationException` when an async-only service is resolved, and recommending `DisposeAsync()` instead - **`ServiceProvider.DisposeAsync()`** — documented as disposing all resolved services that implement `IDisposable` or `IAsyncDisposable` `AsyncServiceScope.cs` and `IServiceScope.cs` are left unchanged, as the throwing behavior is specific to the `ServiceProvider` implementation and is not part of the interface contract. <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>DI Scope Should Dispose All Disposables On Dispose()</issue_title> > <issue_description>**Proposal options**: > 1. `Dispose` should do sync-over-async > 2. Update documentation that we throw from `Dispose` when we should call async > > When Dispose() is invoked on ServiceProviderEngineScope, the engine correctly disposes all members implementing IDisposable. However, if any member implements IAsyncDisposable but not IDisposable, an exception is thrown, effectively forcing consumers to call DisposeAsync() instead. > > I presume the motivation here was well‑intentioned, encouraging callers to prefer DisposeAsync(), thereby avoiding sync‑over‑async execution paths, which are generally undesirable. Since DisposeAsync() returns a ValueTask, it can indeed be regarded as the superior mechanism, however, Dispose() is not marked as deprecated, discouraged, or otherwise inferior, and there is no warning indicating that invoking it may result in an exception, as highlighted by @CarnaViire [in this comment](#123342 (comment)). The current behaviour risks surprising consumers and may obstruct their workflow, especially when they do not explicitly control the disposal flow. > > Consider a system that has historically relied solely on IDisposable services. Introducing a new library that happens to register an IAsyncDisposable component would suddenly produce exceptions out of the blue, effectively compelling system owners to perform a broader refactor simply to adopt the library. > > Similarly, if a library calls Dispose() on a scope rather than DisposeAsync(), its consumers would be unable to use any library that registers a type implementing only IAsyncDisposable. This is problematic because implementing only IAsyncDisposable is entirely legitimate, and the [official guidance](https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-disposeasync) explicitly states that a type may implement both interfaces only when relevant to handling both synchronous and asynchronous disposal flows, not as a general requirement. > > I noticed a [comment](dotnet/extensions#1005 (comment)) on this behaviour in the original PR. However, the resolution appeared to focus primarily on the breaking‑change aspect, rather than evaluating the broader behavioural correctness of the approach. > > @davidfowl, @halter73, I noticed you both contributed to the earlier discussion, and I’m curious what your view is on the behaviour described above. Should Dispose() ultimately support a sync‑over‑async path, or should we at least add a clear warning to the documentation to make this expectation explicit, if the current design is intentional?</issue_description> > > <agent_instructions>Improve the relevant Dispose method to indicate that it throws if any service implements only IAsyncDisposable as we disencourage using sync-over-async and in general recommend to always use DisposeAsync with explicit sync-over-async on user side if they really need to do it.</agent_instructions> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > <comment_new><author>@svick</author><body> > In either case, I think the documentation of both `Dispose` and `DisposeAsync` should be updated. The generic inherited message that's there right now is not very useful.</body></comment_new> > <comment_new><author>@rosebyte</author><body> > Triage: we will improve documentation.</body></comment_new> > </comments> > </details> <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #123620 <!-- START COPILOT CODING AGENT TIPS --> --- 🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. [Learn more about Advanced Security.](https://gh.io/cca-advanced-security) --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>
1 parent e524be6 commit 350951d

File tree

1 file changed

+14
-2
lines changed

1 file changed

+14
-2
lines changed

src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -169,14 +169,26 @@ internal object GetRequiredKeyedService(Type serviceType, object? serviceKey, Se
169169

170170
internal bool IsDisposed() => _disposed;
171171

172-
/// <inheritdoc />
172+
/// <summary>
173+
/// Disposes the service provider and all resolved services that implement <see cref="IDisposable"/>.
174+
/// </summary>
175+
/// <remarks>
176+
/// Prefer calling <see cref="DisposeAsync"/> over this method. If any resolved service implements
177+
/// <see cref="IAsyncDisposable"/> but not <see cref="IDisposable"/>, this method throws an
178+
/// <see cref="InvalidOperationException"/>. Use <see cref="DisposeAsync"/> to properly handle all
179+
/// disposable services, or explicitly perform sync-over-async on the caller side if synchronous
180+
/// disposal is required.
181+
/// </remarks>
173182
public void Dispose()
174183
{
175184
DisposeCore();
176185
Root.Dispose();
177186
}
178187

179-
/// <inheritdoc/>
188+
/// <summary>
189+
/// Asynchronously disposes the service provider and all resolved services that implement <see cref="IDisposable"/> or <see cref="IAsyncDisposable"/>.
190+
/// </summary>
191+
/// <returns>A value task that represents the asynchronous operation.</returns>
180192
public ValueTask DisposeAsync()
181193
{
182194
DisposeCore();

0 commit comments

Comments
 (0)