Skip to content

End a listen stream's keepalive thread when its slot is freed - #569

Open
koic wants to merge 1 commit into
modelcontextprotocol:mainfrom
koic:end_listen_keepalive_threads_with_their_slot
Open

koic wants to merge 1 commit into
modelcontextprotocol:mainfrom
koic:end_listen_keepalive_threads_with_their_slot

Conversation

@koic

@koic koic commented Sep 22, 2026

Copy link
Copy Markdown
Member

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.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

## 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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant