Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix and enhance cancellation operations across MCP Sessions. #179

Conversation

Tyler-R-Kendrick
Copy link
Contributor

@Tyler-R-Kendrick Tyler-R-Kendrick commented Apr 1, 2025

  • made requests notify cancellation when cancellation tokens are used.
  • added extension methods to make sending these custom cancellation notifications simpler.

Motivation and Context

This helps adherence to the spec, specifically for requests - since notifications aren't cancellable according to the spec.

How Has This Been Tested?

Tests were added to cover the usage.

Breaking Changes

NA

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

NA

@stephentoub
Copy link
Contributor

The title and description of this PR are wrong, right?

@Tyler-R-Kendrick Tyler-R-Kendrick changed the title Fix pagination handling in McpServer (#177) Fix and enhance cancellation operations across MCP Sessions. Apr 1, 2025
@PederHP
Copy link
Collaborator

PederHP commented Apr 1, 2025

Did you remove the low level arbitrary notification handler registration option? If so I don't think that's a good idea. It's not something many will use but it should be possible, as MCP is an open standard and being to create servers and clients with arbitrary notifications should be possible. I also think it's important that the option to forego abstractions built on top of the protocol is always present.

I might have misunderstood your last commit. In that case, disregard this.

@Tyler-R-Kendrick
Copy link
Contributor Author

Did you remove the low level arbitrary notification handler registration option? If so I don't think that's a good idea. It's not something many will use but it should be possible, as MCP is an open standard and being to create servers and clients with arbitrary notifications should be possible. I also think it's important that the option to forego abstractions built on top of the protocol is always present.

I might have misunderstood your last commit. In that case, disregard this.

I just moved it to construction-time instead of runtime registration of arbitrary handlers. This avoids a race condition Stephen mentioned above..

@PederHP
Copy link
Collaborator

PederHP commented Apr 2, 2025

Did you remove the low level arbitrary notification handler registration option? If so I don't think that's a good idea. It's not something many will use but it should be possible, as MCP is an open standard and being to create servers and clients with arbitrary notifications should be possible. I also think it's important that the option to forego abstractions built on top of the protocol is always present.
I might have misunderstood your last commit. In that case, disregard this.

I just moved it to construction-time instead of runtime registration of arbitrary handlers. This avoids a race condition Stephen mentioned above..

So how would a server author register an arbitrary (ie custom notification) handler after this change?

Also if this means removing the option to register arbitrary handlers at run-time that is a loss of low-level functionality, which I think is a shame - but on the other hand it's such a niche use-case that it's hard to justify keeping it if removing it avoids a race condition.

@stephentoub
Copy link
Contributor

stephentoub commented Apr 2, 2025

Also if this means removing the option to register arbitrary handlers at run-time that is a loss of low-level functionality, which I think is a shame

What is the use case where, after you already have an established connection, you want to add a handler that you can then never remove? And why is that relevant to notifications but not to requests?

It's also not a loss of functionality. If you really want to do that, you can register a handler that then consults whatever list it wants, and that list can be modified whenever to your heart's content :)

@PederHP
Copy link
Collaborator

PederHP commented Apr 2, 2025

Also if this means removing the option to register arbitrary handlers at run-time that is a loss of low-level functionality, which I think is a shame

What is the use case where, after you already have an established connection, you want to add a handler that you can then never remove? And why is that relevant to notifications but not to requests?

It's also not a loss of functionality. If you really want to do that, you can register a handler that then consults whatever list it wants, and that list can be modified whenever to your heart's content :)

Yeah, as I wrote above in the other comment it's weird and it's easy to work around (if one had a hypothetical need to use it). And I agree that the same would go for requests. Probably shouldn't have mentioned it.

But the custom notification handler registration is important, I think, but maybe I misunderstood about that. It looks to have been removed from what I can see (the helper class is internal)?

Maybe I should clarify that I'm talking about custom handlers for user-defined method notifications, not custom handlers for the "built-in" notifications.

@stephentoub
Copy link
Contributor

I'd told Tyler I would be doing the work to add handlers at construction time today. I think he was just prototyping it here.

@Tyler-R-Kendrick
Copy link
Contributor Author

Tyler-R-Kendrick commented Apr 2, 2025

Also if this means removing the option to register arbitrary handlers at run-time that is a loss of low-level functionality, which I think is a shame

What is the use case where, after you already have an established connection, you want to add a handler that you can then never remove? And why is that relevant to notifications but not to requests?
It's also not a loss of functionality. If you really want to do that, you can register a handler that then consults whatever list it wants, and that list can be modified whenever to your heart's content :)

Yeah, as I wrote above in the other comment it's weird and it's easy to work around (if one had a hypothetical need to use it). And I agree that the same would go for requests. Probably shouldn't have mentioned it.

But the custom notification handler registration is important, I think, but maybe I misunderstood about that. It looks to have been removed from what I can see (the helper class is internal)?

Maybe I should clarify that I'm talking about custom handlers for user-defined method notifications, not custom handlers for the "built-in" notifications.

I was POC-ing for Stephen so he could have a jumping off point.
Custom registration still exists because the of the dictionaries I introduced to the options. If you want a custom message handler as a server author, you would just do something like:

new McpServerOptions()
{
    RequestHandlers = {
        ["customRequest"] = [CustomRequestHandler, ...otherCustomRequestHandlers],
    },
    NotificationHandlers = {
        ["customNotification"] = [CustomNotificationHandler, ...otherCustomNotificationHandlers],
    },
}

You would either do this in your DI configuration, or wherever you construct an MCP server. Same goes for an MCP client for handlers to things like sampling.

@Tyler-R-Kendrick
Copy link
Contributor Author

I'd told Tyler I would be doing the work to add handlers at construction time today. I think he was just prototyping it here.

I'll likely roll back the last 3 commits when you're finished. Don't worry about trying to merge in my attempt to your work if it makes it more difficult for you :)

@stephentoub
Copy link
Contributor

@Tyler-R-Kendrick, now that the handler registration has been fixed up, can you rebase this on main? Thanks.

@Tyler-R-Kendrick
Copy link
Contributor Author

@Tyler-R-Kendrick, now that the handler registration has been fixed up, can you rebase this on main? Thanks.

Working on it now.

@Tyler-R-Kendrick Tyler-R-Kendrick force-pushed the cancellation-enhancements branch from ae90960 to 90536c2 Compare April 6, 2025 00:24
@stephentoub stephentoub force-pushed the cancellation-enhancements branch from d744159 to d195bd6 Compare April 8, 2025 20:01
@stephentoub stephentoub marked this pull request as ready for review April 8, 2025 20:05
@stephentoub stephentoub merged commit 03e3094 into modelcontextprotocol:main Apr 8, 2025
13 of 15 checks passed
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