Pluggable resource leak detector #5392

artgon · 2016-06-14T06:00:23Z

Allow users of Netty to plug in their own leak detector for the purpose of instrumentation.

Motivation:

We are rolling out a large Netty deployment and want to be able to track the amount of leaks we're seeing in production via custom instrumentation. In order to achieve this today, I had to plug in a custom ByteBufAllocator into the bootstrap and have it initialize a custom ResourceLeakDetector. Due to these classes mostly being marked final or having private or static methods, a lot of the code had to be copy-pasted and it's quite ugly.

Modifications:

I've added a static loader method for the ResourceLeakDetector in AbstractByteBuf that tries to instantiate the class passed in via the -Dio.netty.customResourceLeakDetector, otherwise falling back to the default one.
I've modified ResourceLeakDetector to be non-final and to have the reporting broken out in to methods that can be overridden.

Result:

You can instrument leaks in your application by just adding something like the following:

public class InstrumentedResourceLeakDetector<T> extends ResourceLeakDetector<T> {

    @Monitor("InstanceLeakCounter")
    private final AtomicInteger instancesLeakCounter;

    @Monitor("LeakCounter")
    private final AtomicInteger leakCounter;

    public InstrumentedResourceLeakDetector(Class<T> resource) {
        super(resource);
        this.instancesLeakCounter = new AtomicInteger();
        this.leakCounter = new AtomicInteger();
    }

    @Override
    protected void reportTracedLeak(String records) {
        super.reportTracedLeak(records);
        leakCounter.incrementAndGet();
    }

    @Override
    protected void reportUntracedLeak() {
        super.reportUntracedLeak();
        leakCounter.incrementAndGet();
    }

    @Override
    protected void reportInstancesLeak() {
        super.reportInstancesLeak();
        instancesLeakCounter.incrementAndGet();
    }
}

normanmaurer · 2016-06-14T06:05:42Z

buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java

+    static final ResourceLeakDetector<ByteBuf> leakDetector = loadResourceLeakDetector();
+
+    private static ResourceLeakDetector<ByteBuf> loadResourceLeakDetector() {
+        String customLeakDetector = SystemPropertyUtil.get("io.netty.customResourceLeakDetector");


Please use PrivilegedAction to try loading this.

normanmaurer · 2016-06-14T06:08:07Z

@artgon This looks awesome! Just a few comments.

Please also sign the ICLA:

http://netty.io/s/icla

And adjust the commit message to match our template:

http://netty.io/wiki/writing-a-commit-message.html

normanmaurer · 2016-06-14T06:13:53Z

@artgon Also I wonder if it would be good to add something like:

public abstract class ResourceLeakDetectorFactory {
    private static volatile ResourceLeakDetectorFactory INSTANCE = new ResourceLeakDetectorFactory() {....};

    protected ResourceLeakDetectorFactory() { .... }

    public static ResourceLeakDetectorFactory instance() {
        return INSTANCE;
    }

    public static void setResourceLeakDetectorFactory(ResourceLeakDetectorFactory factory) {
        INSTANCE = factory;
    }

    public ResourceLeakDetector(Class<?> clazz) {
        ....
    }
}

Then add all the static loading stuff in this implementation and also provide a setter to do it program and use the factory to obtain the instances. This way the code of loading is in one place and will work even if somewhere else a ResourceLeakDetector is needed.

artgon · 2016-06-14T06:15:29Z

Great, thanks for the feedback. I've signed the agreement and I'll make the changes you suggested. Looks like I followed that commit message template on the comment instead of the commit. Do you want me to just copy it over?

normanmaurer · 2016-06-14T06:22:34Z

Yes please :)

Am 14.06.2016 um 08:15 schrieb Arthur Gonigberg notifications@github.com:

Great, thanks for the feedback. I've signed the agreement and I'll make the changes you suggested. Looks like I followed that commit message template on the comment instead of the commit. Do you want me to just copy it over?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

artgon · 2016-06-14T06:22:44Z

@normanmaurer Yeah, I think that's a good idea. Let me go back to the drawing board and elaborate on this a bit.

normanmaurer · 2016-06-14T06:29:12Z

Cool :) Let me know if you have questions

Am 14.06.2016 um 08:22 schrieb Arthur Gonigberg notifications@github.com:

