Skip to content

Commit

Permalink
Deprecate and ignore ResourceLeakDetector's maxActive parameter
Browse files Browse the repository at this point in the history 
Motivation:
ResourceLeakDetector supports a parameter called maxActive. This parameter is used in attempt to limit the amount of objects which are being tracked for leaks at any given time, and generates an error log message if this limit is exceeded. This assumes that there is a relationship between leak sample rate and object lifetime for objects which are already being tracked. This relationship may appear to work in cases were there are a single leak record per object and those leak records live for the lifetime of the application but in general this relationship doesn't exist. The original motivation was to provide a limit for cases such as HashedWheelTimer to limit the number of instances which exist at any given time. This limit is not enforced in all circumstances in HashedWheelTimer (e.g. if the thread is a daemon) and can be implemented outside ResourceLeakDetector.

Modifications:
- Deprecate all methods which interact with maxActive in ResourceLeakDetectorFactory and ResourceLeakDetector
- Remove all logic related to maxActive in ResourceLeakDetector
- HashedWheelTimer implements its own logic to impose a limit and warn users if too many instances exists at any given time.

Result:
Fixes #6225.
  • Loading branch information
@Scottmitch
Scottmitch committed Feb 7, 2017
1 parent 42c0359 commit 10ef920
Show file tree
Hide file tree
Showing 3 changed files with 159 additions and 60 deletions.
80 changes: 58 additions & 22 deletions common/src/main/java/io/netty/util/HashedWheelTimer.java
View file
Expand Up @@ -29,9 +29,13 @@
import java.util.concurrent.RejectedExecutionException;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
import java.util.concurrent.atomic.AtomicLong;

import static io.netty.util.internal.StringUtil.simpleClassName;

