Merge pull request #1580 from krmahadevan/krmahadevan-remove-synchronized-locks

Removed sync locks as and where possible.

Author: cbeust

src/main/java/org/testng/ClassMethodMap.java

@@ -1,12 +1,13 @@
 package org.testng;
 
-import org.testng.collections.Lists;
 import org.testng.collections.Maps;
 import org.testng.internal.XmlMethodSelector;
 
+import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.ConcurrentLinkedQueue;
 
 /**
  * This class maintains a map of {@code <Class, List<ITestNGMethod>>}.
@@ -17,7 +18,7 @@
  * @author <a href='mailto:the[dot]mindstorm[at]gmail[dot]com'>Alex Popescu</a>
  */
 public class ClassMethodMap {
-  private Map<Object, List<ITestNGMethod>> classMap = Maps.newHashMap();
+  private Map<Object, Collection<ITestNGMethod>> classMap = Maps.newConcurrentMap();
   // These two variables are used throughout the workers to keep track
   // of what beforeClass/afterClass methods have been invoked
   private Map<ITestClass, Set<Object>> beforeClassMethods = Maps.newHashMap();
@@ -33,9 +34,9 @@ public ClassMethodMap(List<ITestNGMethod> methods, XmlMethodSelector xmlMethodSe
       }
 
       Object instance = m.getInstance();
-      List<ITestNGMethod> l = classMap.get(instance);
+      Collection<ITestNGMethod> l = classMap.get(instance);
       if (l == null) {
-        l = Lists.newArrayList();
+        l = new ConcurrentLinkedQueue<>();
         classMap.put(instance, l);
       }
       l.add(m);
@@ -46,8 +47,8 @@ public ClassMethodMap(List<ITestNGMethod> methods, XmlMethodSelector xmlMethodSe
    * Remove the method from this map and returns true if it is the last
    * of its class.
    */
-  public synchronized boolean removeAndCheckIfLast(ITestNGMethod m, Object instance) {
-    List<ITestNGMethod> l = classMap.get(instance);
+  public boolean removeAndCheckIfLast(ITestNGMethod m, Object instance) {
+    Collection<ITestNGMethod> l = classMap.get(instance);
     if (l == null) {
       throw new AssertionError("l should not be null");
     }

src/main/java/org/testng/SuiteRunState.java

@@ -1,18 +1,20 @@
 package org.testng;
 
+import java.util.concurrent.atomic.AtomicBoolean;
+
 /**
  * A state object that records the status of the suite run. Mainly used to
  * figure out if there are any @BeforeSuite failures.
  */
 public class SuiteRunState {
 
-  private boolean m_hasFailures;
+  private final AtomicBoolean m_hasFailures = new AtomicBoolean();
 
-  public synchronized void failed() {
-    m_hasFailures= true;
+  public void failed() {
+    m_hasFailures.set(true);
   }
 
-  public synchronized boolean isFailed() {
-    return m_hasFailures;
+  public boolean isFailed() {
+    return m_hasFailures.get();
   }
 }

src/main/java/org/testng/SuiteRunner.java

@@ -29,6 +29,7 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentLinkedQueue;
 
 import com.google.inject.Injector;
 
@@ -69,7 +70,7 @@ public class SuiteRunner implements ISuite, IInvokedMethodListener {
   private Map<Class<? extends IInvokedMethodListener>, IInvokedMethodListener> invokedMethodListeners;
 
   /** The list of all the methods invoked during this run */
-  private List<IInvokedMethod> invokedMethods = Collections.synchronizedList(Lists.<IInvokedMethod>newArrayList());
+  private final Collection<IInvokedMethod> invokedMethods = new ConcurrentLinkedQueue<>();
 
   private List<ITestNGMethod> allTestMethods = Lists.newArrayList();
   private SuiteRunState suiteState = new SuiteRunState();
@@ -792,7 +793,7 @@ public void beforeInvocation(IInvokedMethod method, ITestResult testResult) {
 
   @Override
   public List<IInvokedMethod> getAllInvokedMethods() {
-    return invokedMethods;
+    return new ArrayList<>(invokedMethods);
   }
 
   @Override

src/main/java/org/testng/TestListenerAdapter.java

@@ -1,12 +1,12 @@
 package org.testng;
 
-import org.testng.collections.Lists;
 import org.testng.collections.Objects;
 import org.testng.internal.IResultListener2;
 
 import java.util.ArrayList;
-import java.util.Collections;
+import java.util.Collection;
 import java.util.List;
+import java.util.concurrent.ConcurrentLinkedQueue;
 
 
 /**
@@ -25,16 +25,15 @@
  * @author <a href='mailto:[email protected]'>Alexandru Popescu</a>
  */
 public class TestListenerAdapter implements IResultListener2 {
-  private List<ITestNGMethod> m_allTestMethods =
-      Collections.synchronizedList(Lists.<ITestNGMethod>newArrayList());
-  private List<ITestResult> m_passedTests = Collections.synchronizedList(Lists.<ITestResult>newArrayList());
-  private List<ITestResult> m_failedTests = Collections.synchronizedList(Lists.<ITestResult>newArrayList());
-  private List<ITestResult> m_skippedTests = Collections.synchronizedList(Lists.<ITestResult>newArrayList());
-  private List<ITestResult> m_failedButWSPerTests = Collections.synchronizedList(Lists.<ITestResult>newArrayList());
-  private List<ITestContext> m_testContexts= Collections.synchronizedList(new ArrayList<ITestContext>());
-  private List<ITestResult> m_failedConfs= Collections.synchronizedList(Lists.<ITestResult>newArrayList());
-  private List<ITestResult> m_skippedConfs= Collections.synchronizedList(Lists.<ITestResult>newArrayList());
-  private List<ITestResult> m_passedConfs= Collections.synchronizedList(Lists.<ITestResult>newArrayList());
+  private Collection<ITestNGMethod> m_allTestMethods = new ConcurrentLinkedQueue<>();
+  private Collection<ITestResult> m_passedTests = new ConcurrentLinkedQueue<>();
+  private Collection<ITestResult> m_failedTests = new ConcurrentLinkedQueue<>();
+  private Collection<ITestResult> m_skippedTests = new ConcurrentLinkedQueue<>();
+  private Collection<ITestResult> m_failedButWSPerTests = new ConcurrentLinkedQueue<>();
+  private Collection<ITestContext> m_testContexts = new ConcurrentLinkedQueue<>();
+  private Collection<ITestResult> m_failedConfs = new ConcurrentLinkedQueue<>();
+  private Collection<ITestResult> m_skippedConfs = new ConcurrentLinkedQueue<>();
+  private Collection<ITestResult> m_passedConfs = new ConcurrentLinkedQueue<>();
 
   @Override
   public void onTestSuccess(ITestResult tr) {
@@ -138,11 +137,11 @@ public void onTestStart(ITestResult result) {
   }
 
   public List<ITestContext> getTestContexts() {
-    return m_testContexts;
+    return new ArrayList<>(m_testContexts);
   }
 
   public List<ITestResult> getConfigurationFailures() {
-    return m_failedConfs;
+    return new ArrayList<>(m_failedConfs);
   }
 
   /**
@@ -154,7 +153,7 @@ public void onConfigurationFailure(ITestResult itr) {
   }
 
   public List<ITestResult> getConfigurationSkips() {
-    return m_skippedConfs;
+    return new ArrayList<>(m_skippedConfs);
   }
 
   @Override

src/main/java/org/testng/TestRunner.java

@@ -10,6 +10,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.LinkedBlockingQueue;
 import java.util.concurrent.TimeUnit;
 
@@ -1407,9 +1408,7 @@ public void addSkippedTest(ITestNGMethod tm, ITestResult tr) {
 
   @Override
   public void addInvokedMethod(InvokedMethod im) {
-    synchronized(m_invokedMethods) {
-      m_invokedMethods.add(im);
-    }
+    m_invokedMethods.add(im);
   }
 
   @Override
@@ -1540,7 +1539,7 @@ void addConfigurationListener(IConfigurationListener icl) {
   // Listeners
   /////
 
-  private final List<InvokedMethod> m_invokedMethods = Lists.newArrayList();
+  private final Collection<InvokedMethod> m_invokedMethods = new ConcurrentLinkedQueue<>();
 
   private void dumpInvokedMethods() {
     System.out.println("===== Invoked methods");
@@ -1561,12 +1560,10 @@ else if (im.isConfigurationMethod()) {
 
   public List<ITestNGMethod> getInvokedMethods() {
     List<ITestNGMethod> result= Lists.newArrayList();
-    synchronized(m_invokedMethods) {
-      for (IInvokedMethod im : m_invokedMethods) {
-        ITestNGMethod tm= im.getTestMethod();
-        tm.setDate(im.getDate());
-        result.add(tm);
-      }
+    for (IInvokedMethod im : m_invokedMethods) {
+      ITestNGMethod tm = im.getTestMethod();
+      tm.setDate(im.getDate());
+      result.add(tm);
     }
 
     return result;

src/main/java/org/testng/internal/BaseTestMethod.java

@@ -1,6 +1,7 @@
 package org.testng.internal;
 
 import java.lang.reflect.Method;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -9,6 +10,7 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.Callable;
+import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.regex.Pattern;
 
@@ -64,7 +66,7 @@ public abstract class BaseTestMethod implements ITestNGMethod {
   private long m_invocationTimeOut = 0L;
 
   private List<Integer> m_invocationNumbers = Lists.newArrayList();
-  private final List<Integer> m_failedInvocationNumbers = Collections.synchronizedList(Lists.<Integer>newArrayList());
+  private final Collection<Integer> m_failedInvocationNumbers = new ConcurrentLinkedQueue<>();
   private long m_timeOut = 0;
 
   private boolean m_ignoreMissingDependencies;
@@ -742,7 +744,7 @@ public void setInvocationNumbers(List<Integer> numbers) {
 
   @Override
   public List<Integer> getFailedInvocationNumbers() {
-    return m_failedInvocationNumbers;
+    return new ArrayList<>(m_failedInvocationNumbers);
   }
 
   @Override

src/main/java/org/testng/internal/ConfigurationGroupMethods.java

@@ -8,6 +8,7 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * This class wraps access to beforeGroups and afterGroups methods,
@@ -28,15 +29,15 @@ public class ConfigurationGroupMethods {
   private final ITestNGMethod[] m_allMethods;
 
   /**A map that returns the last method belonging to the given group */
-  private Map<String, List<ITestNGMethod>> m_afterGroupsMap= null;
+  private Map<String, List<ITestNGMethod>> m_afterGroupsMap = null;
 
   public ConfigurationGroupMethods(ITestNGMethod[] allMethods,
                                    Map<String, List<ITestNGMethod>> beforeGroupsMethods,
                                    Map<String, List<ITestNGMethod>> afterGroupsMethods)
   {
-    m_allMethods= allMethods;
-    m_beforeGroupsMethods= beforeGroupsMethods;
-    m_afterGroupsMethods= afterGroupsMethods;
+    m_allMethods = allMethods;
+    m_beforeGroupsMethods = new ConcurrentHashMap<>(beforeGroupsMethods);
+    m_afterGroupsMethods = new ConcurrentHashMap<>(afterGroupsMethods);
   }
 
   public ITestNGMethod[] getAllTestMethods() {
@@ -56,7 +57,7 @@ public Map<String, List<ITestNGMethod>> getAfterGroupsMethods() {
    * This method is used to figure out when is the right time to invoke
    * afterGroups methods.
    */
-  public synchronized boolean isLastMethodForGroup(String group, ITestNGMethod method) {
+  public boolean isLastMethodForGroup(String group, ITestNGMethod method) {
 
     // If we have more invocation to do, this is not the last one yet
     if(method.hasMoreInvocation()) {
@@ -65,7 +66,11 @@ public synchronized boolean isLastMethodForGroup(String group, ITestNGMethod met
 
     // Lazy initialization since we might never be called
     if(m_afterGroupsMap == null) {
-      m_afterGroupsMap= initializeAfterGroupsMap();
+      synchronized (this) {
+        if (m_afterGroupsMap == null) {
+          m_afterGroupsMap = initializeAfterGroupsMap();
+        }
+      }
     }
 
     List<ITestNGMethod> methodsInGroup= m_afterGroupsMap.get(group);
@@ -81,8 +86,8 @@ public synchronized boolean isLastMethodForGroup(String group, ITestNGMethod met
 
   }
 
-  private synchronized Map<String, List<ITestNGMethod>> initializeAfterGroupsMap() {
-    Map<String, List<ITestNGMethod>> result= Maps.newHashMap();
+  private Map<String, List<ITestNGMethod>> initializeAfterGroupsMap() {
+    Map<String, List<ITestNGMethod>> result= Maps.newConcurrentMap();
     for(ITestNGMethod m : m_allMethods) {
       String[] groups= m.getGroups();
       for(String g : groups) {
@@ -98,7 +103,7 @@ private synchronized Map<String, List<ITestNGMethod>> initializeAfterGroupsMap()
     return result;
   }
 
-  public synchronized void removeBeforeMethod(String group, ITestNGMethod method) {
+  public void removeBeforeMethod(String group, ITestNGMethod method) {
     List<ITestNGMethod> methods= m_beforeGroupsMethods.get(group);
     if(methods != null) {
       Object success= methods.remove(method);
@@ -115,22 +120,22 @@ private void log(String string) {
     Utils.log("ConfigurationGroupMethods", 2, string);
   }
 
-  synchronized public Map<String, List<ITestNGMethod>> getBeforeGroupsMap() {
+  public Map<String, List<ITestNGMethod>> getBeforeGroupsMap() {
     return m_beforeGroupsMethods;
   }
 
-  synchronized public Map<String, List<ITestNGMethod>> getAfterGroupsMap() {
+  public Map<String, List<ITestNGMethod>> getAfterGroupsMap() {
     return m_afterGroupsMethods;
   }
 
-  synchronized public void removeBeforeGroups(String[] groups) {
+  public void removeBeforeGroups(String[] groups) {
     for(String group : groups) {
 //      log("Removing before group " + group);
       m_beforeGroupsMethods.remove(group);
     }
   }
 
-  synchronized public void removeAfterGroups(Collection<String> groups) {
+  public void removeAfterGroups(Collection<String> groups) {
     for(String group : groups) {
 //      log("Removing before group " + group);
       m_afterGroupsMethods.remove(group);

src/main/java/org/testng/internal/TestMethodWorker.java

@@ -186,32 +186,31 @@ protected void invokeAfterClassMethods(ITestClass testClass, IMethodInstance mi)
     //
     List<Object> invokeInstances= Lists.newArrayList();
     ITestNGMethod tm= mi.getMethod();
-    if (m_classMethodMap.removeAndCheckIfLast(tm, mi.getInstance())) {
-      Map<ITestClass, Set<Object>> invokedAfterClassMethods
-          = m_classMethodMap.getInvokedAfterClassMethods();
-      synchronized(invokedAfterClassMethods) {
-        Set<Object> instances = invokedAfterClassMethods.get(testClass);
-        if(null == instances) {
-          instances= new HashSet<>();
-          invokedAfterClassMethods.put(testClass, instances);
-        }
-        Object inst = mi.getInstance();
-        if(!instances.contains(inst)) {
-          invokeInstances.add(inst);
-        }
-      }
+    boolean removalSuccessful = m_classMethodMap.removeAndCheckIfLast(tm, mi.getInstance());
+    if (!removalSuccessful) {
+      return;
+    }
+    Map<ITestClass, Set<Object>> invokedAfterClassMethods = m_classMethodMap.getInvokedAfterClassMethods();
+    Set<Object> instances = invokedAfterClassMethods.get(testClass);
+    if (null == instances) {
+      instances = new HashSet<>();
+      invokedAfterClassMethods.put(testClass, instances);
+    }
+    Object inst = mi.getInstance();
+    if (!instances.contains(inst)) {
+      invokeInstances.add(inst);
+    }
 
-      for (IClassListener listener : m_listeners) {
-        listener.onAfterClass(testClass);
-      }
-      for(Object inst: invokeInstances) {
-        m_invoker.invokeConfigurations(testClass,
-                                       testClass.getAfterClassMethods(),
-                                       m_suite,
-                                       m_parameters,
-                                       null, /* no parameter values */
-                                       inst);
-      }
+    for (IClassListener listener : m_listeners) {
+      listener.onAfterClass(testClass);
+    }
+    for (Object invokeInstance : invokeInstances) {
+      m_invoker.invokeConfigurations(testClass,
+              testClass.getAfterClassMethods(),
+              m_suite,
+              m_parameters,
+              null, /* no parameter values */
+              invokeInstance);
     }
   }