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

Add testing ByteBufAllocator for leak unit testing (almson version) #8431

Open
wants to merge 2 commits into
base: 4.1
from

Conversation

Projects
None yet
5 participants
@almson
Contributor

almson commented Oct 26, 2018

This is an alternate proposal for PR #8423 #8423 It makes fewer changes and is more general, in my opinion. Does not require subclassing the leak detector.

Motivation:

Currently the only way to detect leaks during testing is log scanning which is
difficult to automate. As an alternative it would be nice to fail unit tests
that leak buffers.

Modifications:

Added ResourceLeakDetector.assertAllResourcesDisposed which can be used in unit tests to ensure that all resources have been disposed. However, invoking assertAllResourcesDisposed directly can be problematic due to the fact that the leak detector is usually a static singleton and/or private in the classes that use it.

To that end, added TestingPooledByteBufAllocator which maintains a non-static leak detector and is useful for concurrent tests. It implements AutoCloseable (for use with try-with-resources) and invokes assertAllResourcesDisposed in the close method. To implement this, AbstractByteBufAllocator.toLeakAwareBuffer was made non-static to allow sub classes to use their own leak detector.

ResourceLeakDetector.clearRefQueue was removed because it created a danger that assertAllResourcesDisposed would not detect all leaks. This method is a useless optimization which saves a few cycles in the unlikely event that error logging is off, leaks are happening, and the leak detector is enabled.

Result:

Developers using Netty can now easily verify that all buffers have been released in unit tests.

Add testing ByteBufAllocator for leak unit testing (almson version)
Motivation:

Currently the only way to detect leaks during testing is log scanning which is
difficult to automate. As an alternative it would be nice to fail unit tests
that leak buffers.

Modifications:

Added ResourceLeakDetector.assertAllResourcesDisposed which can be used in unit tests to ensure that all resources have been disposed. However, invoking assertAllResourcesDisposed directly can be problematic due to the fact that the leak detector is usually a static singleton and/or private in the classes that use it.

To that end, added TestingPooledByteBufAllocator which maintains a non-static leak detector and is useful for concurrent tests. It implements AutoCloseable (for use with try-with-resources) and invokes assertAllResourcesDisposed in the close method. To implement this, AbstractByteBufAllocator.toLeakAwareBuffer was made non-static to allow sub classes to use their own leak detector.

ResourceLeakDetector.clearRefQueue was removed because it created a danger that assertAllResourcesDisposed would not detect all leaks. This method is a useless optimization which saves a few cycles in the unlikely event that error logging is off, leaks are happening, and the leak detector is enabled.

Result:

Developers using Netty can now easily verify that all buffers have been released in unit tests.
@netty-bot

This comment has been minimized.

netty-bot commented Oct 26, 2018

Can one of the admins verify this patch?

@dain

I'm pretty sure that is version will not throw an assertion error for any leak that is "reported" as part of the normal garbage collection cycle (via the WeakReference processing in reportLeak()). The prior version catches these by adding hooks for when a leak is detected and stores the records, so that when close is called, all leaks are reported.

@almson

This comment has been minimized.

Contributor

almson commented Oct 27, 2018

@dain It will assert them. All of those errors are stored in Set<String> reportedLeaks, even if logging is disabled. I've verified it works. The leak is both printed by the test logger and then again in the AssertionError.

@almson

This comment has been minimized.

Contributor

almson commented Oct 27, 2018

@dain I'm new to GitHub. I see it says "dain suggested changes View Changes" but I just see my own changes. Did you suggest a code change?

Screenshot-from-2018-10-26-20-32-32.png

@dain

This comment has been minimized.

dain commented Oct 29, 2018

@almson, I see it now. All reported leaks are already stored in ConcurrentMap<String, Boolean> reportedLeaks. I'll read this again tomorrow.

BTW, I didn't make any suggested changes, and I'm not sure why Github said that. That is a brand new feature, so maybe it was a bug.

@normanmaurer

Generally LGTM... Just a few suggestions. Thanks!

@@ -34,7 +34,7 @@
ResourceLeakDetector.addExclusions(AbstractByteBufAllocator.class, "toLeakAwareBuffer");
}
protected static ByteBuf toLeakAwareBuffer(ByteBuf buf) {
protected ByteBuf toLeakAwareBuffer(ByteBuf buf) {

This comment has been minimized.

@normanmaurer

normanmaurer Oct 31, 2018

Member

This is a breaking change so you will need to preserve the old method signature as well as people may depend on it to be static atm when sub-classing AbstractByteBufAllocator

@@ -56,7 +56,7 @@ protected static ByteBuf toLeakAwareBuffer(ByteBuf buf) {
return buf;
}
protected static CompositeByteBuf toLeakAwareBuffer(CompositeByteBuf buf) {
protected CompositeByteBuf toLeakAwareBuffer(CompositeByteBuf buf) {

This comment has been minimized.

@normanmaurer

normanmaurer Oct 31, 2018

Member

same comment as above

* and performs leak tracking on all allocated buffers. At the end the test, call {@link #close} to assert
* that all buffers have been freed.
*/
public class TestingPooledByteBufAllocator extends PooledByteBufAllocator implements Closeable {

This comment has been minimized.

@normanmaurer

normanmaurer Oct 31, 2018

Member

As this is mainly used for testing I think we should better extend UnpooledByteBufAllocator.

This comment has been minimized.

@normanmaurer

normanmaurer Oct 31, 2018

Member

Also maybe we should call it AssertingByteBufAllocator.

This comment has been minimized.

@dain

dain Nov 1, 2018

@normanmaurer, what are the implications of extending UnpooledByteBufAllocator? I ask because I always use pooled in my tests because that is what I use in production. IIRC, the unpooled uses different methods for the leak callbacks.

import static org.hamcrest.CoreMatchers.containsString;
import static org.junit.Assert.assertThat;
public class TestingPooledByteBufAllocatorTest {

This comment has been minimized.

@normanmaurer

normanmaurer Oct 31, 2018

Member

when rename the class also rename the test please.

}
private void reportLeak() {
if (!logger.isErrorEnabled()) {
clearRefQueue();

This comment has been minimized.

@normanmaurer

normanmaurer Oct 31, 2018

Member

@almson shouldn't we presume this ?

}
}
}
if (!reportedLeaks.keySet().isEmpty()) {

This comment has been minimized.

@normanmaurer

normanmaurer Oct 31, 2018

Member

Store the Set before calling isEmpty() a you also access it in the for loop.

import java.io.Closeable;
/**
* A {@link ByteBufAllocator} for unit testing. This allocator maintains a private, non-static leak detector

This comment has been minimized.

@normanmaurer

normanmaurer Oct 31, 2018

Member

maybe reword to:

A  {@link ByteBufAllocator} that can enforce an `AssertionError` when any leaks are detected.
* @throws AssertionError if buffers have been leaked.
*/
@Override
public void close() {

This comment has been minimized.

@normanmaurer

normanmaurer Oct 31, 2018

Member

I wonder if we should better not implement Closeable but just expose a method like assertNoLeaks() as technical speaking even after close is called the allocator is still usable and so not really "closed".

This comment has been minimized.

@dain

dain Nov 1, 2018

In my original proposal, I went with Closeable because it makes using this in tests very easy because you can use try with resources syntax. So it is a tradeoff of convenient test code verses explicit naming. I went with the former because the code gets pretty deeply nested due to the other IO related bits that need try with resources.

}
/**
* Throws an exception if all allocated buffers have been not freed.

This comment has been minimized.

@normanmaurer

normanmaurer Oct 31, 2018

Member

Throws an {@link AssertionError} if all allocated {@link ByteBuf}s ...

*/
public class TestingPooledByteBufAllocator extends PooledByteBufAllocator implements Closeable {
private final ResourceLeakDetector leakDetector = new ResourceLeakDetector(AbstractByteBuf.class, 1);

This comment has been minimized.

@normanmaurer

normanmaurer Oct 31, 2018

Member

Do not hardcode 1 but add a constructor to the allocator that allows to specify the sampling rate.

@normanmaurer

This comment has been minimized.

Member

normanmaurer commented Oct 31, 2018

@netty-bot test this please

@normanmaurer

This comment has been minimized.

Member

normanmaurer commented Oct 31, 2018

@almson also there is a checkstyle error.

@dain

@normanmaurer this is an alternate design to #8423. I believe the main differences, is this one has more public API changes, so this one does not require a sublcass of ResourceLeakDetector . Can you give direction on which design you prefer and then @almson and I can focus on one of the two?

* and performs leak tracking on all allocated buffers. At the end the test, call {@link #close} to assert
* that all buffers have been freed.
*/
public class TestingPooledByteBufAllocator extends PooledByteBufAllocator implements Closeable {

This comment has been minimized.

@dain

dain Nov 1, 2018

@normanmaurer, what are the implications of extending UnpooledByteBufAllocator? I ask because I always use pooled in my tests because that is what I use in production. IIRC, the unpooled uses different methods for the leak callbacks.

* @throws AssertionError if buffers have been leaked.
*/
@Override
public void close() {

This comment has been minimized.

@dain

dain Nov 1, 2018

In my original proposal, I went with Closeable because it makes using this in tests very easy because you can use try with resources syntax. So it is a tradeoff of convenient test code verses explicit naming. I went with the former because the code gets pretty deeply nested due to the other IO related bits that need try with resources.

@dain

@normanmaurer, actually, after further review, I believe this PR also requires a subclass of ResourceLeakDetector to override the shouldTrack method. @almson, I believe the main difference is the addition of the single method ResourceLeakDectector.assertAllResourcesDisposed() to dispose all references and assert no leaks, whereas the other PR #8423 adds a method to "disposes" all references, and assert leaks in a separate "subclass" method.

I don't have a strong preference for either approach.

@almson, I set "request changes" for the comment about overriding shouldTrack.

// While we might try to set the level manually, or try other hacks,
// this check maintains generality should ResourceLeakDetector change in the future.
// Don't run your tests with leak detection disabled.
if (!ResourceLeakDetector.isEnabled()) {

This comment has been minimized.

@dain

dain Nov 1, 2018

I believe you need to override the shouldTrack method instead of checking the level here. For this class to work perfectly, the level would be required to be PARANOID, and since level is a global static, that requirement would effect every test in the system.

@trustin

This comment has been minimized.

Member

trustin commented Nov 5, 2018

What do you think about building a plugin for testing framework or a JVM agent to make this functionality available without making any user code change? A user has to add some code to take advantage of this, but it's not always doable especially for integration tests that involve a lot of components. A user would find it is tedious to add try-with-resources or assertion for all test cases.

@trustin

This comment has been minimized.

Member

trustin commented Nov 5, 2018

For example, we could provide a new artifact 'netty-testing-junit4' and provide a RunListener implementation and document how to plug it into a build tool.

@normanmaurer

This comment has been minimized.

Member

normanmaurer commented Nov 5, 2018

@trustin this sounds like an awesome idea actually

@trustin

This comment has been minimized.

Member

trustin commented Nov 5, 2018

I agree though that this still may be useful if a user needs to find a leak in a very specific scope, but I guess the problem we are trying to solve here in general is to fail the build reliably when there is a leak regardless of where it really is? Given the nature of how leak detector works, the point of leak detection may be much later than when a buffer actually leaked.

@dain

This comment has been minimized.

dain commented Nov 5, 2018

I agree that that might be interesting to those trying to catch general leaks. In my case, I am interested in failing the specific test that created the leak, and I run tests in parallel, so a global tracker would not work for me.

@normanmaurer or @trustin, can we get a call on which direction you want to go for this feature? As I mentioned above, I believe the main difference is the addition of the new method ResourceLeakDectector.assertAllResourcesDisposed() to dispose all references and assert no leaks, whereas the other PR #8423 adds a method to "disposes" all references, and assert leaks in a separate "subclass" method. If you want to go with my PR (#8423), I can may any changes you need quickly, but I'm more interested in getting this feature added, than my specific proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment