Instrumented Java 11 HttpClient does not re-throw exceptions in sendAsync call

[alex-inv]

Describe the bug
After instrumenting our java.net.HttpClient with newly released MicrometerHttpClient decorator, we've noticed our tests started failing. It seems that if any exception (e.g. network error) occurs, it will be caught in MicrometerHttpClient and not propagated further to the next CompletionStage available to the user, contrary to what happens in the original HttpClient implementation.

Implementation of sendAsync decorator in MicrometerHttpClient does not re-throw exception in the CompletableFuture.handle() method, but returns only response. In case handle got an exception as its input, null result will be returned instead:

return client.sendAsync(request, bodyHandler, pushPromiseHandler).handle((response, throwable) -> {
            if (throwable != null) {
                instrumentation.setThrowable(throwable);
            }
            instrumentation.setResponse(response);
            stopObservationOrTimer(instrumentation, request, response);
            return response;
        });

Environment

• Micrometer version: 1.13.0
• OS: macOS Sonoma 14.4.1
• Java version: Corretto-17.0.6.10.1

To Reproduce
The easiest way to reproduce the bug is to write a test simulating a faulty HTTP server, for example using WireMock library. I've attached a minimal demo application (built with JDK 17 and Gradle) with a failing test.

Here I show two tests, first passing and second failing, highlighting the reported issue:

@Test
public void testError() {
        WireMock.stubFor(WireMock.get(WireMock.urlEqualTo("/test-url"))
                .willReturn(new ResponseDefinitionBuilder().withFault(Fault.CONNECTION_RESET_BY_PEER)));

        var client = HttpClient.newBuilder().build();

        var request = HttpRequest.newBuilder(URI.create(wireMockServer.baseUrl() + "/test-url"))
                .GET().build();

        var response = client.sendAsync(request, HttpResponse.BodyHandlers.ofString());

        assertThrows(CompletionException.class, response::join);
}

@Test
public void testErrorMicrometer() {
        WireMock.stubFor(WireMock.get(WireMock.urlEqualTo("/test-url"))
                .willReturn(new ResponseDefinitionBuilder().withFault(Fault.CONNECTION_RESET_BY_PEER)));

        var client = MicrometerHttpClient.instrumentationBuilder(
                HttpClient.newBuilder().build(),
                new SimpleMeterRegistry()
        ).build();

        var request = HttpRequest.newBuilder(URI.create(wireMockServer.baseUrl() + "/test-url"))
                .GET().build();

        var response = client.sendAsync(request, HttpResponse.BodyHandlers.ofString());

        // This test fails - nothing is thrown
        assertThrows(CompletionException.class, response::join);
}

Expected behavior
Underlying error should be re-thrown and CompletionStage returned to the user should be completed exceptionally too, as per original API behavior.

While it's not possible to re-throw a generic Throwable in CompletableFuture.handle method due to limitations of checked exceptions handling, it's possible to wrap the original Throwable in a CompletionException, which would not break the CompletableFuture flow of executions.

[shakuzen]

Thanks for all the analysis and code. Would you be willing to submit a pull request for this?

[shakuzen]

Resolved by #5553