Skip to content

fix: add timeout for MCP ClientSession#364

Closed
njbrake wants to merge 6 commits intogoogle:mainfrom
njbrake:patch-1
Closed

fix: add timeout for MCP ClientSession#364
njbrake wants to merge 6 commits intogoogle:mainfrom
njbrake:patch-1

Conversation

@njbrake
Copy link
Copy Markdown

@njbrake njbrake commented Apr 23, 2025

I expect that we want to make this a configurable parameter that we elevate somewhere, but this seems like the most efficient way to start the 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."

I picked 5 seconds since that's the default for SSE


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."

I picked 5 seconds since that's the default for SSE
@google-cla
Copy link
Copy Markdown

google-cla Bot commented Apr 23, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@njbrake njbrake changed the title Prevent MCP ClientSession hang fix: add timeout for MCP ClientSession Apr 23, 2025
@hangfei hangfei requested a review from seanzhou1023 April 29, 2025 02:48
@njbrake
Copy link
Copy Markdown
Author

njbrake commented May 30, 2025

Hi @seanzhou1023 Do you have any updates about the status of this PR? Not sure if there's something else you'd like me to do. Thanks!

Copy link
Copy Markdown
Contributor

@genquan9 genquan9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contributions!

The team fixed the issue in 45ef668.

Instead of adding a new parameter, we decided to add a default timeout for StdioServerParameters.
Note, SseServerParams and StreamableHTTPServerParams already provide default timeout.

@genquan9 genquan9 closed this May 30, 2025
@njbrake
Copy link
Copy Markdown
Author

njbrake commented May 30, 2025

Thanks @genquan9: if I understand that other PR correctly though, isn't the timeout now hardcoded to 5 seconds? What if I need to increase it to something like 15 seconds for my application?

@genquan9
Copy link
Copy Markdown
Contributor

Currently it is default to 5 seconds.
Do you have a high requirement for the customization of the timeout?

The team is still discussing for the proper interface for the timeout when use StdioServerParameters.

@njbrake
Copy link
Copy Markdown
Author

njbrake commented May 31, 2025

Currently it is default to 5 seconds. Do you have a high requirement for the customization of the timeout?

The team is still discussing for the proper interface for the timeout when use StdioServerParameters.

Thanks! Yes we have a requirement for customization. Just today we were working with a developer who was using the Github MCP and it was timing out because it was a big query he was making that was taking longer than 5 seconds. Our framework supports Google ADK as well as a few other frameworks, and they have all added support for this customization of the timeout (you can see links to the PRs for all the other frameworks I made PRs for here mozilla-ai/any-agent#129), since it complies with the requirement that is stated in the MCP protocol definition (stated in the description of this PR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants