Skip to content

fix(session): Prevent memory leak in pendingResponses on request timeout/cancellation #287

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 1 commit into
base: main
Choose a base branch
from

Conversation

beomsampyeon
Copy link

This PR fixes a potential memory leak in the pendingResponses map within both McpClientSession and McpServerSession when requests time out or are cancelled.

Motivation and Context

A potential memory leak exists in the pendingResponses map within both McpClientSession and McpServerSession.

Problem Details:
When a request is initiated using the sendRequest() method, a MonoSink is stored in the pendingResponses map, awaiting a response from the peer (server or client). If this peer does not respond and the operation times out (due to the .timeout() operator), or if the Mono returned by sendRequest() is cancelled for other reasons before a response is received, the original code did not explicitly remove the MonoSink from the pendingResponses map.

This could lead to an accumulation of unresolved sinks, causing a memory leak over time, especially in scenarios with many unanswered or timed-out requests. This change is needed to enhance the stability and robustness of the session management.

How Has This Been Tested?

  • Verified that all existing project tests pass locally after applying the changes.
  • The fix involves adding a standard Reactor onDispose handler, which is a well-understood pattern for resource cleanup in asynchronous scenarios like timeouts or cancellations.
  • Manually reviewed the code logic for timeout and cancellation scenarios to confirm that the onDispose handler would be invoked as expected to remove entries from pendingResponses.

Breaking Changes

None. This change only affects internal resource management and does not alter any public APIs or existing behavior from a user's perspective.

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

Solution Details:
This commit introduces an onDispose() callback within the Mono.create()
block in the sendRequest() method of both McpClientSession and
McpServerSession. This onDispose() handler ensures that
pendingResponses.remove(requestId) is reliably invoked when the
MonoSink is disposed for any reason, including:

  • Timeout triggered by the .timeout() operator.
  • Explicit cancellation by a downstream subscriber.

This approach ensures that resources associated with pending requests are
cleaned up properly under various termination conditions.

…out/cancellation

Fixes a potential memory leak in the `pendingResponses` map within

both `McpClientSession` and `McpServerSession`.

Problem:

When a request is initiated using the `sendRequest()` method, a `MonoSink`

is stored in the `pendingResponses` map, awaiting a response from the

peer (server or client). If this peer does not respond and the operation

times out (due to the `.timeout()` operator), or if the `Mono` returned

by `sendRequest()` is cancelled for other reasons before a response is

received, the original code did not explicitly remove the `MonoSink`

from the `pendingResponses` map. This could lead to an accumulation of

unresolved sinks, causing a memory leak over time, especially in

scenarios with many unanswered or timed-out requests.

Solution:

This commit introduces an `onDispose()` callback within the `Mono.create()`

block in the `sendRequest()` method of both `McpClientSession` and

`McpServerSession`. This `onDispose()` handler ensures that

`pendingResponses.remove(requestId)` is reliably invoked when the

`MonoSink` is disposed for any reason, including:

  - Timeout triggered by the `.timeout()` operator.

  - Explicit cancellation by a downstream subscriber.

This change ensures that resources associated with pending requests are

cleaned up properly, enhancing the stability and robustness of the

session management in both client and server implementations.

Affected files:

  - McpClientSession.java

  - McpServerSession.java
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