Skip to content

Fix connection closure handling in Protocol and InMemoryTransport #833

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alasano
Copy link

@alasano alasano commented Aug 2, 2025

Fix connection closure handling in Protocol and InMemoryTransport

Motivation and Context

This PR fixes two critical bugs that cause promises to hang indefinitely when connections are closed:

  1. InMemoryTransport deadlock: When closing one transport, the peer's onclose callback was never fired, causing server-side promises to hang forever
  2. Protocol request handler leak: Active request handlers were not aborted on connection close, causing them to continue running indefinitely

These bugs manifested as timeout errors in applications where connections were closed while requests were in-flight (e.g., user disconnection during a pending UI prompt).

How Has This Been Tested?

  • Added comprehensive unit tests for InMemoryTransport close behavior (idempotency, concurrent closes, post-close operations)
  • Tested with real-world scenario: closing connection while server has pending elicitation/create request
  • All existing tests pass
  • Verified fix resolves timeout issues in a multi-user MCP client implementation where disconnecting users with pending server-initiated requests (e.g., elicitation/create) would cause tests to hang indefinitely

Breaking Changes

None. These are bug fixes that make the SDK behave as expected.

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

Additional context

The fix ensures proper cleanup in both directions:

  • InMemoryTransport now guarantees both transports fire their onclose callbacks
  • Protocol now aborts all active request handlers via their AbortControllers
  • Together, these changes prevent resource leaks and hanging promises during connection closure

alasano added 2 commits August 2, 2025 00:34
- Make close() idempotent with _isClosed flag
- Prevent infinite recursion by clearing _otherTransport before calling peer.close()
- Ensure both transports' onclose callbacks fire
- Add tests for idempotency, concurrent closes, and post-close behavior

Fixes deadlock where closing one transport would not properly close its peer,
causing promises to hang indefinitely.
- Track request handlers with AbortControllers in _requestHandlerAbortControllers
- Abort all active request handlers in _onclose() to prevent hanging operations
- Ensure both outgoing requests (response handlers) and incoming requests (request handlers) are properly terminated

Fixes issue where request handlers would continue running after connection
closure, causing timeouts and resource leaks.
@alasano alasano requested a review from a team as a code owner August 2, 2025 04:44
@alasano alasano requested a review from ihrpr August 2, 2025 04:44
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