Conversation
## Motivation and Context Each `subscriptions/listen` stream has a keepalive thread that sleeps for the configured interval and then writes a comment frame, so a silently dropped peer is noticed and its slot freed. The thread checked for its entry only between sleeps, so when another path removed the entry, a failed notification write or the transport closing, the thread slept out the rest of its interval, up to 15 seconds by default, before it noticed and exited. How many such threads existed therefore depended on how quickly clients opened and dropped listen streams, not on `max_listen_subscriptions`, which bounds only the entries. The TypeScript SDK stops its keepalive timer the moment it tears a stream down. The wait between pings is now a condition variable wait under the registry lock, and whatever removes an entry signals it under the same lock. The presence check and the wait cannot be separated by a removal, so a thread waiting out its interval wakes and exits as soon as its entry goes, whichever path removed it; one already past the wait, in a ping, finishes that write first. Pings still happen outside the lock, and a dropped peer is still detected by the ping's write failure as before. The ping itself is now written under the stream's write mutex, as notification delivery and the closing result are, and skipped once the transport has marked the entry closed. It used to be written outside that mutex, so a comment frame could follow the closing result, or land between the bytes of a notification on a stream that does not serialize its writes itself. ## How Has This Been Tested? New tests in `test/mcp/server/transports/streamable_http_transport_test.rb` open listen streams with a 30 second interval, free their slots through a failed delivery write and through `close`, and check that every keepalive thread ends within a bound far below that interval. Against the previous library the threads are still alive at that bound. Two more tests hold a stream's write mutex and check that the ping waits for it, and mark an entry closed and check that the ping writes nothing. ## Breaking Changes None.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Each
subscriptions/listenstream has a keepalive thread that sleeps for the configured interval and then writes a comment frame, so a silently dropped peer is noticed and its slot freed. The thread checked for its entry only between sleeps, so when another path removed the entry, a failed notification write or the transport closing, the thread slept out the rest of its interval, up to 15 seconds by default, before it noticed and exited. How many such threads existed therefore depended on how quickly clients opened and dropped listen streams, not onmax_listen_subscriptions, which bounds only the entries. The TypeScript SDK stops its keepalive timer the moment it tears a stream down.The wait between pings is now a condition variable wait under the registry lock, and whatever removes an entry signals it under the same lock. The presence check and the wait cannot be separated by a removal, so a thread waiting out its interval wakes and exits as soon as its entry goes, whichever path removed it; one already past the wait, in a ping, finishes that write first. Pings still happen outside the lock, and a dropped peer is still detected by the ping's write failure as before.
The ping itself is now written under the stream's write mutex, as notification delivery and the closing result are, and skipped once the transport has marked the entry closed. It used to be written outside that mutex, so a comment frame could follow the closing result, or land between the bytes of a notification on a stream that does not serialize its writes itself.
How Has This Been Tested?
New tests in
test/mcp/server/transports/streamable_http_transport_test.rbopen listen streams with a 30 second interval, free their slots through a failed delivery write and throughclose, and check that every keepalive thread ends within a bound far below that interval. Against the previous library the threads are still alive at that bound.Two more tests hold a stream's write mutex and check that the ping waits for it, and mark an entry closed and check that the ping writes nothing.
Breaking Changes
None.
Types of changes
Checklist