diff --git a/mcp-core/src/main/java/io/modelcontextprotocol/client/transport/HttpClientSseClientTransport.java b/mcp-core/src/main/java/io/modelcontextprotocol/client/transport/HttpClientSseClientTransport.java index ae093316f..ccd0d7bf0 100644 --- a/mcp-core/src/main/java/io/modelcontextprotocol/client/transport/HttpClientSseClientTransport.java +++ b/mcp-core/src/main/java/io/modelcontextprotocol/client/transport/HttpClientSseClientTransport.java @@ -93,6 +93,13 @@ public class HttpClientSseClientTransport implements McpClientTransport { */ private final HttpClient httpClient; + /** + * Flag indicating whether this transport should close the HttpClient when closing + * gracefully. Set to true when the HttpClient is created internally by the builder, + * false when provided externally. + */ + private final boolean shouldCloseHttpClient; + /** HTTP request builder for building requests to send messages to the server */ private final HttpRequest.Builder requestBuilder; @@ -129,7 +136,8 @@ public class HttpClientSseClientTransport implements McpClientTransport { * @throws IllegalArgumentException if objectMapper, clientBuilder, or headers is null */ HttpClientSseClientTransport(HttpClient httpClient, HttpRequest.Builder requestBuilder, String baseUri, - String sseEndpoint, McpJsonMapper jsonMapper, McpAsyncHttpClientRequestCustomizer httpRequestCustomizer) { + String sseEndpoint, McpJsonMapper jsonMapper, McpAsyncHttpClientRequestCustomizer httpRequestCustomizer, + boolean shouldCloseHttpClient) { Assert.notNull(jsonMapper, "jsonMapper must not be null"); Assert.hasText(baseUri, "baseUri must not be empty"); Assert.hasText(sseEndpoint, "sseEndpoint must not be empty"); @@ -142,6 +150,7 @@ public class HttpClientSseClientTransport implements McpClientTransport { this.httpClient = httpClient; this.requestBuilder = requestBuilder; this.httpRequestCustomizer = httpRequestCustomizer; + this.shouldCloseHttpClient = shouldCloseHttpClient; } @Override @@ -169,6 +178,8 @@ public static class Builder { private HttpClient.Builder clientBuilder = HttpClient.newBuilder().version(HttpClient.Version.HTTP_1_1); + private HttpClient externalHttpClient; + private McpJsonMapper jsonMapper; private HttpRequest.Builder requestBuilder = HttpRequest.newBuilder(); @@ -227,6 +238,20 @@ public Builder sseEndpoint(String sseEndpoint) { public Builder clientBuilder(HttpClient.Builder clientBuilder) { Assert.notNull(clientBuilder, "clientBuilder must not be null"); this.clientBuilder = clientBuilder; + this.externalHttpClient = null; // Clear external client if builder is set + return this; + } + + /** + * Sets an external HttpClient instance to use instead of creating a new one. When + * an external HttpClient is provided, the transport will not attempt to close it + * during graceful shutdown, leaving resource management to the caller. + * @param httpClient the HttpClient instance to use + * @return this builder + */ + public Builder httpClient(HttpClient httpClient) { + Assert.notNull(httpClient, "httpClient must not be null"); + this.externalHttpClient = httpClient; return this; } @@ -325,9 +350,23 @@ public Builder connectTimeout(Duration connectTimeout) { * @return a new transport instance */ public HttpClientSseClientTransport build() { - HttpClient httpClient = this.clientBuilder.connectTimeout(this.connectTimeout).build(); + HttpClient httpClient; + boolean shouldCloseHttpClient; + + if (externalHttpClient != null) { + // Use external HttpClient, don't close it + httpClient = externalHttpClient; + shouldCloseHttpClient = false; + } + else { + // Create new HttpClient, should close it + httpClient = this.clientBuilder.connectTimeout(this.connectTimeout).build(); + shouldCloseHttpClient = true; + } + return new HttpClientSseClientTransport(httpClient, requestBuilder, baseUri, sseEndpoint, - jsonMapper == null ? McpJsonMapper.getDefault() : jsonMapper, httpRequestCustomizer); + jsonMapper == null ? McpJsonMapper.getDefault() : jsonMapper, httpRequestCustomizer, + shouldCloseHttpClient); } } @@ -495,7 +534,40 @@ public Mono closeGracefully() { if (subscription != null && !subscription.isDisposed()) { subscription.dispose(); } - }); + }).then(shouldCloseHttpClient ? Mono.fromRunnable(this::closeHttpClientResources) : Mono.empty()); + } + + /** + * Closes HttpClient resources including connection pools and selector threads. This + * method uses reflection to access internal HttpClient implementation details. + */ + private void closeHttpClientResources() { + try { + // Access HttpClientImpl internal fields via reflection + Class httpClientClass = httpClient.getClass(); + + // Close SelectorManager if present + try { + java.lang.reflect.Field selectorManagerField = httpClientClass.getDeclaredField("selectorManager"); + selectorManagerField.setAccessible(true); + Object selectorManager = selectorManagerField.get(httpClient); + + if (selectorManager != null) { + java.lang.reflect.Method shutdownMethod = selectorManager.getClass().getMethod("shutdown"); + shutdownMethod.invoke(selectorManager); + logger.debug("HttpClient SelectorManager shutdown completed"); + } + } + catch (NoSuchFieldException | NoSuchMethodException e) { + // Field/method might not exist in different JDK versions, continue with + // other cleanup + logger.debug("SelectorManager field/method not found, skipping: {}", e.getMessage()); + } + + } + catch (Exception e) { + logger.warn("Failed to close HttpClient resources cleanly: {}", e.getMessage()); + } } /** diff --git a/mcp-core/src/main/java/io/modelcontextprotocol/client/transport/HttpClientStreamableHttpTransport.java b/mcp-core/src/main/java/io/modelcontextprotocol/client/transport/HttpClientStreamableHttpTransport.java index f4505c898..72168318c 100644 --- a/mcp-core/src/main/java/io/modelcontextprotocol/client/transport/HttpClientStreamableHttpTransport.java +++ b/mcp-core/src/main/java/io/modelcontextprotocol/client/transport/HttpClientStreamableHttpTransport.java @@ -87,6 +87,13 @@ public class HttpClientStreamableHttpTransport implements McpClientTransport { */ private final HttpClient httpClient; + /** + * Flag indicating whether this transport should close the HttpClient when closing + * gracefully. Set to true when the HttpClient is created internally by the builder, + * false when provided externally. + */ + private final boolean shouldCloseHttpClient; + /** HTTP request builder for building requests to send messages to the server */ private final HttpRequest.Builder requestBuilder; @@ -126,7 +133,8 @@ public class HttpClientStreamableHttpTransport implements McpClientTransport { private HttpClientStreamableHttpTransport(McpJsonMapper jsonMapper, HttpClient httpClient, HttpRequest.Builder requestBuilder, String baseUri, String endpoint, boolean resumableStreams, - boolean openConnectionOnStartup, McpAsyncHttpClientRequestCustomizer httpRequestCustomizer) { + boolean openConnectionOnStartup, McpAsyncHttpClientRequestCustomizer httpRequestCustomizer, + boolean shouldCloseHttpClient) { this.jsonMapper = jsonMapper; this.httpClient = httpClient; this.requestBuilder = requestBuilder; @@ -136,6 +144,7 @@ private HttpClientStreamableHttpTransport(McpJsonMapper jsonMapper, HttpClient h this.openConnectionOnStartup = openConnectionOnStartup; this.activeSession.set(createTransportSession()); this.httpRequestCustomizer = httpRequestCustomizer; + this.shouldCloseHttpClient = shouldCloseHttpClient; } @Override @@ -211,13 +220,48 @@ public Mono closeGracefully() { return Mono.defer(() -> { logger.debug("Graceful close triggered"); DefaultMcpTransportSession currentSession = this.activeSession.getAndSet(createTransportSession()); - if (currentSession != null) { - return currentSession.closeGracefully(); + Mono sessionClose = currentSession != null ? currentSession.closeGracefully() : Mono.empty(); + + if (shouldCloseHttpClient) { + return sessionClose.then(Mono.fromRunnable(this::closeHttpClientResources)); } - return Mono.empty(); + return sessionClose; }); } + /** + * Closes HttpClient resources including connection pools and selector threads. This + * method uses reflection to access internal HttpClient implementation details. + */ + private void closeHttpClientResources() { + try { + // Access HttpClientImpl internal fields via reflection + Class httpClientClass = httpClient.getClass(); + + // Close SelectorManager if present + try { + java.lang.reflect.Field selectorManagerField = httpClientClass.getDeclaredField("selectorManager"); + selectorManagerField.setAccessible(true); + Object selectorManager = selectorManagerField.get(httpClient); + + if (selectorManager != null) { + java.lang.reflect.Method shutdownMethod = selectorManager.getClass().getMethod("shutdown"); + shutdownMethod.invoke(selectorManager); + logger.debug("HttpClient SelectorManager shutdown completed"); + } + } + catch (NoSuchFieldException | NoSuchMethodException e) { + // Field/method might not exist in different JDK versions, continue with + // other cleanup + logger.debug("SelectorManager field/method not found, skipping: {}", e.getMessage()); + } + + } + catch (Exception e) { + logger.warn("Failed to close HttpClient resources cleanly: {}", e.getMessage()); + } + } + private Mono reconnect(McpTransportStream stream) { return Mono.deferContextual(ctx -> { @@ -603,6 +647,8 @@ public static class Builder { private HttpClient.Builder clientBuilder = HttpClient.newBuilder().version(HttpClient.Version.HTTP_1_1); + private HttpClient externalHttpClient; + private String endpoint = DEFAULT_ENDPOINT; private boolean resumableStreams = true; @@ -632,6 +678,20 @@ private Builder(String baseUri) { public Builder clientBuilder(HttpClient.Builder clientBuilder) { Assert.notNull(clientBuilder, "clientBuilder must not be null"); this.clientBuilder = clientBuilder; + this.externalHttpClient = null; // Clear external client if builder is set + return this; + } + + /** + * Sets an external HttpClient instance to use instead of creating a new one. When + * an external HttpClient is provided, the transport will not attempt to close it + * during graceful shutdown, leaving resource management to the caller. + * @param httpClient the HttpClient instance to use + * @return this builder + */ + public Builder httpClient(HttpClient httpClient) { + Assert.notNull(httpClient, "httpClient must not be null"); + this.externalHttpClient = httpClient; return this; } @@ -769,10 +829,23 @@ public Builder connectTimeout(Duration connectTimeout) { * @return a new instance of {@link HttpClientStreamableHttpTransport} */ public HttpClientStreamableHttpTransport build() { - HttpClient httpClient = this.clientBuilder.connectTimeout(this.connectTimeout).build(); + HttpClient httpClient; + boolean shouldCloseHttpClient; + + if (externalHttpClient != null) { + // Use external HttpClient, don't close it + httpClient = externalHttpClient; + shouldCloseHttpClient = false; + } + else { + // Create new HttpClient, should close it + httpClient = this.clientBuilder.connectTimeout(this.connectTimeout).build(); + shouldCloseHttpClient = true; + } + return new HttpClientStreamableHttpTransport(jsonMapper == null ? McpJsonMapper.getDefault() : jsonMapper, httpClient, requestBuilder, baseUri, endpoint, resumableStreams, openConnectionOnStartup, - httpRequestCustomizer); + httpRequestCustomizer, shouldCloseHttpClient); } } diff --git a/mcp-core/src/test/java/io/modelcontextprotocol/client/HttpClientStreamableHttpSyncClientTests.java b/mcp-core/src/test/java/io/modelcontextprotocol/client/HttpClientStreamableHttpSyncClientTests.java index d59ae35b4..f90c606ba 100644 --- a/mcp-core/src/test/java/io/modelcontextprotocol/client/HttpClientStreamableHttpSyncClientTests.java +++ b/mcp-core/src/test/java/io/modelcontextprotocol/client/HttpClientStreamableHttpSyncClientTests.java @@ -5,6 +5,8 @@ package io.modelcontextprotocol.client; import java.net.URI; +import java.net.http.HttpClient; +import java.time.Duration; import java.util.Map; import org.junit.jupiter.api.AfterAll; @@ -19,6 +21,7 @@ import io.modelcontextprotocol.common.McpTransportContext; import io.modelcontextprotocol.spec.McpClientTransport; +import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.atLeastOnce; @@ -70,4 +73,39 @@ void customizesRequests() { }); } + @Test + void supportsExternalHttpClient() { + // Create an external HttpClient + HttpClient externalHttpClient = HttpClient.newBuilder().connectTimeout(Duration.ofSeconds(5)).build(); + + // Create transport with external HttpClient + McpClientTransport transport = HttpClientStreamableHttpTransport.builder(host) + .httpClient(externalHttpClient) + .build(); + + withClient(transport, syncSpec -> syncSpec, mcpSyncClient -> { + mcpSyncClient.initialize(); + // Test should complete without errors + }); + + // External HttpClient should still be usable after transport closes + // (This is a basic test - in practice you'd verify the client is still + // functional) + assertThat(externalHttpClient).isNotNull(); + } + + @Test + void closesInternalHttpClientGracefully() { + // Create transport with internal HttpClient (default behavior) + McpClientTransport transport = HttpClientStreamableHttpTransport.builder(host).build(); + + withClient(transport, syncSpec -> syncSpec, mcpSyncClient -> { + mcpSyncClient.initialize(); + // Test should complete and close gracefully + }); + + // This test verifies that internal HttpClient resources are cleaned up + // The actual verification happens during the graceful close process + } + } diff --git a/mcp-core/src/test/java/io/modelcontextprotocol/client/HttpSseMcpSyncClientTests.java b/mcp-core/src/test/java/io/modelcontextprotocol/client/HttpSseMcpSyncClientTests.java index 483d38669..e6ac52fa4 100644 --- a/mcp-core/src/test/java/io/modelcontextprotocol/client/HttpSseMcpSyncClientTests.java +++ b/mcp-core/src/test/java/io/modelcontextprotocol/client/HttpSseMcpSyncClientTests.java @@ -5,6 +5,8 @@ package io.modelcontextprotocol.client; import java.net.URI; +import java.net.http.HttpClient; +import java.time.Duration; import java.util.Map; import org.junit.jupiter.api.AfterAll; @@ -19,6 +21,7 @@ import io.modelcontextprotocol.common.McpTransportContext; import io.modelcontextprotocol.spec.McpClientTransport; +import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNull; @@ -75,4 +78,37 @@ void customizesRequests() { }); } + @Test + void supportsExternalHttpClient() { + // Create an external HttpClient + HttpClient externalHttpClient = HttpClient.newBuilder().connectTimeout(Duration.ofSeconds(5)).build(); + + // Create transport with external HttpClient + McpClientTransport transport = HttpClientSseClientTransport.builder(host) + .httpClient(externalHttpClient) + .build(); + + withClient(transport, syncSpec -> syncSpec, mcpSyncClient -> { + mcpSyncClient.initialize(); + // Test should complete without errors + }); + + // External HttpClient should still be usable after transport closes + assertThat(externalHttpClient).isNotNull(); + } + + @Test + void closesInternalHttpClientGracefully() { + // Create transport with internal HttpClient (default behavior) + McpClientTransport transport = HttpClientSseClientTransport.builder(host).build(); + + withClient(transport, syncSpec -> syncSpec, mcpSyncClient -> { + mcpSyncClient.initialize(); + // Test should complete and close gracefully + }); + + // This test verifies that internal HttpClient resources are cleaned up + // The actual verification happens during the graceful close process + } + } diff --git a/mcp-core/src/test/java/io/modelcontextprotocol/client/transport/HttpClientSseClientTransportTests.java b/mcp-core/src/test/java/io/modelcontextprotocol/client/transport/HttpClientSseClientTransportTests.java index c5c365798..8dbf344f6 100644 --- a/mcp-core/src/test/java/io/modelcontextprotocol/client/transport/HttpClientSseClientTransportTests.java +++ b/mcp-core/src/test/java/io/modelcontextprotocol/client/transport/HttpClientSseClientTransportTests.java @@ -78,7 +78,7 @@ static class TestHttpClientSseClientTransport extends HttpClientSseClientTranspo public TestHttpClientSseClientTransport(final String baseUri) { super(HttpClient.newBuilder().version(HttpClient.Version.HTTP_1_1).build(), HttpRequest.newBuilder().header("Content-Type", "application/json"), baseUri, "/sse", JSON_MAPPER, - McpAsyncHttpClientRequestCustomizer.NOOP); + McpAsyncHttpClientRequestCustomizer.NOOP, true); } public int getInboundMessageCount() {