quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.77k stars 2.68k forks source link

Memory leak when using @ClientObjectMapper with QuarkusRestClientBuilder #44180

Open jbgomond opened 1 week ago

jbgomond commented 1 week ago

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 commented 6 days ago

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

We had some issues with this in the past (https://github.com/quarkusio/quarkus/issues/36067) but they have been addressed (via https://github.com/quarkusio/quarkus/pull/36126 and https://github.com/quarkusio/quarkus/pull/36719) so I would expect 3.5.1+ to be fine.

quarkus-bot[bot] commented 6 days ago

/cc @cescoffier (rest-client)

geoand commented 6 days ago

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

+100

jbgomond commented 6 days ago

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

I added a reproducer :)

geoand commented 5 days ago

@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 commented 5 days ago

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

geoand commented 5 days ago

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 commented 5 days ago

One of the reasons that used to be necessary in the past, is no longer necessary since 3.16 with the introduction of https://github.com/quarkusio/quarkus/pull/43331

cescoffier commented 2 days ago

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 commented 1 day ago

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 https://github.com/quarkusio/quarkus/pull/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 commented 20 hours ago

@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 https://github.com/quarkusio/quarkus/pull/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 commented 4 hours ago

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";
}