Skip to content

Optimize nested streams #33

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

Optimize nested streams #33

wants to merge 8 commits into from

Conversation

CrazyHZM
Copy link
Contributor

@CrazyHZM CrazyHZM commented Mar 7, 2025 •
edited
Loading

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@CrazyHZM CrazyHZM force-pushed the fix/non-blocking-context branch from 7893d2b to 28594fb Compare March 7, 2025 16:25
@CrazyHZM CrazyHZM changed the title Fix subscribe in non-blocking context Optimize nested streams Mar 7, 2025
@chemicL
Copy link
Member

chemicL commented Mar 27, 2025

@CrazyHZM the changes are now finalized and released as 0.8.0. Additionally, the deprecations are removed. You can notice that the DefaultMcpSession is now McpClientSession. For comparison of your suggested changes, please review io.modelcontextprotocol.spec.McpServerSession#handle. I suppose similar avoidance of fire-and-forget subscribe() calls here and there can be implemented. Would you like to revisit this subject?

@CrazyHZM
Copy link
Contributor Author

CrazyHZM commented Mar 28, 2025 •
edited
Loading

@CrazyHZM the changes are now finalized and released as 0.8.0. Additionally, the deprecations are removed. You can notice that the DefaultMcpSession is now McpClientSession. For comparison of your suggested changes, please review io.modelcontextprotocol.spec.McpServerSession#handle. I suppose similar avoidance of fire-and-forget subscribe() calls here and there can be implemented. Would you like to revisit this subject?

@chemicL I reviewed the code again, as you said, it can follow the io.modelcontextprotocol.spec.McpServerSession#handle logic consistent, I think if we want to get rid of the behaviors of the subscribe, we need a McpClientTransportProvider to hold McpClientSession object, it functions similarly to ServerTransportProvider, as such, the subscribe method of ' handle message` is no longer needed.
What do you think? I will modify this part of the logic if there are no further questions.

@CrazyHZM CrazyHZM force-pushed the fix/non-blocking-context branch 5 times, most recently from e62cc90 to 5f2fc53 Compare March 28, 2025 18:14
@CrazyHZM CrazyHZM force-pushed the fix/non-blocking-context branch from 5f2fc53 to 56facbb Compare March 28, 2025 18:25
CrazyHZM added 4 commits April 10, 2025 00:04
@CrazyHZM
refactor:The connection does not need to be in McpClientSession
77e871a 
Signed-off-by: JermaineHua <[email protected]>
@CrazyHZM
Merge branch 'refs/heads/fix/non-blocking-context_test' into fix/non-…
5efe8e6 
…blocking-context

# Conflicts:
#	mcp/src/main/java/io/modelcontextprotocol/spec/McpClientSession.java
@CrazyHZM
Merge branch 'refs/heads/main' into fix/non-blocking-context
2c231df 
# Conflicts:
#	mcp-spring/mcp-spring-webflux/src/test/java/io/modelcontextprotocol/WebFluxSseIntegrationTests.java
#	mcp-spring/mcp-spring-webmvc/src/test/java/io/modelcontextprotocol/server/WebMvcSseIntegrationTests.java
#	mcp/src/main/java/io/modelcontextprotocol/client/transport/HttpClientSseClientTransport.java
#	mcp/src/main/java/io/modelcontextprotocol/client/transport/StdioClientTransport.java
#	mcp/src/main/java/io/modelcontextprotocol/spec/McpClientSession.java
#	mcp/src/test/java/io/modelcontextprotocol/client/HttpSseMcpAsyncClientTests.java
#	mcp/src/test/java/io/modelcontextprotocol/client/HttpSseMcpSyncClientTests.java
#	mcp/src/test/java/io/modelcontextprotocol/client/StdioMcpAsyncClientTests.java
#	mcp/src/test/java/io/modelcontextprotocol/client/StdioMcpSyncClientTests.java
#	mcp/src/test/java/io/modelcontextprotocol/client/transport/HttpClientSseClientTransportTests.java
#	mcp/src/test/java/io/modelcontextprotocol/spec/McpClientSessionTests.java
@CrazyHZM
Merge branch 'refs/heads/main' into fix/non-blocking-context
d18d478 
# Conflicts:
#	mcp-test/src/main/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java
#	mcp/src/main/java/io/modelcontextprotocol/client/McpAsyncClient.java
#	mcp/src/test/java/io/modelcontextprotocol/client/AbstractMcpAsyncClientTests.java
@chemicL
Copy link
Member

chemicL commented Apr 14, 2025

Thank you for the work on this so far @CrazyHZM. It seems the code has diverged again, I'm sorry. Would you be interested in refreshing this PR or would you like me to take over and refactor the code to try and avoid the disconnected subscriptions?

@CrazyHZM
Copy link
Contributor Author

Thank you for the work on this so far @CrazyHZM. It seems the code has diverged again, I'm sorry. Would you be interested in refreshing this PR or would you like me to take over and refactor the code to try and avoid the disconnected subscriptions?

@chemicL I will continue this PR.

@CrazyHZM CrazyHZM force-pushed the fix/non-blocking-context branch from 6bf6f72 to 7a77925 Compare April 14, 2025 16:19
@CrazyHZM
Fix some unit tests
a736ea0 
Signed-off-by: JermaineHua <[email protected]>
@CrazyHZM CrazyHZM force-pushed the fix/non-blocking-context branch from 7a77925 to a736ea0 Compare April 14, 2025 16:36
@chemicL
Copy link
Member

chemicL commented Apr 15, 2025 •
edited
Loading

@CrazyHZM I see you merged the actual main into your branch but not into your own main on which this branch is based on. It makes it really difficult to review this PR with all the unrelated changes. Please ensure the PR has only the relevant changes in the commits and in the diff.

@CrazyHZM
Copy link
Contributor Author

CrazyHZM commented Apr 16, 2025 •
edited
Loading

@chemicL It does not carry other changes in the main branch, perhaps this change is too big? I can submit relevant steps, I can simply describe the change of the content, mainly providing similar in ServerTransportProvider and McpClientTransportProvider, used to manage the session and transport. It borrows from the idea you provided earlier.
If we don't, there's no way to really avoid fire-and-forget subscribe(), like this PR one down here:
#165

chemicL pushed a commit that referenced this pull request Apr 23, 2025
@CrazyHZM @chemicL
Optimize client nested streams in McpClientSession (#33)
04046ca 
Signed-off-by: JermaineHua <[email protected]>
@chemicL
Copy link
Member

chemicL commented Apr 23, 2025

@CrazyHZM thank you. I checked out your code and applied it over the recent main branch. I also added a few modifications and the result is 04046ca. Thank you for your contribution!

@chemicL chemicL closed this Apr 23, 2025
chemicL added a commit that referenced this pull request Apr 23, 2025
@chemicL
Polish #33
866732c 
Signed-off-by: Dariusz Jędrzejczyk <[email protected]>
@chemicL
Copy link
Member

chemicL commented Apr 23, 2025

I noticed that during the Easter break I somehow lost my changes - I applied them in 866732c.

@chemicL chemicL mentioned this pull request Apr 23, 2025
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chemicL chemicL chemicL requested changes

Assignees

@chemicL chemicL

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@CrazyHZM @chemicL