Skip to content

Conversation

@iSevenDays
Copy link
Contributor

This PR addresses potential ASGI compliance issues in mcpm.router.app.py when it's run as a standalone ASGI application, specifically concerning its SSE (Server-Sent Events) route handling. This complements the fixes made separately for mcpm.router.transport.py.

Problem:

If mcpm.router.app.py is used to serve the MCPM router directly, its SSE endpoint (/sse) needed to ensure full ASGI compliance to prevent TypeError: 'NoneType' object is not callable or RuntimeError: No response returned. issues, similar to those observed in other parts of the MCPM routing logic. Specifically, the SSE handler could implicitly return None upon normal stream completion.

Solution:

The mcpm.router.app.py has been updated as follows:

  1. Restructured for Clarity: The file now more explicitly defines its own Starlette application, MCPRouter instance, and RouterSseTransport instance for clarity when run directly.
  2. NoOpsResponse Class: A NoOpsResponse class (returning HTTP 204) has been included. This is used as a compliant placeholder response when an SSE stream handler concludes but Starlette requires a Response object to be returned.
  3. ASGI Compliant handle_sse:
    • A dedicated handle_sse function is implemented for the /sse route.
    • This function now uses a try...finally block to ensure NoOpsResponse() is always returned upon completion, satisfying Starlette's requirements.
    • A while not await request.is_disconnected(): await asyncio.sleep(0.1) loop has been added within the try block. This ensures the handler remains active and correctly manages the SSE connection lifecycle until the client explicitly disconnects.
  4. Necessary Imports: Required modules such as asyncio and various starlette components have been added to support these changes.

Impact:

These changes ensure that if mcpm.router.app.py is run as the main application, its /sse endpoint is stable, robust, and adheres to ASGI specifications. This prevents unexpected errors and contributes to the overall reliability of the MCPM server.

Context:
This PR builds upon the fixes in commit 27dc4d6 (which addressed transport.py). The related ASGI compliance for mcpm.router.router.py (specifically handle_sse in get_sse_server_app) was already addressed in upstream/main via commit 076a6bf.

- Modified mcpm.router.transport.py:

  - Refactored handle_post_message to correctly send a 202 Accepted response as a raw ASGI application before proceeding with internal message forwarding. It now implicitly returns None as expected.

(Note: Complementary ASGI fixes for mcpm.router.router.py's handle_sse were already present in the base branch 076a6bf)
Copy link
Contributor

@JoJoJoJoJoJoJo JoJoJoJoJoJoJo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks but it seems the changes are in the wrong file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be noted that currently app.py has been updated to start a streamable http app, the sse app has been deprecated and moved to sse_app.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoJoJoJoJoJoJo NoOpsResponse was required to fix crashes in OpenHands. Please let me know where (what classes, sse_app.py?) should I look to integrate this part.

Copy link
Contributor

@JoJoJoJoJoJoJo JoJoJoJoJoJoJo Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked the usage of mcpm in Openhands code base.

Which means only works on src/mcpm/router/router.py is enough as far as I understand. And after your pr merged in mcpm.sh repo, you may also need to bump the package dependency version in OpenHands

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.

2 participants