-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add idle timeout termination for StreamableHTTPServerTransport #1159
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?
Add idle timeout termination for StreamableHTTPServerTransport #1159
Conversation
ec3655c
to
df9d081
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.
Hi @hopeful0 thank you for this contribution! And apologies for the time it took to get back to you on this.
For this change, I think we'd want more motivation that a timeout on the server is the right approach. The memory leak seems valid but there might be alternative ways to address it that don't rely on users to set the timeout (only users aware of and using this timeout param will actually benefit from the improvement.)
I'd like to bring this to the team and will come back with feedback asap.
Related (though I think between these 2 this is the right approach): #1343
Thank you @felixweinberger for your attention to this PR. I'd like to provide some additional clarification to further elaborate on the motivation behind this PR. The core issue this PR aims to solve is the server's lack of a defensive mechanism to terminate sessions proactively (at least, this was the case when the PR was submitted). This leaves the server constantly exposed to the risk of memory leaks. The server is powerless against clients that fail to terminate sessions as expected, whether intentionally or unintentionally, in scenarios such as:
To address these issues, I believe that idle timeout termination is a time-tested and reasonable countermeasure (also common in protocols like HTTP), which has the following characteristics:
In summary, there is a strong motivation for adding such a defensive mechanism to the server. Idle timeout termination is a simple and elegant solution. I hope these explanations address your concerns, and I'm more than happy to hear any other thoughts you or the team might have on this matter. |
By adding an optional idle timeout termination to the StreamableHTTPServerTransport, this PR allows the stateful server to close long-idle sessions to avoid memory leak issues when the client does not terminate the session as expected
These changes comply with the specifications for Streamable HTTP transports session management in the MCP protocol.
Meanwhile, optimized the logic for transport termination and session management without affecting existing functionality, including:
finally
blockfinally
block of the run_server inhandle_stateful_request
, because that instances should be cleared even if transport terminates firstMotivation and Context
#1116 addressed the memory leak issue caused by session not terminating in stateless requests, but in stateful requests, if the client does not close the session as expected, the session on the server will never be closed, leading to a similar memory leak issue.
This PR adds an optional idle timeout parameter to StreamableHTTPServerTransport, a context manager for tracking idle status, and an
_idle_timeout_terminate
method. When the idle timeout parameter is set, an_idle_timeout_terminate
task is initiated when transportconnect
is called. Once the transport does not receive or hanle any requests within the timeout period, the transport will be automatically terminated. To ensure that correctly tracks the idle status of transport, thehandle_request
method of the transport needs to be called within the context manager.How Has This Been Tested?
I have added a test to verify that the transfer and session will exist before the idle timeout and will be terminated after the idle timeout.
Breaking Changes
Types of changes
Checklist
Additional context