[Test] Release unpooled buffer after usage to avoid leakage

[andsel]

Release notes

[rn:skip]

What does this PR do?

Fix unpooled buffer leakage in tests.
Resolves leakage of Netty pooled buffers like in https://github.com/moquette-io/moquette/pull/872/checks#step:5:285

[andsel]

When broker instance is shutdown, also all the reference counted objects kept live by inflight or qos2Receiving (phase 2 of QoS2) need to be un-referenced, so that others Netty libraries doesn't expose buffer leakage when just the broker part is stopped.

[andsel]

Hi @hylkevds I ask if you could review this PR, you have previous experience with buffer leakage :-)

[hylkevds]

Hi @hylkevds I ask if you could review this PR, you have previous experience with buffer leakage :-)

Sure, I'll go over it.

[hylkevds]

Man, finding buffer leaks in tests is annoying! At the time the leak warning pops up, there is no relation to the test that actually caused the leak!

So I made a modified netty ResourceLeakDetector.java

Then a search/replace on all tests. Multi-line regex Search (including final line ending):

    @Test
    public void ([a-zA-Z0-9_-]+)\(\)([^\n]+)

replace:

    @Test
    public void $1()$2
        System.out.println("$1");
        io.netty.util.ResourceLeakDetector.statusHint = "$1";

And netty will tell the test that caused the resource leak:

Created at: receiveInflightPublishesAfterAReconnect

Now to find the actual cause :)

[andsel]

I've done manually 😄 by dissection, finding the minimal 2 or 3 tests that made the problem to trigger.
Your suggestion is really nice! We can add that in a follow up PR. Just one question, how do you replace the default Netty's ResourceLeakDetector ? Do you create a customized Netty jar?

[hylkevds]

Yep, I patched Netty and re-build the netty-common jar.

git clone [email protected]:netty/netty.git#
cd netty/common
git checkout netty-4.1.100.Final
curl https://gist.githubusercontent.com/hylkevds/44b238676ee727d20ed451b208c7ff2a/raw/c40464507f6e42bc02fd3ea12b129b93f9273dae/ResourceLeakDetector.java > src/main/java/io/netty/util/ResourceLeakDetector.java 
mvn install -DskipTests=true

Maven then automatically picks up the patched version.

The problem I had with running individual tests is that the garbage collection was never triggered, so the leak-detection also never triggered...

[andsel]

Maybe it worthwhile to propose on the upstream Netty project, that hint would help also others, I think.

[hylkevds]

I could not imagine Netty didn't already have something similar. The ResourceLeakDetector has something called initialHint that seemed to do something similar... It was just some digging to find out how to set that!

Add this to Utils:

    private static boolean resourceLeakDetectorFactoryOverridden = false;

    public static void setResourceLeakHint(String hint) {
        if (!resourceLeakDetectorFactoryOverridden) {
            ResourceLeakDetectorFactory.setResourceLeakDetectorFactory(new HintingResourceLeakDetectorFactory());
            resourceLeakDetectorFactoryOverridden = true;
        }
        HintingResourceLeakDetector.initialHint = hint;
    }

    public static class HintingResourceLeakDetector<T> extends ResourceLeakDetector<T> {

        public static String initialHint = "";

        public HintingResourceLeakDetector(Class<T> resource, int samplingInterval, long maxActive) {
            super(resource, samplingInterval, maxActive);
        }

        @Override
        protected Object getInitialHint(String resourceType) {
            return initialHint;
        }
    }

    public static class HintingResourceLeakDetectorFactory extends ResourceLeakDetectorFactory {

        @Override
        public <T> ResourceLeakDetector<T> newResourceLeakDetector(Class<T> resource, int samplingInterval, long maxActive) {
            return new HintingResourceLeakDetector(resource, samplingInterval, maxActive);
        }
    }

And then in each @test (using the search & replace):

io.moquette.Utils.setResourceLeakHint("sendConnectOnDisconnectedConnection");

And no need to patch Netty anymore!

[andsel]

@hylkevds this is a good hint, it will deserve a follow up PR, if you want to contribute I'll be happy t review. Just a note, instead of using System.out.println maybe worthwhile to use a global test logger, so that we can switch on and off from log4j.properties under test folder.

[hylkevds]

Changes look good. There seems to be just one leak left to find.