mcp: add Session.ID #67
Conversation
|
@findleyr Please take a careful look. I'm not familiar with the machinery underlying sessions, and there may be a much simpler way. I'm also not sure if I've gotten all the locking right. |
findleyr
left a comment
findleyr
left a comment
There was a problem hiding this comment.
Why don't we just set the session ID when creating each session? Then no locking is required. What am I missing?
IIUC, the session ID is not known when the session is created but only when a successful POST happens. I think we could change the sessionID to an atomic.Value to avoid locking. |
Support retrieving the session ID from client and server sessions. For modelcontextprotocol#65.
The ID comes from the server, so clients can't create it. streamableClientConn locks because you already had locking around its sessionID. I frankly didn't look deeply into why. There is no other locking in this PR. @samthanawalla We're not concerned with locking overhead here, because it's dwarfed by the cost of the networking. @findleyr I was more concerned with the twists and turns I needed to surface these IDs, but as I look over the diffs, there's really not much code. So I'm satisfied with this PR if you are. |
findleyr
left a comment
findleyr
left a comment
There was a problem hiding this comment.
LGTM, with a couple actionable comments.
mcp/transport.go
Outdated
| }, | ||
| }) | ||
| assert(preempter.conn != nil, "unbound preempter") | ||
| h.setSessionIDFunc(conn.sessionID) |
There was a problem hiding this comment.
Since this is an internal detail, I think we could just set the store the Connection on the session, rather than indirecting through this func value. WDYT?
There was a problem hiding this comment.
I did what I think you said. PTAL.
4 participants
Support retrieving the session ID from client and server sessions.
For #65.