testng-team / testng

TestNG testing framework
https://testng.org
Apache License 2.0
1.99k stars 1.02k forks source link

Memory leak (TestNG seems to keep all test object in memory) #1461

Closed gluche closed 7 years ago

gluche commented 7 years ago

TestNG Version

6.9.10

Expected behavior

After completing a test, we expect test object to be garbage collected.

Actual behavior

Test objects are not finalized / garbage collected => which leads to an out of memory problem when running a large test suite

Is the issue reproductible on runner?

Test case sample

Please find below a standalone main class reproducing this problem. It shows that test objects are not garbage collected / finalized


package test;

import org.testng.TestNG;
import org.testng.annotations.Test;

/**
 * This class reproduces a memory leak problem when running TestNG tests.
 * The same (memory behavior will be shown when running this test as normal TestNG test (e.g. using Intellij)
 */
public class MemoryLeakTestNg {

    public static void main(String[] args) throws Exception {

        // we run the test programmatically
        runTest();
        // lets wait for garbage collection
        waitForAllObjectsDestructed();
    }

    public static void waitForAllObjectsDestructed() throws InterruptedException {
        while (true) {
            System.out.println("waiting for clean up...");
            // enforce a full gc
            System.gc();

            // check if there are still instances of our test class
            if (MyTestClassWithGlobalReferenceCounter.currentNumberOfMyTestObjects == 0) {
                // if we reach this point, all test instances are gone,
                // ... however this never happens ...
                break;
            }
            // let's wait 5 seconds and try again ...
            Thread.sleep(1000);
            System.out.println("["+MyTestClassWithGlobalReferenceCounter.currentNumberOfMyTestObjects+"] test object(s) still exist.");
        }
    }

    private static void runTest() {

        // create TestNG class
        TestNG testng = new TestNG() {
            @Override
            protected void finalize() throws Throwable {
                // it seems that this object will never be finalized !!!
                System.out.println("TestNG finalized");
            }
        };

        // and set a test (which also will never be finalized ... see later)
        testng.setTestClasses(new Class[]{
            MyTestClassWithGlobalReferenceCounter.class,
        });

        // lets run the test
        testng.run();

        // At this point the test run through and we expect both instances
        // - testng and
        // - the test object of type (MyTest)
        // will be garbage collected when leaving this method
        // ...

    }

    /** we create a test NG class here, which has a global counter, counting all instances. */
    public static class MyTestClassWithGlobalReferenceCounter {

        /** global counter that keeps track on how many objects are currently on the heap */
        public static int currentNumberOfMyTestObjects = 0;

        public MyTestClassWithGlobalReferenceCounter() {
            System.out.println("constructor");
            // increase the counter
            ++currentNumberOfMyTestObjects;
        }

        @Test
        public void aTestMethod1() {
            System.out.println("test method 1");
        }

        @Test
        public void aTestMethod2() {
            System.out.println("test method 2");
        }

        @Override
        protected void finalize() throws Throwable {
            System.out.println("finalize");
            // this will be called when this object is removed from the heap
            --currentNumberOfMyTestObjects;
        }
    }
}

juherr commented 7 years ago

Thanks for the report.

First, could you try with the latest version? Maybe it is already fixed.

But, by design, TestNG is keeping objects in memory for its reporters. An option could be to disable them.

krmahadevan commented 7 years ago

@gluche

This was a neat trick. I never knew one could do that :) Thanks for sharing this.

TestNG testng = new TestNG() {
    @Override
    protected void finalize() throws Throwable {
        // it seems that this object will never be finalized !!!
        System.out.println("TestNG finalized");
    }
};

I have a couple of observations. I believe the call to System.gc() doesn't necessarily always force a GC to happen but its more of like a Dear JVM please clean up un-used memory and that is the only time when finalize() is invoked. But in your case the greedy while loop is constantly querying the static data member in your MyTestClassWithGlobalReferenceCounter class and this class's finalize() method is also having a reference to the static counter.

So I guess your while loop is actually the one that is stopping the JVM from gc'ing MyTestClassWithGlobalReferenceCounter instance. I tried running your sample from the IDE and found it running continuously. So I thought I should give the benefit of doubt to the built in reporters from IntelliJ (Since @juherr mentioned that TestNG does hold reference to objects in reports) and run the test from the command prompt via maven (Here I guess the IntelliJ TestNG plugin has no role to play because the test class is being executed via a main() method rather than a @Test triggering it.)

When I ran the code using the command

mvn exec:java -Dexec.mainClass="com.rationaleemotions.github.issue1461.MemoryLeakTestNg"

I found the same behavior i.e., the test doesn't run to completion.

So I was wondering, is the code you shared that is causing the problem rather than TestNG blocking gc by holding up references to test class objects ?

@juherr

But, by design, TestNG is keeping objects in memory for its reporters. An option could be to disable them.

When the execution of TestNG.run() finishes and when the method runTest() exits TestNG has already done its part, I was under the assumption that at that time even the report execution should have completed. Is that a correct understanding ? So it looks like somewhere a reference to TestNG instance is still being maintained because I don't see the finalize() method of TestNG also be invoked. Do you think that the sample has any issues ?

