Some Data Races

[ThomasKrieger]

Hi,

I want to use testng together with vmlens to test concurrent java programs. Running testng with the following empty testclass:

package com.anarsoft.agent.performance;
import org.testng.annotations.Test;


public class TestNG {

     @Test(threadPoolSize = 3, invocationCount = 10,  timeOut = 40000)
      public void test(){ }
 }

led to some data races which might lead to unexpected behavior:

1. Inside org.testng.internal.invokers.InvokedMethodListenerInvoker the unsynchronized map strategies is filled in one of the TestNG threads. I would suggest to fill the map in the main Thread before starting the testng Threads. The fields java/util/HashMap$Node.value@12096,
   java/util/HashMap$Node.value@12110, java/util/HashMap.table@12094, java/util/HashMap.table@12104 in the generated trace are all related to this issue.

2. The Field m_hasTests in the class org.testng.TestNG is written by all TestNG and by the main Thread and read by the main Thread.I would suggest to declare the field as volatile

3. The Field m_matchingInterface in the enum org.testng.internal.invokers.InvokedMethodListenerSubtype is written in one of the TestNG threads
   in the constructor and read by all TestNG threads. I would suggest to make it final.

4. The field m_numPassed in org.testng.reporters.JUnitXMLReporter is written and read without synchronization in the method:

   public void onTestSuccess(ITestResult tr) {
       m_allTests.add(tr); 
       m_numPassed++;
    }

By the way, the collection m_allTests is a synchronized Collection. I would suggest to synchronize the method onTestSuccess.

You can see the complete trace under: http://traces.vmlens.com/testng/dist/overview.html

Thank you very much
Thomas Krieger

[rschmitt]

Thanks for this great writeup. I'm convinced that there are tons of race conditions in TestNG; I've found one or two myself. In (1), it sounds like you're describing unsynchronized multi-threaded access to a java.util.HashMap, which is insanely dangerous (on some JDKs this can result in an infinite loop); that fix should probably be prioritized.

[cbeust]

Indeed, thanks for taking the time to write this up, @ThomasKrieger. Would you be willing to send a few pull requests to address those issues?