@normanmaurer Yeah, I think that's a good idea. Let me go back to the drawing board and elaborate on this a bit.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

normanmaurer · 2016-06-14T06:36:36Z

Just ping me when you are ready for another review!

Scottmitch · 2016-06-14T16:15:36Z

buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java

+                    logger.error("Class {} does not inherit from ResourceLeakDetector.", customLeakDetector);
+                }
+            }
+        } catch (Exception e) {


catch a Throwable instead of Exception

artgon · 2016-06-15T07:22:12Z

@normanmaurer I've taken another run at it, let me know what you think.

normanmaurer · 2016-06-15T07:34:02Z

Let me check

Am 15.06.2016 um 09:22 schrieb Arthur Gonigberg notifications@github.com:

@normanmaurer I've taken another run at it, let me know what you think.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

normanmaurer · 2016-06-15T07:49:31Z

common/src/main/java/io/netty/util/ResourceLeakDetectorFactory.java

+        INSTANCE = factory;
+    }
+
+    public abstract <T> ResourceLeakDetector<T> loadResourceLeakDetector(final Class<T> resource);


Please rename to newResourceLeakDetector to be more inline with the rest of netty.

also javadocs please :)

Ah, yes of course. Will do :)

What about the general approach? Is that what you had in mind with the factory?

normanmaurer · 2016-06-16T12:10:01Z

common/src/main/java/io/netty/util/ResourceLeakDetector.java