kiru commented 7 years ago

@krmahadevan

So I guess your while loop is actually the one that is stopping the JVM from gc'ing MyTestClassWithGlobalReferenceCounter instance.

You are right, the MemoryLeakTestNg instance keeps querying the counter, but that should not keep the GC from getting rid of the MyTestClassWithGlobalReferenceCounter instance ?

I actually tried to run the same setup with the latest JUnit version ( just to be sure, that not this construct is causing the GC from removing our Test instance ). As expected the test exists with the following output:

constructor
test method 1
constructor
test method 2
waiting for clean up...
TestNG finalized
finalize
finalize
[0] test object(s) still exist.
waiting for clean up...

Here is source code (tested with Java 1.8.0_101-b13 and JUnit 4.12 ): https://gist.github.com/kiru/b87a1d42c720966f8fe0b9b99a4bedd6

@juherr

First, could you try with the latest version? Maybe it is already fixed.

Tested with the latest stable version ( 6.11 ), memory leak is still present.

juherr commented 7 years ago

@kiru What if you disable all reporters?

kiru commented 7 years ago

@juherr Disabling all reporters should not be the solution, but maybe a good workaround. I am not sure how to disable all the reporters, I tried with the following changes, but had no effect.

        testng.setUseDefaultListeners(false);
        testng.getReporters().clear();;
juherr commented 7 years ago
testng.setUseDefaultListeners(false);

should be enough.

And the idea was to find the root cause. If the problem still exists without listeners/reporters, then the problem is somewhere else.

kiru commented 7 years ago

Hello Julien,

I found the reason why in this case the TestNG instance is not GC'ed. It seems that TestNG is a singleton class. It keeps the latest created instance as a reference: private static TestNG m_instance; and later

  private void init(boolean useDefaultListeners) {
    m_instance = this;

    m_useDefaultListeners = useDefaultListeners;
    m_configuration = new Configuration();
  }

Currently, that instance is only used by ExitCodeListener. To fix the above test case, I changed the run- method as follows:

  public void run() {
    // ... skipped .. 
    m_instance = null;
    m_jCommander = null;
  }

Nevertheless, it does not fix the actual memory leak. You mentioned that by design the reporters keep an instance of the test instance. I don't see that with the JProfiler. What I see is, that the TestNG.m_configuration keeps a list of m_configurationListeners. One of the listeners seems to keep an instance to TestNGMethod.

Thank you, Kiru

ben-manes commented 7 years ago

The ITestResult retains the test parameters. I use an IInvokedMethodListener to replace the parameters with their toString representation. That lets my build run millions of tests with low memory usage.

kiru commented 7 years ago

@ben-manes I'm not quite familiar with the IInvokedMethodListener. Do you have any code samples?

ben-manes commented 7 years ago

Sure

jeffwcook commented 6 years ago

I'm seeing a similar issue.

We use an IInvokedMethodListener and after each invocation, we write the results to a DB and then technically we are done with the TestResult.

I'm seeing that org.testng.internal.MethodHelper is holding onto a bunch of records in a static field and eventually is causing an OOM. I bumped to 6.14.3 hoping the above fix would help, but it hasn't.

Is there some way of getting testng to let go of these results? We basically are running a suite of tests on demand and creating a new TestNG object each time. In theory, when the suite of tests is done we should be able to clear everything.

Note I do have an hprof if anyone is interested in it.

kiru commented 6 years ago

@jeffwcook The patch for this issue did not fix the memory leak. We ended up writing a method to set all the non static fields in the tests to null after the tests were done. That way our objects were freed, but TestNG was still keeping an instance to the Test.

jeffwcook commented 6 years ago

That's interesting. Do you have some sample code of that?

kiru commented 6 years ago

Most of the code is proprietary, but it looks similar to this:

FieldUtils is from https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/reflect/FieldUtils.html

void setAllFieldsToNull(Object object) throws IllegalAccessException {

    Field[] allFields = FieldUtils.getAllFields(getClass()); 
    for (Field eachField : allFields) {
        boolean notPrimitive = !eachField.getType().isPrimitive();
        boolean notStatic = !Modifier.isStatic(eachField.getModifiers())
        if(notPrimitive && notStatic){
            eachField.set(object, null);
        }
    }
}

The above snippet should be enhanced to add all the Fields from superclasses as well. Hope this gives you a starting point.

ben-manes commented 6 years ago

You might also want to look at the sample I gave earlier doing this in an IInvokedMethodListener.

Pr0methean commented 5 years ago

I'm seeing a similar issue where an object doesn't get cleared from a WeakHashMap by System.gc(), and a WeakReference or PhantomReference to it never gets queued. Replacing System.gc() with Thread.sleep(50) updates the WeakHashMap, but even System.gc(); Thread.sleep(1000); doesn't queue the reference in my own ReferenceQueue.

ben-manes commented 5 years ago

@Pr0methean try using Guava's GcFinalization which does a good job before failing on a timeout.

ben-manes commented 5 years ago

Also, don’t use G1 for reference caching tests. It uses regions which are collected independently, so it might not eagerly collect soft/weak references. Use ParallelGC for predictable tests if you rely on GC behavior.