-
Notifications
You must be signed in to change notification settings - Fork 261
feat: pass timeout config while init mcp client #272
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
feat: pass timeout config while init mcp client #272
Conversation
47b5cbb to
b76a6db
Compare
@wenhaozhao can you elaborate on how this would be used?
How would one configure this? It might be useful to document this. PS: Looks like this could be a solution for #285 from @rsaulo. |
McpSyncClient is a wrapper around McpAsyncClient that provides a blocking API via block(). When sending requests, the timeout parameter becomes effective within the reactor-chain.
so, you can config timeout like this: |
and there a another pr #273, also include timeout config |
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.
Pull Request Overview
This PR adds support for passing timeout configuration when initializing an MCP client. The changes allow customizable initialization and request timeouts to be extracted from connection parameters instead of using hardcoded values.
- Extracts timeout values from
SseServerParameterswhen available - Replaces hardcoded 10-second timeout with configurable timeouts
- Falls back to default 10-second timeouts when no custom values are provided
core/src/main/java/com/google/adk/tools/mcp/McpSessionManager.java
Outdated
Show resolved
Hide resolved
| .initializationTimeout(initializationTimeout == null ? Duration.ofSeconds(10) : initializationTimeout) | ||
| .requestTimeout(requestTimeout == null ? Duration.ofSeconds(10) : requestTimeout) | ||
| .capabilities(ClientCapabilities.builder().build()) | ||
| .build(); | ||
| InitializeResult initResult = client.initialize(); | ||
| logger.debug("Initialize Client Result: {}", initResult); | ||
|
|
||
| return client; | ||
| } |
Copilot
AI
Jul 22, 2025
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.
[nitpick] The ternary operator with null checks makes the code less readable. Consider extracting the default timeout logic into a helper method or using Optional.ofNullable().orElse() for cleaner null handling.
| .initializationTimeout(initializationTimeout == null ? Duration.ofSeconds(10) : initializationTimeout) | ||
| .requestTimeout(requestTimeout == null ? Duration.ofSeconds(10) : requestTimeout) |
Copilot
AI
Jul 22, 2025
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.
The default timeout value Duration.ofSeconds(10) is duplicated. Consider extracting this magic number into a named constant to improve maintainability and ensure consistency.
| .initializationTimeout(initializationTimeout == null ? Duration.ofSeconds(10) : initializationTimeout) | |
| .requestTimeout(requestTimeout == null ? Duration.ofSeconds(10) : requestTimeout) | |
| .initializationTimeout(initializationTimeout == null ? DEFAULT_TIMEOUT : initializationTimeout) | |
| .requestTimeout(requestTimeout == null ? DEFAULT_TIMEOUT : requestTimeout) |
Awesome! We should probably document this, somehow somewhere? Would you be willing to raise a PR to add a short section e.g. on https://github.com/google/adk-docs/blob/main/docs/get-started/streaming/quickstart-streaming-java.md for https://google.github.io/adk-docs/get-started/streaming/quickstart-streaming-java/ about this? |
Probably better on https://google.github.io/adk-docs/tools/mcp-tools, actually? |
9f6f419 to
a4a625a
Compare
https://google.github.io/adk-docs/tools/mcp-tools maybe better, but it's written in python. |
core/src/main/java/com/google/adk/tools/mcp/McpSessionManager.java
Outdated
Show resolved
Hide resolved
vorburger
left a comment
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.
Could you address the build failure due to the code format? (It's something we just added.)
fine, i've resloved |
99984af to
d255167
Compare
Watch new issue #297 re. the missing doc (in general about MCP, not about this timeout); maybe once that's resolved, you could add this. |
|
Just tested with main branch and it is working. For me is working and it is OK. Just a comment, now I see that in McpSessionManager, the class uses the sseReadTimeout parameter from SseServerParameters to set the requestTimeout, BUT, this parameter has a default of 5 MINUTES on the SseServerParameters builder, what is turning this parameter as 5 minutes forever. I understand that we need a specific parameter on SseServerParameters like "mcpClientRequestTimeout" to use here as sseReadTimeout is not the same as requestTimeout. But for my use case it is working!! Thanks. @vorburger , thanks for the quick attention on this matter. |

allow to pass timout config while create mcp-client