/**
* A {@link Timer} optimized for approximated I/O timeout scheduling.
*
Expand Down Expand Up @@ -78,8 +82,11 @@ public class HashedWheelTimer implements Timer {
static final InternalLogger logger =
InternalLoggerFactory.getInstance(HashedWheelTimer.class);

private static final AtomicInteger INSTANCE_COUNTER = new AtomicInteger();
private static final AtomicBoolean WARNED_TOO_MANY_INSTANCES = new AtomicBoolean();
private static final int INSTANCE_COUNT_LIMIT = 64;
private static final ResourceLeakDetector<HashedWheelTimer> leakDetector = ResourceLeakDetectorFactory.instance()
.newResourceLeakDetector(HashedWheelTimer.class, 1, Runtime.getRuntime().availableProcessors() * 4L);
.newResourceLeakDetector(HashedWheelTimer.class, 1);

private static final AtomicIntegerFieldUpdater<HashedWheelTimer> WORKER_STATE_UPDATER =
AtomicIntegerFieldUpdater.newUpdater(HashedWheelTimer.class, "workerState");
Expand Down Expand Up @@ -266,6 +273,24 @@ public HashedWheelTimer(
leak = leakDetection || !workerThread.isDaemon() ? leakDetector.track(this) : null;

this.maxPendingTimeouts = maxPendingTimeouts;

if (INSTANCE_COUNTER.incrementAndGet() > INSTANCE_COUNT_LIMIT &&
WARNED_TOO_MANY_INSTANCES.compareAndSet(false, true)) {
reportTooManyInstances();
}
}

@Override
protected void finalize() throws Throwable {
try {
super.finalize();
} finally {
// This object is going to be GCed and it is assumed the ship has sailed to do a proper shutdown. If
// we have not yet shutdown then we want to make sure we decrement the active instance count.
if (WORKER_STATE_UPDATER.getAndSet(this, WORKER_STATE_SHUTDOWN) != WORKER_STATE_SHUTDOWN) {
INSTANCE_COUNTER.decrementAndGet();
}
}
}

private static HashedWheelBucket[] createWheel(int ticksPerWheel) {
Expand Down Expand Up @@ -337,33 +362,37 @@ public Set<Timeout> stop() {

if (!WORKER_STATE_UPDATER.compareAndSet(this, WORKER_STATE_STARTED, WORKER_STATE_SHUTDOWN)) {
// workerState can be 0 or 2 at this moment - let it always be 2.
WORKER_STATE_UPDATER.set(this, WORKER_STATE_SHUTDOWN);

if (leak != null) {
boolean closed = leak.close(this);
assert closed;
if (WORKER_STATE_UPDATER.getAndSet(this, WORKER_STATE_SHUTDOWN) != WORKER_STATE_SHUTDOWN) {
INSTANCE_COUNTER.decrementAndGet();
if (leak != null) {
boolean closed = leak.close(this);
assert closed;
}
}

return Collections.emptySet();
}

boolean interrupted = false;
while (workerThread.isAlive()) {
workerThread.interrupt();
try {
workerThread.join(100);
} catch (InterruptedException ignored) {
interrupted = true;
try {
boolean interrupted = false;
while (workerThread.isAlive()) {
workerThread.interrupt();
try {
workerThread.join(100);
} catch (InterruptedException ignored) {
interrupted = true;
}
}
}

if (interrupted) {
Thread.currentThread().interrupt();
}

if (leak != null) {
boolean closed = leak.close(this);
assert closed;
if (interrupted) {
Thread.currentThread().interrupt();
}
} finally {
INSTANCE_COUNTER.decrementAndGet();
if (leak != null) {
boolean closed = leak.close(this);
assert closed;
}
}
return worker.unprocessedTimeouts();
}
Expand Down Expand Up @@ -400,6 +429,13 @@ private boolean shouldLimitTimeouts() {
return maxPendingTimeouts > 0;
}

private static void reportTooManyInstances() {
String resourceType = simpleClassName(HashedWheelTimer.class);
logger.error("You are creating too many " + resourceType + " instances. " +
resourceType + " is a shared resource that must be reused across the JVM," +
"so that only a few instances are created.");
}

private final class Worker implements Runnable {
private final Set<Timeout> unprocessedTimeouts = new HashSet<Timeout>();

Expand Down Expand Up @@ -636,7 +672,7 @@ public String toString() {
long remaining = deadline - currentTime + timer.startTime;

StringBuilder buf = new StringBuilder(192)
.append(StringUtil.simpleClassName(this))
.append(simpleClassName(this))
.append('(')
.append("deadline: ");
if (remaining > 0) {
Expand Down
39 changes: 20 additions & 19 deletions common/src/main/java/io/netty/util/ResourceLeakDetector.java
View file
Expand Up @@ -161,8 +161,6 @@ public static Level getLevel() {

private final String resourceType;
private final int samplingInterval;
private final long maxActive;
private final AtomicBoolean loggedTooManyActive = new AtomicBoolean();

/**
* @deprecated use {@link ResourceLeakDetectorFactory#newResourceLeakDetector(Class, int, long)}.
Expand All @@ -181,30 +179,42 @@ public ResourceLeakDetector(String resourceType) {
}

/**
* @deprecated Use {@link ResourceLeakDetector#ResourceLeakDetector(Class, int)}.
* <p>
* This should not be used directly by users of {@link ResourceLeakDetector}.
* Please use {@link ResourceLeakDetectorFactory#newResourceLeakDetector(Class)}
* or {@link ResourceLeakDetectorFactory#newResourceLeakDetector(Class, int, long)}
*
* @param maxActive This is deprecated and will be ignored.
*/
@SuppressWarnings("deprecation")
@Deprecated
public ResourceLeakDetector(Class<?> resourceType, int samplingInterval, long maxActive) {
this(simpleClassName(resourceType), samplingInterval, maxActive);
this(resourceType, samplingInterval);
}

/**
* This should not be used directly by users of {@link ResourceLeakDetector}.
* Please use {@link ResourceLeakDetectorFactory#newResourceLeakDetector(Class)}
* or {@link ResourceLeakDetectorFactory#newResourceLeakDetector(Class, int, long)}
*/
@SuppressWarnings("deprecation")
public ResourceLeakDetector(Class<?> resourceType, int samplingInterval) {
this(simpleClassName(resourceType), samplingInterval, Long.MAX_VALUE);
}

/**
* @deprecated use {@link ResourceLeakDetectorFactory#newResourceLeakDetector(Class, int, long)}.
* <p>
* @param maxActive This is deprecated and will be ignored.
*/
@Deprecated
public ResourceLeakDetector(String resourceType, int samplingInterval, long maxActive) {
if (resourceType == null) {
throw new NullPointerException("resourceType");
}
if (maxActive <= 0) {
throw new IllegalArgumentException("maxActive: " + maxActive + " (expected: 1+)");
}

this.resourceType = resourceType;
this.samplingInterval = samplingInterval;
this.maxActive = maxActive;
}

