From 6ebd42f3a28eb11e0d6b47b44c75f16f8f0e6fa1 Mon Sep 17 00:00:00 2001 From: Arnab Nandy Date: Wed, 8 Jul 2026 15:40:03 +0530 Subject: [PATCH] #2283 #2284 fix: harden HTTPServer failure handling Signed-off-by: Arnab Nandy --- ...prometheus-metrics-exporter-httpserver.txt | 4 +- .../metrics/it/exporter/test/ExporterIT.java | 6 +- .../it/exporter/test/HttpServerIT.java | 9 +++ .../BlockingRejectedExecutionHandler.java | 18 ------ .../exporter/httpserver/HTTPServer.java | 27 ++++----- .../httpserver/HttpExchangeAdapter.java | 58 ++++++++----------- .../exporter/httpserver/HTTPServerTest.java | 37 +++++++++++- .../httpserver/HttpExchangeAdapterTest.java | 23 ++++++++ 8 files changed, 111 insertions(+), 71 deletions(-) delete mode 100644 prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/BlockingRejectedExecutionHandler.java diff --git a/docs/apidiffs/current_vs_latest/prometheus-metrics-exporter-httpserver.txt b/docs/apidiffs/current_vs_latest/prometheus-metrics-exporter-httpserver.txt index 17fccaa45..8bc12b84f 100644 --- a/docs/apidiffs/current_vs_latest/prometheus-metrics-exporter-httpserver.txt +++ b/docs/apidiffs/current_vs_latest/prometheus-metrics-exporter-httpserver.txt @@ -1,2 +1,4 @@ Comparing source compatibility of prometheus-metrics-exporter-httpserver-1.8.1-SNAPSHOT.jar against prometheus-metrics-exporter-httpserver-1.8.0.jar -No changes. +*** MODIFIED CLASS: PUBLIC io.prometheus.metrics.exporter.httpserver.HTTPServer (not serializable) + === CLASS FILE FORMAT VERSION: 52.0 <- 52.0 + diff --git a/integration-tests/it-exporter/it-exporter-test/src/test/java/io/prometheus/metrics/it/exporter/test/ExporterIT.java b/integration-tests/it-exporter/it-exporter-test/src/test/java/io/prometheus/metrics/it/exporter/test/ExporterIT.java index 5a80d8bdf..3ba88dcc1 100644 --- a/integration-tests/it-exporter/it-exporter-test/src/test/java/io/prometheus/metrics/it/exporter/test/ExporterIT.java +++ b/integration-tests/it-exporter/it-exporter-test/src/test/java/io/prometheus/metrics/it/exporter/test/ExporterIT.java @@ -157,7 +157,11 @@ void testErrorHandling() throws IOException { start("error"); Response response = scrape("GET", ""); assertThat(response.status).isEqualTo(500); - assertThat(response.stringBody()).contains("Simulating an error."); + assertErrorResponseBody(response.stringBody()); + } + + protected void assertErrorResponseBody(String body) { + assertThat(body).contains("Simulating an error."); } @Test diff --git a/integration-tests/it-exporter/it-exporter-test/src/test/java/io/prometheus/metrics/it/exporter/test/HttpServerIT.java b/integration-tests/it-exporter/it-exporter-test/src/test/java/io/prometheus/metrics/it/exporter/test/HttpServerIT.java index 4c7e61472..d04664718 100644 --- a/integration-tests/it-exporter/it-exporter-test/src/test/java/io/prometheus/metrics/it/exporter/test/HttpServerIT.java +++ b/integration-tests/it-exporter/it-exporter-test/src/test/java/io/prometheus/metrics/it/exporter/test/HttpServerIT.java @@ -1,5 +1,7 @@ package io.prometheus.metrics.it.exporter.test; +import static org.assertj.core.api.Assertions.assertThat; + import java.io.IOException; import java.net.URISyntaxException; @@ -7,4 +9,11 @@ class HttpServerIT extends ExporterIT { public HttpServerIT() throws IOException, URISyntaxException { super("exporter-httpserver-sample"); } + + @Override + protected void assertErrorResponseBody(String body) { + assertThat(body) + .isEqualTo("An internal error occurred while scraping metrics.\n") + .doesNotContain("Simulating an error."); + } } diff --git a/prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/BlockingRejectedExecutionHandler.java b/prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/BlockingRejectedExecutionHandler.java deleted file mode 100644 index 023d3f2f0..000000000 --- a/prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/BlockingRejectedExecutionHandler.java +++ /dev/null @@ -1,18 +0,0 @@ -package io.prometheus.metrics.exporter.httpserver; - -import java.util.concurrent.RejectedExecutionHandler; -import java.util.concurrent.ThreadPoolExecutor; - -class BlockingRejectedExecutionHandler implements RejectedExecutionHandler { - - @Override - public void rejectedExecution(Runnable runnable, ThreadPoolExecutor threadPoolExecutor) { - if (!threadPoolExecutor.isShutdown()) { - try { - threadPoolExecutor.getQueue().put(runnable); - } catch (InterruptedException ignored) { - // ignore - } - } - } -} diff --git a/prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/HTTPServer.java b/prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/HTTPServer.java index e93b122b0..fe55e6e9d 100644 --- a/prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/HTTPServer.java +++ b/prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/HTTPServer.java @@ -12,14 +12,13 @@ import io.prometheus.metrics.model.registry.PrometheusRegistry; import java.io.Closeable; import java.io.IOException; -import java.io.InputStream; import java.net.InetAddress; import java.net.InetSocketAddress; import java.security.PrivilegedActionException; import java.security.PrivilegedExceptionAction; +import java.util.concurrent.ArrayBlockingQueue; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; -import java.util.concurrent.SynchronousQueue; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; @@ -39,6 +38,10 @@ @StableApi public class HTTPServer implements Closeable { + private static final int DEFAULT_MIN_THREADS = 1; + private static final int DEFAULT_MAX_THREADS = 10; + private static final int DEFAULT_QUEUE_SIZE = 100; + static { if (!System.getProperties().containsKey("sun.net.httpserver.maxReqTime")) { System.setProperty("sun.net.httpserver.maxReqTime", "60"); @@ -153,22 +156,13 @@ public void handle(HttpExchange exchange) throws IOException { } } } else { - drainInputAndClose(exchange); + exchange.getRequestBody().close(); exchange.sendResponseHeaders(403, -1); } } }; } - private void drainInputAndClose(HttpExchange httpExchange) throws IOException { - InputStream inputStream = httpExchange.getRequestBody(); - byte[] b = new byte[4096]; - while (inputStream.read(b) != -1) { - // nop - } - inputStream.close(); - } - /** Stop the HTTP server. Same as {@link #close()}. */ public void stop() { close(); @@ -337,13 +331,12 @@ private ExecutorService makeExecutorService() { return executorService; } else { return new ThreadPoolExecutor( - 1, - 10, + DEFAULT_MIN_THREADS, + DEFAULT_MAX_THREADS, 120, TimeUnit.SECONDS, - new SynchronousQueue<>(true), - NamedDaemonThreadFactory.defaultThreadFactory(true), - new BlockingRejectedExecutionHandler()); + new ArrayBlockingQueue<>(DEFAULT_QUEUE_SIZE), + NamedDaemonThreadFactory.defaultThreadFactory(true)); } } diff --git a/prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/HttpExchangeAdapter.java b/prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/HttpExchangeAdapter.java index df99837cb..b18dbb83d 100644 --- a/prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/HttpExchangeAdapter.java +++ b/prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/HttpExchangeAdapter.java @@ -6,8 +6,6 @@ import io.prometheus.metrics.exporter.common.PrometheusHttpResponse; import java.io.IOException; import java.io.OutputStream; -import java.io.PrintWriter; -import java.io.StringWriter; import java.net.URI; import java.nio.charset.StandardCharsets; import java.util.Collections; @@ -18,6 +16,10 @@ public class HttpExchangeAdapter implements PrometheusHttpExchange { + private static final Logger logger = Logger.getLogger(HttpExchangeAdapter.class.getName()); + private static final byte[] ERROR_RESPONSE = + "An internal error occurred while scraping metrics.\n".getBytes(StandardCharsets.UTF_8); + private final HttpExchange httpExchange; private final HttpRequest request = new HttpRequest(); private final HttpResponse response = new HttpResponse(); @@ -92,52 +94,42 @@ public HttpResponse getResponse() { @Override public void handleException(IOException e) throws IOException { - sendErrorResponseWithStackTrace(e); + sendErrorResponse(e); } @Override public void handleException(RuntimeException e) { - sendErrorResponseWithStackTrace(e); + sendErrorResponse(e); } - private void sendErrorResponseWithStackTrace(Exception requestHandlerException) { + private void sendErrorResponse(Exception requestHandlerException) { if (!responseSent) { responseSent = true; + logger.log( + Level.SEVERE, + "The Prometheus metrics HTTPServer caught an Exception during scrape.", + requestHandlerException); try { - StringWriter stringWriter = new StringWriter(); - PrintWriter printWriter = new PrintWriter(stringWriter); - printWriter.write("An Exception occurred while scraping metrics: "); - requestHandlerException.printStackTrace(new PrintWriter(printWriter)); - byte[] stackTrace = stringWriter.toString().getBytes(StandardCharsets.UTF_8); httpExchange.getResponseHeaders().set("Content-Type", "text/plain; charset=utf-8"); - httpExchange.sendResponseHeaders(500, stackTrace.length); - httpExchange.getResponseBody().write(stackTrace); + httpExchange.sendResponseHeaders(500, ERROR_RESPONSE.length); + httpExchange.getResponseBody().write(ERROR_RESPONSE); } catch (IOException errorWriterException) { - // We want to avoid logging so that we don't mess with application logs when the HTTPServer - // is used in a Java agent. - // However, if we can't even send an error response to the client there's nothing we can do - // but logging a message. - Logger.getLogger(this.getClass().getName()) - .log( - Level.SEVERE, - "The Prometheus metrics HTTPServer caught an Exception during scrape and " - + "failed to send an error response to the client.", - errorWriterException); - Logger.getLogger(this.getClass().getName()) - .log( - Level.SEVERE, - "Original Exception that caused the Prometheus scrape error:", - requestHandlerException); + // If we can't even send an error response to the client, logging is the only remaining + // signal. + logger.log( + Level.SEVERE, + "The Prometheus metrics HTTPServer caught an Exception during scrape and " + + "failed to send an error response to the client.", + errorWriterException); } } else { // If the exception occurs after response headers have been sent, it's too late to respond // with HTTP 500. - Logger.getLogger(this.getClass().getName()) - .log( - Level.SEVERE, - "The Prometheus metrics HTTPServer caught an Exception while trying to send " - + "the metrics response.", - requestHandlerException); + logger.log( + Level.SEVERE, + "The Prometheus metrics HTTPServer caught an Exception while trying to send " + + "the metrics response.", + requestHandlerException); } } diff --git a/prometheus-metrics-exporter-httpserver/src/test/java/io/prometheus/metrics/exporter/httpserver/HTTPServerTest.java b/prometheus-metrics-exporter-httpserver/src/test/java/io/prometheus/metrics/exporter/httpserver/HTTPServerTest.java index ff2d55048..f0f8ffd83 100644 --- a/prometheus-metrics-exporter-httpserver/src/test/java/io/prometheus/metrics/exporter/httpserver/HTTPServerTest.java +++ b/prometheus-metrics-exporter-httpserver/src/test/java/io/prometheus/metrics/exporter/httpserver/HTTPServerTest.java @@ -25,6 +25,7 @@ import java.security.Principal; import java.util.List; import java.util.concurrent.Executors; +import java.util.concurrent.ThreadPoolExecutor; import javax.net.ssl.SSLContext; import javax.security.auth.Subject; import org.junit.jupiter.api.BeforeEach; @@ -158,6 +159,21 @@ void metricsCustomRootPath() throws Exception { "my_counter_total 1.0"); } + @Test + void defaultExecutorHasBoundedQueueAndNonBlockingRejection() throws Exception { + HTTPServer server = HTTPServer.builder().port(0).buildAndStart(); + try { + assertThat(server.executorService).isInstanceOf(ThreadPoolExecutor.class); + ThreadPoolExecutor executor = (ThreadPoolExecutor) server.executorService; + assertThat(executor.getMaximumPoolSize()).isEqualTo(10); + assertThat(executor.getQueue().remainingCapacity()).isEqualTo(100); + assertThat(executor.getRejectedExecutionHandler()) + .isInstanceOf(ThreadPoolExecutor.AbortPolicy.class); + } finally { + server.stop(); + } + } + @Test void registryThrows() throws Exception { HTTPServer server = @@ -171,7 +187,13 @@ public MetricSnapshots scrape(PrometheusScrapeRequest scrapeRequest) { } }) .buildAndStart(); - run(server, "/metrics", 500, "An Exception occurred while scraping metrics"); + run( + server, + "/metrics", + 500, + "An internal error occurred while scraping metrics.", + "IllegalStateException", + "test"); } @Test @@ -237,6 +259,16 @@ void healthDisabled() throws Exception { private static void run( HTTPServer server, String path, int expectedStatusCode, String expectedBody) throws Exception { + run(server, path, expectedStatusCode, expectedBody, new String[0]); + } + + private static void run( + HTTPServer server, + String path, + int expectedStatusCode, + String expectedBody, + String... unexpectedBody) + throws Exception { // we cannot use try-with-resources or even client.close(), or the test will fail with Java 17 @SuppressWarnings("resource") final HttpClient client = HttpClient.newBuilder().build(); @@ -248,6 +280,9 @@ private static void run( client.send(request, HttpResponse.BodyHandlers.ofString()); assertThat(response.statusCode()).isEqualTo(expectedStatusCode); assertThat(response.body()).contains(expectedBody); + if (unexpectedBody.length > 0) { + assertThat(response.body()).doesNotContain(unexpectedBody); + } } finally { server.stop(); } diff --git a/prometheus-metrics-exporter-httpserver/src/test/java/io/prometheus/metrics/exporter/httpserver/HttpExchangeAdapterTest.java b/prometheus-metrics-exporter-httpserver/src/test/java/io/prometheus/metrics/exporter/httpserver/HttpExchangeAdapterTest.java index 19bc0d66e..8aaef9c42 100644 --- a/prometheus-metrics-exporter-httpserver/src/test/java/io/prometheus/metrics/exporter/httpserver/HttpExchangeAdapterTest.java +++ b/prometheus-metrics-exporter-httpserver/src/test/java/io/prometheus/metrics/exporter/httpserver/HttpExchangeAdapterTest.java @@ -2,11 +2,14 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.sun.net.httpserver.Headers; import com.sun.net.httpserver.HttpExchange; +import java.io.ByteArrayOutputStream; import java.net.URI; +import java.nio.charset.StandardCharsets; import java.util.List; import org.junit.jupiter.api.Test; @@ -46,4 +49,24 @@ void getHeadersWhenNotPresent() { HttpExchangeAdapter adapter = new HttpExchangeAdapter(httpExchange); assertThat(adapter.getRequest().getHeaders("Accept").hasMoreElements()).isFalse(); } + + @Test + void handleExceptionReturnsGenericMessageWithoutStackTrace() throws Exception { + HttpExchange httpExchange = mock(HttpExchange.class); + Headers headers = new Headers(); + ByteArrayOutputStream responseBody = new ByteArrayOutputStream(); + when(httpExchange.getResponseHeaders()).thenReturn(headers); + when(httpExchange.getResponseBody()).thenReturn(responseBody); + HttpExchangeAdapter adapter = new HttpExchangeAdapter(httpExchange); + + adapter.handleException(new IllegalStateException("secret failure")); + + String body = new String(responseBody.toByteArray(), StandardCharsets.UTF_8); + assertThat(body).isEqualTo("An internal error occurred while scraping metrics.\n"); + assertThat(body).doesNotContain("IllegalStateException"); + assertThat(body).doesNotContain("secret failure"); + assertThat(body).doesNotContain("at "); + assertThat(headers.getFirst("Content-Type")).isEqualTo("text/plain; charset=utf-8"); + verify(httpExchange).sendResponseHeaders(500, body.getBytes(StandardCharsets.UTF_8).length); + } }