Skip to content

fix: propagate reactor context up till transport #154

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

Merged
merged 1 commit into from
Apr 23, 2025

Conversation

FH-30
Copy link
Contributor

@FH-30 FH-30 commented Apr 14, 2025

Motivation and Context

When implementing my own custom transport class which has the requirement of passing in a unique JWT Token per incoming request to call tool, I came to realize that context propagation up till the transport's sendMessage function is not working for regular requests but only working for notifications.

Upon further inspection this is because of how sendRequest and sendNotification are implemented in the McpClientSession class.

The sendRequest method creates a response mono and the transport's sendMessage creates a separate mono, the context from the client is only propagated into the response mono but isn't passed into the mono returned by sendMessage. This differs from sendNotification that returns the mono created by the transport's sendMessage back to the client so the context attached by the client gets propagated into the mono in the transport's sendMessage for notifications.
image

This MR modifies the McpClientSession's sendRequest to explicitly propagate the context into the mono created by transport's sendMessage allowing the transport's sendMessage to access the JWT Token from this context and perform the required logic.

How Has This Been Tested?

yes, ran the existing tests.

Breaking Changes

None

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

@FH-30 FH-30 force-pushed the fix-lost-reactor-ctx branch 4 times, most recently from 518b6fb to 4f5ae36 Compare April 14, 2025 09:46
@FH-30 FH-30 force-pushed the fix-lost-reactor-ctx branch from 4f5ae36 to 84ed798 Compare April 14, 2025 09:49
@FH-30
Copy link
Contributor Author

FH-30 commented Apr 14, 2025

Hi @tzolov, would like to know your thoughts on this, is there any hope of getting this PR merged in? Could let me know as well if you think there is a better solution to this issue, thanks in advance!

@tzolov tzolov added this to the 0.10.0 milestone Apr 15, 2025
Copy link
Member

@chemicL chemicL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution! I agree with the change 100%. As a follow-up it would be nice to add a test to validate the context in the transport sendMessage method but it can be challenging. However, please feel free to open a PR for it.

@chemicL chemicL merged commit 261554b into modelcontextprotocol:main Apr 23, 2025
1 check passed
@FH-30
Copy link
Contributor Author

FH-30 commented May 7, 2025

Hi @chemicL, regarding this change it actually only solves the problem of context passing for the async client, what do you think about supporting this for the sync client as well? The change however will probably be a breaking change as it requires the signature of the sync client's functions to change as they need to accept the context as an argument and pass it into the underlying mono returned by the underlying async client. Any thoughts on this?

@chemicL
Copy link
Member

chemicL commented May 7, 2025

@FH-30 I don't know your use case that much, but for transitioning between a synchronous world and a reactive one we can rely on Context Propagation. Typically, when it comes to security, as I understand, for instance with Spring Security, the security context can be bound to a ThreadLocal in case of thread-per-request scenarios. In the case of reactive programming, other means should be used - in case of Reactor it can be attached to the Context. Perhaps for your use case you could take advantage of the propagation mechanism, register a ThreadLocalAccessor and rely on its capture into the reactive Context when required? Then you can also enable automatic context propagation if you'd like to rely on the presence of the ThreadLocals. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants