StreamableHttp - client refactoring and resumability support #595
Conversation
There was a problem hiding this comment.
@ihrpr posting some comments early for curiosity's sake, then continuing review
src/mcp/client/session.py
Outdated
| @@ -255,9 +260,20 @@ async def unsubscribe_resource(self, uri: AnyUrl) -> types.EmptyResult: | |||
| ) | |||
|
|
|||
| async def call_tool( | |||
| self, name: str, arguments: dict[str, Any] | None = None | |||
There was a problem hiding this comment.
I'm kind of curious why you decided against a separate resume_tool method - at first blush it feels clearer as an API and it avoids the need to pass dummy arguments.
edit: looking at the way resume_tool was implemented in the earlier commits, I can see how the refactor disentangles the logic and avoids the fake "resume_from_token" tool call, but I do wonder whether resume_tool is a better Session API and (if so) whether it can be implemented in a way that preserves the current improvements. Interested in your thoughts (and definitely not looking to hold up the PR over it regardless)
There was a problem hiding this comment.
resume_tool will only be applicable for one transport, introducing a new API for only one transport feels confusing
But in the same way I'm not happy with calling tool to resume the tool.
It's all back to the discussion how we support long runnng tools. Now I feel like we should just remove the resumption from session API call_tool and later introduce clear API how to do it for any transport
There was a problem hiding this comment.
Update: removed from call_tool API, left only on the lowest level send_request. Once we are happy with long running tools discussion we can add it properly
| ctx.metadata.on_resumption_token_update if ctx.metadata else None, | ||
| ) | ||
| if is_complete: | ||
| break |
There was a problem hiding this comment.
possibly a dumb question, but if we're not returning anything to indicate that we got something back before timing out, how does the client know to (e.g.) retry the resumption request?
There was a problem hiding this comment.
in _handle_sse_event we are sending the reponses and notifications (as well as saving resumtion tokes) before we return true or false if we need to stop listening. Spec states that the last even should be response, hence we need to stop there. Client will already received the response before we got to this break
…essage-for-related-request
…pr/client-resumability
…essage-for-related-request
…pr/client-resumability
The base branch was changed.
This PR implements client-side support for resumable execution in MCP, allowing
clients to resume interrupted long-running requests, for example, after network disconnections.
NOTE: support was added only on the lowest level, to send request, as we are not ready to add it to
call_toolAPI as there is ongoing discussion about long running tasks. Once we are clear on the API design for long running tools, we will add it to session API.Summary
Note: stacked on top of #591