Skip to content

Conversation

@stephentoub
Copy link
Contributor

Currently request handlers are set on the capability objects, but notification handlers are set after construction via an AddNotificationHandler method on the IMcpEndpoint interface. This moves handler specification to be at construction as well. This makes it more consistent with request handlers, simplifies the IMcpEndpoint interface to just be about message sending, and avoids a concurrency bug that could occur if someone tried to add a handler while the endpoint was processing notifications.

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.

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

Comments suppressed due to low confidence (1)

tests/ModelContextProtocol.Tests/SseIntegrationTests.cs:233

  • The square bracket initializer syntax used for NotificationHandlers may not be standard C#. Consider replacing it with a conventional collection initializer (e.g., 'new [] { ... }' or 'new List<KeyValuePair<string, Func<JsonRpcNotification, Task>>>() { ... }') to ensure compatibility.
NotificationHandlers = [new("test/notification", (args) =>

Copy link
Contributor

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

While I think it might be useful to be able to add handlers mid-session, the fact you couldn't remove them made it half-baked regardless. And we can always add that capability back if there's a real need.

@stephentoub
Copy link
Contributor Author

And we can always add that capability back if there's a real need.

Someone can also do it on their own, just by adding a handler that reads from any collection they want to subsequently mutate. That single handler will be invoked and can effectively proxy the call to any number of handlers elsewhere.

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Thanks

Currently request handlers are set on the capability objects, but notification handlers are set after construction via an AddNotificationHandler method on the IMcpEndpoint interface. This moves handler specification to be at construction as well. This makes it more consistent with request handlers, simplifies the IMcpEndpoint interface to just be about message sending, and avoids a concurrency bug that could occur if someone tried to add a handler while the endpoint was processing notifications.
@stephentoub stephentoub force-pushed the movenotificationregistration branch from c57d65d to 824d01d Compare April 4, 2025 17:03
@stephentoub stephentoub merged commit d21d933 into modelcontextprotocol:main Apr 4, 2025
8 checks passed
@stephentoub stephentoub deleted the movenotificationregistration branch April 4, 2025 17:11
@stephentoub
Copy link
Contributor Author

I've been thinking more about this. This is a good direction over what was there before, which prohibited removal of added handlers, limiting their use. But if we instead allow removal, it becomes much more useful. Like with CancellationToken.Register, it the enables transiently listening for certain notifications, so for example you could create a progress token, hook up a notification handler that pays attention to only that token, then start the operation using that token, and then after the operation completes, remove that handler. It's hard to imagine how this could be achieved in a general fashion against an arbitrary IMcpClient/Server in other ways.

Given that, I'm going to move back in that direction, putting it back onto the interface, but with a shape like CT.Register that enables scoped removal.

cc: @halter73, @eiriktsarpalis, @PederHP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants