-
-
Notifications
You must be signed in to change notification settings - Fork 15.9k
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
Deprecate and ignore ResourceLeakDetector's maxActive parameter #6289
Conversation
@jroper - FYI |
} | ||
|
||
@Override | ||
protected void finalize() throws Throwable { |
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.
@Scottmitch finalizers make me 😢
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.
me too 😭
I'm open for alternatives.
Thread.currentThread().interrupt(); | ||
} | ||
} finally { | ||
INSTANCE_COUNTER.decrementAndGet(); |
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 add any assert that this can never be negative ?
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.
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.
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.
ah gotcha... good as it is
* @param <T> the type of the resource class | ||
* @return a new instance of {@link ResourceLeakDetector} | ||
*/ | ||
public abstract <T> ResourceLeakDetector<T> newResourceLeakDetector(Class<T> resource, int samplingInterval); |
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.
@Scottmitch this is a breaking change. We can not add a new abstract method as the class is public.
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.
true dat ... let me fix this.
df2120b
to
185388a
Compare
@normanmaurer - PTAL |
} | ||
|
||
@Override | ||
protected void finalize() throws Throwable { |
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.
* @return a new instance of {@link ResourceLeakDetector} | ||
*/ | ||
@SuppressWarnings("deprecation") | ||
public <T> ResourceLeakDetector<T> newResourceLeakDetector(Class<T> resource, int samplingInterval) { |
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.
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 netty#6225.
185388a
to
10ef920
Compare
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:
Result:
Fixes #6225.