Skip to content

[Server] Flush output buffers on stateless SSE streams - #520

Open
SammyTourani wants to merge 1 commit into
modelcontextprotocol:mainfrom
SammyTourani:fix/issue-516
Open

SammyTourani wants to merge 1 commit into
modelcontextprotocol:mainfrom
SammyTourani:fix/issue-516

Conversation

@SammyTourani

Copy link
Copy Markdown

Fixes #516

Fixes #516.

  • src/Server/Transport/Http/StatelessResponder.php: call @ob_flush() before
    flush() after each frame, the same two calls StreamableHttpTransport
    already makes. The @ suppresses the notice ob_flush() raises when no
    output buffer is active.
  • tests/Integration/StatelessLifecycleTest.php:
    • The example server is now started with -d output_buffering=4096. Without
      that, whether it buffers depends on the php.ini the machine loads.
    • The new testListenIsAcknowledgedAtOnce opens subscriptions/listen and
      requires the first data: frame to be the acknowledgement. It reads with a
      5 s timeout, well inside the example's 20 s lifetime.

Verification

Command: vendor/bin/phpunit --testsuite=unit && vendor/bin/phpunit --testsuite=integration

Result (PHP 8.4.23):

OK (1582 tests, 4107 assertions)
OK (69 tests, 156 assertions)
  • The new test, without the fix: it fails after the 5 s read timeout with
    Failed asserting that null is identical to 'notifications/subscriptions/acknowledged'.
    With the fix it passes in 0.07 s.
  • PHP 8.1.34: the new test likewise fails without the fix and passes with
    it. tests/Integration/StatelessLifecycleTest.php gives
    OK (11 tests, 39 assertions) and the unit suite gives
    OK (1582 tests, 4107 assertions).
  • Manual check: I ran the stateless-lifecycle example under
    php -d output_buffering=4096 -S … and timed each SSE line of a
    subscriptions/listen stream over a raw socket.
    • Before the fix, the acknowledgement arrived at 20.22 s, together with the
      closing result.
    • After the fix, it arrived at 0.01 s.
    • With output_buffering=0, both versions deliver it at 0.01 s.
  • Client hanging up after 0.5 s (same setup): without the fix, the single
    php -S worker stayed busy for another 19.59 s. With the fix it answered the
    next request 0.02 s later.
  • Style and static analysis: php-cs-fixer fix --dry-run --diff on both
    files found 0 of 2 files to fix, and phpstan reported no errors.

StatelessResponder called only flush(), which does not empty PHP's own
output buffer. With output_buffering enabled, as php.ini-production and
php.ini-development ship it, the frames of a subscriptions/listen stream
(starting with the acknowledgement) stayed buffered until the stream
closed, so clients waiting for the acknowledgement timed out.

Call @ob_flush() before flush(), as StreamableHttpTransport already does.

Fixes modelcontextprotocol#516

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.

[Server] Connections to Github Copilot CLI time out on subscriptions/listen

1 participant