-
Notifications
You must be signed in to change notification settings - Fork 167
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
[RFC] Explicitly require JSON-RPC batch support #228
Conversation
- An array [batching](https://www.jsonrpc.org/specification#batch) one or more | ||
_requests and/or notifications_ | ||
- An array [batching](https://www.jsonrpc.org/specification#batch) one or more | ||
_responses_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implies mutual exclusion between requests/notifications and responses. Is that the intent?
It seems interesting to support the batching of all of them in a single batch. The structure of JSON-RPC supports this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is indeed the intent, based on how JSON-RPC defines batching in https://www.jsonrpc.org/specification#batch.
I don't think plain JSON-RPC permits mixing the two, although it's not really a bidirectional spec, so hard to say. My main concern here would be remaining compatible with any off-the-shelf JSON-RPC libraries that one might happen to grab.
- The SSE stream **SHOULD** eventually include one JSON-RPC _response_ per each | ||
JSON-RPC _request_ sent in the POST body. | ||
- The server **MAY** send JSON-RPC _requests_ and _notifications_ before sending a | ||
JSON-RPC _response_. These messages **SHOULD** relate to the originating client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a wrinkle/complexity in SDK and client implementations regarding to how this relation to an original request is transmitted inside the message - especially if the messages are replayed as the response to a stream GET request. For progress to requests we have progress tokens whereas there is no such facility for the Sampling. Maybe we could specify using a Progress Token inside of the meta of the SamplingRequest in this case? This would be consistent at least with progress.
As an example - consider the possibility that Claude Desktop gains the ability to do Sampling/Progress - since it communicates purely through a serialised stream of StdIO - there is no way to relate a Sampling Request to an outgoing request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Relate to" is specified quite loosely, on purpose. There doesn't need to be an identifier that links the two, this is basically just saying that servers should not abuse the per-request SSE stream to send unrelated requests.
A neat (and probably simple to implement from the server-side) enhancement here might be for replayability to be available to clients via HTTP GET with the last-message-id and session id headers. This would return a JSONRPC batch array in the response. Considerations:
|
Resumability is already supported with
Limits can be implementation-defined. I don't follow how a query parameter would work here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we had separated the addition of batching and reformatting a bit more, this was not super easy to read through and understand the exact delta.
They are separated per commit. |
A JSON-RPC peculiarity that might throw implementers is that a response to a batch request may be collapsed into a single object if it ends up being an array with a single object. Maybe we can make a note of that somewhere. |
Hmm, my interpretation of the spec is that an array is required unless an overall error (like a parse error) occurs—in which case, a single response object can be used to indicate that error. |
From the discussion in this https://github.com/modelcontextprotocol/specification/pull/206, I still don't understand why batch support is being added, or is it just to improve JSON-RPC compatibility? |
Following up on discussion around the Streamable HTTP transport, this codifies a requirement that MCP implementations MUST support JSON-RPC batching in the new draft protocol revision. Until now, batching wasn't mentioned at all in the spec, and our SDKs have not generally supported it (which technically puts us out of compliance with JSON-RPC).
This required some modification to the language around the Streamable HTTP transport—good thing it's still a draft! Basically, if a POST request contains zero JSON-RPC requests, the response should be HTTP 202 Accepted; otherwise, the server can respond with one JSON object (which may itself be a batch—i.e., an array of objects) or an SSE stream.
As a minor cleanup thing, I also coalesced multiple different docs pages that all had slightly varying explanation of the core message types of JSON-RPC.