-
Notifications
You must be signed in to change notification settings - Fork 771
Support server to client notifications from the stateless transport #472
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
Support server to client notifications from the stateless transport #472
Conversation
efd322f to
fdb7799
Compare
| * @param method notification method name | ||
| * @param params any parameters or {@code null} | ||
| */ | ||
| default void sendNotification(String method, Object params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative to this might be a separate McpNotifier instance that would be accessed by calling McpTransportContext.get() with a lib-defined key. wdyt?
fdb7799 to
27ef5a6
Compare
The MCP spec allows stateless servers to send notifications to the client during a request. The response needs to be upgraded to SSE and the notifications are send in a stream until the final result is sent. This commit adds a `sendNotification` method to the transport context allowing each transport implementation to implement it or not. In this commit, HttpServletStatelessServerTransport implements the method and when the caller first sends a notification, the response is changed to `TEXT_EVENT_STREAM` and events are then streamed until the final result. This change will allow future features such as logging, list changes, etc. should we ever decide to support sessions in some manner. Even if we don't support sessions, sending progress notifications is a useful feature by itself.
27ef5a6 to
3f83ef2
Compare
|
@tzolov do you have plans to support sessions for the stateless transport/server? We have some ideas that we will likely implement in our own code but could contribute here if there's interest. |
|
Noe: I've only done |
|
@tzolov any interest in this? If so, I can rebase, etc. |
|
I will update the PR if the project is positive on the idea |
|
Hey, @Randgalt. I have been away from the project for a while so I apologize your proposal has not attracted a proper review this time. I agree we should definitely support upgrade to SSE in stateless responses to allow for notifications to be part of the stream. The PR, however, relies on the ability to use imperative APIs of the servlet stack, while it would not fly in other server implementations that expect a Publisher. The correct approach requires some research on how to effectively communicate the intent of the server developer - either by providing a fixed response, driving the transport to return a plain content or providing a Publisher, allowing the developer to send notifications which are terminated with the actual response to the originating request. |
|
Thanks @chemicL - FWIW we've decided to implement our own backend instead of relying on this implementation so our contributions here will be limited. |
The MCP spec allows stateless servers to send notifications to the client during a request. The response needs to be upgraded to SSE and the notifications are send in a stream until the final result is sent.
This commit adds a
sendNotificationmethod to the transport context allowing each transport implementation to implement it or not. In this commit, HttpServletStatelessServerTransport implements the method and when the caller first sends a notification, the response is changed toTEXT_EVENT_STREAMand events are then streamed until the final result.This change will allow future features such as logging, list changes, etc. should we ever decide to support sessions in some manner. Even if we don't support sessions, sending progress notifications is a useful feature by itself.
Motivation and Context
We would like to have support for progress notifications in the stateless implementation.
How Has This Been Tested?
testNotifications()added toHttpServletStatelessIntegrationTestsBreaking Changes
none
Types of changes
Checklist