-
Notifications
You must be signed in to change notification settings - Fork 233
mcp: add StreamableHTTPOptions.SessionTimeout #594
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
Conversation
a8a2ec8 to
753347d
Compare
| handler := NewStreamableHTTPHandler( | ||
| func(req *http.Request) *Server { return server }, | ||
| &StreamableHTTPOptions{ | ||
| SessionTimeout: 50 * time.Millisecond, |
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.
This would be a good use for testing/synctest, but it would require limiting the test to only running on Go 1.25+.
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.
Unfortunately, it doesn't work because the test uses httptest.NewServer, which does (real) I/O. Therefore, synctest.Wait never terminates.
022deb2 to
1b00937
Compare
230e352 to
1b00937
Compare
|
Staticcheck failures are being addressed in #599. |
Add a timeout option for the streamable HTTP handler that automatically cleans up idle sessions. Also, fix a bug in the streamable client, where we hang on a request even though the client can never get a response (because the HTTP request terminated without a response or Last-Event-Id). Fixes modelcontextprotocol#499
1b00937 to
3188ac5
Compare
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.
PTAL, the only substantive change was updating the test to have multiple goroutines keeping the session alive (as suggested), but I also had to rebase to pick up the fixes to CI.
Add a timeout option for the streamable HTTP handler that automatically cleans up idle sessions.
Also, fix a bug in the streamable client, where we hang on a request even though the client can never get a response (because the HTTP request terminated without a response or Last-Event-Id).
Fixes #499