Use Locks instead of synchronised keyword

CHANGES.txt

@@ -1,4 +1,5 @@
 Current (7.10.0)
+Fixed: GITHUB:3040: replace the usages of synchronized with ReentrantLock (Krishnan Mahadevan)
 Fixed: GITHUB-3041: TestNG 7.x DataProvider works in opposite to TestNG 6.x when retrying tests. (Krishnan Mahadevan)
 Fixed: GITHUB-3066: How to dynamically adjust the number of TestNG threads after IExecutorFactory is deprecated? (Krishnan Mahadevan)
 New:   GITHUB-2874: Allow users to define ordering for TestNG listeners (Krishnan Mahadevan)

testng-core-api/src/main/java/org/testng/Reporter.java

@@ -5,6 +5,7 @@
 import java.util.Map;
 import org.testng.collections.Lists;
 import org.testng.collections.Maps;
+import org.testng.internal.AutoCloseableLock;
 import org.testng.internal.Utils;
 import org.testng.util.Strings;
 
@@ -67,7 +68,15 @@ public static void setEscapeHtml(boolean escapeHtml) {
     m_escapeHtml = escapeHtml;
   }
 
-  private static synchronized void log(String s, ITestResult m) {
+  private static final AutoCloseableLock lockForLogging = new AutoCloseableLock();
+
+  private static void log(String s, ITestResult m) {
+    try (AutoCloseableLock ignore = lockForLogging.lock()) {
+      logToReports(s, m);
+    }
+  }
+
+  private static void logToReports(String s, ITestResult m) {
     // Escape for the HTML reports.
     if (m_escapeHtml) {
       s = Strings.escapeHtml(s);
@@ -157,7 +166,15 @@ public static ITestResult getCurrentTestResult() {
     return m_currentTestResult.get();
   }
 
-  public static synchronized List<String> getOutput(ITestResult tr) {
+  private static final AutoCloseableLock lockForOutputs = new AutoCloseableLock();
+
+  public static List<String> getOutput(ITestResult tr) {
+    try (AutoCloseableLock ignore = lockForOutputs.lock()) {
+      return getOutputFromResult(tr);
+    }
+  }
+
+  private static List<String> getOutputFromResult(ITestResult tr) {
     List<String> result = Lists.newArrayList();
     if (tr == null) {
       // Guard against a possible NPE in scenarios wherein the test result object itself could be a

testng-core-api/src/main/java/org/testng/internal/AutoCloseableLock.java

@@ -0,0 +1,41 @@
+package org.testng.internal;
+
+import java.io.Closeable;
+import java.util.Objects;
+import java.util.concurrent.locks.ReentrantLock;
+
+/**
+ * A simple abstraction over {@link ReentrantLock} that can be used in conjunction with <code>
+ * try..resources</code> constructs.
+ */
+public final class AutoCloseableLock implements Closeable {
+
+  private final ReentrantLock internalLock = new ReentrantLock();
+
+  public AutoCloseableLock lock() {
+    internalLock.lock();
+    return this;
+  }
+
+  public boolean isHeldByCurrentThread() {
+    return internalLock.isHeldByCurrentThread();
+  }
+
+  @Override
+  public void close() {
+    internalLock.unlock();
+  }
+
+  @Override
+  public boolean equals(Object object) {
+    if (this == object) return true;
+    if (object == null || getClass() != object.getClass()) return false;
+    AutoCloseableLock that = (AutoCloseableLock) object;
+    return Objects.equals(internalLock, that.internalLock);
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(internalLock);
+  }
+}

testng-core-api/src/main/java/org/testng/internal/KeyAwareAutoCloseableLock.java

@@ -0,0 +1,51 @@
+package org.testng.internal;
+
+import java.util.Map;
+import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * A simple abstraction over {@link java.util.concurrent.locks.ReentrantLock} that can be used when
+ * we need to be dealing with a dictionary of lockable objects wherein we traditionally would have
+ * used the <code>synchronized</code> keyword.
+ */
+public final class KeyAwareAutoCloseableLock {
+  private final Map<Object, AutoCloseableLock> internalMap = new ConcurrentHashMap<>();
+
+  public AutoReleasable lockForObject(Object key) {
+    AutoCloseableLock internal =
+        internalMap.computeIfAbsent(Objects.requireNonNull(key), k -> new AutoCloseableLock());
+    return new AutoReleasable(internal.lock(), () -> internalMap.remove(key));
+  }
+
+  public static class AutoReleasable implements AutoCloseable {
+
+    private final AutoCloseableLock lock;
+    private final Runnable cleanupAction;
+
+    AutoReleasable(AutoCloseableLock lock, Runnable cleanupAction) {
+      this.lock = Objects.requireNonNull(lock);
+      this.cleanupAction =
+          this.lock.isHeldByCurrentThread() ? () -> {} : Objects.requireNonNull(cleanupAction);
+    }
+
+    @Override
+    public void close() {
+      lock.close();
+      cleanupAction.run();
+    }
+
+    @Override
+    public boolean equals(Object object) {
+      if (this == object) return true;
+      if (object == null || getClass() != object.getClass()) return false;
+      AutoReleasable that = (AutoReleasable) object;
+      return Objects.equals(lock, that.lock);
+    }
+
+    @Override
+    public int hashCode() {
+      return Objects.hash(lock);
+    }
+  }
+}

testng-core/src/main/java/org/testng/SkipException.java

@@ -1,5 +1,7 @@
 package org.testng;
 
+import org.testng.internal.AutoCloseableLock;
+
 /**
  * The root exception for special skip handling. In case a @Test or @Configuration throws this
  * exception the method will be considered a skip or a failure according to the return of {@link
@@ -34,14 +36,16 @@ public boolean isSkip() {
     return true;
   }
 
+  private final AutoCloseableLock internalLock = new AutoCloseableLock();
+
   /**
    * Subclasses may use this method to reduce the printed stack trace. This method keeps only the
    * last frame. <b>Important</b>: after calling this method the preserved internally and can be
    * restored called {@link #restoreStackTrace}.
    */
   protected void reduceStackTrace() {
     if (!m_stackReduced) {
-      synchronized (this) {
+      try (AutoCloseableLock ignore = internalLock.lock()) {
         StackTraceElement[] newStack = new StackTraceElement[1];
         StackTraceElement[] originalStack = getStackTrace();
         if (originalStack.length > 0) {
@@ -60,7 +64,7 @@ protected void reduceStackTrace() {
    */
   protected void restoreStackTrace() {
     if (m_stackReduced && null != m_stackTrace) {
-      synchronized (this) {
+      try (AutoCloseableLock ignore = internalLock.lock()) {
         setStackTrace(m_stackTrace);
         m_stackReduced = false;
       }

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

@@ -415,12 +415,14 @@ private void runSequentially() {
     }
   }
 
+  private final AutoCloseableLock suiteResultsLock = new AutoCloseableLock();
+
   private void runTest(TestRunner tr) {
     visualisers.forEach(tr::addListener);
     tr.run();
 
     ISuiteResult sr = new SuiteResult(xmlSuite, tr);
-    synchronized (suiteResults) {
+    try (AutoCloseableLock ignore = suiteResultsLock.lock()) {
       suiteResults.put(tr.getName(), sr);
     }
   }

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

@@ -28,8 +28,11 @@ public class ConfigurationGroupMethods {
   /** The list of beforeGroups methods keyed by the name of the group */
   private final Map<String, List<ITestNGMethod>> m_beforeGroupsMethods;
 
+  private final AutoCloseableLock beforeGroups = new AutoCloseableLock();
   private final Map<String, CountDownLatch> beforeGroupsThatHaveAlreadyRun =
       new ConcurrentHashMap<>();
+
+  private final AutoCloseableLock afterGroups = new AutoCloseableLock();
   private final Set<String> afterGroupsThatHaveAlreadyRun =
       Collections.newSetFromMap(new ConcurrentHashMap<>());
 
@@ -64,7 +67,7 @@ public List<ITestNGMethod> getBeforeGroupMethodsForGroup(String[] groups) {
       return Collections.emptyList();
     }
 
-    synchronized (beforeGroupsThatHaveAlreadyRun) {
+    try (AutoCloseableLock ignore = beforeGroups.lock()) {
       return Arrays.stream(groups)
           .map(t -> retrieve(beforeGroupsThatHaveAlreadyRun, m_beforeGroupsMethods, t))
           .filter(Objects::nonNull)
@@ -79,8 +82,7 @@ public List<ITestNGMethod> getAfterGroupMethods(ITestNGMethod testMethod) {
     }
 
     Set<String> methodGroups = new HashSet<>(Arrays.asList(testMethod.getGroups()));
-
-    synchronized (afterGroupsThatHaveAlreadyRun) {
+    try (AutoCloseableLock ignore = afterGroups.lock()) {
       if (m_afterGroupsMap == null) {
         m_afterGroupsMap = initializeAfterGroupsMap();
       }
@@ -151,7 +153,7 @@ private Map<String, List<ITestNGMethod>> initializeAfterGroupsMap() {
       }
     }
 
-    synchronized (afterGroupsThatHaveAlreadyRun) {
+    try (AutoCloseableLock ignore = afterGroups.lock()) {
       afterGroupsThatHaveAlreadyRun.clear();
     }
 

testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java

@@ -28,6 +28,7 @@
 import org.testng.annotations.IConfigurationAnnotation;
 import org.testng.collections.Maps;
 import org.testng.collections.Sets;
+import org.testng.internal.AutoCloseableLock;
 import org.testng.internal.ClassHelper;
 import org.testng.internal.ConfigurationMethod;
 import org.testng.internal.ConstructorOrMethod;
@@ -556,8 +557,10 @@ private void setMethodInvocationFailure(ITestNGMethod method, Object instance) {
     instances.add(TestNgMethodUtils.getMethodInvocationToken(method, instance));
   }
 
+  private final AutoCloseableLock internalLock = new AutoCloseableLock();
+
   private void setClassInvocationFailure(Class<?> clazz, Object instance) {
-    synchronized (m_classInvocationResults) {
+    try (AutoCloseableLock ignore = internalLock.lock()) {
       Set<Object> instances =
           m_classInvocationResults.computeIfAbsent(clazz, k -> Sets.newHashSet());
       Object objectToAdd = instance == null ? NULL_OBJECT : instance;

testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java

@@ -848,50 +848,58 @@ public ITestResult registerSkippedTestResult(
     return result;
   }
 
-  private static final Object LOCK = new Object();
+  private static final AutoCloseableLock internalLock = new AutoCloseableLock();
 
   private StatusHolder considerExceptions(
       ITestNGMethod tm,
       ITestResult testResult,
       ExpectedExceptionsHolder exceptionsHolder,
       FailureContext failure) {
-    synchronized (LOCK) {
-      StatusHolder holder = new StatusHolder();
-      int status = testResult.getStatus();
-      holder.handled = false;
-
-      Throwable ite = testResult.getThrowable();
-      if (status == ITestResult.FAILURE && ite != null) {
-
-        //  Invocation caused an exception, see if the method was annotated with @ExpectedException
-        if (exceptionsHolder != null) {
-          if (exceptionsHolder.isExpectedException(ite)) {
-            testResult.setStatus(ITestResult.SUCCESS);
-            status = ITestResult.SUCCESS;
+    try (AutoCloseableLock ignore = internalLock.lock()) {
+      return considerExceptionsInternal(tm, testResult, exceptionsHolder, failure);
+    }
+  }
+
+  private StatusHolder considerExceptionsInternal(
+      ITestNGMethod tm,
+      ITestResult testResult,
+      ExpectedExceptionsHolder exceptionsHolder,
+      FailureContext failure) {
+    StatusHolder holder = new StatusHolder();
+    int status = testResult.getStatus();
+    holder.handled = false;
+
+    Throwable ite = testResult.getThrowable();
+    if (status == ITestResult.FAILURE && ite != null) {
+
+      //  Invocation caused an exception, see if the method was annotated with @ExpectedException
+      if (exceptionsHolder != null) {
+        if (exceptionsHolder.isExpectedException(ite)) {
+          testResult.setStatus(ITestResult.SUCCESS);
+          status = ITestResult.SUCCESS;
+        } else {
+          if (isSkipExceptionAndSkip(ite)) {
+            status = ITestResult.SKIP;
           } else {
-            if (isSkipExceptionAndSkip(ite)) {
-              status = ITestResult.SKIP;
-            } else {
-              testResult.setThrowable(exceptionsHolder.wrongException(ite));
-              status = ITestResult.FAILURE;
-            }
+            testResult.setThrowable(exceptionsHolder.wrongException(ite));
+            status = ITestResult.FAILURE;
           }
-        } else {
-          handleException(ite, tm, testResult, failure.count.getAndIncrement());
-          holder.handled = true;
-          status = testResult.getStatus();
-        }
-      } else if (status != ITestResult.SKIP && exceptionsHolder != null) {
-        TestException exception = exceptionsHolder.noException(tm);
-        if (exception != null) {
-          testResult.setThrowable(exception);
-          status = ITestResult.FAILURE;
         }
+      } else {
+        handleException(ite, tm, testResult, failure.count.getAndIncrement());
+        holder.handled = true;
+        status = testResult.getStatus();
+      }
+    } else if (status != ITestResult.SKIP && exceptionsHolder != null) {
+      TestException exception = exceptionsHolder.noException(tm);
+      if (exception != null) {
+        testResult.setThrowable(exception);
+        status = ITestResult.FAILURE;
       }
-      holder.originalStatus = testResult.getStatus();
-      holder.status = status;
-      return holder;
     }
+    holder.originalStatus = testResult.getStatus();
+    holder.status = status;
+    return holder;
   }
 
   private static void updateStatusHolderAccordingToTestResult(

testng-core/src/main/java/org/testng/internal/thread/graph/GraphOrchestrator.java

@@ -6,6 +6,7 @@
 import java.util.concurrent.*;
 import org.testng.IDynamicGraph;
 import org.testng.collections.Maps;
+import org.testng.internal.AutoCloseableLock;
 import org.testng.internal.RuntimeBehavior;
 import org.testng.log4testng.Logger;
 import org.testng.thread.IThreadWorkerFactory;
@@ -23,6 +24,8 @@ public class GraphOrchestrator<T> {
   private final Comparator<T> comparator;
   private final IThreadWorkerFactory<T> factory;
 
+  private final AutoCloseableLock internalLock = new AutoCloseableLock();
+
   public GraphOrchestrator(
       ExecutorService service,
       IThreadWorkerFactory<T> factory,
@@ -35,7 +38,7 @@ public GraphOrchestrator(
   }
 
   public void run() {
-    synchronized (graph) {
+    try (AutoCloseableLock ignore = internalLock.lock()) {
       List<T> freeNodes = graph.getFreeNodes();
       if (comparator != null) {
         freeNodes.sort(comparator);
@@ -71,7 +74,7 @@ private void mapNodeToParent(List<T> freeNodes) {
   }
 
   private void afterExecute(IWorker<T> r, Throwable t) {
-    synchronized (graph) {
+    try (AutoCloseableLock ignore = internalLock.lock()) {
       setStatus(r, computeStatus(r));
       if (graph.getNodeCount() == graph.getNodeCountWithStatus(IDynamicGraph.Status.FINISHED)) {
         service.shutdown();
@@ -112,7 +115,7 @@ private IDynamicGraph.Status computeStatus(IWorker<T> worker) {
   }
 
   private void setStatus(IWorker<T> worker, IDynamicGraph.Status status) {
-    synchronized (graph) {
+    try (AutoCloseableLock ignore = internalLock.lock()) {
       for (T m : worker.getTasks()) {
         graph.setStatus(m, status);
       }