/**
Expand Down Expand Up @@ -261,12 +271,6 @@ private void reportLeak(Level level) {
return;
}

// Report too many instances.
int samplingInterval = level == Level.PARANOID? 1 : this.samplingInterval;
if (allLeaks.size() * samplingInterval > maxActive && loggedTooManyActive.compareAndSet(false, true)) {
reportInstancesLeak(resourceType);
}

// Detect and report previous leaks.
for (;;) {
@SuppressWarnings("unchecked")
Expand Down Expand Up @@ -317,13 +321,10 @@ protected void reportUntracedLeak(String resourceType) {
}

/**
* This method is called when instance leaks are detected. It can be overridden for tracking how many times leaks
* have been detected.
* @deprecated This method will no longer be invoked by {@link ResourceLeakDetector}.
*/
@Deprecated
protected void reportInstancesLeak(String resourceType) {
logger.error("LEAK: You are creating too many " + resourceType + " instances. " +
resourceType + " is a shared resource that must be reused across the JVM," +
"so that only a few instances are created.");
}

@SuppressWarnings("deprecation")
Expand Down
100 changes: 81 additions & 19 deletions common/src/main/java/io/netty/util/ResourceLeakDetectorFactory.java
View file
Expand Up @@ -37,7 +37,7 @@ public abstract class ResourceLeakDetectorFactory {
/**
* Get the singleton instance of this factory class.
*
* @return - the current {@link ResourceLeakDetectorFactory}
* @return the current {@link ResourceLeakDetectorFactory}
*/
public static ResourceLeakDetectorFactory instance() {
return factoryInstance;
Expand All @@ -48,7 +48,7 @@ public static ResourceLeakDetectorFactory instance() {
* {@link ResourceLeakDetector} is called by all the callers of this factory. That is, before initializing a
* Netty Bootstrap.
*
* @param factory - the instance that will become the current {@link ResourceLeakDetectorFactory}'s singleton
* @param factory the instance that will become the current {@link ResourceLeakDetectorFactory}'s singleton
*/
public static void setResourceLeakDetectorFactory(ResourceLeakDetectorFactory factory) {
factoryInstance = ObjectUtil.checkNotNull(factory, "factory");
Expand All @@ -57,30 +57,47 @@ public static void setResourceLeakDetectorFactory(ResourceLeakDetectorFactory fa
/**
* Returns a new instance of a {@link ResourceLeakDetector} with the given resource class.
*
* @param resource - the resource class used to initialize the {@link ResourceLeakDetector}
* @param <T> - the type of the resource class
* @return - a new instance of {@link ResourceLeakDetector}
* @param resource the resource class used to initialize the {@link ResourceLeakDetector}
* @param <T> the type of the resource class
* @return a new instance of {@link ResourceLeakDetector}
*/
public final <T> ResourceLeakDetector<T> newResourceLeakDetector(Class<T> resource) {
return newResourceLeakDetector(resource, ResourceLeakDetector.DEFAULT_SAMPLING_INTERVAL, Long.MAX_VALUE);
return newResourceLeakDetector(resource, ResourceLeakDetector.DEFAULT_SAMPLING_INTERVAL);
}

/**
* @deprecated Use {@link #newResourceLeakDetector(Class, int)} instead.
* <p>
* Returns a new instance of a {@link ResourceLeakDetector} with the given resource class.
*
* @param resource - the resource class used to initialize the {@link ResourceLeakDetector}
* @param samplingInterval - the interval on which sampling takes place
* @param maxActive - the maximum active instances
* @param <T> - the type of the resource class
* @return - a new instance of {@link ResourceLeakDetector}
* @param resource the resource class used to initialize the {@link ResourceLeakDetector}
* @param samplingInterval the interval on which sampling takes place
* @param maxActive This is deprecated and will be ignored.
* @param <T> the type of the resource class
* @return a new instance of {@link ResourceLeakDetector}
*/
@Deprecated
public abstract <T> ResourceLeakDetector<T> newResourceLeakDetector(
Class<T> resource, int samplingInterval, long maxActive);

/**
* Returns a new instance of a {@link ResourceLeakDetector} with the given resource class.
*
* @param resource the resource class used to initialize the {@link ResourceLeakDetector}
* @param samplingInterval the interval on which sampling takes place
* @param <T> the type of the resource class
* @return a new instance of {@link ResourceLeakDetector}
*/
@SuppressWarnings("deprecation")
public <T> ResourceLeakDetector<T> newResourceLeakDetector(Class<T> resource, int samplingInterval) {
return newResourceLeakDetector(resource, ResourceLeakDetector.DEFAULT_SAMPLING_INTERVAL, Long.MAX_VALUE);
}

/**
* Default implementation that loads custom leak detector via system property
*/
private static final class DefaultResourceLeakDetectorFactory extends ResourceLeakDetectorFactory {
private final Constructor<?> obsoleteCustomClassConstructor;
private final Constructor<?> customClassConstructor;

DefaultResourceLeakDetectorFactory() {
Expand All @@ -96,10 +113,15 @@ public String run() {
logger.error("Could not access System property: io.netty.customResourceLeakDetector", cause);
customLeakDetector = null;
}
customClassConstructor = customLeakDetector == null ? null : customClassConstructor(customLeakDetector);
if (customLeakDetector == null) {
obsoleteCustomClassConstructor = customClassConstructor = null;
} else {
obsoleteCustomClassConstructor = obsoleteCustomClassConstructor(customLeakDetector);
customClassConstructor = customClassConstructor(customLeakDetector);
}
}

private static Constructor<?> customClassConstructor(String customLeakDetector) {
private static Constructor<?> obsoleteCustomClassConstructor(String customLeakDetector) {
try {
final Class<?> detectorClass = Class.forName(customLeakDetector, true,
PlatformDependent.getSystemClassLoader());
Expand All @@ -116,15 +138,56 @@ private static Constructor<?> customClassConstructor(String customLeakDetector)
return null;
}

private static Constructor<?> customClassConstructor(String customLeakDetector) {
try {
final Class<?> detectorClass = Class.forName(customLeakDetector, true,
PlatformDependent.getSystemClassLoader());

if (ResourceLeakDetector.class.isAssignableFrom(detectorClass)) {
return detectorClass.getConstructor(Class.class, int.class);
} else {
logger.error("Class {} does not inherit from ResourceLeakDetector.", customLeakDetector);
}
} catch (Throwable t) {
logger.error("Could not load custom resource leak detector class provided: {}",
customLeakDetector, t);
}
return null;
}

@SuppressWarnings("deprecation")
@Override
public <T> ResourceLeakDetector<T> newResourceLeakDetector(
Class<T> resource, int samplingInterval, long maxActive) {
if (customClassConstructor != null) {
public <T> ResourceLeakDetector<T> newResourceLeakDetector(Class<T> resource, int samplingInterval,
long maxActive) {
if (obsoleteCustomClassConstructor != null) {
try {
@SuppressWarnings("unchecked")
ResourceLeakDetector<T> leakDetector =
(ResourceLeakDetector<T>) customClassConstructor.newInstance(
(ResourceLeakDetector<T>) obsoleteCustomClassConstructor.newInstance(
resource, samplingInterval, maxActive);
logger.debug("Loaded custom ResourceLeakDetector: {}",
obsoleteCustomClassConstructor.getDeclaringClass().getName());
return leakDetector;
} catch (Throwable t) {
logger.error(
"Could not load custom resource leak detector provided: {} with the given resource: {}",
obsoleteCustomClassConstructor.getDeclaringClass().getName(), resource, t);
}
}

ResourceLeakDetector<T> resourceLeakDetector = new ResourceLeakDetector<T>(resource, samplingInterval,
maxActive);
logger.debug("Loaded default ResourceLeakDetector: {}", resourceLeakDetector);
return resourceLeakDetector;
}

@Override
public <T> ResourceLeakDetector<T> newResourceLeakDetector(Class<T> resource, int samplingInterval) {
if (customClassConstructor != null) {
try {
@SuppressWarnings("unchecked")
ResourceLeakDetector<T> leakDetector =
(ResourceLeakDetector<T>) customClassConstructor.newInstance(resource, samplingInterval);
logger.debug("Loaded custom ResourceLeakDetector: {}",
customClassConstructor.getDeclaringClass().getName());
return leakDetector;
Expand All @@ -135,8 +198,7 @@ public <T> ResourceLeakDetector<T> newResourceLeakDetector(
}
}

ResourceLeakDetector<T> resourceLeakDetector = new ResourceLeakDetector<T>(
resource, samplingInterval, maxActive);
ResourceLeakDetector<T> resourceLeakDetector = new ResourceLeakDetector<T>(resource, samplingInterval);
logger.debug("Loaded default ResourceLeakDetector: {}", resourceLeakDetector);
return resourceLeakDetector;
}
Expand Down

0 comments on commit 10ef920

Please sign in to comment.