Memory leak when using @ClientObjectMapper with QuarkusRestClientBuilder

[jbgomond]

Describe the bug

Hello !

I needed to customize the ObjectMapper used by Quarkus Rest Client. To do that, I created a @ClientObjectMapper method in the interface as indicated.

@ClientObjectMapper
static ObjectMapper objectMapper(ObjectMapper defaultObjectMapper) {
    return defaultObjectMapper.copy()
            .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
}

The interface is then used with QuarkusRestClientBuilder to create rest client instances dynamically (I need to change some parameters depending on the request).

But when coupling both, the objectMapper method gets triggered every time. The Jackson instance is being stored in the contextResolverMap variable of the class JacksonUtil, and is not removed when the rest client is destroyed.

And so :

40 862 instances of com.fasterxml.jackson.databind.ser.DefaultSerializerProvider$Impl, loaded by io.quarkus.bootstrap.runner.RunnerClassLoader @ 0x9a1e0138 occupy 243 534 496 (23,25 %) bytes.
Most of these instances are referenced from one instance of java.util.concurrent.ConcurrentHashMap$Node[], loaded by , which occupies 98 751 824 (9,43 %) bytes. The instance is referenced by classloader/component io.quarkus.bootstrap.runner.RunnerClassLoader @ 0x9a1e0138.
Thread com.arjuna.ats.internal.arjuna.coordinator.ReaperThread @ 0x9b2aaeb8 Transaction Reaper has a local variable or reference to io.quarkus.bootstrap.runner.RunnerClassLoader @ 0x9a1e0138 which is on the shortest path to java.util.concurrent.ConcurrentHashMap$Node[65536] @ 0xbfbf8ae0. The thread com.arjuna.ats.internal.arjuna.coordinator.ReaperThread @ 0x9b2aaeb8 Transaction Reaper keeps local variables with total size 552 (0,00 %) bytes.

Thanks

Expected behavior

The ObjectMapper is instantiated once or is destroyed with the rest client

Actual behavior

All ObjectMapper instances are kept in memory

How to Reproduce?

github-issue-44180-reproducer.zip

Output of uname -a or ver

No response

Output of java -version

21.0.5

Quarkus version or git rev

3.15.1

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

[gsmet]

Which version of Quarkus are you using? Could you create a small Maven reproducer?

We had some issues with this in the past (#36067) but they have been addressed (via #36126 and #36719) so I would expect 3.5.1+ to be fine.

[quarkus-bot]

/cc @cescoffier (rest-client)

[geoand]

Which version of Quarkus are you using? Could you create a small Maven reproducer?

+100

[jbgomond]

Sure ! Sorry, I thought I added the version. I'm using 3.15.1.

I added a reproducer :)

[geoand]

@jbgomond would you be able to try #44212 ?

In addition to that PR, you would also neeed to change your code to:

    @GET
    @Produces(MediaType.TEXT_PLAIN)
    public String hello() {
        var client = QuarkusRestClientBuilder.newBuilder()
                .baseUri(URI.create("https://jsonplaceholder.typicode.com"))
                .build(JsonPlaceholderApi.class);

        client.getFakeJson();

        if (client instanceof Closeable c) {
            try {
                c.close();
            } catch (IOException e) {
                Log.debug("Unable to close client", e);
            }
        }

        return "Rest client executed. Check contextResolverMap in JacksonUtil";
    }

[gsmet]

That being said, it doesn't look like a good idea to initialize a new REST Client for each call.

[geoand]

Yeah, it's pretty heavy, but there could be some circumstances where you have no other way.

That said, @jbgomond why exactly do you need to create a new instance for every request?

[geoand]

One of the reasons that used to be necessary in the past, is no longer necessary since 3.16 with the introduction of #43331

[cescoffier]

Initializing a rest client on every call also initialized a connection pool... It can lead to many issues if you have a lot of clients created in parallel. Also, it would not apply the "per-host" limit, which may harm the remote service too.

[jbgomond]

Hello ! Sorry for the delay, thanks for the quick fix :)

@geoand I must be missing something while testing, I compiled all the maven models using your quarkus branch locally, used the 999-SNAPSHOT in my project but it's not using the RestClientClosingTask class for some reason ? Did I forget something ?

@gsmet @cescoffier I agree, I'm planning on adding some sort of cache on my side to avoid over instantiating the builder. The addition in #43331 is very good but in my case, I need to use conditionally a proxy depending on the url called (both provided via a database).

[geoand]

@geoand I must be missing something while testing, I compiled all the maven models using your quarkus branch locally, used the 999-SNAPSHOT in my project but it's not using the RestClientClosingTask class for some reason ? Did I forget something ?

If you checked out my branch and built Quarkus like so, you should be able to see that class (which you won't have to interact with however)

The addition in #43331 is very good but in my case, I need to use conditionally a proxy depending on the url called (both provided via a database).

Mind providing some pseudo-code of what you are doing? I want to see if we can cover this without having to resort to creating a new client

[jbgomond]

Yeah I was not awake, I forgot the close() part in my code :D Works fine !

In my case, I have a list of providers in a database. Each line having the url to call and a boolean if I need to use a proxy of not (because all urls are not needing a proxy).

So I'm doing that :

@GET
@Produces(MediaType.TEXT_PLAIN)
public String callProvider(Provider provider) {
    var client = QuarkusRestClientBuilder.newBuilder()
            .baseUri(URI.create(provider.getUrl()))
            .build(JsonPlaceholderApi.class);

    if (provider.getUseProxy() {
        // set proxy
    }

    client.getFakeJson();

    if (client instanceof Closeable c) {
        try {
            c.close();
        } catch (IOException e) {
            Log.debug("Unable to close client", e);
        }
    }

    return "Rest client executed. Check contextResolverMap in JacksonUtil";
}

[geoand]

Works fine !

Glad to hear it!

What exactly does

    if (provider.getUseProxy() {
        // set proxy
    }

do with the client? Does it really need a new instance?