Reactor-netty: Memory leak in HttpClient

[michaelr524]

Please check the reproducible example here: https://github.com/michaelr524/reactor-netty-ssl-leak

mvn clean package
java -jar target/reactor-netty-ssl-leak-1.0-SNAPSHOT.jar 

• Reactor version(s) used: 0.9.4.RELEASE
• Other relevant libraries versions (eg. netty, ...):
• JVM version (javar -version):
  openjdk version "12.0.2" 2019-07-16
  OpenJDK Runtime Environment (build 12.0.2+10)
  OpenJDK 64-Bit Server VM (build 12.0.2+10, mixed mode, sharing)
• OS and version (eg uname -a): Darwin Michaels-MacBook-Pro.local 19.3.0 Darwin Kernel Version 19.3.0: Thu Jan 9 20:58:23 PST 2020; root:xnu-6153.81.5~1/RELEASE_X86_64 x86_64

Looks like something related to the ssl context:

[michaelr524]

Moving the creation of the sslcontext builder out of the lambda seems to work around the problem:

        SslContextBuilder sslCtxBuilder = SslContextBuilder.forClient();

        for (int i = 0; i < 10_000_000; i++) {
            var j = i;
            HttpClient.create()
                    .headers(h -> h.set(HttpHeaderNames.CONTENT_TYPE, "application/json"))
                    .secure(spec -> spec.sslContext(sslCtxBuilder))
                    .get()
                    .uri("https://api.covid19api.com/")
                    .responseContent()
                    .aggregate()
                    .asString()
                    .doOnNext(respBody -> System.out.println(j + ": " + respBody.substring(0, 20)))
                    .single().block();
            Thread.sleep(500);
        }

[violetagg]

@michaelr524 Actually that's not a workaround but the way how you should use it.

With your previous code you always was creating a new TcpClient with its own security settings.

public static TcpClient setTcpOptions(TcpClient tcpClient) {
    return tcpClient
            .secure(spec -> spec.sslContext(SslContextBuilder.forClient()))
            .doOnConnected(conn -> conn
                    .addHandlerLast(new ReadTimeoutHandler(20_000, TimeUnit.MILLISECONDS))
                    .addHandlerLast(new WriteTimeoutHandler(20_000, TimeUnit.MILLISECONDS)))
            .option(ChannelOption.CONNECT_TIMEOUT_MILLIS, 10_000);
}

Either use the way above and configure the security directly on HttpClient level or if you insist of using a TcpClient then configure the TcpClient once and then create the HttpClient from TcpClient.

TcpClient tcpClient = setTcpOptions();
...
HttpClient.from(tcpClient)
          .headers(h -> h.set(HttpHeaderNames.CONTENT_TYPE, "application/json"))
          //.tcpConfiguration(Main::setTcpOptions)
          .get()
...
public static TcpClient setTcpOptions() {
    return TcpClient.create()
            .secure(spec -> spec.sslContext(SslContextBuilder.forClient()))
            .doOnConnected(conn -> conn
                    .addHandlerLast(new ReadTimeoutHandler(20_000, TimeUnit.MILLISECONDS))
                    .addHandlerLast(new WriteTimeoutHandler(20_000, TimeUnit.MILLISECONDS)))
            .option(ChannelOption.CONNECT_TIMEOUT_MILLIS, 10_000);
}

[michaelr524]

Just to clarify a bit:

This also reproduces the issue (without .tcpConfiguration()):

            HttpClient.create()
                    .headers(h -> h.set(HttpHeaderNames.CONTENT_TYPE, "application/json"))
                    .secure(spec -> spec.sslContext(SslContextBuilder.forClient()))
                    .get()

Now I'm calling SslContextBuilder.forClient() just once and reusing it for all HttpClients.

BTW, I'm using .tcpConfiguration() in order to set Read/Write timeouts. Is there a way to do the same on the HttpClient level?

[violetagg]

@michaelr524

BTW, I'm using .tcpConfiguration() in order to set Read/Write timeouts. Is there a way to do the same on the HttpClient level?

See above you can use HttpClient.from(tcpClient)

Reopen to check .secure(spec -> spec.sslContext(SslContextBuilder.forClient())) issue.

[violetagg]

