Iteration over a view on a synchronized collection without obtaining a lock on the collection #3279
Comments
|
Bob Atkey (migrated from Bugzilla):
Obviously, "Android-related" was a mistake here. I meant to write "Java-related". Sorry about that. |
|
@pmouawad (migrated from Bugzilla): Did you report all findings of your tool or only some examples ? |
|
Bob Atkey (migrated from Bugzilla):
Actually, our tool doesn't report the instance on line 456 because it doesn't look into the implementation of PropertyIteratorImpl while analysing the AbstractTestElement class. Therefore, it doesn't realise that everything that is passed in will have .iterator() called on it. This is certainly something that we will look at adding to ThreadSafe in the future. Thanks!
I only reported two examples of findings that I thought looked interesting. If I get more time soon I will report the rest of the interesting looking reports that ThreadSafe gives. |
|
@ham1 (migrated from Bugzilla): Are there any more that the tool found (in the latest code) which should be addressed? |
|
@vlsi (migrated from Bugzilla): So I do not see much reason to complicate code. |
|
@pmouawad (migrated from Bugzilla):
Thanks Felix for digging, interesting indeed. A question for you, I have a doubt. In my understanding, calling synchronized in monothreaded call has a cost: Do you confirm this from benchmarks you did ? |
|
@ham1 (migrated from Bugzilla):
It does say the problem still exists in the comment yet the issue was set to resolved. Maybe we should remove the synchronizedMap as we are accessing it in an unsafe way anyway - or just comment the code and close this bz. |
|
TaoLu (migrated from Bugzilla): Created attachment nextNull1.jpg: Jmeter Test Error When Itr next and remove |
|
TaoLu (migrated from Bugzilla): |
|
@vlsi (migrated from Bugzilla): |
|
@FSchumacher (migrated from Bugzilla): With the attached patch I can reproduce an overlap in the recoverRunningVersion method between more than one thread. The exceptions (with are for example:
Created attachment slow-recoverRunningVersion.diff: Slow down recoverRunningVersion to show problems slow-recoverRunningVersion.diffdiff --git a/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java b/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java
index 93f8369515..571c2a6ac8 100644
--- a/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java
+++ b/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java
@@ -26,6 +26,7 @@ import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
+import java.util.concurrent.ConcurrentSkipListSet;
import org.apache.jmeter.gui.Searchable;
import org.apache.jmeter.testelement.property.BooleanProperty;
@@ -42,6 +43,7 @@ import org.apache.jmeter.testelement.property.StringProperty;
import org.apache.jmeter.testelement.property.TestElementProperty;
import org.apache.jmeter.threads.JMeterContext;
import org.apache.jmeter.threads.JMeterContextService;
+import org.jetbrains.annotations.NotNull;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -67,6 +69,8 @@ public abstract class AbstractTestElement implements TestElement, Serializable,
private transient String threadName = null;
+ private transient ConcurrentSkipListSet<ComparableThread> currentThreads = new ConcurrentSkipListSet<>();
+
@Override
public Object clone() {
try {
@@ -502,6 +506,12 @@ public abstract class AbstractTestElement implements TestElement, Serializable,
*/
@Override
public void recoverRunningVersion() {
+ currentThreads.add(new ComparableThread(Thread.currentThread()));
+ if (currentThreads.size() > 1) {
+ log.warn("Recover in different threads: {}; Element: {}",
+ currentThreads, this,
+ new RuntimeException("Thread mismatch"));
+ }
Iterator<Map.Entry<String, JMeterProperty>> iter = propMap.entrySet().iterator();
while (iter.hasNext()) {
Map.Entry<String, JMeterProperty> entry = iter.next();
@@ -514,8 +524,29 @@ public abstract class AbstractTestElement implements TestElement, Serializable,
}
}
emptyTemporary();
+ try {
+ Thread.sleep(500);
+ } catch (InterruptedException e) {
+ // nothing to do
+ }
}
+ private static class ComparableThread extends Thread implements Comparable {
+ final Thread t;
+ ComparableThread(Thread t) {
+ this.t = t;
+ }
+
+ @Override
+ public int compareTo(@NotNull Object o) {
+ return this.t.getName().compareTo(((Thread) o).getName()) ;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ return o instanceof ComparableThread && compareTo((ComparableThread) o) == 0;
+ }
+ }
/**
* Clears temporaryProperties
*/ |
|
@FSchumacher (migrated from Bugzilla): |
|
@FSchumacher (migrated from Bugzilla): When the threads are removed at the end (in my simple test plan) one Test Element is getting flagged, only. 2022-03-08 21:55:40,771 WARN o.a.j.t.AbstractTestElement: Recover in different threads: [Thread[Thread-53,6,main], Thread[Thread-54,6,main]]; Element: org.apache.jmeter.reporters.ResultCollector@42f6fa63
The code for ResultCollector explicitly mentions, that it has to be thread-safe (which recoverRunningVersion is not). So maybe, we can make it for ResultCollector thread-safe instead of all? But on the other hand, if we obtain a lock for a single thread, it is not that expensive any more. So I think we could try to add it. Created attachment slow-recoverRunningVersion.diff: Slow down recoverRunningVersion to show problems slow-recoverRunningVersion.diffdiff --git a/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java b/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java
index 93f8369515..5d7c0ecbac 100644
--- a/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java
+++ b/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java
@@ -25,7 +25,9 @@ import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
+import java.util.concurrent.ConcurrentSkipListSet;
import org.apache.jmeter.gui.Searchable;
import org.apache.jmeter.testelement.property.BooleanProperty;
@@ -42,6 +44,7 @@ import org.apache.jmeter.testelement.property.StringProperty;
import org.apache.jmeter.testelement.property.TestElementProperty;
import org.apache.jmeter.threads.JMeterContext;
import org.apache.jmeter.threads.JMeterContextService;
+import org.jetbrains.annotations.NotNull;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -67,6 +70,8 @@ public abstract class AbstractTestElement implements TestElement, Serializable,
private transient String threadName = null;
+ private transient ConcurrentSkipListSet<ComparableThread> currentThreads = new ConcurrentSkipListSet<>();
+
@Override
public Object clone() {
try {
@@ -502,6 +507,13 @@ public abstract class AbstractTestElement implements TestElement, Serializable,
*/
@Override
public void recoverRunningVersion() {
+ ComparableThread currentThread = new ComparableThread(Thread.currentThread());
+ currentThreads.add(currentThread);
+ if (currentThreads.size() > 1) {
+ log.warn("Recover in different threads: {}; Element: {}",
+ currentThreads, this,
+ new RuntimeException("Thread mismatch"));
+ }
Iterator<Map.Entry<String, JMeterProperty>> iter = propMap.entrySet().iterator();
while (iter.hasNext()) {
Map.Entry<String, JMeterProperty> entry = iter.next();
@@ -514,8 +526,35 @@ public abstract class AbstractTestElement implements TestElement, Serializable,
}
}
emptyTemporary();
+ try {
+ Thread.sleep(500);
+ } catch (InterruptedException e) {
+ // nothing to do
+ }
+ currentThreads.remove(currentThread);
}
+ private static class ComparableThread extends Thread implements Comparable {
+ final Thread t;
+ ComparableThread(Thread t) {
+ this.t = t;
+ }
+
+ @Override
+ public int hashCode() {
+ return t.hashCode();
+ }
+
+ @Override
+ public int compareTo(@NotNull Object o) {
+ return this.t.getName().compareTo(((ComparableThread) o).t.getName()) ;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ return o instanceof ComparableThread && compareTo((ComparableThread) o) == 0;
+ }
+ }
/**
* Clears temporaryProperties
*/ |
|
@FSchumacher (migrated from Bugzilla): slow-recoverRunningVersion.diffdiff --git a/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java b/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java
index 93f8369515..739c588842 100644
--- a/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java
+++ b/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java
@@ -25,7 +25,9 @@ import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
+import java.util.concurrent.ConcurrentSkipListSet;
import org.apache.jmeter.gui.Searchable;
import org.apache.jmeter.testelement.property.BooleanProperty;
@@ -42,6 +44,7 @@ import org.apache.jmeter.testelement.property.StringProperty;
import org.apache.jmeter.testelement.property.TestElementProperty;
import org.apache.jmeter.threads.JMeterContext;
import org.apache.jmeter.threads.JMeterContextService;
+import org.jetbrains.annotations.NotNull;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -67,6 +70,8 @@ public abstract class AbstractTestElement implements TestElement, Serializable,
private transient String threadName = null;
+ private transient ConcurrentSkipListSet<ComparableThread> currentThreads = new ConcurrentSkipListSet<>();
+
@Override
public Object clone() {
try {
@@ -502,6 +507,15 @@ public abstract class AbstractTestElement implements TestElement, Serializable,
*/
@Override
public void recoverRunningVersion() {
+ ComparableThread currentThread = new ComparableThread(Thread.currentThread());
+ currentThreads.add(currentThread);
+ if (currentThreads.size() > 1) {
+ log.warn("Recover in different threads: {}; Element: {}, temporary.size(): {}",
+ currentThreads,
+ this,
+ temporaryProperties == null ? -1 : temporaryProperties.size(),
+ new RuntimeException("Thread mismatch"));
+ }
Iterator<Map.Entry<String, JMeterProperty>> iter = propMap.entrySet().iterator();
while (iter.hasNext()) {
Map.Entry<String, JMeterProperty> entry = iter.next();
@@ -514,8 +528,35 @@ public abstract class AbstractTestElement implements TestElement, Serializable,
}
}
emptyTemporary();
+ try {
+ Thread.sleep(500);
+ } catch (InterruptedException e) {
+ // nothing to do
+ }
+ currentThreads.remove(currentThread);
}
+ private static class ComparableThread extends Thread implements Comparable {
+ final Thread t;
+ ComparableThread(Thread t) {
+ this.t = t;
+ }
+
+ @Override
+ public int hashCode() {
+ return t.hashCode();
+ }
+
+ @Override
+ public int compareTo(@NotNull Object o) {
+ return this.t.getName().compareTo(((ComparableThread) o).t.getName()) ;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ return o instanceof ComparableThread && compareTo((ComparableThread) o) == 0;
+ }
+ }
/**
* Clears temporaryProperties
*/ |
|
@FSchumacher (migrated from Bugzilla): So, the question is, which Element is it and where does the concurrent access comes from? @taolu, are you using a third party plugin? What elements do you use apart from CSV Data Source? How big are your CSV sources, how many elements do they have? Created attachment slow-recoverRunningVersion.diff: Slow down recoverRunningVersion to show problems slow-recoverRunningVersion.diffdiff --git a/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java b/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java
index 93f8369515..739c588842 100644
--- a/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java
+++ b/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java
@@ -25,7 +25,9 @@ import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
+import java.util.concurrent.ConcurrentSkipListSet;
import org.apache.jmeter.gui.Searchable;
import org.apache.jmeter.testelement.property.BooleanProperty;
@@ -42,6 +44,7 @@ import org.apache.jmeter.testelement.property.StringProperty;
import org.apache.jmeter.testelement.property.TestElementProperty;
import org.apache.jmeter.threads.JMeterContext;
import org.apache.jmeter.threads.JMeterContextService;
+import org.jetbrains.annotations.NotNull;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -67,6 +70,8 @@ public abstract class AbstractTestElement implements TestElement, Serializable,
private transient String threadName = null;
+ private transient ConcurrentSkipListSet<ComparableThread> currentThreads = new ConcurrentSkipListSet<>();
+
@Override
public Object clone() {
try {
@@ -502,6 +507,15 @@ public abstract class AbstractTestElement implements TestElement, Serializable,
*/
@Override
public void recoverRunningVersion() {
+ ComparableThread currentThread = new ComparableThread(Thread.currentThread());
+ currentThreads.add(currentThread);
+ if (currentThreads.size() > 1) {
+ log.warn("Recover in different threads: {}; Element: {}, temporary.size(): {}",
+ currentThreads,
+ this,
+ temporaryProperties == null ? -1 : temporaryProperties.size(),
+ new RuntimeException("Thread mismatch"));
+ }
Iterator<Map.Entry<String, JMeterProperty>> iter = propMap.entrySet().iterator();
while (iter.hasNext()) {
Map.Entry<String, JMeterProperty> entry = iter.next();
@@ -514,8 +528,35 @@ public abstract class AbstractTestElement implements TestElement, Serializable,
}
}
emptyTemporary();
+ try {
+ Thread.sleep(500);
+ } catch (InterruptedException e) {
+ // nothing to do
+ }
+ currentThreads.remove(currentThread);
}
+ private static class ComparableThread extends Thread implements Comparable {
+ final Thread t;
+ ComparableThread(Thread t) {
+ this.t = t;
+ }
+
+ @Override
+ public int hashCode() {
+ return t.hashCode();
+ }
+
+ @Override
+ public int compareTo(@NotNull Object o) {
+ return this.t.getName().compareTo(((ComparableThread) o).t.getName()) ;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ return o instanceof ComparableThread && compareTo((ComparableThread) o) == 0;
+ }
+ }
/**
* Clears temporaryProperties
*/ |
|
@FSchumacher (migrated from Bugzilla): (with locking) (without locking): |
Bob Atkey (Bug 55827):
We ran our static analysis tool ThreadSafe [1] on version 2.10 of JMeter, which appeared to uncover a couple of concurrency issues. One of the most interesting was the possibility of an unsynchronised iteration over a synchronised collection in the class 'AbstractTestElement'.
The relevant line is 499:
Iterator<Map.Entry<String, JMeterProperty>> iter = propMap.entrySet().iterator();
where the propMap field always contains a synchronised collection created at line 57 in the same file.
The JDK documentation for Collections.synchronizedMap states that:
It appears that the code on line 499 does not correctly synchronize on propMap, leading to the possibility of the non-deterministic behaviour the JDK documentation warns about.
We're not sure that this can actually result in a user-visible bug, but we thought you'd like to know.
We are also planning to use this finding as an example of Android-related concurrency mistakes in an article about ThreadSafe. Obviously, if you, as the developers of JMeter, have any objections to our using this as an example, then we won't.
[1] ThreadSafe is a static analysis tool for Java concurrency, developed by Contemplate Ltd.:
http://www.contemplateltd.com/
Severity: major
OS: Linux
The text was updated successfully, but these errors were encountered: