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

Deprecate and ignore ResourceLeakDetector's maxActive parameter #6289

Closed
wants to merge 1 commit into from
Closed

Deprecate and ignore ResourceLeakDetector's maxActive parameter #6289

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
80 changes: 58 additions & 22 deletions common/src/main/java/io/netty/util/HashedWheelTimer.java
View file
Open in desktop
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 {
Copy link
Member

Choose a reason for hiding this comment

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

@Scottmitch finalizers make me 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

me too 😭

I'm open for alternatives.

Copy link

Choose a reason for hiding this comment

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

CRITICAL Do not override the Object.finalize() method. rule

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();
Copy link
Member

Choose a reason for hiding this comment

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

maybe add any assert that this can never be negative ?

Copy link
Member Author

Choose a reason for hiding this comment

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

it could actually go negative (e.g. overflow) as we don't strictly enforce the limit ... we just use it to warn for "too many instances". In practice I wouldn't expect this to happen but I'm not sure we should add an assert for it unless we strictly enforce it.

Copy link
Member

Choose a reason for hiding this comment

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

ah gotcha... good as it is

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
Open in desktop
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
Open in desktop
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) {
Copy link

Choose a reason for hiding this comment

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

MAJOR Remove this unused method parameter "samplingInterval". rule

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