Fix false-positives when using ResourceLeakDetector. by normanmaurer · Pull Request #6087 · netty/netty

normanmaurer · 2016-12-01T07:37:23Z

Motivation:

We need to ensure the tracked object can not be GC'ed before ResourceLeak.close() is called as otherwise we may get false-positives reported by the ResourceLeakDetector. This can happen as the JIT / GC may be able to figure out that we do not need the tracked object anymore and so already enqueue it for collection before we actually get a chance to close the enclosing ResourceLeak.

Modifications:

Add ResourceLeakTracker and deprecate the old ResourceLeak
Fix some javadocs to correctly release buffers.
Add a unit test for ResourceLeakDetector that shows that ResourceLeakTracker has not the problems.

Result:

No more false-positives reported by ResourceLeakDetector when ResourceLeakDetector.track(...) is used.

Scottmitch · 2016-12-01T17:19:30Z

+        //
+        // See also https://github.com/netty/netty/issues/6034
+        //
+        return leak.close() && trackedObject != null;


I wonder if this may be optimized away at some point...

I think it can't as we use it in the return value... if I remove it the false positives start again, with it not

@normanmaurer - I wonder if we should consider this an API bug and just change the ResourceLeak API ... if others are using this interface they may be susceptible to the same bug without knowing ... changing the API will force them to fix the bug.

@Scottmitch I was considering this but decided against this as this may break code when people upgrade. False-positives are bad but not as bad as breaking code.

That said I wonder if we maybe could just extend ResourceLeak and add the method there and then do an instanceof and if so call the new method. WDYT ? We would still deprecate the old method tho.

Even better we could also add another method to ResourceLeakDetector which returns this new subclass and use it in our code. Then we mark ResourceLeakDetector.open(...) as deprecated.

@Scottmitch let me do this and add as a separate commit to this pr so you can review more easily

@normanmaurer - the new ResourceLeakTracker lgtm ... can we just delete this utility method and add the warning on the close method in ResourceLeak? Just less code to maintain.

Scottmitch · 2016-12-01T17:32:56Z

can we just return derived in this case? What benefit does wrapping in a "leak aware buffer" with NoopResourceLeak provide? Or should we add a package private method on ResourceLeakDetector so that we can force a DefaultResourceLeak to be allocated (only used in this situation when the buffer is derived from another buffer which already has leak info)?

@Scottmitch it was only basically to be consistent and always return the wrapper. That said I think it would also be fine to just return derived. Let me change this

Scottmitch · 2016-12-01T17:34:55Z

are there other types of buffers that we should do buf.unwrap() to? Also is it possible that the SwappedByteBuf will not be the "top level" and we need to go through the "unwrap" chain to properly find/unwrap it?

Let me think a bit more about this but I think no...

Scottmitch · 2016-12-01T17:45:37Z

reproducible

what is its here? can you link to an issue or provide more context?

Scottmitch · 2016-12-01T17:50:18Z

should we catch Throwable after the other exceptions (e.g. InterruptedException) to make sure we bubble up the exception and it is visible from the junit thread?

Scottmitch · 2016-12-01T17:51:36Z

nit: final?

Scottmitch · 2016-12-01T17:51:58Z

nit: final?

normanmaurer · 2016-12-01T20:14:18Z

@Scottmitch I need to adjust the comment here if you agree how I fixed it now makes sense in terms of API changes.

Scottmitch · 2016-12-01T21:13:06Z

nit: kill line

Scottmitch · 2016-12-01T21:14:11Z

I wonder if ResourceLeakRecoder makes more sense since the action this interface provides is record ... or maybe change the methods to track?

I kind of like ResourceLeakTracker more... @nmittler any preferences ?

Scottmitch · 2016-12-01T21:15:11Z

Maybe consider rewording or adding the following:

After this method is called a leak associated with this ResourceLeakTracker should not be reported.

Scottmitch · 2016-12-01T21:16:17Z

Add a warning as to this API (specifically close) may result in false positives.

Deprecated -> @deprecated

Scottmitch · 2016-12-01T21:23:28Z

also add a todo here to make private again once #6081 is fixed.

Scottmitch · 2016-12-01T21:24:38Z

rename to NoopByteBufResourceLeakTracker or just make this class generic (preferably the latter)

Scottmitch · 2016-12-01T21:26:29Z

+        //
+        // See also https://github.com/netty/netty/issues/6034
+        //
+        return leak.close() && trackedObject != null;


@normanmaurer - the new ResourceLeakTracker lgtm ... can we just delete this utility method and add the warning on the close method in ResourceLeak? Just less code to maintain.

Scottmitch

ResourceLeakTracker lgtm .. few more comments though

