Skip to content

Commit e94a93a

Browse files
quafftzolov
authored andcommitted
fix: Add null check for response ID to prevent memory leaks
- Check if response.id() is not null before processing in MCP session classes - Log error when MCP response lacks session ID to warn about potential memory leaks - Improve error handling in McpStreamableServerSession with proper error codes The missing null check could lead to memory leaks as pending requests would never be completed when responses lack session IDs. This fix ensures proper handling of such cases with appropriate error logging. Resolves #506 Signed-off-by: Yanming Zhou <[email protected]>
1 parent 210a813 commit e94a93a

File tree

3 files changed

+49
-21
lines changed

3 files changed

+49
-21
lines changed

mcp/src/main/java/io/modelcontextprotocol/spec/McpClientSession.java

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
*
3636
* @author Christian Tzolov
3737
* @author Dariusz Jędrzejczyk
38+
* @author Yanming Zhou
3839
*/
3940
public class McpClientSession implements McpSession {
4041

@@ -146,13 +147,20 @@ private void dismissPendingResponses() {
146147

147148
private void handle(McpSchema.JSONRPCMessage message) {
148149
if (message instanceof McpSchema.JSONRPCResponse response) {
149-
logger.debug("Received Response: {}", response);
150-
var sink = pendingResponses.remove(response.id());
151-
if (sink == null) {
152-
logger.warn("Unexpected response for unknown id {}", response.id());
150+
logger.debug("Received response: {}", response);
151+
if (response.id() != null) {
152+
var sink = pendingResponses.remove(response.id());
153+
if (sink == null) {
154+
logger.warn("Unexpected response for unknown id {}", response.id());
155+
}
156+
else {
157+
sink.success(response);
158+
}
153159
}
154160
else {
155-
sink.success(response);
161+
logger.error("Discarded MCP request response without session id. "
162+
+ "This is an indication of a bug in the request sender code that can lead to memory "
163+
+ "leaks as pending requests will never be completed.");
156164
}
157165
}
158166
else if (message instanceof McpSchema.JSONRPCRequest request) {

mcp/src/main/java/io/modelcontextprotocol/spec/McpServerSession.java

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -204,13 +204,20 @@ public Mono<Void> handle(McpSchema.JSONRPCMessage message) {
204204
// TODO handle errors for communication to without initialization happening
205205
// first
206206
if (message instanceof McpSchema.JSONRPCResponse response) {
207-
logger.debug("Received Response: {}", response);
208-
var sink = pendingResponses.remove(response.id());
209-
if (sink == null) {
210-
logger.warn("Unexpected response for unknown id {}", response.id());
207+
logger.debug("Received response: {}", response);
208+
if (response.id() != null) {
209+
var sink = pendingResponses.remove(response.id());
210+
if (sink == null) {
211+
logger.warn("Unexpected response for unknown id {}", response.id());
212+
}
213+
else {
214+
sink.success(response);
215+
}
211216
}
212217
else {
213-
sink.success(response);
218+
logger.error("Discarded MCP request response without session id. "
219+
+ "This is an indication of a bug in the request sender code that can lead to memory "
220+
+ "leaks as pending requests will never be completed.");
214221
}
215222
return Mono.empty();
216223
}

mcp/src/main/java/io/modelcontextprotocol/spec/McpStreamableServerSession.java

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import io.modelcontextprotocol.server.McpAsyncServerExchange;
2222
import io.modelcontextprotocol.server.McpNotificationHandler;
2323
import io.modelcontextprotocol.server.McpRequestHandler;
24+
import io.modelcontextprotocol.spec.McpSchema.ErrorCodes;
2425
import io.modelcontextprotocol.util.Assert;
2526
import reactor.core.publisher.Flux;
2627
import reactor.core.publisher.Mono;
@@ -33,6 +34,7 @@
3334
* capability without the insight into the transport-specific details of HTTP handling.
3435
*
3536
* @author Dariusz Jędrzejczyk
37+
* @author Yanming Zhou
3638
*/
3739
public class McpStreamableServerSession implements McpLoggableSession {
3840

@@ -214,19 +216,30 @@ public Mono<Void> accept(McpSchema.JSONRPCNotification notification) {
214216
*/
215217
public Mono<Void> accept(McpSchema.JSONRPCResponse response) {
216218
return Mono.defer(() -> {
217-
var stream = this.requestIdToStream.get(response.id());
218-
if (stream == null) {
219-
return Mono.error(new McpError("Unexpected response for unknown id " + response.id())); // TODO
220-
// JSONize
221-
}
222-
// TODO: encapsulate this inside the stream itself
223-
var sink = stream.pendingResponses.remove(response.id());
224-
if (sink == null) {
225-
return Mono.error(new McpError("Unexpected response for unknown id " + response.id())); // TODO
226-
// JSONize
219+
logger.debug("Received response: {}", response);
220+
221+
if (response.id() != null) {
222+
var stream = this.requestIdToStream.get(response.id());
223+
if (stream == null) {
224+
return Mono.error(McpError.builder(ErrorCodes.INTERNAL_ERROR)
225+
.message("Unexpected response for unknown id " + response.id())
226+
.build());
227+
}
228+
// TODO: encapsulate this inside the stream itself
229+
var sink = stream.pendingResponses.remove(response.id());
230+
if (sink == null) {
231+
return Mono.error(McpError.builder(ErrorCodes.INTERNAL_ERROR)
232+
.message("Unexpected response for unknown id " + response.id())
233+
.build());
234+
}
235+
else {
236+
sink.success(response);
237+
}
227238
}
228239
else {
229-
sink.success(response);
240+
logger.error("Discarded MCP request response without session id. "
241+
+ "This is an indication of a bug in the request sender code that can lead to memory "
242+
+ "leaks as pending requests will never be completed.");
230243
}
231244
return Mono.empty();
232245
});

0 commit comments

Comments
 (0)