Skip to content

Commit

Permalink
Fix false-positives when using ResourceLeakDetector.
Browse files Browse the repository at this point in the history
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 committed Dec 4, 2016
1 parent 7ce0f35 commit c2f4daa
Showing 20 changed files with 549 additions and 227 deletions.
Original file line number Diff line number Diff line change
@@ -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<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);
}
Original file line number Diff line number Diff line change
@@ -43,6 +43,12 @@ abstract class AbstractPooledDerivedByteBuf extends AbstractReferenceCountedByte
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;
73 changes: 27 additions & 46 deletions buffer/src/main/java/io/netty/buffer/AdvancedLeakAwareByteBuf.java
Original file line number Diff line number Diff line change
@@ -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 @@ final class AdvancedLeakAwareByteBuf extends WrappedByteBuf {
}
}

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);
}
}
Original file line number Diff line number Diff line change
@@ -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);
}
}
Loading

0 comments on commit c2f4daa

Please sign in to comment.