From c2f4daa7398a9363fde8e51bf52c0d0323f870a5 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 1 Dec 2016 08:36:16 +0100 Subject: [PATCH] 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. --- .../buffer/AbstractByteBufAllocator.java | 14 +- .../buffer/AbstractPooledDerivedByteBuf.java | 6 + .../buffer/AdvancedLeakAwareByteBuf.java | 93 ++++------ .../AdvancedLeakAwareCompositeByteBuf.java | 57 ++---- .../netty/buffer/SimpleLeakAwareByteBuf.java | 172 ++++++++++++------ .../SimpleLeakAwareCompositeByteBuf.java | 74 +++++--- .../io/netty/buffer/AbstractByteBufTest.java | 68 ++++--- .../buffer/AdvancedLeakAwareByteBufTest.java | 2 +- ...AdvancedLeakAwareCompositeByteBufTest.java | 2 +- ...Leak.java => NoopResourceLeakTracker.java} | 21 ++- .../buffer/SimpleLeakAwareByteBufTest.java | 2 +- .../SimpleLeakAwareCompositeByteBufTest.java | 2 +- .../handler/codec/dns/AbstractDnsMessage.java | 9 +- .../java/io/netty/util/HashedWheelTimer.java | 10 +- .../main/java/io/netty/util/ResourceLeak.java | 4 + .../io/netty/util/ResourceLeakDetector.java | 43 ++++- .../io/netty/util/ResourceLeakTracker.java | 39 ++++ .../netty/util/ResourceLeakDetectorTest.java | 172 ++++++++++++++++++ .../ssl/ReferenceCountedOpenSslContext.java | 9 +- .../ssl/ReferenceCountedOpenSslEngine.java | 9 +- 20 files changed, 565 insertions(+), 243 deletions(-) rename buffer/src/test/java/io/netty/buffer/{NoopResourceLeak.java => NoopResourceLeakTracker.java} (61%) create mode 100644 common/src/main/java/io/netty/util/ResourceLeakTracker.java create mode 100644 common/src/test/java/io/netty/util/ResourceLeakDetectorTest.java diff --git a/buffer/src/main/java/io/netty/buffer/AbstractByteBufAllocator.java b/buffer/src/main/java/io/netty/buffer/AbstractByteBufAllocator.java index 50d82e2512..c29619ad34 100644 --- a/buffer/src/main/java/io/netty/buffer/AbstractByteBufAllocator.java +++ b/buffer/src/main/java/io/netty/buffer/AbstractByteBufAllocator.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 @@ public abstract class AbstractByteBufAllocator implements ByteBufAllocator { static final int DEFAULT_MAX_COMPONENTS = 16; protected static ByteBuf toLeakAwareBuffer(ByteBuf buf) { - ResourceLeak leak; + ResourceLeakTracker 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 @@ public abstract class AbstractByteBufAllocator implements ByteBufAllocator { } protected static CompositeByteBuf toLeakAwareBuffer(CompositeByteBuf buf) { - ResourceLeak leak; + ResourceLeakTracker 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); } diff --git a/buffer/src/main/java/io/netty/buffer/AbstractPooledDerivedByteBuf.java b/buffer/src/main/java/io/netty/buffer/AbstractPooledDerivedByteBuf.java index 0dfe5e3cf2..80f33914d7 100644 --- a/buffer/src/main/java/io/netty/buffer/AbstractPooledDerivedByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/AbstractPooledDerivedByteBuf.java @@ -43,6 +43,12 @@ abstract class AbstractPooledDerivedByteBuf extends AbstractReferenceCountedByte this.recyclerHandle = (Handle) 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; diff --git a/buffer/src/main/java/io/netty/buffer/AdvancedLeakAwareByteBuf.java b/buffer/src/main/java/io/netty/buffer/AdvancedLeakAwareByteBuf.java index 9bb6ca02c1..3b08f85120 100644 --- a/buffer/src/main/java/io/netty/buffer/AdvancedLeakAwareByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/AdvancedLeakAwareByteBuf.java @@ -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.GatheringByteChannel; 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 @@ final class AdvancedLeakAwareByteBuf extends WrappedByteBuf { } } - private final ResourceLeak leak; - - AdvancedLeakAwareByteBuf(ByteBuf buf, ResourceLeak leak) { - super(buf); - this.leak = leak; + AdvancedLeakAwareByteBuf(ByteBuf buf, ResourceLeakTracker leak) { + super(buf, leak); } - static void recordLeakNonRefCountingOperation(ResourceLeak leak) { + AdvancedLeakAwareByteBuf(ByteBuf wrapped, ByteBuf trackedByteBuf, ResourceLeakTracker leak) { + super(wrapped, trackedByteBuf, leak); + } + + static void recordLeakNonRefCountingOperation(ResourceLeakTracker leak) { if (!ACQUIRE_AND_RELEASE_ONLY) { leak.record(); } @@ -63,59 +64,55 @@ final class AdvancedLeakAwareByteBuf extends WrappedByteBuf { @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); - } - - @Override - public ByteBuf retainedSlice() { - recordLeakNonRefCountingOperation(leak); - return new AdvancedLeakAwareByteBuf(super.retainedSlice(), leak); + return super.slice(); } @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() { + recordLeakNonRefCountingOperation(leak); + return super.retainedSlice(); } @Override public ByteBuf retainedSlice(int index, int length) { recordLeakNonRefCountingOperation(leak); - return new AdvancedLeakAwareByteBuf(super.retainedSlice(index, length), leak); - } - - @Override - public ByteBuf duplicate() { - recordLeakNonRefCountingOperation(leak); - return new AdvancedLeakAwareByteBuf(super.duplicate(), leak); + return super.retainedSlice(index, length); } @Override public ByteBuf retainedDuplicate() { recordLeakNonRefCountingOperation(leak); - return new AdvancedLeakAwareByteBuf(super.retainedDuplicate(), leak); - } - - @Override - public ByteBuf readSlice(int length) { - recordLeakNonRefCountingOperation(leak); - return new AdvancedLeakAwareByteBuf(super.readSlice(length), leak); + return super.retainedDuplicate(); } @Override public ByteBuf readRetainedSlice(int length) { recordLeakNonRefCountingOperation(leak); - return new AdvancedLeakAwareByteBuf(super.readRetainedSlice(length), leak); + return super.readRetainedSlice(length); + } + + @Override + public ByteBuf duplicate() { + recordLeakNonRefCountingOperation(leak); + return super.duplicate(); + } + + @Override + public ByteBuf readSlice(int length) { + recordLeakNonRefCountingOperation(leak); + return super.readSlice(length); } @Override @@ -919,7 +916,7 @@ final class AdvancedLeakAwareByteBuf extends WrappedByteBuf { @Override public ByteBuf asReadOnly() { recordLeakNonRefCountingOperation(leak); - return new AdvancedLeakAwareByteBuf(super.asReadOnly(), leak); + return super.asReadOnly(); } @Override @@ -947,24 +944,8 @@ final class AdvancedLeakAwareByteBuf extends WrappedByteBuf { } @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 leakTracker) { + return new AdvancedLeakAwareByteBuf(buf, trackedByteBuf, leakTracker); } } diff --git a/buffer/src/main/java/io/netty/buffer/AdvancedLeakAwareCompositeByteBuf.java b/buffer/src/main/java/io/netty/buffer/AdvancedLeakAwareCompositeByteBuf.java index 932a0e70a6..1c9155fcbd 100644 --- a/buffer/src/main/java/io/netty/buffer/AdvancedLeakAwareCompositeByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/AdvancedLeakAwareCompositeByteBuf.java @@ -17,7 +17,7 @@ package io.netty.buffer; 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 java.util.List; 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 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 @@ final class AdvancedLeakAwareCompositeByteBuf extends WrappedCompositeByteBuf { } @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 leakTracker) { + return new AdvancedLeakAwareByteBuf(wrapped, trackedByteBuf, leakTracker); } } diff --git a/buffer/src/main/java/io/netty/buffer/SimpleLeakAwareByteBuf.java b/buffer/src/main/java/io/netty/buffer/SimpleLeakAwareByteBuf.java index b14cb5d7af..02b73de989 100644 --- a/buffer/src/main/java/io/netty/buffer/SimpleLeakAwareByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/SimpleLeakAwareByteBuf.java @@ -16,17 +16,75 @@ package io.netty.buffer; -import io.netty.util.ResourceLeak; +import io.netty.util.ResourceLeakDetector; +import io.netty.util.ResourceLeakTracker; +import io.netty.util.internal.ObjectUtil; import java.nio.ByteOrder; -final class SimpleLeakAwareByteBuf extends WrappedByteBuf { +class SimpleLeakAwareByteBuf extends WrappedByteBuf { - private final ResourceLeak leak; + /** + * 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}. + */ + private final ByteBuf trackedByteBuf; + final ResourceLeakTracker leak; - SimpleLeakAwareByteBuf(ByteBuf buf, ResourceLeak leak) { - super(buf); - this.leak = leak; + SimpleLeakAwareByteBuf(ByteBuf wrapped, ByteBuf trackedByteBuf, ResourceLeakTracker leak) { + super(wrapped); + this.trackedByteBuf = ObjectUtil.checkNotNull(trackedByteBuf, "trackedByteBuf"); + this.leak = ObjectUtil.checkNotNull(leak, "leak"); + } + + SimpleLeakAwareByteBuf(ByteBuf wrapped, ResourceLeakTracker leak) { + this(wrapped, wrapped, leak); + } + + @Override + public ByteBuf slice() { + return newSharedLeakAwareByteBuf(super.slice()); + } + + @Override + public ByteBuf retainedSlice() { + return unwrappedDerived(super.retainedSlice()); + } + + @Override + public ByteBuf retainedSlice(int index, int length) { + return unwrappedDerived(super.retainedSlice(index, length)); + } + + @Override + public ByteBuf retainedDuplicate() { + return unwrappedDerived(super.retainedDuplicate()); + } + + @Override + public ByteBuf readRetainedSlice(int length) { + return unwrappedDerived(super.readRetainedSlice(length)); + } + + @Override + public ByteBuf slice(int index, int length) { + return newSharedLeakAwareByteBuf(super.slice(index, length)); + } + + @Override + public ByteBuf duplicate() { + return newSharedLeakAwareByteBuf(super.duplicate()); + } + + @Override + public ByteBuf readSlice(int length) { + return newSharedLeakAwareByteBuf(super.readSlice(length)); + } + + @Override + public ByteBuf asReadOnly() { + return newSharedLeakAwareByteBuf(super.asReadOnly()); } @Override @@ -40,75 +98,83 @@ final class SimpleLeakAwareByteBuf extends WrappedByteBuf { } @Override - public boolean release() { - boolean deallocated = super.release(); - if (deallocated) { - leak.close(); + public final boolean release() { + if (super.release()) { + closeLeak(); + return true; } - return deallocated; + return false; } @Override - public boolean release(int decrement) { - boolean deallocated = super.release(decrement); - if (deallocated) { - leak.close(); + public final boolean release(int decrement) { + if (super.release(decrement)) { + closeLeak(); + return true; } - return deallocated; + return false; + } + + 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); + assert closed; } @Override public ByteBuf order(ByteOrder endianness) { - leak.record(); if (order() == endianness) { return this; } else { - return new SimpleLeakAwareByteBuf(super.order(endianness), leak); + return newSharedLeakAwareByteBuf(super.order(endianness)); } } - @Override - public ByteBuf slice() { - return new SimpleLeakAwareByteBuf(super.slice(), leak); + private ByteBuf unwrappedDerived(ByteBuf derived) { + // We only need to unwrap SwappedByteBuf implementations as these will be the only ones that may end up in + // the AbstractLeakAwareByteBuf implementations beside slices / duplicates and "real" buffers. + ByteBuf unwrappedDerived = unwrapSwapped(derived); + + if (unwrappedDerived instanceof AbstractPooledDerivedByteBuf) { + ResourceLeakTracker newLeak = AbstractByteBuf.leakDetector.track(derived); + if (newLeak == null) { + // No leak detection, just return the derived buffer. + return derived; + } + ByteBuf leakAwareBuf = newLeakAwareByteBuf(derived, newLeak); + + // Update the parent to point to this buffer. + ((AbstractPooledDerivedByteBuf) unwrappedDerived).parent(this); + return leakAwareBuf; + } + return newSharedLeakAwareByteBuf(derived); } - @Override - public ByteBuf retainedSlice() { - return new SimpleLeakAwareByteBuf(super.retainedSlice(), leak); + @SuppressWarnings("deprecation") + private static ByteBuf unwrapSwapped(ByteBuf buf) { + if (buf instanceof SwappedByteBuf) { + do { + buf = buf.unwrap(); + } while (buf instanceof SwappedByteBuf); + + return buf; + } + return buf; } - @Override - public ByteBuf slice(int index, int length) { - return new SimpleLeakAwareByteBuf(super.slice(index, length), leak); + private SimpleLeakAwareByteBuf newSharedLeakAwareByteBuf( + ByteBuf wrapped) { + return newLeakAwareByteBuf(wrapped, trackedByteBuf, leak); } - @Override - public ByteBuf retainedSlice(int index, int length) { - return new SimpleLeakAwareByteBuf(super.retainedSlice(index, length), leak); + private SimpleLeakAwareByteBuf newLeakAwareByteBuf( + ByteBuf wrapped, ResourceLeakTracker leakTracker) { + return newLeakAwareByteBuf(wrapped, wrapped, leakTracker); } - @Override - public ByteBuf duplicate() { - return new SimpleLeakAwareByteBuf(super.duplicate(), leak); - } - - @Override - public ByteBuf retainedDuplicate() { - return new SimpleLeakAwareByteBuf(super.retainedDuplicate(), leak); - } - - @Override - public ByteBuf readSlice(int length) { - return new SimpleLeakAwareByteBuf(super.readSlice(length), leak); - } - - @Override - public ByteBuf readRetainedSlice(int length) { - return new SimpleLeakAwareByteBuf(super.readRetainedSlice(length), leak); - } - - @Override - public ByteBuf asReadOnly() { - return new SimpleLeakAwareByteBuf(super.asReadOnly(), leak); + protected SimpleLeakAwareByteBuf newLeakAwareByteBuf( + ByteBuf buf, ByteBuf trackedByteBuf, ResourceLeakTracker leakTracker) { + return new SimpleLeakAwareByteBuf(buf, trackedByteBuf, leakTracker); } } diff --git a/buffer/src/main/java/io/netty/buffer/SimpleLeakAwareCompositeByteBuf.java b/buffer/src/main/java/io/netty/buffer/SimpleLeakAwareCompositeByteBuf.java index 6def3c9612..fda06fdf8a 100644 --- a/buffer/src/main/java/io/netty/buffer/SimpleLeakAwareCompositeByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/SimpleLeakAwareCompositeByteBuf.java @@ -16,89 +16,111 @@ package io.netty.buffer; -import io.netty.util.ResourceLeak; +import io.netty.util.ResourceLeakTracker; +import io.netty.util.internal.ObjectUtil; import java.nio.ByteOrder; -final class SimpleLeakAwareCompositeByteBuf extends WrappedCompositeByteBuf { +class SimpleLeakAwareCompositeByteBuf extends WrappedCompositeByteBuf { - private final ResourceLeak leak; + final ResourceLeakTracker leak; - SimpleLeakAwareCompositeByteBuf(CompositeByteBuf wrapped, ResourceLeak leak) { + SimpleLeakAwareCompositeByteBuf(CompositeByteBuf wrapped, ResourceLeakTracker leak) { super(wrapped); - this.leak = leak; + this.leak = ObjectUtil.checkNotNull(leak, "leak"); } @Override - public boolean release() { - boolean deallocated = super.release(); - if (deallocated) { - leak.close(); + public final boolean release() { + // Call unwrap() before just in case that super.release() will change the ByteBuf instance that is returned + // by unwrap(). + ByteBuf unwrapped = unwrap(); + if (super.release()) { + closeLeak(unwrapped); + return true; } - return deallocated; + return false; } @Override - public boolean release(int decrement) { - boolean deallocated = super.release(decrement); - if (deallocated) { - leak.close(); + public final boolean release(int decrement) { + // Call unwrap() before just in case that super.release() will change the ByteBuf instance that is returned + // by unwrap(). + ByteBuf unwrapped = unwrap(); + if (super.release(decrement)) { + closeLeak(unwrapped); + return true; } - return deallocated; + return false; + } + + 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); + assert closed; } @Override public ByteBuf order(ByteOrder endianness) { - leak.record(); if (order() == endianness) { return this; } else { - return new SimpleLeakAwareByteBuf(super.order(endianness), leak); + return newLeakAwareByteBuf(super.order(endianness)); } } @Override public ByteBuf slice() { - return new SimpleLeakAwareByteBuf(super.slice(), leak); + return newLeakAwareByteBuf(super.slice()); } @Override public ByteBuf retainedSlice() { - return new SimpleLeakAwareByteBuf(super.retainedSlice(), leak); + return newLeakAwareByteBuf(super.retainedSlice()); } @Override public ByteBuf slice(int index, int length) { - return new SimpleLeakAwareByteBuf(super.slice(index, length), leak); + return newLeakAwareByteBuf(super.slice(index, length)); } @Override public ByteBuf retainedSlice(int index, int length) { - return new SimpleLeakAwareByteBuf(super.retainedSlice(index, length), leak); + return newLeakAwareByteBuf(super.retainedSlice(index, length)); } @Override public ByteBuf duplicate() { - return new SimpleLeakAwareByteBuf(super.duplicate(), leak); + return newLeakAwareByteBuf(super.duplicate()); } @Override public ByteBuf retainedDuplicate() { - return new SimpleLeakAwareByteBuf(super.retainedDuplicate(), leak); + return newLeakAwareByteBuf(super.retainedDuplicate()); } @Override public ByteBuf readSlice(int length) { - return new SimpleLeakAwareByteBuf(super.readSlice(length), leak); + return newLeakAwareByteBuf(super.readSlice(length)); } @Override public ByteBuf readRetainedSlice(int length) { - return new SimpleLeakAwareByteBuf(super.readRetainedSlice(length), leak); + return newLeakAwareByteBuf(super.readRetainedSlice(length)); } @Override public ByteBuf asReadOnly() { - return new SimpleLeakAwareByteBuf(super.asReadOnly(), leak); + return newLeakAwareByteBuf(super.asReadOnly()); + } + + private SimpleLeakAwareByteBuf newLeakAwareByteBuf(ByteBuf wrapped) { + return newLeakAwareByteBuf(wrapped, unwrap(), leak); + } + + protected SimpleLeakAwareByteBuf newLeakAwareByteBuf( + ByteBuf wrapped, ByteBuf trackedByteBuf, ResourceLeakTracker leakTracker) { + return new SimpleLeakAwareByteBuf(wrapped, trackedByteBuf, leakTracker); } } diff --git a/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java b/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java index 3474422127..35613786b1 100644 --- a/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java +++ b/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java @@ -1762,18 +1762,37 @@ public abstract class AbstractByteBufTest { @Test public void testRetainedSliceIndex() throws Exception { - assertEqualsAndRelease(buffer, 0, buffer.retainedSlice(0, buffer.capacity()).readerIndex()); - assertEqualsAndRelease(buffer, 0, buffer.retainedSlice(0, buffer.capacity() - 1).readerIndex()); - assertEqualsAndRelease(buffer, 0, buffer.retainedSlice(1, buffer.capacity() - 1).readerIndex()); - assertEqualsAndRelease(buffer, 0, buffer.retainedSlice(1, buffer.capacity() - 2).readerIndex()); + ByteBuf retainedSlice = buffer.retainedSlice(0, buffer.capacity()); + assertEquals(0, retainedSlice.readerIndex()); + retainedSlice.release(); - assertEqualsAndRelease(buffer, buffer.capacity(), buffer.retainedSlice(0, buffer.capacity()).writerIndex()); - assertEqualsAndRelease(buffer, - buffer.capacity() - 1, buffer.retainedSlice(0, buffer.capacity() - 1).writerIndex()); - assertEqualsAndRelease(buffer, - buffer.capacity() - 1, buffer.retainedSlice(1, buffer.capacity() - 1).writerIndex()); - assertEqualsAndRelease(buffer, - buffer.capacity() - 2, buffer.retainedSlice(1, buffer.capacity() - 2).writerIndex()); + retainedSlice = buffer.retainedSlice(0, buffer.capacity() - 1); + assertEquals(0, retainedSlice.readerIndex()); + retainedSlice.release(); + + retainedSlice = buffer.retainedSlice(1, buffer.capacity() - 1); + assertEquals(0, retainedSlice.readerIndex()); + retainedSlice.release(); + + retainedSlice = buffer.retainedSlice(1, buffer.capacity() - 2); + assertEquals(0, retainedSlice.readerIndex()); + retainedSlice.release(); + + retainedSlice = buffer.retainedSlice(0, buffer.capacity()); + assertEquals(buffer.capacity(), retainedSlice.writerIndex()); + retainedSlice.release(); + + retainedSlice = buffer.retainedSlice(0, buffer.capacity() - 1); + assertEquals(buffer.capacity() - 1, retainedSlice.writerIndex()); + retainedSlice.release(); + + retainedSlice = buffer.retainedSlice(1, buffer.capacity() - 1); + assertEquals(buffer.capacity() - 1, retainedSlice.writerIndex()); + retainedSlice.release(); + + retainedSlice = buffer.retainedSlice(1, buffer.capacity() - 2); + assertEquals(buffer.capacity() - 2, retainedSlice.writerIndex()); + retainedSlice.release(); } @Test @@ -1832,9 +1851,14 @@ public abstract class AbstractByteBufTest { assertTrue(buffer.compareTo(wrappedBuffer(value, 0, 31).order(LITTLE_ENDIAN)) > 0); assertTrue(buffer.slice(0, 31).compareTo(wrappedBuffer(value)) < 0); assertTrue(buffer.slice(0, 31).compareTo(wrappedBuffer(value).order(LITTLE_ENDIAN)) < 0); - assertTrueAndRelease(buffer, buffer.retainedSlice(0, 31).compareTo(wrappedBuffer(value)) < 0); - assertTrueAndRelease(buffer, - buffer.retainedSlice(0, 31).compareTo(wrappedBuffer(value).order(LITTLE_ENDIAN)) < 0); + + ByteBuf retainedSlice = buffer.retainedSlice(0, 31); + assertTrue(retainedSlice.compareTo(wrappedBuffer(value)) < 0); + retainedSlice.release(); + + retainedSlice = buffer.retainedSlice(0, 31); + assertTrue(retainedSlice.compareTo(wrappedBuffer(value).order(LITTLE_ENDIAN)) < 0); + retainedSlice.release(); } @Test @@ -3820,22 +3844,6 @@ public abstract class AbstractByteBufTest { } } - private static void assertTrueAndRelease(ByteBuf buf, boolean actual) { - try { - assertTrue(actual); - } finally { - buf.release(); - } - } - - private static void assertEqualsAndRelease(ByteBuf buf, int expected, int actual) { - try { - assertEquals(expected, actual); - } finally { - buf.release(); - } - } - private void testRefCnt0(final boolean parameter) throws Exception { for (int i = 0; i < 10; i++) { final CountDownLatch latch = new CountDownLatch(1); diff --git a/buffer/src/test/java/io/netty/buffer/AdvancedLeakAwareByteBufTest.java b/buffer/src/test/java/io/netty/buffer/AdvancedLeakAwareByteBufTest.java index 9e1f73a2c1..bec7694609 100644 --- a/buffer/src/test/java/io/netty/buffer/AdvancedLeakAwareByteBufTest.java +++ b/buffer/src/test/java/io/netty/buffer/AdvancedLeakAwareByteBufTest.java @@ -24,6 +24,6 @@ public class AdvancedLeakAwareByteBufTest extends SimpleLeakAwareByteBufTest { @Override protected ByteBuf wrap(ByteBuf buffer) { - return new AdvancedLeakAwareByteBuf(buffer, NoopResourceLeak.INSTANCE); + return new AdvancedLeakAwareByteBuf(buffer, new NoopResourceLeakTracker()); } } diff --git a/buffer/src/test/java/io/netty/buffer/AdvancedLeakAwareCompositeByteBufTest.java b/buffer/src/test/java/io/netty/buffer/AdvancedLeakAwareCompositeByteBufTest.java index 5f89ea5905..045fd354c9 100644 --- a/buffer/src/test/java/io/netty/buffer/AdvancedLeakAwareCompositeByteBufTest.java +++ b/buffer/src/test/java/io/netty/buffer/AdvancedLeakAwareCompositeByteBufTest.java @@ -19,7 +19,7 @@ public class AdvancedLeakAwareCompositeByteBufTest extends SimpleLeakAwareCompos @Override protected WrappedCompositeByteBuf wrap(CompositeByteBuf buffer) { - return new AdvancedLeakAwareCompositeByteBuf(buffer, NoopResourceLeak.INSTANCE); + return new AdvancedLeakAwareCompositeByteBuf(buffer, new NoopResourceLeakTracker()); } @Override diff --git a/buffer/src/test/java/io/netty/buffer/NoopResourceLeak.java b/buffer/src/test/java/io/netty/buffer/NoopResourceLeakTracker.java similarity index 61% rename from buffer/src/test/java/io/netty/buffer/NoopResourceLeak.java rename to buffer/src/test/java/io/netty/buffer/NoopResourceLeakTracker.java index 417bf86b31..860f135dc4 100644 --- a/buffer/src/test/java/io/netty/buffer/NoopResourceLeak.java +++ b/buffer/src/test/java/io/netty/buffer/NoopResourceLeakTracker.java @@ -15,22 +15,27 @@ */ package io.netty.buffer; -import io.netty.util.ResourceLeak; +import io.netty.util.ResourceLeakTracker; -final class NoopResourceLeak implements ResourceLeak { +import java.util.concurrent.atomic.AtomicBoolean; - static final NoopResourceLeak INSTANCE = new NoopResourceLeak(); - private NoopResourceLeak() { } +final class NoopResourceLeakTracker extends AtomicBoolean implements ResourceLeakTracker { + + private static final long serialVersionUID = 7874092436796083851L; @Override - public void record() { } + public void record() { + // NOOP + } @Override - public void record(Object hint) { } + public void record(Object hint) { + // NOOP + } @Override - public boolean close() { - return false; + public boolean close(T trackedObject) { + return compareAndSet(false, true); } } diff --git a/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareByteBufTest.java b/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareByteBufTest.java index d598be29c4..bf0e326165 100644 --- a/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareByteBufTest.java +++ b/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareByteBufTest.java @@ -27,7 +27,7 @@ public class SimpleLeakAwareByteBufTest extends BigEndianHeapByteBufTest { } protected ByteBuf wrap(ByteBuf buffer) { - return new SimpleLeakAwareByteBuf(buffer, NoopResourceLeak.INSTANCE); + return new SimpleLeakAwareByteBuf(buffer, new NoopResourceLeakTracker()); } protected Class leakClass() { diff --git a/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareCompositeByteBufTest.java b/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareCompositeByteBufTest.java index 1029d26cb0..d483f9b7df 100644 --- a/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareCompositeByteBufTest.java +++ b/buffer/src/test/java/io/netty/buffer/SimpleLeakAwareCompositeByteBufTest.java @@ -24,7 +24,7 @@ public class SimpleLeakAwareCompositeByteBufTest extends WrappedCompositeByteBuf @Override protected WrappedCompositeByteBuf wrap(CompositeByteBuf buffer) { - return new SimpleLeakAwareCompositeByteBuf(buffer, NoopResourceLeak.INSTANCE); + return new SimpleLeakAwareCompositeByteBuf(buffer, new NoopResourceLeakTracker()); } protected Class leakClass() { diff --git a/codec-dns/src/main/java/io/netty/handler/codec/dns/AbstractDnsMessage.java b/codec-dns/src/main/java/io/netty/handler/codec/dns/AbstractDnsMessage.java index bdd537879e..5d0a3933e0 100644 --- a/codec-dns/src/main/java/io/netty/handler/codec/dns/AbstractDnsMessage.java +++ b/codec-dns/src/main/java/io/netty/handler/codec/dns/AbstractDnsMessage.java @@ -18,9 +18,9 @@ package io.netty.handler.codec.dns; import io.netty.util.AbstractReferenceCounted; import io.netty.util.ReferenceCountUtil; import io.netty.util.ReferenceCounted; -import io.netty.util.ResourceLeak; import io.netty.util.ResourceLeakDetector; import io.netty.util.ResourceLeakDetectorFactory; +import io.netty.util.ResourceLeakTracker; import io.netty.util.internal.StringUtil; import io.netty.util.internal.UnstableApi; @@ -41,7 +41,7 @@ public abstract class AbstractDnsMessage extends AbstractReferenceCounted implem private static final int SECTION_QUESTION = DnsSection.QUESTION.ordinal(); private static final int SECTION_COUNT = 4; - private final ResourceLeak leak = leakDetector.open(this); + private final ResourceLeakTracker leak = leakDetector.track(this); private short id; private DnsOpCode opCode; private boolean recursionDesired; @@ -379,9 +379,10 @@ public abstract class AbstractDnsMessage extends AbstractReferenceCounted implem protected void deallocate() { clear(); - final ResourceLeak leak = this.leak; + final ResourceLeakTracker leak = this.leak; if (leak != null) { - leak.close(); + boolean closed = leak.close(this); + assert closed; } } diff --git a/common/src/main/java/io/netty/util/HashedWheelTimer.java b/common/src/main/java/io/netty/util/HashedWheelTimer.java index 053d1d83c6..0909514df8 100644 --- a/common/src/main/java/io/netty/util/HashedWheelTimer.java +++ b/common/src/main/java/io/netty/util/HashedWheelTimer.java @@ -91,7 +91,7 @@ public class HashedWheelTimer implements Timer { WORKER_STATE_UPDATER = workerStateUpdater; } - private final ResourceLeak leak; + private final ResourceLeakTracker leak; private final Worker worker = new Worker(); private final Thread workerThread; @@ -270,7 +270,7 @@ public class HashedWheelTimer implements Timer { } workerThread = threadFactory.newThread(worker); - leak = leakDetection || !workerThread.isDaemon() ? leakDetector.open(this) : null; + leak = leakDetection || !workerThread.isDaemon() ? leakDetector.track(this) : null; this.maxPendingTimeouts = maxPendingTimeouts; } @@ -347,7 +347,8 @@ public class HashedWheelTimer implements Timer { WORKER_STATE_UPDATER.set(this, WORKER_STATE_SHUTDOWN); if (leak != null) { - leak.close(); + boolean closed = leak.close(this); + assert closed; } return Collections.emptySet(); @@ -368,7 +369,8 @@ public class HashedWheelTimer implements Timer { } if (leak != null) { - leak.close(); + boolean closed = leak.close(this); + assert closed; } return worker.unprocessedTimeouts(); } diff --git a/common/src/main/java/io/netty/util/ResourceLeak.java b/common/src/main/java/io/netty/util/ResourceLeak.java index e3b10bd128..6f6247079f 100644 --- a/common/src/main/java/io/netty/util/ResourceLeak.java +++ b/common/src/main/java/io/netty/util/ResourceLeak.java @@ -16,6 +16,10 @@ package io.netty.util; +/** + * @deprecated please use {@link ResourceLeakTracker} as it may lead to false-positives. + */ +@Deprecated public interface ResourceLeak { /** * Records the caller's current stack trace so that the {@link ResourceLeakDetector} can tell where the leaked diff --git a/common/src/main/java/io/netty/util/ResourceLeakDetector.java b/common/src/main/java/io/netty/util/ResourceLeakDetector.java index a4cd566553..5b040b3e46 100644 --- a/common/src/main/java/io/netty/util/ResourceLeakDetector.java +++ b/common/src/main/java/io/netty/util/ResourceLeakDetector.java @@ -202,8 +202,24 @@ public class ResourceLeakDetector { * related resource is deallocated. * * @return the {@link ResourceLeak} or {@code null} + * @deprecated use {@link #track(Object)} */ + @Deprecated public final ResourceLeak open(T obj) { + return track0(obj); + } + + /** + * Creates a new {@link ResourceLeakTracker} which is expected to be closed via + * {@link ResourceLeakTracker#close(Object)} when the related resource is deallocated. + * + * @return the {@link ResourceLeakTracker} or {@code null} + */ + public final ResourceLeakTracker track(T obj) { + return track0(obj); + } + + private DefaultResourceLeak track0(T obj) { Level level = ResourceLeakDetector.level; if (level == Level.DISABLED) { return null; @@ -300,9 +316,13 @@ public class ResourceLeakDetector { "so that only a few instances are created."); } - private final class DefaultResourceLeak extends PhantomReference implements ResourceLeak { + @SuppressWarnings("deprecation") + private final class DefaultResourceLeak extends PhantomReference implements ResourceLeakTracker, + ResourceLeak { private final String creationRecord; private final Deque lastRecords = new ArrayDeque(); + private final int trackedHash; + private int removedRecords; DefaultResourceLeak(Object referent) { @@ -310,13 +330,17 @@ public class ResourceLeakDetector { assert referent != null; + // 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); + Level level = getLevel(); if (level.ordinal() >= Level.ADVANCED.ordinal()) { creationRecord = newRecord(null, 3); } else { creationRecord = null; } - allLeaks.put(this, LeakEntry.INSTANCE); } @@ -353,6 +377,18 @@ public class ResourceLeakDetector { return allLeaks.remove(this, LeakEntry.INSTANCE); } + @Override + public boolean close(T trackedObject) { + // Ensure that the object that was tracked is the same as the one that was passed to close(...). + assert trackedHash == System.identityHashCode(trackedObject); + + // We need to actually do the null check of the trackedObject after we close the leak because 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 trackedObject anymore and so already enqueue it for + // collection before we actually get a chance to close the enclosing ResourceLeak. + return close() && trackedObject != null; + } + @Override public String toString() { if (creationRecord == null) { @@ -454,7 +490,8 @@ public class ResourceLeakDetector { static final LeakEntry INSTANCE = new LeakEntry(); private static final int HASH = System.identityHashCode(INSTANCE); - private LeakEntry() { } + private LeakEntry() { + } @Override public int hashCode() { diff --git a/common/src/main/java/io/netty/util/ResourceLeakTracker.java b/common/src/main/java/io/netty/util/ResourceLeakTracker.java new file mode 100644 index 0000000000..0fdb21f236 --- /dev/null +++ b/common/src/main/java/io/netty/util/ResourceLeakTracker.java @@ -0,0 +1,39 @@ +/* + * Copyright 2016 The Netty Project + * + * The Netty Project licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ +package io.netty.util; + +public interface ResourceLeakTracker { + + /** + * Records the caller's current stack trace so that the {@link ResourceLeakDetector} can tell where the leaked + * resource was accessed lastly. This method is a shortcut to {@link #record(Object) record(null)}. + */ + void record(); + + /** + * Records the caller's current stack trace and the specified additional arbitrary information + * so that the {@link ResourceLeakDetector} can tell where the leaked resource was accessed lastly. + */ + void record(Object hint); + + /** + * Close the leak so that {@link ResourceLeakTracker} does not warn about leaked resources. + * After this method is called a leak associated with this ResourceLeakTracker should not be reported. + * + * @return {@code true} if called first time, {@code false} if called already + */ + boolean close(T trackedObject); +} diff --git a/common/src/test/java/io/netty/util/ResourceLeakDetectorTest.java b/common/src/test/java/io/netty/util/ResourceLeakDetectorTest.java new file mode 100644 index 0000000000..d35bd19577 --- /dev/null +++ b/common/src/test/java/io/netty/util/ResourceLeakDetectorTest.java @@ -0,0 +1,172 @@ +/* + * Copyright 2016 The Netty Project + * + * The Netty Project licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ +package io.netty.util; + +import org.junit.Test; + +import java.util.ArrayDeque; +import java.util.Queue; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; + +public class ResourceLeakDetectorTest { + + @Test(timeout = 30000) + public void testConcurentUsage() throws Throwable { + final AtomicBoolean finished = new AtomicBoolean(); + final AtomicReference error = new AtomicReference(); + // With 50 threads issue #6087 is reproducible on every run. + Thread[] threads = new Thread[50]; + final CyclicBarrier barrier = new CyclicBarrier(threads.length); + for (int i = 0; i < threads.length; i++) { + Thread t = new Thread(new Runnable() { + Queue resources = new ArrayDeque(100); + + @Override + public void run() { + try { + barrier.await(); + + // Run 10000 times or until the test is marked as finished. + for (int b = 0; b < 1000 && !finished.get(); b++) { + + // Allocate 100 LeakAwareResource per run and close them after it. + for (int a = 0; a < 100; a++) { + DefaultResource resource = new DefaultResource(); + ResourceLeakTracker leak = DefaultResource.detector.track(resource); + LeakAwareResource leakAwareResource = new LeakAwareResource(resource, leak); + resources.add(leakAwareResource); + } + if (closeResources(true)) { + finished.set(true); + } + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } catch (Throwable e) { + error.compareAndSet(null, e); + } finally { + // Just close all resource now without assert it to eliminate more reports. + closeResources(false); + } + } + + private boolean closeResources(boolean checkClosed) { + for (;;) { + LeakAwareResource r = resources.poll(); + if (r == null) { + return false; + } + boolean closed = r.close(); + if (checkClosed && !closed) { + error.compareAndSet(null, + new AssertionError("ResourceLeak.close() returned 'false' but expected 'true'")); + return true; + } + } + } + }); + threads[i] = t; + t.start(); + } + + // Just wait until all threads are done. + for (Thread t: threads) { + t.join(); + } + + // Check if we had any leak reports in the ResourceLeakDetector itself + DefaultResource.detector.assertNoErrors(); + + assertNoErrors(error); + } + + // Mimic the way how we implement our classes that should help with leak detection + private static final class LeakAwareResource implements Resource { + private final Resource resource; + private final ResourceLeakTracker leak; + + LeakAwareResource(Resource resource, ResourceLeakTracker leak) { + this.resource = resource; + this.leak = leak; + } + + @Override + public boolean close() { + // Using ResourceLeakDetector.close(...) to prove this fixes the leak problem reported + // in https://github.com/netty/netty/issues/6034 . + // + // The following implementation would produce a leak: + // return leak.close(); + return leak.close(resource); + } + } + + private static final class DefaultResource implements Resource { + // Sample every allocation + static final TestResourceLeakDetector detector = new TestResourceLeakDetector( + Resource.class, 1, Integer.MAX_VALUE); + + @Override + public boolean close() { + return true; + } + } + + private interface Resource { + boolean close(); + } + + private static void assertNoErrors(AtomicReference ref) throws Throwable { + Throwable error = ref.get(); + if (error != null) { + throw error; + } + } + + private static final class TestResourceLeakDetector extends ResourceLeakDetector { + + private final AtomicReference error = new AtomicReference(); + + TestResourceLeakDetector(Class resourceType, int samplingInterval, long maxActive) { + super(resourceType, samplingInterval, maxActive); + } + + @Override + protected void reportTracedLeak(String resourceType, String records) { + reportError(new AssertionError("Leak reported for '" + resourceType + "':\n" + records)); + } + + @Override + protected void reportUntracedLeak(String resourceType) { + reportError(new AssertionError("Leak reported for '" + resourceType + '\'')); + } + + @Override + protected void reportInstancesLeak(String resourceType) { + reportError(new AssertionError("Leak reported for '" + resourceType + '\'')); + } + + private void reportError(AssertionError cause) { + error.compareAndSet(null, cause); + } + + void assertNoErrors() throws Throwable { + ResourceLeakDetectorTest.assertNoErrors(error); + } + } +} diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java index a25b0a247c..880918b7b8 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java @@ -19,9 +19,9 @@ import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufAllocator; import io.netty.util.AbstractReferenceCounted; import io.netty.util.ReferenceCounted; -import io.netty.util.ResourceLeak; import io.netty.util.ResourceLeakDetector; import io.netty.util.ResourceLeakDetectorFactory; +import io.netty.util.ResourceLeakTracker; import io.netty.util.internal.PlatformDependent; import io.netty.util.internal.StringUtil; import io.netty.util.internal.SystemPropertyUtil; @@ -108,7 +108,7 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen private final int mode; // Reference Counting - private final ResourceLeak leak; + private final ResourceLeakTracker leak; private final AbstractReferenceCounted refCnt = new AbstractReferenceCounted() { @Override public ReferenceCounted touch(Object hint) { @@ -123,7 +123,8 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen protected void deallocate() { destroy(); if (leak != null) { - leak.close(); + boolean closed = leak.close(ReferenceCountedOpenSslContext.this); + assert closed; } } }; @@ -217,7 +218,7 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen if (mode != SSL.SSL_MODE_SERVER && mode != SSL.SSL_MODE_CLIENT) { throw new IllegalArgumentException("mode most be either SSL.SSL_MODE_SERVER or SSL.SSL_MODE_CLIENT"); } - leak = leakDetection ? leakDetector.open(this) : null; + leak = leakDetection ? leakDetector.track(this) : null; this.mode = mode; this.clientAuth = isServer() ? checkNotNull(clientAuth, "clientAuth") : ClientAuth.NONE; diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java index fdae240202..142651429f 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java @@ -20,9 +20,9 @@ import io.netty.buffer.ByteBufAllocator; import io.netty.buffer.Unpooled; import io.netty.util.AbstractReferenceCounted; import io.netty.util.ReferenceCounted; -import io.netty.util.ResourceLeak; import io.netty.util.ResourceLeakDetector; import io.netty.util.ResourceLeakDetectorFactory; +import io.netty.util.ResourceLeakTracker; import io.netty.util.internal.EmptyArrays; import io.netty.util.internal.PlatformDependent; import io.netty.util.internal.StringUtil; @@ -208,7 +208,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc private volatile int destroyed; // Reference Counting - private final ResourceLeak leak; + private final ResourceLeakTracker leak; private final AbstractReferenceCounted refCnt = new AbstractReferenceCounted() { @Override public ReferenceCounted touch(Object hint) { @@ -223,7 +223,8 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc protected void deallocate() { shutdown(); if (leak != null) { - leak.close(); + boolean closed = leak.close(ReferenceCountedOpenSslEngine.this); + assert closed; } } }; @@ -269,7 +270,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc int peerPort, boolean leakDetection) { super(peerHost, peerPort); OpenSsl.ensureAvailability(); - leak = leakDetection ? leakDetector.open(this) : null; + leak = leakDetection ? leakDetector.track(this) : null; this.alloc = checkNotNull(alloc, "alloc"); apn = (OpenSslApplicationProtocolNegotiator) context.applicationProtocolNegotiator(); ssl = SSL.newSSL(context.ctx, !context.isClient());