Memory leak in MicrometerHttpClientInterceptor

[Stephan202]

Describe the bug

In MicrometerHttpClientInterceptor a ConcurrentHashMap is used to track Timer.ResourceSamples by HttpContext. The sample is registered when the request is fired, and only deregistered if a response is received. In particular, if the request is cancelled while in flight, then the associated map entry is never cleared.

This introduces a memory leak. We noticed this issue because (in a very roundabout manner involving a very complex reactive chain; but that's not relevant for this issue) we have a situation in which some of the cached HttpContexts retain a few humongous objects that would otherwise have been garbage collected. As a result one of our applications frequently gets OOMKilled.

Environment

• Observed in prod with Micrometer 1.11.0 and org.apache.httpcomponents:httpasyncclient:jar:4.1.5.
• Also impacts current main (see reproduction description below).

To Reproduce

1. Apply the following patch to main:

   diff --git a/micrometer-core/src/test/java/io/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpClientInterceptorTest.java b/micrometer-core/src/test/java/io/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpClientInterceptorTest.java
   index e53d27bbe..1d24b5a1f 100644
   --- a/micrometer-core/src/test/java/io/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpClientInterceptorTest.java
   +++ b/micrometer-core/src/test/java/io/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpClientInterceptorTest.java
   @@ -34,6 +34,7 @@ import java.util.concurrent.Future;
   
    import static com.github.tomakehurst.wiremock.client.WireMock.any;
    import static com.github.tomakehurst.wiremock.client.WireMock.anyUrl;
   +import static com.github.tomakehurst.wiremock.client.WireMock.ok;
    import static org.assertj.core.api.Assertions.assertThat;
   
    /**
   @@ -52,6 +53,21 @@ class MicrometerHttpClientInterceptorTest {
            registry = new SimpleMeterRegistry(SimpleConfig.DEFAULT, new MockClock());
        }
   
   +    // If this test is executed in debug mode, with breakpoints in the `HttpRequestInterceptor` and
   +    // `HttpResponseInterceptor` lambda expressions defined inside `MicrometerHttpClientInterceptor`,
   +    // then only the former breakpoint is hit. As a result the `timerByHttpContext` map entry is
   +    // never removed, causing a memory leak.
   +    @Test
   +    void cancelledRequest(@WiremockResolver.Wiremock WireMockServer server) throws Exception {
   +        server.stubFor(any(anyUrl()).willReturn(ok().withFixedDelay(Integer.MAX_VALUE)));
   +        CloseableHttpAsyncClient client = asyncClient();
   +        client.start();
   +        HttpGet request = new HttpGet(server.baseUrl());
   +
   +        Future<HttpResponse> future = client.execute(request, null);
   +        future.cancel(true);
   +    }
   +
        @Test
        void asyncRequest(@WiremockResolver.Wiremock WireMockServer server) throws Exception {
            server.stubFor(any(anyUrl()));

2. Set breakpoints on MicrometerHttpClientInterceptor:76 and MicrometerHttpClientInterceptor:80.
3. Run the new cancelledRequest test in debug mode.
4. Observe that only the breakpoint on line 76 is hit, and that the test completes without clearing the map entry.

Expected behavior

No memory leak 🙃.

Additional context

This also appears to impact the io.micrometer.core.instrument.binder.httpcomponents.hc5 variant of the class. I see that in #3801 these classes are deleted, so maybe that PR also resolves this issue. I didn't investigate closer. CC @cachescrubber.

[bclozel]

@Stephan202 I'm afraid I haven't found a way to properly fix this in the 4.x generation of the client. We cannot apply the same fix as #3801 as the contracts don't exist. See #3932 for context: we might drop support for the 4.x async client as a result.

[Stephan202]

Tnx for the update @bclozel! On our side we've worked around this issue by migrating the relevant client from RestTemplate to WebClient, with the latter using Netty; that was on the TODO list anyway. This has resolved the OOM situation.

[bclozel]

@Stephan202 Thanks for letting us know. FYI as of Spring Framework 6 and Spring Boot 3, both RestTemplate and WebClient are instrumented already so you might not need to instrument the client library underneath. See the Spring Framework reference docs section on Observability.

[bclozel]

Superseded by #3932 for HttpComponents 4.x and #3800 for HttpComponents 5.x