Fix HttpClient resource leak in HTTP transports

mcp-core/src/main/java/io/modelcontextprotocol/client/transport/HttpClientSseClientTransport.java

@@ -12,6 +12,9 @@
 import java.time.Duration;
 import java.util.List;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Consumer;
 import java.util.function.Function;
@@ -116,6 +119,12 @@ public class HttpClientSseClientTransport implements McpClientTransport {
 	 */
 	private final McpAsyncHttpClientRequestCustomizer httpRequestCustomizer;
 
+	/**
+	 * Consumer to handle HttpClient closure. If null, no cleanup is performed (external
+	 * HttpClient).
+	 */
+	private final Consumer<HttpClient> onCloseClient;
+
 	/**
 	 * Creates a new transport instance with custom HTTP client builder, object mapper,
 	 * and headers.
@@ -129,19 +138,22 @@ 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,
+			Consumer<HttpClient> onCloseClient) {
 		Assert.notNull(jsonMapper, "jsonMapper must not be null");
 		Assert.hasText(baseUri, "baseUri must not be empty");
 		Assert.hasText(sseEndpoint, "sseEndpoint must not be empty");
 		Assert.notNull(httpClient, "httpClient must not be null");
 		Assert.notNull(requestBuilder, "requestBuilder must not be null");
 		Assert.notNull(httpRequestCustomizer, "httpRequestCustomizer must not be null");
+		Assert.notNull(onCloseClient, "onCloseClient must not be null");
 		this.baseUri = URI.create(baseUri);
 		this.sseEndpoint = sseEndpoint;
 		this.jsonMapper = jsonMapper;
 		this.httpClient = httpClient;
 		this.requestBuilder = requestBuilder;
 		this.httpRequestCustomizer = httpRequestCustomizer;
+		this.onCloseClient = onCloseClient;
 	}
 
 	@Override
@@ -167,7 +179,7 @@ public static class Builder {
 
 		private String sseEndpoint = DEFAULT_SSE_ENDPOINT;
 
-		private HttpClient.Builder clientBuilder = HttpClient.newBuilder().version(HttpClient.Version.HTTP_1_1);
+		private HttpClient externalHttpClient;
 
 		private McpJsonMapper jsonMapper;
 
@@ -177,6 +189,9 @@ public static class Builder {
 
 		private Duration connectTimeout = Duration.ofSeconds(10);
 
+		private Consumer<HttpClient> onCloseClient = (HttpClient client) -> {
+		};
+
 		/**
 		 * Creates a new builder instance.
 		 */
@@ -220,24 +235,19 @@ public Builder sseEndpoint(String sseEndpoint) {
 		}
 
 		/**
-		 * Sets the HTTP client builder.
-		 * @param clientBuilder the HTTP client builder
-		 * @return this builder
-		 */
-		public Builder clientBuilder(HttpClient.Builder clientBuilder) {
-			Assert.notNull(clientBuilder, "clientBuilder must not be null");
-			this.clientBuilder = clientBuilder;
-			return this;
-		}
-
-		/**
-		 * Customizes the HTTP client builder.
-		 * @param clientCustomizer the consumer to customize the HTTP client builder
+		 * Provides 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.
+		 * <p>
+		 * Use this method when you want to share a single HttpClient instance across
+		 * multiple transports or when you need fine-grained control over HttpClient
+		 * lifecycle.
+		 * @param httpClient the HttpClient instance to use
 		 * @return this builder
 		 */
-		public Builder customizeClient(final Consumer<HttpClient.Builder> clientCustomizer) {
-			Assert.notNull(clientCustomizer, "clientCustomizer must not be null");
-			clientCustomizer.accept(clientBuilder);
+		public Builder withExternalHttpClient(HttpClient httpClient) {
+			Assert.notNull(httpClient, "httpClient must not be null");
+			this.externalHttpClient = httpClient;
 			return this;
 		}
 
@@ -310,13 +320,17 @@ public Builder asyncHttpRequestCustomizer(McpAsyncHttpClientRequestCustomizer as
 		}
 
 		/**
-		 * Sets the connection timeout for the HTTP client.
-		 * @param connectTimeout the connection timeout duration
+		 * Sets a custom consumer to handle HttpClient closure when the transport is
+		 * closed. This allows for custom cleanup logic beyond the default behavior.
+		 * <p>
+		 * Note: This is typically used for advanced use cases. The default behavior
+		 * (shutting down the internal ExecutorService) is sufficient for most scenarios.
+		 * @param onCloseClient the consumer to handle HttpClient closure
 		 * @return this builder
 		 */
-		public Builder connectTimeout(Duration connectTimeout) {
-			Assert.notNull(connectTimeout, "connectTimeout must not be null");
-			this.connectTimeout = connectTimeout;
+		public Builder onHttpClientClose(Consumer<HttpClient> onCloseClient) {
+			Assert.notNull(onCloseClient, "onCloseClient must not be null");
+			this.onCloseClient = onCloseClient;
 			return this;
 		}
 
@@ -325,9 +339,39 @@ public Builder connectTimeout(Duration connectTimeout) {
 		 * @return a new transport instance
 		 */
 		public HttpClientSseClientTransport build() {
-			HttpClient httpClient = this.clientBuilder.connectTimeout(this.connectTimeout).build();
+			HttpClient httpClient;
+			Consumer<HttpClient> closeHandler;
+
+			if (externalHttpClient != null) {
+				// Use external HttpClient, use custom close handler or no-op
+				httpClient = externalHttpClient;
+				closeHandler = onCloseClient;
+			}
+			else {
+				// Create internal HttpClient with custom ExecutorService
+				// Create a custom ExecutorService with meaningful thread names
+				ExecutorService internalExecutor = Executors.newCachedThreadPool(runnable -> {
+					Thread thread = new Thread(runnable);
+					thread.setName("MCP-HttpClient-" + thread.getId());
+					thread.setDaemon(true);
+					return thread;
+				});
+
+				httpClient = HttpClient.newBuilder()
+					.version(HttpClient.Version.HTTP_1_1)
+					.connectTimeout(this.connectTimeout)
+					.executor(internalExecutor)
+					.build();
+
+				// Combine default cleanup (shutdown executor) with custom handler if
+				// provided
+				closeHandler = (client) -> shutdownHttpClientExecutor(internalExecutor);
+				closeHandler = closeHandler.andThen(onCloseClient);
+
+			}
+
 			return new HttpClientSseClientTransport(httpClient, requestBuilder, baseUri, sseEndpoint,
-					jsonMapper == null ? McpJsonMapper.getDefault() : jsonMapper, httpRequestCustomizer);
+					jsonMapper == null ? McpJsonMapper.getDefault() : jsonMapper, httpRequestCustomizer, closeHandler);
 		}
 
 	}
@@ -495,7 +539,48 @@ public Mono<Void> closeGracefully() {
 			if (subscription != null && !subscription.isDisposed()) {
 				subscription.dispose();
 			}
-		});
+		}).then(onCloseClient != null ? Mono.fromRunnable(() -> onCloseClient.accept(httpClient)) : Mono.empty());
+	}
+
+	/**
+	 * Closes HttpClient resources by shutting down its associated ExecutorService. This
+	 * allows the GC to reclaim HttpClient-related threads (including SelectorManager) on
+	 * the next garbage collection cycle.
+	 * <p>
+	 * This approach avoids using reflection, Unsafe, or Java 21+ specific APIs, making it
+	 * compatible with Java 17+.
+	 * @param executor the ExecutorService to shutdown
+	 */
+	private static void shutdownHttpClientExecutor(ExecutorService executor) {
+		if (executor == null) {
+			return;
+		}
+
+		try {
+			logger.debug("Shutting down HttpClient ExecutorService");
+			executor.shutdown();
+
+			// Wait for graceful shutdown
+			if (!executor.awaitTermination(5, TimeUnit.SECONDS)) {
+				logger.debug("ExecutorService did not terminate in time, forcing shutdown");
+				executor.shutdownNow();
+
+				// Wait a bit more after forced shutdown
+				if (!executor.awaitTermination(2, TimeUnit.SECONDS)) {
+					logger.warn("ExecutorService did not terminate even after shutdownNow()");
+				}
+			}
+
+			logger.debug("HttpClient ExecutorService shutdown completed");
+		}
+		catch (InterruptedException e) {
+			logger.warn("Interrupted while shutting down HttpClient ExecutorService");
+			executor.shutdownNow();
+			Thread.currentThread().interrupt();
+		}
+		catch (Exception e) {
+			logger.warn("Failed to shutdown HttpClient ExecutorService cleanly: {}", e.getMessage());
+		}
 	}
 
 	/**