Permalink
Browse files

Fix false-positives when using ResourceLeakDetector.

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.
  • Loading branch information...
1 parent 7ce0f35 commit c2f4daa7398a9363fde8e51bf52c0d0323f870a5 @normanmaurer normanmaurer committed with normanmaurer Dec 1, 2016
Showing with 549 additions and 227 deletions.
  1. +7 −7 buffer/src/main/java/io/netty/buffer/AbstractByteBufAllocator.java
  2. +6 −0 buffer/src/main/java/io/netty/buffer/AbstractPooledDerivedByteBuf.java
  3. +27 −46 buffer/src/main/java/io/netty/buffer/AdvancedLeakAwareByteBuf.java
  4. +17 −40 buffer/src/main/java/io/netty/buffer/AdvancedLeakAwareCompositeByteBuf.java
  5. +113 −47 buffer/src/main/java/io/netty/buffer/SimpleLeakAwareByteBuf.java
  6. +48 −26 buffer/src/main/java/io/netty/buffer/SimpleLeakAwareCompositeByteBuf.java
  7. +38 −30 buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java
  8. +1 −1 buffer/src/test/java/io/netty/buffer/AdvancedLeakAwareByteBufTest.java
  9. +1 −1 buffer/src/test/java/io/netty/buffer/AdvancedLeakAwareCompositeByteBufTest.java
  10. +13 −8 buffer/src/test/java/io/netty/buffer/{NoopResourceLeak.java → NoopResourceLeakTracker.java}
  11. +1 −1 buffer/src/test/java/io/netty/buffer/SimpleLeakAwareByteBufTest.java
  12. +1 −1 buffer/src/test/java/io/netty/buffer/SimpleLeakAwareCompositeByteBufTest.java
  13. +5 −4 codec-dns/src/main/java/io/netty/handler/codec/dns/AbstractDnsMessage.java
  14. +6 −4 common/src/main/java/io/netty/util/HashedWheelTimer.java
  15. +4 −0 common/src/main/java/io/netty/util/ResourceLeak.java
  16. +40 −3 common/src/main/java/io/netty/util/ResourceLeakDetector.java
  17. +39 −0 common/src/main/java/io/netty/util/ResourceLeakTracker.java
  18. +172 −0 common/src/test/java/io/netty/util/ResourceLeakDetectorTest.java
  19. +5 −4 handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java
  20. +5 −4 handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
@@ -16,8 +16,8 @@
package io.netty.buffer;
-import io.netty.util.ResourceLeak;
import io.netty.util.ResourceLeakDetector;
+import io.netty.util.ResourceLeakTracker;
import io.netty.util.internal.PlatformDependent;
import io.netty.util.internal.StringUtil;
@@ -29,17 +29,17 @@
static final int DEFAULT_MAX_COMPONENTS = 16;
protected static ByteBuf toLeakAwareBuffer(ByteBuf buf) {
- ResourceLeak leak;
+ ResourceLeakTracker<ByteBuf> leak;
switch (ResourceLeakDetector.getLevel()) {
case SIMPLE:
- leak = AbstractByteBuf.leakDetector.open(buf);
+ leak = AbstractByteBuf.leakDetector.track(buf);
if (leak != null) {
buf = new SimpleLeakAwareByteBuf(buf, leak);
}
break;
case ADVANCED:
case PARANOID:
- leak = AbstractByteBuf.leakDetector.open(buf);
+ leak = AbstractByteBuf.leakDetector.track(buf);
if (leak != null) {
buf = new AdvancedLeakAwareByteBuf(buf, leak);
}
@@ -51,17 +51,17 @@ protected static ByteBuf toLeakAwareBuffer(ByteBuf buf) {
}
protected static CompositeByteBuf toLeakAwareBuffer(CompositeByteBuf buf) {
- ResourceLeak leak;
+ ResourceLeakTracker<ByteBuf> leak;
switch (ResourceLeakDetector.getLevel()) {
case SIMPLE:
- leak = AbstractByteBuf.leakDetector.open(buf);
+ leak = AbstractByteBuf.leakDetector.track(buf);
if (leak != null) {
buf = new SimpleLeakAwareCompositeByteBuf(buf, leak);
}
break;
case ADVANCED:
case PARANOID:
- leak = AbstractByteBuf.leakDetector.open(buf);
+ leak = AbstractByteBuf.leakDetector.track(buf);
if (leak != null) {
buf = new AdvancedLeakAwareCompositeByteBuf(buf, leak);
}
@@ -43,6 +43,12 @@
this.recyclerHandle = (Handle<AbstractPooledDerivedByteBuf>) recyclerHandle;
}
+ // Called from within SimpleLeakAwareByteBuf and AdvancedLeakAwareByteBuf.
+ final void parent(ByteBuf newParent) {
+ assert newParent instanceof SimpleLeakAwareByteBuf;
+ parent = newParent;
+ }
+
@Override
public final AbstractByteBuf unwrap() {
return rootParent;
@@ -17,7 +17,7 @@
package io.netty.buffer;
import io.netty.util.ByteProcessor;
-import io.netty.util.ResourceLeak;
+import io.netty.util.ResourceLeakTracker;
import io.netty.util.internal.SystemPropertyUtil;
import io.netty.util.internal.logging.InternalLogger;
import io.netty.util.internal.logging.InternalLoggerFactory;
@@ -32,7 +32,7 @@
import java.nio.channels.ScatteringByteChannel;
import java.nio.charset.Charset;
-final class AdvancedLeakAwareByteBuf extends WrappedByteBuf {
+final class AdvancedLeakAwareByteBuf extends SimpleLeakAwareByteBuf {
private static final String PROP_ACQUIRE_AND_RELEASE_ONLY = "io.netty.leakDetection.acquireAndReleaseOnly";
private static final boolean ACQUIRE_AND_RELEASE_ONLY;
@@ -47,14 +47,15 @@
}
}
- private final ResourceLeak leak;
+ AdvancedLeakAwareByteBuf(ByteBuf buf, ResourceLeakTracker<ByteBuf> leak) {
+ super(buf, leak);
+ }
- AdvancedLeakAwareByteBuf(ByteBuf buf, ResourceLeak leak) {
- super(buf);
- this.leak = leak;
+ AdvancedLeakAwareByteBuf(ByteBuf wrapped, ByteBuf trackedByteBuf, ResourceLeakTracker<ByteBuf> leak) {
+ super(wrapped, trackedByteBuf, leak);
}
- static void recordLeakNonRefCountingOperation(ResourceLeak leak) {
+ static void recordLeakNonRefCountingOperation(ResourceLeakTracker<ByteBuf> leak) {
if (!ACQUIRE_AND_RELEASE_ONLY) {
leak.record();
}
@@ -63,59 +64,55 @@ static void recordLeakNonRefCountingOperation(ResourceLeak leak) {
@Override
public ByteBuf order(ByteOrder endianness) {
recordLeakNonRefCountingOperation(leak);
- if (order() == endianness) {
- return this;
- } else {
- return new AdvancedLeakAwareByteBuf(super.order(endianness), leak);
- }
+ return super.order(endianness);
}
@Override
public ByteBuf slice() {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.slice(), leak);
+ return super.slice();
}
@Override
- public ByteBuf retainedSlice() {
+ public ByteBuf slice(int index, int length) {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.retainedSlice(), leak);
+ return super.slice(index, length);
}
@Override
- public ByteBuf slice(int index, int length) {
+ public ByteBuf retainedSlice() {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.slice(index, length), leak);
+ return super.retainedSlice();
}
@Override
public ByteBuf retainedSlice(int index, int length) {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.retainedSlice(index, length), leak);
+ return super.retainedSlice(index, length);
}
@Override
- public ByteBuf duplicate() {
+ public ByteBuf retainedDuplicate() {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.duplicate(), leak);
+ return super.retainedDuplicate();
}
@Override
- public ByteBuf retainedDuplicate() {
+ public ByteBuf readRetainedSlice(int length) {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.retainedDuplicate(), leak);
+ return super.readRetainedSlice(length);
}
@Override
- public ByteBuf readSlice(int length) {
+ public ByteBuf duplicate() {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.readSlice(length), leak);
+ return super.duplicate();
}
@Override
- public ByteBuf readRetainedSlice(int length) {
+ public ByteBuf readSlice(int length) {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.readRetainedSlice(length), leak);
+ return super.readSlice(length);
}
@Override
@@ -919,7 +916,7 @@ public int writeBytes(FileChannel in, long position, int length) throws IOExcept
@Override
public ByteBuf asReadOnly() {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.asReadOnly(), leak);
+ return super.asReadOnly();
}
@Override
@@ -947,24 +944,8 @@ public ByteBuf touch(Object hint) {
}
@Override
- public boolean release() {
- boolean deallocated = super.release();
- if (deallocated) {
- leak.close();
- } else {
- leak.record();
- }
- return deallocated;
- }
-
- @Override
- public boolean release(int decrement) {
- boolean deallocated = super.release(decrement);
- if (deallocated) {
- leak.close();
- } else {
- leak.record();
- }
- return deallocated;
+ protected AdvancedLeakAwareByteBuf newLeakAwareByteBuf(
+ ByteBuf buf, ByteBuf trackedByteBuf, ResourceLeakTracker<ByteBuf> leakTracker) {
+ return new AdvancedLeakAwareByteBuf(buf, trackedByteBuf, leakTracker);
}
}
@@ -17,7 +17,7 @@
import io.netty.util.ByteProcessor;
-import io.netty.util.ResourceLeak;
+import io.netty.util.ResourceLeakTracker;
import java.io.IOException;
import java.io.InputStream;
@@ -33,77 +33,70 @@
import static io.netty.buffer.AdvancedLeakAwareByteBuf.recordLeakNonRefCountingOperation;
-final class AdvancedLeakAwareCompositeByteBuf extends WrappedCompositeByteBuf {
+final class AdvancedLeakAwareCompositeByteBuf extends SimpleLeakAwareCompositeByteBuf {
- private final ResourceLeak leak;
-
- AdvancedLeakAwareCompositeByteBuf(CompositeByteBuf wrapped, ResourceLeak leak) {
- super(wrapped);
- this.leak = leak;
+ AdvancedLeakAwareCompositeByteBuf(CompositeByteBuf wrapped, ResourceLeakTracker<ByteBuf> leak) {
+ super(wrapped, leak);
}
@Override
public ByteBuf order(ByteOrder endianness) {
recordLeakNonRefCountingOperation(leak);
- if (order() == endianness) {
- return this;
- } else {
- return new AdvancedLeakAwareByteBuf(super.order(endianness), leak);
- }
+ return super.order(endianness);
}
@Override
public ByteBuf slice() {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.slice(), leak);
+ return super.slice();
}
@Override
public ByteBuf retainedSlice() {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.retainedSlice(), leak);
+ return super.retainedSlice();
}
@Override
public ByteBuf slice(int index, int length) {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.slice(index, length), leak);
+ return super.slice(index, length);
}
@Override
public ByteBuf retainedSlice(int index, int length) {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.retainedSlice(index, length), leak);
+ return super.retainedSlice(index, length);
}
@Override
public ByteBuf duplicate() {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.duplicate(), leak);
+ return super.duplicate();
}
@Override
public ByteBuf retainedDuplicate() {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.retainedDuplicate(), leak);
+ return super.retainedDuplicate();
}
@Override
public ByteBuf readSlice(int length) {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.readSlice(length), leak);
+ return super.readSlice(length);
}
@Override
public ByteBuf readRetainedSlice(int length) {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.readRetainedSlice(length), leak);
+ return super.readRetainedSlice(length);
}
@Override
public ByteBuf asReadOnly() {
recordLeakNonRefCountingOperation(leak);
- return new AdvancedLeakAwareByteBuf(super.asReadOnly(), leak);
+ return super.asReadOnly();
}
@Override
@@ -1037,24 +1030,8 @@ public CompositeByteBuf touch(Object hint) {
}
@Override
- public boolean release() {
- boolean deallocated = super.release();
- if (deallocated) {
- leak.close();
- } else {
- leak.record();
- }
- return deallocated;
- }
-
- @Override
- public boolean release(int decrement) {
- boolean deallocated = super.release(decrement);
- if (deallocated) {
- leak.close();
- } else {
- leak.record();
- }
- return deallocated;
+ protected AdvancedLeakAwareByteBuf newLeakAwareByteBuf(
+ ByteBuf wrapped, ByteBuf trackedByteBuf, ResourceLeakTracker<ByteBuf> leakTracker) {
+ return new AdvancedLeakAwareByteBuf(wrapped, trackedByteBuf, leakTracker);
}
}
Oops, something went wrong.

0 comments on commit c2f4daa

Please sign in to comment.