netkins · 2016-12-01T23:20:28Z

            shutdown();
            if (leak != null) {
-                leak.close();
+                boolean closed = leak.close(ReferenceCountedOpenSslEngine.this);


Remove this useless assignment to local variable "closed".

netkins · 2016-12-01T23:20:29Z


        if (leak != null) {
-            leak.close();
+            boolean closed = leak.close(this);


Remove this useless assignment to local variable "closed".

netkins · 2016-12-01T23:20:29Z


            if (leak != null) {
-                leak.close();
+                boolean closed = leak.close(this);


Remove this useless assignment to local variable "closed".

netkins · 2016-12-01T23:20:30Z

+        final ResourceLeakTracker<DnsMessage> leak = this.leak;
        if (leak != null) {
-            leak.close();
+            boolean closed = leak.close(this);


Remove this useless assignment to local variable "closed".

netkins · 2016-12-01T23:20:30Z

            destroy();
            if (leak != null) {
-                leak.close();
+                boolean closed = leak.close(ReferenceCountedOpenSslContext.this);


Remove this useless assignment to local variable "closed".

normanmaurer · 2016-12-02T12:41:08Z

@Scottmitch PTAL again... I also added a few more asserts and shares some more code. I am quite happy now with the result :)

normanmaurer · 2016-12-02T12:42:41Z

+            // collection before we actually get a chance to close the enclosing ResourceLeak.
+            boolean closed = close() && trackedObject != null;
+
+            // Ensure that the object that was tracked is the same as the one that was passed to close(...).


@Scottmitch note this check... This also ensure we really pass in the correct object and so the GC can not collect it before we called close() :)

Asserts could be eliminated by JIT compiler (especially, when disabled), so I don't think relying on assert here would buy you much.

@vlsi thi was not done to prevent GC or such, just to ensure we do the right thing. That said maybe this should be guarded by an IllegalArgumentException. @Scottmitch WDYT ?

@normanmaurer , I see. Would you probably add an explanation message then to the assert statement?

I'm fine with it being an assert for now as this is typically an internal Netty concern and thus should be caught by unit tests, and it not required to be enforced by the interface. If it becomes an issue we can always make the check more strict.

Can we move the assert to the top of the method and just directly return close() && trackedObject != null after?

normanmaurer · 2016-12-02T12:42:55Z

+            // Store the hash of the tracked object to later assert it in the close(...) method.
+            // It's important that we not store a reference to the referent as this would disallow it from
+            // be collected via the PhantomReference.
+            trackedHash = System.identityHashCode(referent);


@Scottmitch see below why I did this ....

normanmaurer · 2016-12-02T12:43:38Z

-    private final ResourceLeak leak;
+    // This is always the original ByteBuf whichs reference-count is responsible to signal when the ResourceLeakTracker
+    // should be closed. This means this always must be the ByteBuf that is used when calling LeakDetector.track(...).
+    private final ByteBuf trackedByteBuf;


@Scottmitch see the ResourceLeakDetector.close(...) method why I now keep track of this one as well

netkins · 2016-12-02T14:19:45Z

+    private void closeLeak(ByteBuf trackedByteBuf) {
+        // Close the ResourceLeakTracker with the tracked ByteBuf as argument. This must be the same that was used when
+        // calling DefaultResourceLeak.track(...).
+        boolean closed = leak.close(trackedByteBuf);


Remove this useless assignment to local variable "closed".

netkins · 2016-12-02T14:19:45Z

+    private void closeLeak() {
+        // Close the ResourceLeakTracker with the tracked ByteBuf as argument. This must be the same that was used when
+        // calling DefaultResourceLeak.track(...).
+        boolean closed = leak.close(trackedByteBuf);


Remove this useless assignment to local variable "closed".

normanmaurer · 2016-12-02T14:24:06Z


+    // Called from within SimpleLeakAwareByteBuf and AdvancedLeakAwareByteBuf.
+    final void parent(ByteBuf newParent) {
+        assert newParent instanceof SimpleLeakAwareByteBuf || newParent instanceof AdvancedLeakAwareByteBuf;


@Scottmitch I could even change this to assert newParent instanceof SimpleLeakAwareByteBuf as AdvancedLeakAwareByteBufnow extends SimpleLeakAwareByteBuf . Not sure if its better to be explicit here tho. WDYT ?

just checking SimpleLeakAwareByteBuf sgtm ... its an assert meant for debugging ... if the class hierarchy changes the assert will trip and we can update it.

true enough... will change :)

normanmaurer · 2016-12-02T19:43:39Z

@Scottmitch rest is good ?

Scottmitch · 2016-12-02T18:28:24Z

+class SimpleLeakAwareByteBuf extends WrappedByteBuf {

-    private final ResourceLeak leak;
+    // This is always the original ByteBuf whichs reference-count is responsible to signal when the ResourceLeakTracker


/** * This object's is associated with the {@link ResourceLeakTracker}. * When {@link ResourceLeakTracker#close(Object}} is called this object will be * used as the argument. It is also assumed that this object is used when * {@link ResourceLeakDetector#track(Object)} is called to create {@link #leak}. */

Scottmitch · 2016-12-02T21:05:24Z

Scottmitch · 2016-12-02T21:09:30Z

+            // collection before we actually get a chance to close the enclosing ResourceLeak.
+            boolean closed = close() && trackedObject != null;
+
+            // Ensure that the object that was tracked is the same as the one that was passed to close(...).


I'm fine with it being an assert for now as this is typically an internal Netty concern and thus should be caught by unit tests, and it not required to be enforced by the interface. If it becomes an issue we can always make the check more strict.

Can we move the assert to the top of the method and just directly return close() && trackedObject != null after?

Motivation: We need to ensure the tracked object can not be GC'ed before ResourceLeak.close() is called as otherwise we may get false-positives reported by the ResourceLeakDetector. This can happen as the JIT / GC may be able to figure out that we do not need the tracked object anymore and so already enqueue it for collection before we actually get a chance to close the enclosing ResourceLeak. Modifications: - Add ResourceLeakTracker and deprecate the old ResourceLeak - Fix some javadocs to correctly release buffers. - Add a unit test for ResourceLeakDetector that shows that ResourceLeakTracker has not the problems. Result: No more false-positives reported by ResourceLeakDetector when ResourceLeakDetector.track(...) is used.

normanmaurer · 2016-12-03T07:57:32Z

@Scottmitch all addressed ... PTAL again

Motivation: While working on #6087 some buffer leaks showed up. Modifications: Correctly release buffers. Result: No more buffer leaks in memcache and stomp codec tests.

Scottmitch · 2016-12-03T20:09:57Z

@normanmaurer - Good work ... ship it!

normanmaurer · 2016-12-04T08:26:05Z

Cherry-picked into 4.1 (c2f4daa) . Will open an extra pr for 4.0 as there is a bit more work to do.

Motivation: While working on netty#6087 some buffer leaks showed up. Modifications: Correctly release buffers. Result: No more buffer leaks in memcache and stomp codec tests.

normanmaurer self-assigned this Dec 1, 2016

Scottmitch reviewed Dec 1, 2016

View reviewed changes

Scottmitch requested changes Dec 1, 2016

View reviewed changes

normanmaurer force-pushed the fix_false_positive_leak_reports branch 2 times, most recently from 10c5e66 to 8927bfa Compare December 1, 2016 20:06

normanmaurer commented Dec 1, 2016

View reviewed changes

normanmaurer force-pushed the fix_false_positive_leak_reports branch 3 times, most recently from 6cfe500 to e016579 Compare December 1, 2016 20:54

Scottmitch reviewed Dec 1, 2016

View reviewed changes

Scottmitch requested changes Dec 1, 2016

View reviewed changes

normanmaurer force-pushed the fix_false_positive_leak_reports branch from e016579 to 32647a6 Compare December 1, 2016 21:36

netkins reviewed Dec 1, 2016

View reviewed changes

normanmaurer added this to the 4.0.43.Final milestone Dec 2, 2016

normanmaurer added the defect label Dec 2, 2016

normanmaurer force-pushed the fix_false_positive_leak_reports branch 2 times, most recently from 21b5778 to 85e3420 Compare December 2, 2016 12:40

normanmaurer commented Dec 2, 2016

View reviewed changes

netkins reviewed Dec 2, 2016

View reviewed changes

normanmaurer commented Dec 2, 2016

View reviewed changes

Scottmitch requested changes Dec 2, 2016

View reviewed changes

normanmaurer force-pushed the fix_false_positive_leak_reports branch from 85e3420 to fed6acd Compare December 3, 2016 07:57

normanmaurer added a commit that referenced this pull request Dec 3, 2016

Fix buffer leaks in tests

5032e6e

Motivation: While working on #6087 some buffer leaks showed up. Modifications: Correctly release buffers. Result: No more buffer leaks in memcache and stomp codec tests.

normanmaurer mentioned this pull request Dec 3, 2016

Fix buffer leaks in tests #6099

Closed

normanmaurer added a commit that referenced this pull request Dec 3, 2016

Fix buffer leaks in tests

39aefdf

Motivation: While working on #6087 some buffer leaks showed up. Modifications: Correctly release buffers. Result: No more buffer leaks in memcache and stomp codec tests.

Scottmitch approved these changes Dec 3, 2016

View reviewed changes

normanmaurer closed this Dec 4, 2016

normanmaurer deleted the fix_false_positive_leak_reports branch December 4, 2016 08:26

normanmaurer modified the milestones: 4.1.7.Final, 4.0.43.Final Dec 4, 2016

normanmaurer mentioned this pull request Dec 4, 2016

PooledByteBufAllocatorTest#testConcurrentUsage ByteBuf leak #6034

Closed

normanmaurer modified the milestones: 4.0.43.Final, 4.1.7.Final Jan 12, 2017

Uh oh!

Conversation

normanmaurer commented Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Scottmitch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Dec 2, 2016

Uh oh!

normanmaurer commented Dec 1, 2016 •

edited

Loading

normanmaurer Dec 2, 2016 •

edited

Loading

normanmaurer Dec 2, 2016 •

edited

Loading