mcp: Implement subscriptions/listen rpc (SEP-2575)#1007
Conversation
…streams and end-to-end testing
…s type and dedicated methods
…o simplify notification handling
…ncellation signature and request matching logic
…mplify notification handling
…fications to modern sessions
…nd update Server state tracking
nit: subscriptions
There are many parts to that SEP. Is this the last one to be implemented? |
guglielmo-san
changed the title
subscrptions/listen rpc (SEP-2575)subscriptions/listen rpc (SEP-2575)
…sure proper request cleanup on connection loss.
…ification filtering for legacy clients
…e unused listenSubscription struct
…onnectInfo struct
| OnInternalError func(error) // optional | ||
| } | ||
|
|
||
| type IsSubscriptionsListenContextKey struct{} |
There was a problem hiding this comment.
I have two objections to this approach: first, the name is specific to a particular transport-level use case. It should be more general. Second, using a context to pass the information should be a last resort. Can we instead have a different constructor?
There was a problem hiding this comment.
You wrote in your message to me "that's what lets the listen handler observe HTTP disconnect on stateless streamable HTTP. Without this, the handler would never see the client going away." Document that here (but remove references to the specific protocol). And while you're at it, maybe you can document why the default is to wrap with notDone. A lot of this package is mysterious to me and if you understand it, drop some knowledge where you can.
There was a problem hiding this comment.
Done, I added a parameter on the ConnectionConfig to signal the need to propagate the request cancellation, and added some explanation around it
| c.updateInFlight(func(s *inFlightState) { | ||
| err = s.shuttingDown(ErrServerClosing) | ||
| req, ok := msg.(*Request) | ||
| if !ok || req.Method != "notifications/cancelled" { |
There was a problem hiding this comment.
It's unfortunate that this general package is referring to a specific MCP feature. We should be treating it as if we can some day move it somewhere else and export it. Can you think of another way to do this? What if there were a list of methods that
Maybe we can add a list of methods that can succeed during shutdown to ConnectionConfig. And also add a bool there to replace IsSubscriptionsListenContextKey.
There was a problem hiding this comment.
Right, I updated to follow the same logic of Conn.Notify(), if a notification reaches write will be allowed to pass
…handler cancellation on transport disconnect
jba
left a comment
jba
left a comment
There was a problem hiding this comment.
Nice, thanks for those last changes.
2 participants
Fixes #966