-
Notifications
You must be signed in to change notification settings - Fork 538
feat: Add request with context support. #226
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: He-Pin <[email protected]>
@tzolov Wdyt about this, I would like to push this, thank you. Another way to handle it is extends the final Map<String, Object> argumentsMap = new HashMap<>(callToolArguments);
if (myMcpContext != null) {
argumentsMap.put(CONTEXT_KEY, myMcpContext);
}
final var newCallToolRequest = new McpSchema.CallToolRequest(callToolRequest.name(), argumentsMap); |
We would also like to use this feature. If this PR is being blocked due to the breaking changes it introduces, perhaps the alternative approach mentioned in the referenced issue—based on Reactor context and ThreadLocal-based implementation could be considered instead. |
I'm interested in using these changes as well. i was looking through them. @He-Pin i'm curious as to the decision to use a ConcurrentHashMap in the MpcContext. as far as i can see, the context is created for use within a single request invocation. assuming that some initial modification is done during creation and then the context is passed to the handler, it doesn't seem like you would need additional synchronization control within the context itself? am i missing a use case which would require ongoing interaction with the context among multiple threads? also, i assume it's an oversight, but none of the methods on McpContext are public? |
We currently have to encode ctx in the params(patch the json). |
@He-Pin was that an answer to my question? if so, i don't think i understand it. |
looks like a change was committed which removes the need for this context when using the sync server: b5a1e3b |
Motivation and Context
refs: #225
How Has This Been Tested?
During development.
Breaking Changes
Yes, binary not compatibility
Types of changes
Checklist
Additional context