StreamableHttp client transport #573
Conversation
There was a problem hiding this comment.
Looks reasonable - some questions. Also it would be nice to refactor this to be less nested (I will re-review quick if you decide to - also, I asked claude what it thought for a refactor - I don't love it but it could be a good start to iterate on)
src/mcp/client/streamableHttp.py
Outdated
| Tuple of (read_stream, write_stream, terminate_callback) | ||
| """ | ||
|
|
||
| read_stream_writer, read_stream = anyio.create_memory_object_stream[ |
There was a problem hiding this comment.
[nit] I find these names kinda confusing - I've started using client_to_server_read/client_to_server_write etc because I find it easier to understand
I know that we use this names in a lot of places, just flagging here because I think it would be good to start changing
There was a problem hiding this comment.
I was planning to change it everywhere when I get a moment. I literally had to write it down on a piece of paper to remember which one is which, lol. I think having them in one place different will complicate things, but agree need to change it everywhere!
src/mcp/client/streamableHttp.py
Outdated
| continue | ||
| # Check for 404 (session expired/invalid) | ||
| if response.status_code == 404: | ||
| if is_initialization and session_id: |
There was a problem hiding this comment.
Initializing with an existing session-id seems like an odd edge case - if initializing wouldn't we expect there to be no session id? perhaps ignoring session id on initialization (or raising?) would be better / more in line with the spec?
There was a problem hiding this comment.
oh, it's wrong. Basically this is to cover the re-connect for:
When a client receives HTTP 404 in response to a request containing an Mcp-Session-Id, it MUST start a new session by sending a new InitializeRequest without a session ID attached.
if is_initialization - we just need to fail
if session_id - try to reconnect, BUT we need to do it on the client not client transport to have the correct initializatoin handshake
src/mcp/client/streamableHttp.py
Outdated
| @@ -0,0 +1,309 @@ | |||
| """ | |||
There was a problem hiding this comment.
[non-blocking] some of the code here is getting super nested - would love to see things broken out into functions where possible
src/mcp/client/streamableHttp.py
Outdated
| url: str, | ||
| headers: dict[str, Any] | None = None, | ||
| timeout: float = 30, | ||
| sse_read_timeout: float = 60 * 5, |
src/mcp/client/streamableHttp.py
Outdated
| async def streamablehttp_client( | ||
| url: str, | ||
| headers: dict[str, Any] | None = None, | ||
| timeout: float = 30, |
There was a problem hiding this comment.
Also here? I assume generally we have used float for timeouts, so this is likely fine, but i assume using timedeltas would be more correct here.
src/mcp/client/streamableHttp.py
Outdated
| async def post_writer(): | ||
| nonlocal session_id | ||
| try: | ||
| async with write_stream_reader: | ||
| async for message in write_stream_reader: |
There was a problem hiding this comment.
We should probably move post_writer out or at least find another way to not have so many indentions that the code becomes very much squeezed.
src/mcp/client/streamableHttp.py
Outdated
There was a problem hiding this comment.
This package name is weird and not according to pep 8 recommendations: https://peps.python.org/pep-0008/#package-and-module-names. Should be all lowercase, something like mcp.client.http.streamable
There was a problem hiding this comment.
oh, Typescript... thanks, will update!
Adding support for Streamable Http client transport
Follow ups