fix: avoid dropped errors when transport is closed or uninitialized#995
fix: avoid dropped errors when transport is closed or uninitialized#995chemicL wants to merge 2 commits into
Conversation
Signed-off-by: Dariusz Jędrzejczyk <2554306+chemicL@users.noreply.github.com>
Signed-off-by: Dariusz Jędrzejczyk <2554306+chemicL@users.noreply.github.com>
| @Test | ||
| void testCloseInitialized() { | ||
| var transport = HttpClientStreamableHttpTransport.builder(host).build(); | ||
| // transport.connect(Function.identity()).block(); |
There was a problem hiding this comment.
How come we don't need to call "connect" anymore?
If we are not "connect"-ed, shouldn't sendMessage error rather than complete?
There was a problem hiding this comment.
Yeah, notice that this commented line is added. Previously there was no connect at all and it generated drops. Now you get a warning.
The thing is that for streamable http there is no "connect", it just sets up a json-rpc handler.
If you call sendMessage it is an asynchronous operation. We don't have any feedback on the sending whether the server responded. The historical reason is the separate channel on STDIO and on the legacy SSE transport. With Streamable HTTP it should actually be possible to change this paradigm but the intention at the time was not to break the common interfaces.
Since the test case used to work without the connect call, I didn't want to suddenly break it.
Apparently this test passes despite the transport ignoring all the responses.
WDYT should be the right behaviour? I can either remove the commented code (it slipped, but maybe it's good that it stayed here so we can have the discussion) or uncomment it and have the sendMessage fail if connect was not setup.
| import io.modelcontextprotocol.spec.McpTransportSessionNotFoundException; | ||
| import io.modelcontextprotocol.spec.McpTransportStream; | ||
| import io.modelcontextprotocol.spec.ProtocolVersions; | ||
| import io.modelcontextprotocol.spec.*; |
There was a problem hiding this comment.
nit - we typically don't do star imports.
| }) | ||
| .retryWhen(authorizationErrorRetrySpec()) | ||
| .flatMap(jsonrpcMessage -> this.handler.get().apply(Mono.just(jsonrpcMessage))) | ||
| .flatMap(jsonrpcMessage -> requestHandler.apply(Mono.just(jsonrpcMessage))) |
2 participants
Also avoid logging peer disconnect at warn level.