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.
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
Immutable leak detection level #13459
base: 4.1
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -97,10 +97,12 @@ static Level parseLevel(String levelStr) { | |||||
} | ||||||
|
||||||
private static Level level; | ||||||
private static final Level stableLevel; | ||||||
|
||||||
private static final InternalLogger logger = InternalLoggerFactory.getInstance(ResourceLeakDetector.class); | ||||||
|
||||||
static { | ||||||
final boolean immutableLevel = SystemPropertyUtil.get("io.netty.immutableResourceLeakDetection") != null; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but use |
||||||
final boolean disabled; | ||||||
if (SystemPropertyUtil.get("io.netty.noResourceLeakDetection") != null) { | ||||||
disabled = SystemPropertyUtil.getBoolean("io.netty.noResourceLeakDetection", false); | ||||||
|
@@ -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); | ||||||
|
@@ -139,33 +147,43 @@ public static void setEnabled(boolean enabled) { | |||||
setLevel(enabled? Level.SIMPLE : Level.DISABLED); | ||||||
} | ||||||
|
||||||
private static boolean isImmutableDisabled() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be slightly better:
Suggested change
|
||||||
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; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should better throw in this case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a stackless Unsupported Ex? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Me neither, be silent doesn't seem right, I agree...a warn? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure ... @chrisvest @trustin thoughts ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking out loud, what about:
|
||||||
} | ||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't these changes break |
||||||
Collections.newSetFromMap(new ConcurrentHashMap<String, Boolean>()); | ||||||
|
||||||
private final String resourceType; | ||||||
|
@@ -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)) { | ||||||
|
@@ -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. | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call it
immutableLevel
?