Sending cancellation notification to server based on client anyio.CancelScope status #628
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davemssavage
changed the title
davemssavage
commented
May 5, 2025
| f"{request.__class__.__name__}. Waited " | ||
| f"{timeout} seconds." | ||
| ), | ||
| with anyio.CancelScope(shield=not cancellable): |
There was a problem hiding this comment.
Alternative to this might be to separate client cancelation and server cancelation, e.g. client can be cancelled and server cancellation event is only set if a flag such as 'propagate_client_cancelation_to_server' (shorter names available) is set on the request.
davemssavage
changed the title
davemssavage
changed the title
…which states that initialised is not cancellable as per https://modelcontextprotocol.io/specification/2025-03-26/basic/utilities/cancellation
davemssavage
commented
May 5, 2025
| @@ -33,6 +35,12 @@ | |||
| SendRequestT = TypeVar("SendRequestT", ClientRequest, ServerRequest) | |||
| SendResultT = TypeVar("SendResultT", ClientResult, ServerResult) | |||
| SendNotificationT = TypeVar("SendNotificationT", ClientNotification, ServerNotification) | |||
| SendNotificationInternalT = TypeVar( | |||
There was a problem hiding this comment.
This code is ugly appears to need some python typechecking sorcery to tidy up though
davemssavage
commented
May 5, 2025
| await client_session.call_tool( | ||
| "slow_tool", | ||
| cancellable=False, | ||
| read_timeout_seconds=timedelta(seconds=10), |
There was a problem hiding this comment.
A test that validates a short timeout with an uncancelable call would be sensible
|
Also potential fix for #230 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
MCP 2025-03-26 introduces a cancellation notification [1] this patch simplifies the usage of this function for the ClientSession by using the anyio.CancelScope as a trigger to send the cancellation notification and is a potential fix for #160
How Has This Been Tested?
Added unit tests to assert behaviour and tidied up previous work around for lack of this behaviour in tests/shared/test_session.py#test_request_cancellation
Breaking Changes
This patch sends a CancelNotification to any in flight requests to the server at the point the client is cancelled.
Clients that would have expected calls to complete on the server regardless of status of cancel scope on the client will need to be updated.
As a work around for any clients where this behavior is not desired this patch adds a cancellable flag to send_request and call_tool to enable clients to opt out of this behaviour so server tasks must respond prior to exitting.
This cancellable=False behaviour is required for the initialised call from the client as described in [1] though this does open up a raft of questions on what is supposed to happen if the initialised call blocks on the server. A timeout on the client might be appropriate however this opens up follow on questions on what subsequent behaviour should be after an initialisation timeout.
This approach of a breaking change is suggested as author expects in general most clients would prefer to cancel an expensive server tasks if client is cancelled and initialisation tasks are expected to be quick and non-blocking on the server.
Types of changes
Checklist
Additional context
I've added a docstring to describe the behaviour to send request. I suspect wider documentation updates would be sensible, I'm happy to write something if this patch looks useful.
It might also be useful to separate out the idea of client cancelation with server cancelation such that a client can cancel its work but leave the server task running. Comments on the pull request on the best approach is probably necessary.
[1] https://modelcontextprotocol.io/specification/2025-03-26/basic/utilities/cancellation