Add IAsyncDisposable to CosmosService and fix async disposal pattern#1532
Add IAsyncDisposable to CosmosService and fix async disposal pattern#1532anuchandy merged 2 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements IAsyncDisposable in CosmosService to fix an anti-pattern where the Dispose() method was implemented as async void, which prevented callers from awaiting the disposal operation. The implementation follows the Microsoft async disposal pattern guidelines for sealed classes.
Changes:
- Added
IAsyncDisposableto theICosmosServiceinterface andCosmosServiceclass - Replaced the
async void Dispose(bool disposing)method with a properDisposeAsyncCore()implementation - Added
DisposeAsync()method that properly implements the async disposal pattern - Modified synchronous
Dispose()to call the async core method synchronously
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.Cosmos/src/Services/ICosmosService.cs | Added IAsyncDisposable interface to ICosmosService |
| tools/Azure.Mcp.Tools.Cosmos/src/Services/CosmosService.cs | Implemented async disposal pattern with DisposeAsyncCore(), DisposeAsync(), and updated Dispose() methods; marked class as sealed |
| servers/Azure.Mcp.Server/changelog-entries/1768937539777.yaml | Added changelog entry documenting the bug fix |
eacd917 to
5372ccb
Compare
g2vinay
left a comment
There was a problem hiding this comment.
Any unit tests we can add for this ?
|
Hi Vinay - sorry I missed responding to your comment earlier. This follows Microsoft’s documented DI pattern for IAsyncDisposable: the DI container is responsible for when and how disposal is invoked. Adding a unit test here would mostly end up testing the framework/container behavior rather than our code, which is why I didn’t include a dedicated test for IAsyncDisposable. |
…icrosoft#1532) * Add IAsyncDisposable to CosmosService and fix async disposal pattern * address review feedback
What does this PR do?
[Provide a clear, concise description of the changes]The .NET DI container prefers DisposeAsync() during shutdown, pr Implement IAsyncDisposable in CosmosService. Following https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-disposeasync
Fixes: #1148
[Any additional context, screenshots, or information that helps reviewers]GitHub issue number?
[Link to the GitHub issue this PR addresses]Pre-merge Checklist
servers/Azure.Mcp.Server/CHANGELOG.mdand/orservers/Fabric.Mcp.Server/CHANGELOG.mdfor product changes (features, bug fixes, UI/UX, updated dependencies)servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationeng/scripts/Process-PackageReadMe.ps1. See Package README/servers/Azure.Mcp.Server/docs/azmcp-commands.mdand/or/docs/fabric-commands.md.\eng\scripts\Update-AzCommandsMetadata.ps1to update tool metadata in azmcp-commands.md (required for CI)ToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.json/servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline