Skip to content

docs: document that ServiceProvider.Dispose() throws for async-only disposables, recommend DisposeAsync()#124966

Merged
rosebyte merged 6 commits intomainfrom
copilot/fix-di-scope-disposal
Mar 4, 2026
Merged

docs: document that ServiceProvider.Dispose() throws for async-only disposables, recommend DisposeAsync()#124966
rosebyte merged 6 commits intomainfrom
copilot/fix-di-scope-disposal

Conversation

Copy link
Contributor

Copilot AI commented Feb 27, 2026

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.

Original prompt

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. 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 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 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)

@svick 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. @rosebyte Triage: we will improve documentation.

🔒 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.

…disposable behavior

Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix DI scope to dispose all disposables on Dispose() docs: document that DI scope Dispose() throws for async-only disposables, recommend DisposeAsync() Feb 27, 2026
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection
See info in area-owners.md if you want to be subscribed.

@rosebyte
Copy link
Member

rosebyte commented Mar 2, 2026

@copilot , I commented one file but the came comments apply to the other files too, fix them everywhere.

Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 2, 2026

@copilot , I commented one file but the came comments apply to the other files too, fix them everywhere.

Applied all three changes to AsyncServiceScope.cs, IServiceScope.cs, and ServiceProvider.cs in 4c6d9e0.

@rosebyte rosebyte marked this pull request as ready for review March 2, 2026 18:03
Copilot AI review requested due to automatic review settings March 2, 2026 18:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates API documentation in Microsoft.Extensions.DependencyInjection to make it explicit that synchronous Dispose() can throw when a scope/provider contains services that implement only IAsyncDisposable, and to recommend using DisposeAsync() / await using to ensure all disposables are handled.

Changes:

  • Add explicit XML docs to ServiceProvider.Dispose()/DisposeAsync() describing disposal behavior and the async-only disposable pitfall.
  • Add similar XML docs to AsyncServiceScope.Dispose()/DisposeAsync().
  • Extend IServiceScope remarks to call out the async-disposal recommendation and mention AsyncServiceScope.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs Replaces <inheritdoc /> with explicit docs describing sync vs async disposal behavior.
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/IServiceScope.cs Adds remarks warning about sync disposal when async-only disposables exist, and recommends async disposal patterns.
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/AsyncServiceScope.cs Documents the sync/async disposal behavior for the wrapper scope.

@rosebyte
Copy link
Member

rosebyte commented Mar 4, 2026

@copilot, I see the warning with IAsncDisposable instances throwing with Dispose is specific to ServiceProvider, remove changes in the other two files.

Copy link
Contributor Author

Copilot AI commented Mar 4, 2026

@copilot, I see the warning with IAsncDisposable instances throwing with Dispose is specific to ServiceProvider, remove changes in the other two files.

Done in e953d19 — reverted AsyncServiceScope.cs and IServiceScope.cs back to their original /// <inheritdoc /> state. The disposal warning now lives only in ServiceProvider.cs.

Copilot AI changed the title docs: document that DI scope Dispose() throws for async-only disposables, recommend DisposeAsync() docs: document that ServiceProvider.Dispose() throws for async-only disposables, recommend DisposeAsync() Mar 4, 2026
@rosebyte rosebyte enabled auto-merge (squash) March 4, 2026 10:55
@rosebyte rosebyte disabled auto-merge March 4, 2026 12:41
@rosebyte rosebyte enabled auto-merge (squash) March 4, 2026 12:43
@rosebyte rosebyte merged commit 350951d into main Mar 4, 2026
90 of 92 checks passed
@rosebyte rosebyte deleted the copilot/fix-di-scope-disposal branch March 4, 2026 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DI Scope Should Dispose All Disposables On Dispose()

4 participants