Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

netty-reactor instrumentation leaks Token on cancellation #1970

Closed
chringwer opened this issue Jul 4, 2024 · 4 comments · Fixed by #1978
Closed

netty-reactor instrumentation leaks Token on cancellation #1970

chringwer opened this issue Jul 4, 2024 · 4 comments · Fixed by #1978
Assignees
Labels
bug Something isn't working as designed/intended jul-sep qtr Represents proposed work item for the Jul-Sep quarter

Comments

@chringwer
Copy link

chringwer commented Jul 4, 2024

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

@chringwer chringwer added the bug Something isn't working as designed/intended label Jul 4, 2024
@workato-integration
Copy link

@jbedell-newrelic
Copy link
Contributor

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
Copy link
Author

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.

@kford-newrelic kford-newrelic added the jul-sep qtr Represents proposed work item for the Jul-Sep quarter label Jul 9, 2024
@kford-newrelic kford-newrelic moved this from Triage to In Quarter in Java Engineering Board Jul 9, 2024
@jbedell-newrelic jbedell-newrelic self-assigned this Jul 9, 2024
@jbedell-newrelic jbedell-newrelic moved this from In Quarter to In Sprint in Java Engineering Board Jul 9, 2024
@jbedell-newrelic jbedell-newrelic moved this from In Sprint to Needs Review in Java Engineering Board Jul 15, 2024
@github-project-automation github-project-automation bot moved this from Needs Review to Code Complete/Done in Java Engineering Board Jul 15, 2024
@jbedell-newrelic
Copy link
Contributor

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as designed/intended jul-sep qtr Represents proposed work item for the Jul-Sep quarter
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants