Add transport-level cancellation support and proper cancellation token propagation (client->server)#1377
Conversation
…n propagation (client->server) Propagate HTTP request cancellation tokens to tool executions via JsonRpcMessageContext having the server able to handle the cancel notification directly or the server to detect the broken request
halter73
left a comment
There was a problem hiding this comment.
If we did something like this, we'd want to disable it when either Tasks or the EventStreamStore are in use where it's expected the client can abort the underlying HTTP request the started a given MCP request and then make a subsequent HTTP request to get the final MCP response.
And even in the default case where there's no way for the client to get the final MCP response after aborting the underlying HTTP request, I don't think it makes sense to cancel the primary token passed into the request handlers. An HTTP request could be aborted for any number of reasons outside of the app developer's control including network failures, and that's different than an explicit application-level request to cancel an operation which is signaled by a JSON-RPC notification.
For the cases where you do want to treat an aborted HTTP request the same as an explicit cancelation, because continuing the operation doesn't matter if the client cannot see the result, it's already possible to do so using IHttpContextAccessor as described in in https://csharp.sdk.modelcontextprotocol.io/concepts/httpcontext/httpcontext.html
So long story short, I don't think there are any changes we need to make around cancelation with the Streamable HTTP transport currently. But if we did, we should make sure you can still easily distinguish HTTP request cancellation vs receiving an explicit "notifications/cancelled" message. And I don't think we'd ever want to fire any existing CancellationTokens that don't fire currently when the HTTP request is aborted. Doing so would be a major behavioral breaking change.
|
I think the just-works nature of the cancel token client side triggering a cancel notification event to the server is fairly straightforward. Similarly server side having the cancel token fire when the client disconnects again would seem to make sense in many situations. I guess with the idea with the context accessor you mentioned was each method could use the injected IHttpContextAccessor to bind the RequestAborted cancel token to the method cancel token. I get not changing default behavior but one could add it as a simple option to the client/server if you want said auto-wiring or not (and defaulting to not). I think there are probably more scenarios though where a user using a cancel token client side would want it to cancel server side than not. I think there and more situations server side where a dropped http connection would be desirable to trigger the cancel token than not. Heck right now if the user calls dispose on the MCP client it sends a cancel notice so some auto wiring does exist. Anyway just my two cents, at a minimum adding more documentation around the default behaviors for cancel tokens (and when /what hey are expected to do) might be a plus. |
Closes #1365
Propagate HTTP request cancellation tokens to tool executions via JsonRpcMessageContext having the server able to handle the cancel notification directly or the server to detect the broken request.
The server should properly trigger the cancel token for any tools taking it if either of the above two happen allowing for the server to properly kill any long running tasks.
Motivation and Context
Right now it looks like cancelling works but it does nothing from a server perspective.
How Has This Been Tested?
Breaking Changes
Shouldn't, technically its a behavior change but I would assume most implementations that take the token would not have a reason for it to not cancel when the client request aborts.
Types of changes
Checklist
Additional context
Unable to figure out how to do a proper socket closure in a test case that would be helpful for the 3rd situation.