Skip to content

Commit

Permalink
Use Locks instead of synchronised keyword (#3077)
Browse files Browse the repository at this point in the history
* Use Locks instead of synchronised keyword

Closes #3040
  • Loading branch information
krmahadevan committed Mar 5, 2024
1 parent 823799c commit eb80671
Show file tree
Hide file tree
Showing 26 changed files with 285 additions and 256 deletions.
1 change: 1 addition & 0 deletions CHANGES.txt
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
21 changes: 19 additions & 2 deletions testng-core-api/src/main/java/org/testng/Reporter.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
8 changes: 6 additions & 2 deletions testng-core/src/main/java/org/testng/SkipException.java
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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;
}
Expand Down
4 changes: 3 additions & 1 deletion testng-core/src/main/java/org/testng/SuiteRunner.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<>());

Expand Down Expand Up @@ -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)
Expand All @@ -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();
}
Expand Down Expand Up @@ -151,7 +153,7 @@ private Map<String, List<ITestNGMethod>> initializeAfterGroupsMap() {
}
}

synchronized (afterGroupsThatHaveAlreadyRun) {
try (AutoCloseableLock ignore = afterGroups.lock()) {
afterGroupsThatHaveAlreadyRun.clear();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand All @@ -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);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
}
Expand Down
Loading

0 comments on commit eb80671

Please sign in to comment.