feat(client): Improve initialization state handling in McpAsyncClient #39
Conversation
tzolov
commented
Mar 11, 2025
tzolov
commented
- Add proper initialization state tracking using AtomicBoolean and Sinks
- Implement timeout handling for requests requiring initialization
- Ensure all client methods verify initialization state before proceeding
- Fix rootsListChangedNotification to check initialization state
- Improve error messages for uninitialized client operations
- Add proper initialization state tracking using AtomicBoolean and Sinks - Implement timeout handling for requests requiring initialization - Ensure all client methods verify initialization state before proceeding - Fix rootsListChangedNotification to check initialization state - Improve error messages for uninitialized client operations Signed-off-by: Christian Tzolov <[email protected]>
| * @param operation The operation to execute if the client is initialized | ||
| * @return A Mono that completes with the result of the operation | ||
| */ | ||
| private <T> Mono<T> withInitializationCheck(String errorMessage, |
There was a problem hiding this comment.
"Client must be initialized before " is repeated in each use, consider replacing errorMessage with action and just specify what's about to happen, e.g. action="pinging the server".
| // -------------------------- | ||
| // Utility Methods | ||
| // -------------------------- |
There was a problem hiding this comment.
It's a not some "utility" but a fundamental prerequisite to almost any operation, perhaps we can group it under "initialization methods"?
| return this.mcpSession.sendNotification(McpSchema.METHOD_NOTIFICATION_INITIALIZED, null).doOnSuccess(v -> { | ||
| this.initialized.set(true); | ||
| this.initializedSink.tryEmitValue(initializeResult); | ||
| }).thenReturn(initializeResult); |
There was a problem hiding this comment.
Awesome, this will make the client more robust :)
| @@ -149,6 +158,7 @@ public class McpAsyncClient { | |||
| this.clientCapabilities = features.clientCapabilities(); | |||
| this.transport = transport; | |||
| this.roots = new ConcurrentHashMap<>(features.roots()); | |||
| this.initializedTimeout = requestTimeout.multipliedBy(2); | |||
There was a problem hiding this comment.
Should it be this value and undocumented? I fear it would be worthwhile to add logging with a note after how long the client gave up. Also, this will have to be revisited once reconnects are considered.
Signed-off-by: Christian Tzolov <[email protected]>
|
Thanks for the feedback @chemicL . I've tried to address the review comments and to improve the documentation. |
Signed-off-by: Christian Tzolov <[email protected]>
Signed-off-by: Christian Tzolov <[email protected]>
…ents Replace hardcoded timeout constants with configurable getTimeoutDuration() method Remove automatic initialization in setUp methods to allow explicit testing Signed-off-by: Christian Tzolov <[email protected]>
Signed-off-by: Christian Tzolov <[email protected]>
| @@ -92,8 +96,16 @@ void testConstructorWithInvalidArguments() { | |||
| .hasMessage("Request timeout must not be null"); | |||
| } | |||
|
|
|||
| @Test | |||
| void testListToolsWithoutInitialization() { | |||
| assertThatThrownBy(() -> mcpAsyncClient.listTools(null).block()).isInstanceOf(McpError.class) | |||
There was a problem hiding this comment.
The test will last at least 600ms in case of WebClient and 4s for JDK HttpClient before the exception is observed, correct? If I am correct, it would be useful to consider using StepVerifier.withVirtualTime to emulate time passing by instead of adding more seconds to the time w validate the behaviour.
…#39) - Add proper initialization state tracking using AtomicBoolean and Sinks - Implement timeout handling for requests requiring initialization - Ensure all client methods verify initialization state before proceeding - Fix rootsListChangedNotification to check initialization state - Improve error messages for uninitialized client operations - improve JavaDoc - Add tests to verify proper error handling for uninitialized clients - Replace hardcoded timeout constants with configurable getTimeoutDuration() method - Remove automatic initialization in setUp methods to allow explicit testing Signed-off-by: Christian Tzolov <[email protected]>
