Skip to content

Add SubscribeToResourceAsync overload with handler delegate#1069

Merged
stephentoub merged 5 commits intomainfrom
copilot/add-overload-to-subscribe
Dec 5, 2025
Merged

Add SubscribeToResourceAsync overload with handler delegate#1069
stephentoub merged 5 commits intomainfrom
copilot/add-overload-to-subscribe

Conversation

Copy link
Contributor

Copilot AI commented Dec 5, 2025 •
edited
Loading

  • Explore the repository structure and understand the codebase
  • Identify SubscribeToResourceAsync method location and implementation
  • Understand the current subscription pattern (Subscribe + RegisterNotificationHandler + Unsubscribe)
  • Build and verify the solution works
  • Add new overload for SubscribeToResourceAsync that accepts a handler delegate
  • Implement the overload to register a notification handler and return IAsyncDisposable
  • Ensure the IAsyncDisposable disposes both the handler and calls UnsubscribeFromResourceAsync
  • Add comprehensive tests for the new overload
  • Update documentation for Subscribe/Unsubscribe/Register methods
  • Build and test the solution
  • Run code review and address feedback
  • Run security checks
  • Address PR review feedback
    • Make Uri overload non-async
    • Register handler before subscribing
    • Use UriTemplateComparer for URI equality
    • Improve disposal pattern
    • Add test for multiple handlers on same URI
    • Fix test to properly track second handler invocation
Original prompt

This section details on the original issue you should resolve

<issue_title>SubscribeToResourceAsync is difficult to use</issue_title>
<issue_description>Discussed here #1001 (comment)

It's not clear how to use SubscribeToResourceAsync from the signature / docs. You can subscribe, but what gets notified? To remove the subscription you need to Unsubscribe, which is different than RegisterNotificationHandler which provides an IAsyncDisposable for removal.

We discussed adding an overload for SubscribeToResourceAsync that would accept the handler and return an IAsyncDisposable. This would allow for a single call to subscribe without needing to handle unsubscribe separately.

We should also improve the documents for Subscribe/Unsubscribe/Register so it's clear how they work together.</issue_description>

<agent_instructions>Please add an overload to SubscribeToResourceAsync that accepts a delegate that will be invoked when a resource update notification is received. The overload should register than delegate as a notification handler for notifications specific to the specified resource, which likely means the delegate that's actually registered should be a wrapper around the user's and that should only invoke the user's when the notification is for the relevant resource. The method should return a Task<IAsyncDisable>, where disposing of the returned disposable will call UnsubscribeToResourceAsync for the corresponding resource and remove the registered notification handler.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits December 5, 2025 22:17
@stephentoub
Add overload for SubscribeToResourceAsync with handler delegate
df051c1 
- Add new overloads for SubscribeToResourceAsync that accept a handler delegate
- Handler is invoked only for notifications matching the subscribed resource URI
- Return IAsyncDisposable that unsubscribes and removes the handler when disposed
- Add comprehensive tests for the new functionality
- Update documentation with remarks explaining usage patterns

Co-authored-by: stephentoub <[email protected]>
@stephentoub
Fix code review feedback - explicitly pass CancellationToken.None dur…
9a065c5 
…ing disposal

Co-authored-by: stephentoub <[email protected]>
Copilot AI changed the title [WIP] Add overload to SubscribeToResourceAsync for easier use Add SubscribeToResourceAsync overload with handler delegate Dec 5, 2025
Copilot AI requested a review from stephentoub December 5, 2025 22:24
stephentoub
stephentoub approved these changes Dec 5, 2025
View reviewed changes
@stephentoub stephentoub marked this pull request as ready for review December 5, 2025 22:37
@stephentoub stephentoub requested a review from halter73 December 5, 2025 22:37
@stephentoub
Address PR feedback on SubscribeToResourceAsync implementation
c46f73f 
- Change Uri overload to non-async to avoid unnecessary async state machine
- Register handler before subscribing and cleanup on subscribe failure
- Use UriTemplateComparer.Instance.Equals for URI comparison
- Invert disposal check to early return pattern
- Use finally block for handler disposal instead of swallowing exceptions
- Add test for multiple handlers on same URI

Co-authored-by: stephentoub <[email protected]>
Copilot AI requested a review from stephentoub December 5, 2025 22:49
stephentoub
stephentoub reviewed Dec 5, 2025
View reviewed changes
@stephentoub
Fix test to properly track second handler invocation
e8e4b9a 
- Moved handler2CalledAgain TaskCompletionSource to top of test
- Modified handler2 to set handler2CalledAgain on second invocation
- Replaced Task.Delay with proper WaitAsync on handler2CalledAgain
- Test now correctly validates handler2 is invoked after handler1 disposal

Co-authored-by: stephentoub <[email protected]>
Copilot AI requested a review from stephentoub December 5, 2025 23:05
@stephentoub stephentoub merged commit 862492d into main Dec 5, 2025
17 checks passed
@stephentoub stephentoub deleted the copilot/add-overload-to-subscribe branch December 5, 2025 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@halter73 halter73 halter73 approved these changes

@stephentoub stephentoub Awaiting requested review from stephentoub

Assignees

@stephentoub stephentoub

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SubscribeToResourceAsync is difficult to use

3 participants

@halter73 @stephentoub