feat(ws): adds ws transport client#139
feat(ws): adds ws transport client#139Aliaksie wants to merge 2 commits intomodelcontextprotocol:mainfrom
Conversation
Aliaksie
changed the title
| .version(HttpClient.Version.HTTP_1_1) | ||
| .connectTimeout(Duration.ofSeconds(10)); | ||
|
|
||
| private ObjectMapper objectMapper = new ObjectMapper(); |
There was a problem hiding this comment.
Lots of object mappers defined in lots of files. It would seem like it might be time to build a Jackson module and lock in on one that can be used across the code base.
|
|
||
| public Mono<Void> connect(final Function<Mono<McpSchema.JSONRPCMessage>, Mono<McpSchema.JSONRPCMessage>> handler) { | ||
| if (!state.compareAndSet(TransportState.DISCONNECTED, TransportState.CONNECTING)) { | ||
| return Mono.error(new IllegalStateException("WebSocket is already connecting or connected")); |
There was a problem hiding this comment.
It might be better to point the actual state in the exception. I always hate error messages like "its either this or that", instead tell me what it was.
| fullMessage); | ||
| handler.apply(Mono.just(msg)).subscribe(); | ||
| } | ||
| catch (Exception e) { |
There was a problem hiding this comment.
I fixed this in #156. It would be nice to merge that and then we can stop catch Exception use a more fine grained type.
| WebSocket webSocket = webSocketRef.getAndSet(null); | ||
| if (webSocket != null && state.get() == TransportState.CONNECTED) { | ||
| state.set(TransportState.CLOSED); | ||
| return Mono.fromFuture(webSocket.sendClose(WebSocket.NORMAL_CLOSURE, "Closing")).then(); |
There was a problem hiding this comment.
A constant here would be better possibly.
| String json = objectMapper.writeValueAsString(message); | ||
| return Mono.fromFuture(ws.sendText(json, true)).then(); | ||
| } | ||
| catch (Exception e) { |
There was a problem hiding this comment.
UncheckedIOException seems nice here.
| catch (Exception e) { | ||
| return Mono.error(e); | ||
| } | ||
| }).retryWhen(Retry.backoff(3, Duration.ofSeconds(3)).filter(err -> { |
There was a problem hiding this comment.
Externalize or at least put in constants.
|
Hi, @Aliaksie. Thank you for the effort on this. Did you notice WebSocket mentioned somewhere in the MCP specification or discussions? My personal take would be that less is more - although the WebSocket transport would be a nice-to-have, it would add a maintenance obligation. Once it becomes included in the spec, we would of course welcome contributions. I am tempted to discard the PR unfortunately, but I'll wait for @tzolov to make the call. |
Hey @chemicL ! thank you for your answer! honestly I created this PR before I studied the specification more deeply... and you are right at the moment the specification does not define this type of transport... maybe it makes sense to abandon this PR for now in favor of more important tasks like this(PR, PR) :-) |
4 participants
This pull request introduces the initial implementation for a WebSocket (WS) client transport in the system. It focuses on enabling communication over WebSocket for MCP client-side transport, laying the foundation for future enhancements such as adding a WebSocket server provider.
Motivation and Context
How Has This Been Tested?
This functionality has been tested in a local environment using WebSocket server containers.
Breaking Changes
NO
Types of changes
Checklist
Additional context
This pull request introduces the first implementation for the ws client transport. The ws server provider is planned for future implementation. Further improvements and features will be added in subsequent PRs.
let me know if i missed something and need to add or if these changes don't make sense no problem just close this pr :-)