Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Immutable leak detection level #13459

Open
wants to merge 1 commit into
base: 4.1
Choose a base branch
from
Open

Immutable leak detection level #13459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 22 additions & 2 deletions buffer/src/main/java/io/netty/buffer/AbstractByteBufAllocator.java
View file
Open in desktop
Expand Up @@ -16,9 +16,11 @@

package io.netty.buffer;

import static io.netty.util.ResourceLeakDetector.Level.DISABLED;
import static io.netty.util.internal.ObjectUtil.checkPositiveOrZero;

import io.netty.util.ResourceLeakDetector;
import io.netty.util.ResourceLeakDetector.Level;
import io.netty.util.ResourceLeakTracker;
import io.netty.util.internal.MathUtil;
import io.netty.util.internal.PlatformDependent;
Expand All @@ -38,8 +40,17 @@ public abstract class AbstractByteBufAllocator implements ByteBufAllocator {
}

protected static ByteBuf toLeakAwareBuffer(ByteBuf buf) {
final Level level = ResourceLeakDetector.getLevel();
if (level == DISABLED) {
return buf;
}
return toEnabledLeakAwareBuffer(level, buf);
}

private static ByteBuf toEnabledLeakAwareBuffer(Level level, ByteBuf buf) {
assert level != DISABLED;
ResourceLeakTracker<ByteBuf> leak;
switch (ResourceLeakDetector.getLevel()) {
switch (level) {
case SIMPLE:
leak = AbstractByteBuf.leakDetector.track(buf);
if (leak != null) {
Expand All @@ -60,8 +71,17 @@ protected static ByteBuf toLeakAwareBuffer(ByteBuf buf) {
}

protected static CompositeByteBuf toLeakAwareBuffer(CompositeByteBuf buf) {
final Level level = ResourceLeakDetector.getLevel();
if (level == DISABLED) {
return buf;
}
return toEnabledLeakAwareBuffer(level, buf);
}

private static CompositeByteBuf toEnabledLeakAwareBuffer(Level level, CompositeByteBuf buf) {
assert level != DISABLED;
ResourceLeakTracker<ByteBuf> leak;
switch (ResourceLeakDetector.getLevel()) {
switch (level) {
case SIMPLE:
leak = AbstractByteBuf.leakDetector.track(buf);
if (leak != null) {
Expand Down
35 changes: 28 additions & 7 deletions common/src/main/java/io/netty/util/ResourceLeakDetector.java
View file
Open in desktop
Expand Up @@ -97,10 +97,12 @@ static Level parseLevel(String levelStr) {
}

private static Level level;
private static final Level stableLevel;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe call it immutableLevel?


private static final InternalLogger logger = InternalLoggerFactory.getInstance(ResourceLeakDetector.class);

static {
final boolean immutableLevel = SystemPropertyUtil.get("io.netty.immutableResourceLeakDetection") != null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest changing the property name and explicitly treating it as a boolean:

Suggested change
final boolean immutableLevel = SystemPropertyUtil.get("io.netty.immutableResourceLeakDetection") != null;
final boolean immutableLevel = Boolean.getBoolean("io.netty.leakDetection.level.immutable");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but use SystemPropertyUtil.getBoolean() instead of Boolean.getBoolean()

final boolean disabled;
if (SystemPropertyUtil.get("io.netty.noResourceLeakDetection") != null) {
disabled = SystemPropertyUtil.getBoolean("io.netty.noResourceLeakDetection", false);
Expand All @@ -124,7 +126,13 @@ static Level parseLevel(String levelStr) {
TARGET_RECORDS = SystemPropertyUtil.getInt(PROP_TARGET_RECORDS, DEFAULT_TARGET_RECORDS);
SAMPLING_INTERVAL = SystemPropertyUtil.getInt(PROP_SAMPLING_INTERVAL, DEFAULT_SAMPLING_INTERVAL);

ResourceLeakDetector.level = level;
if (immutableLevel) {
ResourceLeakDetector.level = null;
stableLevel = level;
} else {
ResourceLeakDetector.level = level;
stableLevel = null;
}
if (logger.isDebugEnabled()) {
logger.debug("-D{}: {}", PROP_LEVEL, level.name().toLowerCase());
logger.debug("-D{}: {}", PROP_TARGET_RECORDS, TARGET_RECORDS);
Expand All @@ -139,33 +147,43 @@ public static void setEnabled(boolean enabled) {
setLevel(enabled? Level.SIMPLE : Level.DISABLED);
}

private static boolean isImmutableDisabled() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be slightly better:

Suggested change
private static boolean isImmutableDisabled() {
private static boolean isImmutablyDisabled() {

return stableLevel == Level.DISABLED;
}

/**
* Returns {@code true} if resource leak detection is enabled.
*/
public static boolean isEnabled() {
return getLevel().ordinal() > Level.DISABLED.ordinal();
return getLevel() != Level.DISABLED;
franz1981 marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Sets the resource leak detection level.
*/
public static void setLevel(Level level) {
if (stableLevel != null) {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should better throw in this case.

Copy link
Contributor Author

@franz1981 franz1981 Jun 22, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a stackless Unsupported Ex?
I'm saying that because maybe users don't want to have some special paths in their code able to set the level at runtime to handle the case when the level is "immutable"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what is better... At least it wouldn't be surprising in a sense that while they think they use paranoid (for example) it not actually does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me neither, be silent doesn't seem right, I agree...a warn?

Copy link
Contributor Author

@franz1981 franz1981 Jun 22, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have noticed https://github.com/eclipse-vertx/vert.x/blob/6a8aefb6ec84278774d2c03fdae83fdf1deafded/src/main/java/io/vertx/core/impl/VertxImpl.java#L104: it means that if I won't expose something public to check if the immutable sys prop is there, with an exception, this one will crash the application while starting

or...

I just add a warn?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any suggestion @normanmaurer ? do you want me to add an exception for this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure ... @chrisvest @trustin thoughts ?

Copy link
Contributor

@zakkak zakkak Jan 23, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking out loud, what about:

  1. Throwing an unsupported exception if the new level is different than the immutable one.
  2. Always require io.netty.leakDetection.level to be set when setting io.netty.immutableResourceLeakDetection. This way we avoid the vert.x case.

}
ResourceLeakDetector.level = ObjectUtil.checkNotNull(level, "level");
}

/**
* Returns the current resource leak detection level.
*/
public static Level getLevel() {
if (stableLevel != null) {
return stableLevel;
}
return level;
}

/** the collection of active resources */
private final Set<DefaultResourceLeak<?>> allLeaks =
private final Set<DefaultResourceLeak<?>> allLeaks = isImmutableDisabled()? null :
Collections.newSetFromMap(new ConcurrentHashMap<DefaultResourceLeak<?>, Boolean>());

private final ReferenceQueue<Object> refQueue = new ReferenceQueue<Object>();
private final Set<String> reportedLeaks =
private final ReferenceQueue<Object> refQueue = isImmutableDisabled()? null : new ReferenceQueue<Object>();
private final Set<String> reportedLeaks = isImmutableDisabled()? null :
Comment on lines -164 to +186
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't these changes break trackForcibly?

Collections.newSetFromMap(new ConcurrentHashMap<String, Boolean>());

private final String resourceType;
Expand Down Expand Up @@ -261,7 +279,7 @@ public ResourceLeakTracker<T> trackForcibly(T obj) {

@SuppressWarnings("unchecked")
private DefaultResourceLeak track0(T obj, boolean force) {
Level level = ResourceLeakDetector.level;
Level level = getLevel();
if (force ||
level == Level.PARANOID ||
(level != Level.DISABLED && PlatformDependent.threadLocalRandom().nextInt(samplingInterval) == 0)) {
Expand Down Expand Up @@ -588,10 +606,13 @@ private String generateReport(TraceRecord oldHead) {
}
}

private static final AtomicReference<String[]> excludedMethods =
private static final AtomicReference<String[]> excludedMethods = isImmutableDisabled()? null :
new AtomicReference<String[]>(EmptyArrays.EMPTY_STRINGS);

public static void addExclusions(Class clz, String ... methodNames) {
if (isImmutableDisabled()) {
return;
}
Set<String> nameSet = new HashSet<String>(Arrays.asList(methodNames));
// Use loop rather than lookup. This avoids knowing the parameters, and doesn't have to handle
// NoSuchMethodException.
Expand Down
3 changes: 1 addition & 2 deletions transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java
View file
Open in desktop
Expand Up @@ -67,7 +67,6 @@ protected Map<Class<?>, String> initialValue() {
private final Channel channel;
private final ChannelFuture succeededFuture;
private final VoidChannelPromise voidPromise;
private final boolean touch = ResourceLeakDetector.isEnabled();

private Map<EventExecutorGroup, EventExecutor> childExecutors;
private volatile MessageSizeEstimator.Handle estimatorHandle;
Expand Down Expand Up @@ -113,7 +112,7 @@ final MessageSizeEstimator.Handle estimatorHandle() {
}

final Object touch(Object msg, AbstractChannelHandlerContext next) {
return touch ? ReferenceCountUtil.touch(msg, next) : msg;
return ResourceLeakDetector.isEnabled() ? ReferenceCountUtil.touch(msg, next) : msg;
franz1981 marked this conversation as resolved.
Show resolved Hide resolved
}

private AbstractChannelHandlerContext newContext(EventExecutorGroup group, String name, ChannelHandler handler) {
Expand Down