Prevent MCP ClientSession hang #35
Conversation
Per https://modelcontextprotocol.io/specification/draft/basic/lifecycle#timeouts "Implementations SHOULD establish timeouts for all sent requests, to prevent hung connections and resource exhaustion. When the request has not received a success or error response within the timeout period, the sender SHOULD issue a cancellation notification for that request and stop waiting for a response. SDKs and other middleware SHOULD allow these timeouts to be configured on a per-request basis."
|
@njbrake thanks for your contribution reviewing this ASAP |
There was a problem hiding this comment.
Overall I think this is a good change definetly going in the good direction with the current python SDK. I think we should go through with that.
However for me it still doesnt comply with the MCP spec. As you correctly stated, the MCP says timeouts should be configurable on a per-request basis. Here we only set it on a per-session basis. So I made a PR on the python-sdk to enable the per-request basis that we could then also allow here:
modelcontextprotocol/python-sdk#601
modelcontextprotocol/python-sdk#600
src/mcpadapt/core.py
Outdated
| @@ -71,6 +72,7 @@ def async_adapt( | |||
| @asynccontextmanager | |||
| async def mcptools( | |||
| serverparams: StdioServerParameters | dict[str, Any], | |||
| client_session_timeout_seconds: float | None = 5, | |||
There was a problem hiding this comment.
should we also support timedelta as in the official python sdk?
There was a problem hiding this comment.
TBH I kind of like it as seconds, but I don't have a strong opinion :)
For whatever it's worth, I was following the style that the openai agent sdk used for their implementation (which I made a similar patch to via https://github.com/openai/openai-agents-python/pull/580/files#diff-2432430f74d90a790e0707564a11b5773b85707bca4d4e1e03c6608a8ae48137R68)
There was a problem hiding this comment.
could be float | timedelta to support both maybe? Otherwise ok as is, maybe one thing I would change maybe though is the 5 sec default I think it's quite low especially for tool use maybe more around 15sec? What do you think?
There was a problem hiding this comment.
We can bump it up if you'd like, though TBH if any tool call takes more than 5 seconds I would probably want to know that it will happen and bump up the timeout on the application side (my side), I feel like 5 seconds should work for most use cases, and if it goes over 5 seconds personally I like that the tool will throw an error to let me know that the tool is taking a long time. (The 5 second default timeout is also consistent with the other framework defaults).
If I change it to support float | timedelta I'll need to update the parameter name. Do you have any opinions about what I should change the parameter name to be? It's hard to pick a parameter name that is reasonable for both timedelta and float. If you would rather use timedelta instead of float, I can make that change.
There was a problem hiding this comment.
you can keep the same name to be consistent with the official mcp python sdk, I also made the same comment that _seconds suffix is not ideal for a timedelta but in this case if we support both I guess that's fine
There was a problem hiding this comment.
Sounds good. One sec I'll update the code 👍
Co-authored-by: Guillaume Raille <[email protected]>
|
All good many thanks, for your contribution! |
|
@njbrake find your changes in v0.1.4 on onward should be published soon |
Follow up from grll#35
* Pass client_session_timeout through from MCPAdapt Follow up from #35 * Add tests for timeout parameters - Add test for connect_timeout that verifies TimeoutError is raised when server starts slowly - Add test for client_session_timeout_seconds parameter propagation and storage - Tests run quickly using 1-2 second timeouts for fast execution * ruff format --------- Co-authored-by: Guillaume Raille <[email protected]>
* Prevent MCP ClientSession hang Per https://modelcontextprotocol.io/specification/draft/basic/lifecycle#timeouts "Implementations SHOULD establish timeouts for all sent requests, to prevent hung connections and resource exhaustion. When the request has not received a success or error response within the timeout period, the sender SHOULD issue a cancellation notification for that request and stop waiting for a response. SDKs and other middleware SHOULD allow these timeouts to be configured on a per-request basis." * Update core.py * Update core.py * Update core.py * Update based on PR comments * Update core.py Co-authored-by: Guillaume Raille <[email protected]> --------- Co-authored-by: Guillaume Raille <[email protected]>
* Pass client_session_timeout through from MCPAdapt Follow up from grll#35 * Add tests for timeout parameters - Add test for connect_timeout that verifies TimeoutError is raised when server starts slowly - Add test for client_session_timeout_seconds parameter propagation and storage - Tests run quickly using 1-2 second timeouts for fast execution * ruff format --------- Co-authored-by: Guillaume Raille <[email protected]>
Per https://modelcontextprotocol.io/specification/draft/basic/lifecycle#timeouts
"Implementations SHOULD establish timeouts for all sent requests, to prevent hung connections and resource exhaustion. When the request has not received a success or error response within the timeout period, the sender SHOULD issue a cancellation notification for that request and stop waiting for a response.
SDKs and other middleware SHOULD allow these timeouts to be configured on a per-request basis."