newrelic / newrelic-java-agent

The New Relic Java agent
Apache License 2.0
202 stars 144 forks source link

netty-reactor instrumentation leaks Token on cancellation #1970

Closed chringwer closed 4 months ago

chringwer commented 4 months ago

Description

After upgrading the NR java agent to 8.12.0 we observed much higher GC pressure than before. Investigating some heap dumps revealed an issue with the reactor-netty instrumentation module which seems to not handle cancellations properly in some cases so that tokens are not expired timely.

In particular FluxMapFuseable_Instrumentation needs to expire the token when cancelled which is a pretty common scenario for example when doing .flatMap(...).next(). LambdaSubscriber_Instrumentation and LambdaMonoSubscriber_Instrumentation should handle dispose() and expire their tokens accordingly, I guess.

For us, patching FluxMapFuseable_Instrumentation with a cancel() implementation resolved the memory issues:

    @Weave(type = MatchType.ExactClass, originalName = "reactor.core.publisher.FluxMapFuseable$MapFuseableSubscriber")
    static final class MapFuseableSubscriber_Instrumentation<T, R> {
     ...
        public void cancel() {
            if (token != null) {
                token.linkAndExpire();
                token = null;
            }
            Weaver.callOriginal();
        }
     ...
    }

Expected Behavior

Cancelling an instrumented publisher should not leak any objects

Troubleshooting or NR Diag results

Steps to Reproduce

Your Environment

Java 17, NR agent 8.12.0

Additional context

workato-integration[bot] commented 4 months ago

https://new-relic.atlassian.net/browse/NR-288379

jbedell-newrelic commented 4 months ago

HI @chringwer , thanks for the report. I'm having a little trouble reproducing the issue for testing. Is there any chance you have a quick repro app? Or perhaps a more detailed code example?

chringwer commented 4 months ago

I am thinking of something like this:

    Flux.<Integer>create(sink -> {})
        .map(num -> num * 10)
        .timeout(Duration.ofMillis(10L), Mono.empty())
        .blockLast();

So, basically whenever you have a Fuseable source which does not emit before being cancelled. In that case the Token will leak.

jbedell-newrelic commented 4 months ago

Thanks @chringwer for the bug report. The fix should be in the next release. (Note: It didn't quite make it into last week's release.)