@michaelr524
The code below creates an issue, because you create always a new HttpClient

 HttpClient.create()
           .headers(h -> h.set(HttpHeaderNames.CONTENT_TYPE, "application/json"))
           .secure(spec -> spec.sslContext(SslContextBuilder.forClient()))
           .get()

You can extract this code before the loop and use the configured client in the loop:

 HttpClient client = HttpClient.create()
                               .headers(h -> h.set(HttpHeaderNames.CONTENT_TYPE, "application/json"))
                               .secure(spec -> spec.sslContext(SslContextBuilder.forClient()));

[michaelr524]

@violetagg Thanks for clarifying again.
I think that it may help new users if you change the documentation a bit so that they won't use this pattern. It's not obvious from the documentation that a new client is created.
Also, it's not obvious that this client will not be released after the request is complete.
Looks like this pattern is replicated by many developers: https://github.com/search?l=Java&q=spec.sslContext%28SslContextBuilder.forClient%28%29%29&type=Code

[violetagg]

@michaelr524 Would you like to provide a PR with an update to the documentation?

[michaelr524]

Will do

[gtomassi]

@michaelr524 @violetagg
Furthermore, if HttpClient has a life cycle and should be re-used then it is unclear from any documentation how to shut one down and cleanup any backing resources?

Whether or not to re-use HttpClient(s) and clean them up has been a big question for us for over a year now and so I'm happy to finally see an answer to that question, but we're still unclear how to clean up an instance? We also thought the docs were suggesting creating a new instance on each request.

For example, at the abstracted layer of just dealing with the HttpClient, what triggers the Netty shutdown (or equivalent) behind the scenes? (something like this):
```java
f.channel().closeFuture().sync();
...
workerGroup.shutdownGracefully();

[violetagg]

@gtomassi

We also thought the docs were suggesting creating a new instance on each request.

Where did you find a suggestion for creating a new instance every time? I should fix that. The documentation shows how you can make one request.
I'll update javadoc so that it says that invoking create you will receive a new HttpClient.

I'm opened for suggestions (better if it is a PR) how to improve the reference documentation so that it is clear that with invoking HttpClient.create you will receive a new instance.

You can achieve the code above from Netty like this:

1.

HttpClient.create() <- this uses the so called global HttpResources
...

HttpResources.disposeLoopsAndConnections(); <- this is fire and forget
or
HttpResources.disposeLoopsAndConnectionsLater().block() <- if you need to wait for the result

2.

LoopResources loop = LoopResources.create(...)
HttpClient.create()
     .runOn(loop)
...

loop.dispose(); <- this is fire and forget
or
loop.disposeLater().block() <- if you need to wait for the result

[gtomassi]

@violetagg
It is not a question that invoking create would produce a new instance, but rather when/why to do it. As it reads to me, the HttpClient is ambiguous in many scenarios, for instance a very common use case is when someone needs to change headers on each request when querying a single server or API (CONTENT_LENGTH, AUTHORIZATION, CONTENT_TYPE, etc). The same applies for numerous other types of situations that require changing options on each request; as another example of a point of confusion, port can be specified to the HttpClient request "factory" but overridden in the URI of an individual request? It is the mixing of "request factory" and "request builder" in the builder pattern that could use some clarification.

As far as I can tell the only way to do these examples (at least in a thread-safe manner) is by creating a new instance of HttpClient on each request. Thus it is the natural and only sensible conclusion that it is the design of the library to recreate many client instances on demand as lightweight wrappers around a global resource pool.

Where is the barrier of when the transition is made from "request factory" to "single request builder" -> perhaps after get() or post() is called and one has moved to "ResponseReceiver"? This is entirely not obvious, a response receiver would receive responses, not be the obvious candidate to represent the abstract level of building a request -- and if it is it doesn't do a lot of necessary things. Only looking through the HTTP client unit tests was I able to see that having a different URI using the same HttpClient is even possible, I don't think that is anywhere in the javadocs nor the example docs, although I could have missed it at the time.

Thank you for the info on the resources shutdown.

[violetagg]

@michaelr524 @gtomassi I changed the examples for HttpClient (See here https://projectreactor.io/docs/netty/snapshot/reference/index.html#_ssl_andtls)

Any suggestions for further improvements are welcome.

[violetagg]

I changed the examples in the reference documentation so that now the HttpClient is created separate from the request.
If we need more improvements we can reopen this one.