+     *
+     * @param records - the trace of the leak
+     */
+    protected void reportTracedLeak(String records, String resourceType) {


just a nit but could you swap both of these.

normanmaurer · 2016-06-16T12:11:06Z

common/src/main/java/io/netty/util/ResourceLeakDetectorFactory.java

+            try {
+                if (customLeakDetector != null) {
+                    Class<?> detectorClass = Class.forName(customLeakDetector, true,
+                            PlatformDependent.getSystemClassLoader());


I think this can be done just one time in a static block. There is no need to load the class multiple times.

Yes, it can but it'll be slightly ugly because it will have to handle the ClassNotFoundException.

Whats the problem ? Just catch Throwable log it and always use the default impl later on. We do this in several places in netty.

normanmaurer · 2016-06-16T12:12:19Z

@artgon just four comments. Rest looks awesome!

normanmaurer · 2016-06-17T04:25:52Z

common/src/main/java/io/netty/util/ResourceLeakDetectorFactory.java

+            }
+
+            ResourceLeakDetector<T> resourceLeakDetector = new ResourceLeakDetector<T>(resource);
+            logger.debug("Loaded default ResourceLeakDetector: {}", resourceLeakDetector);


if we remove the log message for loading above please also remove this one

normanmaurer · 2016-06-17T08:42:54Z

@artgon you rock! Will pull this in shortly. Thanks again for this awesome contribution.

normanmaurer · 2016-06-19T18:48:15Z

@artgon Could you also update AbstractDnsMessage to use the factory ? Once done I will pull in.

@OverRide

Allow users of Netty to plug in their own leak detector for the purpose of instrumentation. Motivation: We are rolling out a large Netty deployment and want to be able to track the amount of leaks we're seeing in production via custom instrumentation. In order to achieve this today, I had to plug in a custom `ByteBufAllocator` into the bootstrap and have it initialize a custom `ResourceLeakDetector`. Due to these classes mostly being marked `final` or having private or static methods, a lot of the code had to be copy-pasted and it's quite ugly. Modifications: * I've added a static loader method for the `ResourceLeakDetector` in `AbstractByteBuf` that tries to instantiate the class passed in via the `-Dio.netty.customResourceLeakDetector`, otherwise falling back to the default one. * I've modified `ResourceLeakDetector` to be non-final and to have the reporting broken out in to methods that can be overridden. Result: You can instrument leaks in your application by just adding something like the following: ```java public class InstrumentedResourceLeakDetector<T> extends ResourceLeakDetector<T> { @monitor("InstanceLeakCounter") private final AtomicInteger instancesLeakCounter; @monitor("LeakCounter") private final AtomicInteger leakCounter; public InstrumentedResourceLeakDetector(Class<T> resource) { super(resource); this.instancesLeakCounter = new AtomicInteger(); this.leakCounter = new AtomicInteger(); } @OverRide protected void reportTracedLeak(String records) { super.reportTracedLeak(records); leakCounter.incrementAndGet(); } @OverRide protected void reportUntracedLeak() { super.reportUntracedLeak(); leakCounter.incrementAndGet(); } @OverRide protected void reportInstancesLeak() { super.reportInstancesLeak(); instancesLeakCounter.incrementAndGet(); } } ```

artgon · 2016-06-20T06:55:08Z

Okay, I've made the changes.

normanmaurer · 2016-06-20T06:57:18Z

@artgon ❤️... will pull in once the CI runs finishes

normanmaurer · 2016-06-20T09:17:34Z

Cherry-picked into 4.0 (cad22bb) and 4.1 (3288cac)

normanmaurer · 2016-06-20T09:17:41Z

@artgon thanks again!

normanmaurer reviewed Jun 14, 2016
View reviewed changes

Scottmitch reviewed Jun 14, 2016
View reviewed changes

artgon force-pushed the 4.1 branch 2 times, most recently from ab9cbe0 to 33a1c3d Compare June 15, 2016 07:19

normanmaurer reviewed Jun 15, 2016
View reviewed changes

normanmaurer self-assigned this Jun 15, 2016

normanmaurer added this to the 4.0.38.Final milestone Jun 15, 2016

normanmaurer added the feature label Jun 15, 2016

artgon force-pushed the 4.1 branch from 33a1c3d to 52d58a8 Compare June 16, 2016 05:47

normanmaurer reviewed Jun 16, 2016
View reviewed changes

normanmaurer reviewed Jun 17, 2016
View reviewed changes

artgon force-pushed the 4.1 branch from 52d58a8 to 0539a58 Compare June 17, 2016 06:47

artgon force-pushed the 4.1 branch from 0539a58 to 20f642d Compare June 20, 2016 06:53

normanmaurer closed this Jun 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pluggable resource leak detector #5392

Pluggable resource leak detector #5392

artgon commented Jun 14, 2016

normanmaurer Jun 14, 2016

normanmaurer commented Jun 14, 2016

normanmaurer commented Jun 14, 2016 •

edited

artgon commented Jun 14, 2016

normanmaurer commented Jun 14, 2016

artgon commented Jun 14, 2016

normanmaurer commented Jun 14, 2016

normanmaurer commented Jun 14, 2016

Scottmitch Jun 14, 2016 •

edited

normanmaurer Jun 14, 2016

artgon commented Jun 15, 2016

normanmaurer commented Jun 15, 2016

normanmaurer Jun 15, 2016

normanmaurer Jun 15, 2016

artgon Jun 15, 2016

normanmaurer Jun 15, 2016

normanmaurer Jun 16, 2016

normanmaurer Jun 16, 2016

artgon Jun 17, 2016

normanmaurer Jun 17, 2016

normanmaurer commented Jun 16, 2016 •

edited

normanmaurer Jun 17, 2016

normanmaurer commented Jun 17, 2016

normanmaurer commented Jun 19, 2016

artgon commented Jun 20, 2016

normanmaurer commented Jun 20, 2016

normanmaurer commented Jun 20, 2016

normanmaurer commented Jun 20, 2016

Pluggable resource leak detector #5392

Pluggable resource leak detector #5392

Conversation

artgon commented Jun 14, 2016

Choose a reason for hiding this comment

normanmaurer commented Jun 14, 2016

normanmaurer commented Jun 14, 2016 • edited

artgon commented Jun 14, 2016

normanmaurer commented Jun 14, 2016

artgon commented Jun 14, 2016

normanmaurer commented Jun 14, 2016

normanmaurer commented Jun 14, 2016

Scottmitch Jun 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artgon commented Jun 15, 2016

normanmaurer commented Jun 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanmaurer commented Jun 16, 2016 • edited

Choose a reason for hiding this comment

normanmaurer commented Jun 17, 2016

normanmaurer commented Jun 19, 2016

artgon commented Jun 20, 2016

normanmaurer commented Jun 20, 2016

normanmaurer commented Jun 20, 2016

normanmaurer commented Jun 20, 2016

normanmaurer commented Jun 14, 2016 •

edited

Scottmitch Jun 14, 2016 •

edited

normanmaurer commented Jun 16, 2016 •

edited