diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 73f1f9f5fe1..4dfcc413452 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -18,40 +18,58 @@ concurrency: cancel-in-progress: true jobs: - windows: - name: 'Windows (JDK 17)' - runs-on: windows-latest + matrix_prep: + name: Matrix Preparation + runs-on: ubuntu-latest + outputs: + matrix: ${{ steps.set-matrix.outputs.matrix }} + env: + # Number of jobs to generate in matrix.js + MATRIX_JOBS: 4 steps: - - uses: actions/checkout@v3 - - name: 'Set up JDK 17' - uses: actions/setup-java@v3 - with: - java-version: 17 - distribution: 'zulu' - - uses: burrunan/gradle-cache-action@v1 - name: Test - with: - job-id: jdk17 - multi-cache-enabled: false - # An explicit skip for Sha512 tasks is required due to https://github.com/gradle/gradle/issues/16789 - arguments: --scan --no-parallel build -x distTar -x distTarSource -x distTarSha512 -x distTarSourceSha512 + - uses: actions/checkout@v3 + - id: set-matrix + run: | + node .github/workflows/matrix.js - mac: - name: 'macOS (JDK 17)' - runs-on: macos-latest + test: + needs: matrix_prep + name: '${{ matrix.name }}' + runs-on: ${{ matrix.os }} + env: + TZ: ${{ matrix.tz }} + strategy: + matrix: ${{fromJson(needs.matrix_prep.outputs.matrix)}} + fail-fast: false + # max-parallel: 4 steps: - - uses: actions/checkout@v3 - - name: 'Set up JDK 17' + - uses: actions/checkout@v2 + with: + fetch-depth: 50 + - name: Set up Java ${{ matrix.java_version }}, ${{ matrix.java_distribution }} uses: actions/setup-java@v3 with: - java-version: 17 - distribution: 'zulu' + java-version: ${{ matrix.java_version }} + distribution: ${{ matrix.java_distribution }} + architecture: x64 + - name: Steps to reproduce + uses: actions/github-script@v6 + with: + script: | + console.log('The following command might help reproducing CI results, use Java ${{ matrix.java_version }}') + console.log('TZ="${{ matrix.tz }}" _JAVA_OPTIONS="${{ matrix.extraJvmArgs }}" ./gradlew build -x distTar -x distTarSource -x distTarSha512 -x distTarSourceSha512 ${{ matrix.extraGradleArgs }} -PtestExtraJvmArgs="${{ matrix.testExtraJvmArgs }}" -PtestDisableCaching="${{ matrix.testDisableCaching }}"') - uses: burrunan/gradle-cache-action@v1 name: Test with: - job-id: jdk14 + job-id: jdk${{ matrix.java_version }} multi-cache-enabled: false - arguments: --scan --no-parallel build -x distTar -x distTarSource -x distTarSha512 -x distTarSourceSha512 -Dskip.test_TestDNSCacheManager.testWithCustomResolverAnd1Server=true + # An explicit skip for Sha512 tasks is required due to https://github.com/gradle/gradle/issues/16789 + arguments: --scan --no-parallel build -x distTar -x distTarSource -x distTarSha512 -x distTarSourceSha512 ${{ matrix.extraGradleArgs }} + properties: | + testExtraJvmArgs=${{ matrix.testExtraJvmArgs }} + testDisableCaching=${{ matrix.testDisableCaching }} + env: + _JAVA_OPTIONS: ${{ matrix.extraJvmArgs }} errorprone: name: 'Error Prone (JDK 11)' diff --git a/.github/workflows/matrix.js b/.github/workflows/matrix.js new file mode 100644 index 00000000000..20c32abeb71 --- /dev/null +++ b/.github/workflows/matrix.js @@ -0,0 +1,164 @@ +// The script generates a random subset of valid jdk, os, timezone, and other axes. +// You can preview the results by running "node matrix.js" +// See https://github.com/vlsi/github-actions-random-matrix +let fs = require('fs'); +let os = require('os'); +let {MatrixBuilder} = require('./matrix_builder'); +const matrix = new MatrixBuilder(); +matrix.addAxis({ + name: 'java_distribution', + values: [ + {value: 'corretto', weight: 1}, + {value: 'liberica', weight: 1}, + {value: 'microsoft', weight: 1}, + {value: 'oracle', weight: 1}, + {value: 'semeru', weight: 4}, + {value: 'temurin', weight: 1}, + {value: 'zulu', weight: 1}, + ] +}); + +matrix.addAxis({ + name: 'java_version', + // Strings allow versions like 18-ea + values: [ + '8', + '11', + '17', + ] +}); + +matrix.addAxis({ + name: 'tz', + values: [ + 'America/New_York', + 'Pacific/Chatham', + 'UTC' + ] +}); + +matrix.addAxis({ + name: 'os', + title: x => x.replace('-latest', ''), + values: [ + // TODO: X11 is not available. Un-comment when https://github.com/burrunan/gradle-cache-action/issues/48 is resolved + // 'ubuntu-latest', + 'windows-latest', + 'macos-latest' + ] +}); + +// Test cases when Object#hashCode produces the same results +// It allows capturing cases when the code uses hashCode as a unique identifier +matrix.addAxis({ + name: 'hash', + values: [ + {value: 'regular', title: '', weight: 42}, + {value: 'same', title: 'same hashcode', weight: 1} + ] +}); +matrix.addAxis({ + name: 'locale', + title: x => x.language + '_' + x.country, + values: [ + {language: 'de', country: 'DE'}, + {language: 'fr', country: 'FR'}, + // TODO: fix :src:dist-check:batchBUG_62847 + // Fails with "ERROR o.a.j.u.JMeterUtils: Could not find resources for 'ru_EN'" + // {language: 'ru', country: 'RU'}, + {language: 'tr', country: 'TR'}, + ] +}); + +matrix.setNamePattern(['java_version', 'java_distribution', 'hash', 'os', 'tz', 'locale']); + +// Semeru uses OpenJ9 jit which has no option for making hash codes the same +matrix.exclude({java_distribution: {value: 'semeru'}, hash: {value: 'same'}}); +// Microsoft Java has no distribution for 8 +matrix.exclude({java_distribution: {value: 'microsoft'}, java_version: '8'}); +// Oracle JDK is only supported for JDK 17 and later +matrix.exclude({java_distribution: {value: 'oracle'}, java_version: ['8', '11']}); +// Ensure at least one job with "same" hashcode exists +matrix.generateRow({hash: {value: 'same'}}); +// Ensure at least one Windows and at least one Linux job is present (macOS is almost the same as Linux) +matrix.generateRow({os: 'windows-latest'}); +// TODO: un-comment when xvfb will be possible +// matrix.generateRow({os: 'ubuntu-latest'}); +// Ensure there will be at least one job with Java 8 +matrix.generateRow({java_version: "8"}); +// Ensure there will be at least one job with Java 11 +matrix.generateRow({java_version: "11"}); +// Ensure there will be at least one job with Java 17 +matrix.generateRow({java_version: "17"}); +const include = matrix.generateRows(process.env.MATRIX_JOBS || 5); +if (include.length === 0) { + throw new Error('Matrix list is empty'); +} +include.sort((a, b) => a.name.localeCompare(b.name, undefined, {numeric: true})); +include.forEach(v => { + // Pass locale via Gradle arguments in case it won't be inherited from _JAVA_OPTIONS + // In fact, _JAVA_OPTIONS is non-standard and might be ignored by some JVMs + let gradleArgs = [ + `-Duser.country=${v.locale.country}`, + `-Duser.language=${v.locale.language}`, + ]; + if (v.hash.value === 'same' && v.java_version <= 8) { + // processSiteXslt fails with VerifyError when running with Java 8 and the same hashcode + // Skip the task in that case + //java.lang.VerifyError: (class: website_style, method: issue_separator signature: (Lcom/sun/org/apache/xala...) + // Illegal target of jump or branch + gradleArgs.push('-x :src:dist:processSiteXslt'); + // javadoc tool seems take too much CPU when there are many hash collisions + gradleArgs.push('-x :src:dist:javadocAggregate'); + } + v.extraGradleArgs = gradleArgs.join(' '); +}); +include.forEach(v => { + let jvmArgs = []; + // Extra JVM arguments passed to test execution + let testJvmArgs = []; + if (v.hash.value === 'same') { + testJvmArgs.push('-XX:+UnlockExperimentalVMOptions', '-XX:hashCode=2'); + } + // Gradle does not work in tr_TR locale, so pass locale to test only: https://github.com/gradle/gradle/issues/17361 + jvmArgs.push(`-Duser.country=${v.locale.country}`); + jvmArgs.push(`-Duser.language=${v.locale.language}`); + v.java_distribution = v.java_distribution.value; + if (v.java_distribution !== 'semeru' && Math.random() > 0.5) { + // The following options randomize instruction selection in JIT compiler + // so it might reveal missing synchronization in TestNG code + v.name += ', stress JIT'; + v.testDisableCaching = 'JIT randomization should not be cached'; + jvmArgs.push('-XX:+UnlockDiagnosticVMOptions'); + if (v.java_version >= 8) { + // Randomize instruction scheduling in GCM + // share/opto/c2_globals.hpp + jvmArgs.push('-XX:+StressGCM'); + // Randomize instruction scheduling in LCM + // share/opto/c2_globals.hpp + jvmArgs.push('-XX:+StressLCM'); + } + if (v.java_version >= 16) { + // Randomize worklist traversal in IGVN + // share/opto/c2_globals.hpp + jvmArgs.push('-XX:+StressIGVN'); + } + if (v.java_version >= 17) { + // Randomize worklist traversal in CCP + // share/opto/c2_globals.hpp + jvmArgs.push('-XX:+StressCCP'); + } + } + v.extraJvmArgs = jvmArgs.join(' '); + v.testExtraJvmArgs = testJvmArgs.join(' ::: '); + delete v.hash; +}); + +console.log(include); + +let filePath = process.env['GITHUB_OUTPUT'] || ''; +if (filePath) { + fs.appendFileSync(filePath, `matrix< a === (b.weight || 1) ? a : 0, values[0].weight || 1) !== 0 + this.totalWeight = this.uniform ? values.length : values.reduce((a, b) => a + (b.weight || 1), 0); + } + + static matches(row, filter) { + if (typeof filter === 'function') { + return filter(row); + } + if (Array.isArray(filter)) { + // e.g. row={os: 'windows'}; filter=[{os: 'linux'}, {os: 'linux'}] + return filter.find(v => Axis.matches(row, v)); + } + if (typeof filter === 'object') { + // e.g. row={jdk: {name: 'openjdk', version: 8}}; filter={jdk: {version: 8}} + for (const [key, value] of Object.entries(filter)) { + if (!row.hasOwnProperty(key) || !Axis.matches(row[key], value)) { + return false; + } + } + return true; + } + return row === filter; + } + + pickValue(filter) { + let values = this.values; + if (filter) { + values = values.filter(v => Axis.matches(v, filter)); + } + if (values.length === 0) { + const filterStr = typeof filter === 'string' ? filter.toString() : JSON.stringify(filter); + throw Error(`No values produces for axis '${this.name}' from ${JSON.stringify(this.values)}, filter=${filterStr}`); + } + if (values.length === 1) { + return values[0]; + } + if (this.uniform) { + return values[Math.floor(Math.random() * values.length)]; + } + const totalWeight = !filter ? this.totalWeight : values.reduce((a, b) => a + (b.weight || 1), 0); + let weight = Math.random() * totalWeight; + for (let i = 0; i < values.length; i++) { + const value = values[i]; + weight -= value.weight || 1; + if (weight <= 0) { + return value; + } + } + return values[values.length - 1]; + } +} + +class MatrixBuilder { + constructor() { + this.axes = []; + this.axisByName = {}; + this.rows = []; + this.duplicates = {}; + this.excludes = []; + this.includes = []; + this.failOnUnsatisfiableFilters = false; + } + + /** + * Specifies include filter (all the generated rows would comply with all the include filters) + * @param filter + */ + include(filter) { + this.includes.push(filter); + } + + /** + * Specifies exclude filter (e.g. exclude a forbidden combination) + * @param filter + */ + exclude(filter) { + this.excludes.push(filter); + } + + addAxis({name, title, values}) { + const axis = new Axis({name, title, values}); + this.axes.push(axis); + this.axisByName[name] = axis; + return axis; + } + + setNamePattern(names) { + this.namePattern = names; + } + + /** + * Returns true if the row matches the include and exclude filters. + * @param row input row + * @returns {boolean} + */ + matches(row) { + return (this.excludes.length === 0 || !this.excludes.find(f => Axis.matches(row, f))) && + (this.includes.length === 0 || this.includes.find(f => Axis.matches(row, f))); + } + + failOnUnsatisfiableFilters(value) { + this.failOnUnsatisfiableFilters = value; + } + + /** + * Adds a row that matches the given filter to the resulting matrix. + * filter values could be + * - literal values: filter={os: 'windows-latest'} + * - arrays: filter={os: ['windows-latest', 'linux-latest']} + * - functions: filter={os: x => x!='windows-latest'} + * @param filter object with keys matching axes names + * @returns {*} + */ + generateRow(filter) { + let res; + if (filter) { + // If matching row already exists, no need to generate more + res = this.rows.find(v => Axis.matches(v, filter)); + if (res) { + return res; + } + } + for (let i = 0; i < 142; i++) { + res = this.axes.reduce( + (prev, next) => + Object.assign(prev, { + [next.name]: next.pickValue(filter ? filter[next.name] : undefined) + }), + {} + ); + if (!this.matches(res)) { + continue; + } + const key = JSON.stringify(res); + if (!this.duplicates.hasOwnProperty(key)) { + this.duplicates[key] = true; + res.name = + this.namePattern.map(axisName => { + let value = res[axisName]; + const title = value.title; + if (typeof title != 'undefined') { + return title; + } + const computeTitle = this.axisByName[axisName].title; + if (computeTitle) { + return computeTitle(value); + } + if (typeof value === 'object' && value.hasOwnProperty('value')) { + return value.value; + } + return value; + }).filter(Boolean).join(", "); + this.rows.push(res); + return res; + } + } + const filterStr = typeof filter === 'string' ? filter.toString() : JSON.stringify(filter); + const msg = `Unable to generate row for ${filterStr}. Please check include and exclude filters`; + if (this.failOnUnsatisfiableFilters) { + throw Error(msg); + } else { + console.warn(msg); + } + } + + generateRows(maxRows, filter) { + for (let i = 0; this.rows.length < maxRows && i < maxRows; i++) { + this.generateRow(filter); + } + return this.rows; + } + + /** + * Computes the number of all the possible combinations. + * @returns {{bad: number, good: number}} + */ + summary() { + let position = -1; + let indices = []; + let values = {}; + const axes = this.axes; + function resetValuesUpTo(nextPosition) { + for(let i=0; i { }.forEach { systemProperty(it.key.toString().substring("jmeter.properties.".length), it.value) } + props.string("testExtraJvmArgs").trim().takeIf { it.isNotBlank() }?.let { + jvmArgs(it.split(" ::: ")) + } + props.string("testDisableCaching").trim().takeIf { it.isNotBlank() }?.let { + outputs.doNotCacheIf(it) { + true + } + } passProperty("java.awt.headless") passProperty("skip.test_TestDNSCacheManager.testWithCustomResolverAnd1Server") // Spock tests use cglib proxies that access ClassLoader.defineClass reflectively diff --git a/src/components/src/main/java/org/apache/jmeter/timers/ConstantThroughputTimer.java b/src/components/src/main/java/org/apache/jmeter/timers/ConstantThroughputTimer.java index 945f22f5619..4a764cf7be4 100644 --- a/src/components/src/main/java/org/apache/jmeter/timers/ConstantThroughputTimer.java +++ b/src/components/src/main/java/org/apache/jmeter/timers/ConstantThroughputTimer.java @@ -37,6 +37,7 @@ import org.apache.jmeter.threads.AbstractThreadGroup; import org.apache.jmeter.threads.JMeterContextService; import org.apache.jmeter.util.JMeterUtils; +import org.apache.jorphan.collections.IdentityKey; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -103,10 +104,10 @@ public String toString() { private static final ThroughputInfo allThreadsInfo = new ThroughputInfo(); //For holding the ThroughputInfo objects for all ThreadGroups. Keyed by AbstractThreadGroup objects - private static final ConcurrentMap threadGroupsInfoMap = + //TestElements can't be used as keys in HashMap. + private static final ConcurrentMap, ThroughputInfo> threadGroupsInfoMap = new ConcurrentHashMap<>(); - /** * Constructor for a non-configured ConstantThroughputTimer. */ @@ -198,13 +199,10 @@ private long calculateDelay() { case AllActiveThreadsInCurrentThreadGroup_Shared: //All threads in this group - alternate calculation final org.apache.jmeter.threads.AbstractThreadGroup group = JMeterContextService.getContext().getThreadGroup(); - ThroughputInfo groupInfo = threadGroupsInfoMap.get(group); + IdentityKey key = new IdentityKey<>(group); + ThroughputInfo groupInfo = threadGroupsInfoMap.get(key); if (groupInfo == null) { - groupInfo = new ThroughputInfo(); - ThroughputInfo previous = threadGroupsInfoMap.putIfAbsent(group, groupInfo); - if (previous != null) { // We did not replace the entry - groupInfo = previous; // so use the existing one - } + groupInfo = threadGroupsInfoMap.computeIfAbsent(key, (k) -> new ThroughputInfo()); } delay = calculateSharedDelay(groupInfo,Math.round(msPerRequest)); break; diff --git a/src/components/src/main/java/org/apache/jmeter/timers/poissonarrivals/PreciseThroughputTimer.java b/src/components/src/main/java/org/apache/jmeter/timers/poissonarrivals/PreciseThroughputTimer.java index 48a6a75233d..fc3c4a9f187 100644 --- a/src/components/src/main/java/org/apache/jmeter/timers/poissonarrivals/PreciseThroughputTimer.java +++ b/src/components/src/main/java/org/apache/jmeter/timers/poissonarrivals/PreciseThroughputTimer.java @@ -28,6 +28,7 @@ import org.apache.jmeter.testelement.TestStateListener; import org.apache.jmeter.threads.AbstractThreadGroup; import org.apache.jmeter.timers.Timer; +import org.apache.jorphan.collections.IdentityKey; import org.apache.jorphan.util.JMeterStopThreadException; import org.apiguardian.api.API; import org.slf4j.Logger; @@ -44,7 +45,10 @@ public class PreciseThroughputTimer extends AbstractTestElement implements Clone private static final Logger log = LoggerFactory.getLogger(PreciseThroughputTimer.class); private static final long serialVersionUID = 4; - private static final ConcurrentMap groupEvents = new ConcurrentHashMap<>(); + + // TestElements can't be used as keys in a HashMap, so we use IdentityHashMap + private static final ConcurrentMap, EventProducer> groupEvents = + new ConcurrentHashMap<>(); /** * Desired throughput configured as {@code throughput/throughputPeriod} per second. @@ -134,9 +138,14 @@ public long delay() { private EventProducer getEventProducer() { AbstractThreadGroup tg = getThreadContext().getThreadGroup(); + IdentityKey key = new IdentityKey<>(tg); + EventProducer eventProducer = groupEvents.get(key); + if (eventProducer != null) { + return eventProducer; + } Long seed = randomSeed == null || randomSeed == 0 ? null : randomSeed; return - groupEvents.computeIfAbsent(tg, x -> new ConstantPoissonProcessGenerator( + groupEvents.computeIfAbsent(key, x -> new ConstantPoissonProcessGenerator( () -> PreciseThroughputTimer.this.getThroughput() / throughputPeriod, batchSize, batchThreadDelay, this, seed, true)); } diff --git a/src/core/build.gradle.kts b/src/core/build.gradle.kts index af479d8ac18..7572f30d061 100644 --- a/src/core/build.gradle.kts +++ b/src/core/build.gradle.kts @@ -16,6 +16,7 @@ */ import com.github.autostyle.gradle.AutostyleTask +import com.github.vlsi.gradle.ide.IdeExtension plugins { id("build-logic.jvm-published-library") @@ -140,7 +141,10 @@ val versionClass by tasks.registering(Sync::class) { into(generatedVersionDir) } -ide { +// For some reason, using `ide { ... }` sometimes causes +// Caused by: java.lang.IllegalStateException: couldn't find inline method +// Lorg/gradle/kotlin/dsl/Accessorslkzxmv806rumtqvft7195qyhKt;.getIde(Lorg/gradle/api/Project;)Lcom/github/vlsi/gradle/ide/IdeExtension; +configure { generatedJavaSources(versionClass.get(), generatedVersionDir) } diff --git a/src/core/src/main/java/org/apache/jmeter/control/GenericController.java b/src/core/src/main/java/org/apache/jmeter/control/GenericController.java index 4d9d721395d..2fff0cf5f61 100644 --- a/src/core/src/main/java/org/apache/jmeter/control/GenericController.java +++ b/src/core/src/main/java/org/apache/jmeter/control/GenericController.java @@ -21,10 +21,9 @@ import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Deque; +import java.util.IdentityHashMap; import java.util.Iterator; import java.util.List; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; import org.apache.jmeter.engine.event.LoopIterationEvent; import org.apache.jmeter.engine.event.LoopIterationListener; @@ -58,7 +57,7 @@ public class GenericController extends AbstractTestElement implements Controller private transient Deque iterationListeners = new ArrayDeque<>(); // Only create the map if it is required - private transient ConcurrentMap children = new ConcurrentHashMap<>(); + private transient IdentityHashMap children = new IdentityHashMap<>(); private static final Object DUMMY = new Object(); @@ -353,10 +352,13 @@ public void addTestElement(TestElement child) { * {@inheritDoc} */ @Override + @SuppressWarnings("SynchronizeOnNonFinalField") public final boolean addTestElementOnce(TestElement child){ - if (children.putIfAbsent(child, DUMMY) == null) { - addTestElement(child); - return true; + synchronized (children) { + if (children.putIfAbsent(child, DUMMY) == null) { + addTestElement(child); + return true; + } } return false; } @@ -414,7 +416,7 @@ protected void resetIterCount() { protected Object readResolve(){ iterationListeners = new ArrayDeque<>(); - children = new ConcurrentHashMap<>(); + children = new IdentityHashMap<>(); subControllersAndSamplers = new ArrayList<>(); return this; diff --git a/src/core/src/main/java/org/apache/jmeter/gui/GuiPackage.java b/src/core/src/main/java/org/apache/jmeter/gui/GuiPackage.java index 2d3919cc88a..18e0a548e2b 100644 --- a/src/core/src/main/java/org/apache/jmeter/gui/GuiPackage.java +++ b/src/core/src/main/java/org/apache/jmeter/gui/GuiPackage.java @@ -24,6 +24,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.IdentityHashMap; import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; @@ -102,7 +103,7 @@ public final class GuiPackage implements LocaleChangeListener, HistoryListener { * to their corresponding GUI components. *

This enables to associate {@link UnsharedComponent} UIs with their {@link TestElement}.

*/ - private final Map nodesToGui = new HashMap<>(); + private final IdentityHashMap nodesToGui = new IdentityHashMap<>(); /** * Map from Class to JMeterGUIComponent, mapping the Class of a GUI diff --git a/src/core/src/main/java/org/apache/jmeter/testbeans/gui/TestBeanGUI.java b/src/core/src/main/java/org/apache/jmeter/testbeans/gui/TestBeanGUI.java index dfcce91dcb5..59c0fb4e9d9 100644 --- a/src/core/src/main/java/org/apache/jmeter/testbeans/gui/TestBeanGUI.java +++ b/src/core/src/main/java/org/apache/jmeter/testbeans/gui/TestBeanGUI.java @@ -112,7 +112,8 @@ public class TestBeanGUI extends AbstractJMeterGuiComponent implements JMeterGUI * large test plans. */ private final Cache customizers = - Caffeine.newBuilder() // TOOD: should this be made static? + Caffeine.newBuilder() // TODO: should this be made static? + .weakKeys() // So test elements are compared using identity == rather than .equals .maximumSize(20) .build(); diff --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 697d5e86701..3b756498970 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 @@ -122,14 +122,12 @@ public boolean equals(Object o) { } } - // TODO temporary hack to avoid unnecessary bug reports for subclasses - /** * {@inheritDoc} */ @Override - public int hashCode(){ - return System.identityHashCode(this); + public int hashCode() { + return propMap.hashCode(); } /* diff --git a/src/core/src/main/java/org/apache/jmeter/threads/AbstractThreadGroup.java b/src/core/src/main/java/org/apache/jmeter/threads/AbstractThreadGroup.java index ea7c9d20923..16773bb50b0 100644 --- a/src/core/src/main/java/org/apache/jmeter/threads/AbstractThreadGroup.java +++ b/src/core/src/main/java/org/apache/jmeter/threads/AbstractThreadGroup.java @@ -19,8 +19,7 @@ import java.io.Serializable; import java.time.Duration; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; +import java.util.IdentityHashMap; import java.util.concurrent.atomic.AtomicInteger; import org.apache.jmeter.control.Controller; @@ -51,7 +50,7 @@ public abstract class AbstractThreadGroup extends AbstractTestElement private static final long serialVersionUID = 240L; // Only create the map if it is required - private final transient ConcurrentMap children = new ConcurrentHashMap<>(); + private final transient IdentityHashMap children = new IdentityHashMap<>(); private static final Object DUMMY = new Object(); @@ -136,9 +135,11 @@ public void addTestElement(TestElement child) { */ @Override public final boolean addTestElementOnce(TestElement child){ - if (children.putIfAbsent(child, DUMMY) == null) { - addTestElement(child); - return true; + synchronized (children) { + if (children.putIfAbsent(child, DUMMY) == null) { + addTestElement(child); + return true; + } } return false; } diff --git a/src/core/src/main/java/org/apache/jmeter/threads/TestCompiler.java b/src/core/src/main/java/org/apache/jmeter/threads/TestCompiler.java index e0c22641cb2..c3cc460c319 100644 --- a/src/core/src/main/java/org/apache/jmeter/threads/TestCompiler.java +++ b/src/core/src/main/java/org/apache/jmeter/threads/TestCompiler.java @@ -18,12 +18,11 @@ package org.apache.jmeter.threads; import java.util.ArrayList; -import java.util.HashMap; import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.LinkedList; import java.util.List; import java.util.ListIterator; -import java.util.Map; import java.util.Set; import org.apache.jmeter.assertions.Assertion; @@ -69,10 +68,10 @@ public class TestCompiler implements HashTreeTraverser { // TODO: replace with ArrayDequeue private final LinkedList stack = new LinkedList<>(); - private final Map samplerConfigMap = new HashMap<>(); + private final IdentityHashMap samplerConfigMap = new IdentityHashMap<>(); - private final Map transactionControllerConfigMap = - new HashMap<>(); + private final IdentityHashMap transactionControllerConfigMap = + new IdentityHashMap<>(); private final HashTree testTree; diff --git a/src/core/src/test/kotlin/org/apache/jorphan/collections/SearchByClassTest.kt b/src/core/src/test/kotlin/org/apache/jorphan/collections/SearchByClassTest.kt new file mode 100644 index 00000000000..655808fd0ea --- /dev/null +++ b/src/core/src/test/kotlin/org/apache/jorphan/collections/SearchByClassTest.kt @@ -0,0 +1,41 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.jorphan.collections + +import org.apache.jmeter.threads.AbstractThreadGroup +import org.apache.jmeter.threads.ThreadGroup +import org.junit.jupiter.api.Assertions +import org.junit.jupiter.api.Test + +class SearchByClassTest { + @Test + fun `search finds all matching elements`() { + val tree = ListedHashTree() + val count = 100000 + for (i in 0 until count) { + tree.add(ThreadGroup()) + } + val searcher = SearchByClass( + AbstractThreadGroup::class.java + ) + tree.traverse(searcher) + Assertions.assertEquals(count, searcher.searchResults.size) { + "Test plan included $count ThreadGroup elements, so SearchByClass should find all of them" + } + } +} diff --git a/src/functions/src/test/groovy/org/apache/jmeter/functions/ChangeCaseSpec.groovy b/src/functions/src/test/groovy/org/apache/jmeter/functions/ChangeCaseSpec.groovy index 15624ae98ac..e86e5c9fb03 100644 --- a/src/functions/src/test/groovy/org/apache/jmeter/functions/ChangeCaseSpec.groovy +++ b/src/functions/src/test/groovy/org/apache/jmeter/functions/ChangeCaseSpec.groovy @@ -22,12 +22,15 @@ import org.apache.jmeter.samplers.SampleResult import org.apache.jmeter.threads.JMeterContextService import org.apache.jmeter.threads.JMeterVariables +import spock.lang.IgnoreIf import spock.lang.Specification import spock.lang.Unroll @Unroll class ChangeCaseSpec extends Specification { + // See https://github.com/apache/jmeter/issues/5723 + @IgnoreIf({ 'i'.toUpperCase() != 'I' || 'I'.toLowerCase() != 'i' }) def "convert '#input' using mode #mode to '#output'"() { given: def changeCase = new ChangeCase() diff --git a/src/jorphan/src/main/java/org/apache/jorphan/collections/HashTree.java b/src/jorphan/src/main/java/org/apache/jorphan/collections/HashTree.java index b3b56d56b6b..8d3879e7b0d 100644 --- a/src/jorphan/src/main/java/org/apache/jorphan/collections/HashTree.java +++ b/src/jorphan/src/main/java/org/apache/jorphan/collections/HashTree.java @@ -25,6 +25,7 @@ import java.util.Collection; import java.util.HashMap; import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.Map; import java.util.Set; @@ -54,7 +55,7 @@ public class HashTree implements Serializable, Map, Cloneable private static final String FOUND = "found"; // $NON-NLS-1$ // N.B. The keys can be either JMeterTreeNode or TestElement - protected final Map data; + protected final IdentityHashMap data; /** * Creates an empty new HashTree. @@ -78,7 +79,7 @@ protected HashTree(Map _map) { * name of the new top-level node */ public HashTree(Object key) { - this(new HashMap(), key); + this(new IdentityHashMap<>(), key); } /** @@ -94,9 +95,17 @@ public HashTree(Object key) { */ private HashTree(Map _map, Object key) { if(_map != null) { - data = _map; + if (_map instanceof IdentityHashMap) { + data = (IdentityHashMap) _map; + } else { + // Technically speaking, TestElements can't be placed in HashMapk keys, + // so we have to convert the map to an IdentityHashMap. + @SuppressWarnings("IdentityHashMapUsage") + IdentityHashMap identityMap = new IdentityHashMap<>(_map); + data = identityMap; + } } else { - data = new HashMap<>(); + data = new IdentityHashMap<>(); } if(key != null) { data.put(key, new HashTree()); @@ -213,7 +222,7 @@ public void add(HashTree newTree) { * a collection of objects to be added to the created HashTree. */ public HashTree(Collection keys) { - data = new HashMap<>(); + data = new IdentityHashMap<>(); for (Object o : keys) { data.put(o, new HashTree()); } @@ -227,7 +236,7 @@ public HashTree(Collection keys) { * array with names for the new top-level nodes */ public HashTree(Object[] keys) { - data = new HashMap<>(); + data = new IdentityHashMap<>(); for (Object key : keys) { data.put(key, new HashTree()); } @@ -873,7 +882,24 @@ protected HashTree getTreePath(Collection treePath) { */ @Override public int hashCode() { - return data.hashCode() * 7; + int xor = 0; + int sum = 0; + int mul = 0; + // Unordered hash function which uses reference equality for keys, and object equality for values + // We use several commutative functions to reduce the possibility of collisions. + // Note: IdentityHashMap.hashCode uses reference equality for both keys and values, so we can't use it. + for (Entry objectHashTreeEntry : data.entrySet()) { + int key = System.identityHashCode(objectHashTreeEntry.getKey()); + int value = objectHashTreeEntry.getValue().hashCode(); + xor = xor + key ^ value; + sum = sum + key + value; + mul = mul + (key|1) * (value|1); + } + int hash = 1; + hash = hash * 31 + xor; + hash = hash * 31 + sum; + hash = hash * 31 + mul; + return hash; } /** @@ -887,6 +913,9 @@ public int hashCode() { */ @Override public boolean equals(Object o) { + if (o == this) { + return true; + } if (!(o instanceof HashTree)) { return false; } @@ -894,7 +923,14 @@ public boolean equals(Object o) { if (oo.size() != this.size()) { return false; } - return data.equals(oo.data); + // data.equals(oo.data); uses reference identity for keys + for (Entry entry : data.entrySet()) { + HashTree otherValue = oo.data.get(entry.getKey()); + if (!entry.getValue().equals(otherValue)) { + return false; + } + } + return true; } /** diff --git a/src/jorphan/src/main/java/org/apache/jorphan/collections/ListedHashTree.java b/src/jorphan/src/main/java/org/apache/jorphan/collections/ListedHashTree.java index 1a7d58e34b9..8164b82b94e 100644 --- a/src/jorphan/src/main/java/org/apache/jorphan/collections/ListedHashTree.java +++ b/src/jorphan/src/main/java/org/apache/jorphan/collections/ListedHashTree.java @@ -175,7 +175,9 @@ public Collection list() { /** {@inheritDoc} */ @Override public HashTree remove(Object key) { - order.remove(key); + if (data.containsKey(key)) { + order.removeIf(x -> x == key); + } return data.remove(key); } @@ -190,7 +192,9 @@ public Object[] getArray() { @Override public int hashCode() { int hc = 17; - hc = hc * 37 + (order == null ? 0 : order.hashCode()); + for (Object o : order) { + hc = hc * 37 + System.identityHashCode(o); + } hc = hc * 37 + super.hashCode(); return hc; } @@ -202,7 +206,15 @@ public boolean equals(Object o) { return false; } ListedHashTree lht = (ListedHashTree) o; - return super.equals(lht) && order.equals(lht.order); + if (!super.equals(lht)) { + return false; + } + for (int i = 0; i < order.size(); i++) { + if (order.get(i) != lht.order.get(i)) { + return false; + } + } + return true; } diff --git a/src/jorphan/src/main/java/org/apache/jorphan/collections/SearchByClass.java b/src/jorphan/src/main/java/org/apache/jorphan/collections/SearchByClass.java index 22786324935..2d676239eb6 100644 --- a/src/jorphan/src/main/java/org/apache/jorphan/collections/SearchByClass.java +++ b/src/jorphan/src/main/java/org/apache/jorphan/collections/SearchByClass.java @@ -19,9 +19,8 @@ import java.util.ArrayList; import java.util.Collection; -import java.util.HashMap; +import java.util.IdentityHashMap; import java.util.List; -import java.util.Map; /** * Useful for finding all nodes in the tree that represent objects of a @@ -54,7 +53,7 @@ public class SearchByClass implements HashTreeTraverser { private final List objectsOfClass = new ArrayList<>(); - private final Map subTrees = new HashMap<>(); + private final IdentityHashMap subTrees = new IdentityHashMap<>(); private final Class searchClass; diff --git a/src/jorphan/src/main/kotlin/org/apache/jorphan/collections/IdentityKey.kt b/src/jorphan/src/main/kotlin/org/apache/jorphan/collections/IdentityKey.kt new file mode 100644 index 00000000000..05bb891d0ea --- /dev/null +++ b/src/jorphan/src/main/kotlin/org/apache/jorphan/collections/IdentityKey.kt @@ -0,0 +1,38 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.jorphan.collections + +import java.util.concurrent.ConcurrentHashMap + +/** + * Wraps an object so [equals] and [hashCode] compare object identities. + * One of the common usages is to create [ConcurrentHashMap] where + * keys are distinguished by their identities instead of [Object.equals]. + */ +public class IdentityKey( + public val value: T +) { + override fun equals(other: Any?): Boolean = + other is IdentityKey<*> && other.value === value + + override fun hashCode(): Int = + System.identityHashCode(value) + + override fun toString(): String = + "IdentityKey{$value}" +} diff --git a/src/jorphan/src/test/kotlin/org/apache/jorphan/collections/IdentityKeyTest.kt b/src/jorphan/src/test/kotlin/org/apache/jorphan/collections/IdentityKeyTest.kt new file mode 100644 index 00000000000..619ddabe29c --- /dev/null +++ b/src/jorphan/src/test/kotlin/org/apache/jorphan/collections/IdentityKeyTest.kt @@ -0,0 +1,55 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.jorphan.collections + +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.assertAll +import java.util.concurrent.ConcurrentHashMap + +class IdentityKeyTest { + data class Key(val value: Int) + + @Test + fun `concurrentHashMap with same keys`() { + val map = ConcurrentHashMap, Int>() + val k1 = IdentityKey(Key(1)) + map[k1] = 1 + val k2 = IdentityKey(Key(1)) + map[k2] = 2 + val kNull = IdentityKey(null) + map[kNull] = 3 + assertAll( + { + assertEquals(3, map.size) { + "Map should contain 3 elements, actual map is $map" + } + }, + { assertEquals(1, map[k1], "map[Key(1)]") }, + { assertEquals(2, map[k2], "map[Key(1)]") }, + { assertEquals(3, map[kNull], "map[null]") }, + ) + } + + @Test + fun `toString test`() { + assertEquals("IdentityKey{contents}", IdentityKey("contents").toString()) { + """IdentityKey("contents").toString()""" + } + } +} diff --git a/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/util/accesslog/SessionFilter.java b/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/util/accesslog/SessionFilter.java index 6ffef587c35..e7ed3b6d5ec 100644 --- a/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/util/accesslog/SessionFilter.java +++ b/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/util/accesslog/SessionFilter.java @@ -19,7 +19,7 @@ import java.io.Serializable; import java.util.Collections; -import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -69,7 +69,7 @@ public class SessionFilter implements Filter, Serializable, TestCloneable,Thread * Creates a new SessionFilter and initializes its fields to new collections */ public SessionFilter() { - this(new ConcurrentHashMap<>(), Collections.synchronizedSet(new HashSet<>())); + this(new ConcurrentHashMap<>(), Collections.synchronizedSet(Collections.newSetFromMap(new IdentityHashMap<>()))); } /** diff --git a/src/protocol/http/src/test/java/org/apache/jmeter/protocol/http/util/accesslog/TestSessionFilter.java b/src/protocol/http/src/test/java/org/apache/jmeter/protocol/http/util/accesslog/TestSessionFilter.java index 04b852ca4b4..48b3849bd5a 100644 --- a/src/protocol/http/src/test/java/org/apache/jmeter/protocol/http/util/accesslog/TestSessionFilter.java +++ b/src/protocol/http/src/test/java/org/apache/jmeter/protocol/http/util/accesslog/TestSessionFilter.java @@ -21,7 +21,7 @@ import static org.junit.Assert.assertTrue; import java.util.Collections; -import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -66,7 +66,7 @@ public void testGetCookieManagerLastUse() { public void testIsFiltered() throws Exception { Map cm = new ConcurrentHashMap<>(); Set inUse = Collections - .synchronizedSet(new HashSet()); + .synchronizedSet(Collections.newSetFromMap(new IdentityHashMap<>())); SessionFilter filter = new SessionFilter(cm, inUse); HTTPSampler sampler = new HTTPSampler(); filter.isFiltered("1.2.3.4 ...", sampler); diff --git a/src/protocol/java/src/main/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java b/src/protocol/java/src/main/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java index 0d8534bebb3..630c28168f5 100644 --- a/src/protocol/java/src/main/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java +++ b/src/protocol/java/src/main/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java @@ -19,7 +19,6 @@ import java.lang.reflect.Method; import java.util.Arrays; -import java.util.Collections; import java.util.HashSet; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -33,6 +32,7 @@ import org.apache.jmeter.testelement.TestElement; import org.apache.jmeter.testelement.TestStateListener; import org.apache.jmeter.testelement.property.TestElementProperty; +import org.apache.jorphan.collections.IdentityKey; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -58,8 +58,8 @@ public class JavaSampler extends AbstractSampler implements TestStateListener, I * Set used to register instances which implement tearDownTest. * This is used so that the JavaSamplerClient can be notified when the test ends. */ - private static final Set TEAR_DOWN_SET = - Collections.newSetFromMap(new ConcurrentHashMap()); + private static final Set> TEAR_DOWN_SET = + ConcurrentHashMap.newKeySet(); /** * Property key representing the classname of the JavaSamplerClient to user. @@ -226,7 +226,7 @@ private JavaSamplerClient createJavaClient() { } if(isToBeRegistered) { - TEAR_DOWN_SET.add(this); + TEAR_DOWN_SET.add(new IdentityKey<>(this)); } } catch (Exception e) { if (log.isErrorEnabled()) { @@ -292,7 +292,8 @@ public void testEnded() { log.debug("{}\ttestEnded", whoAmI()); } synchronized (TEAR_DOWN_SET) { - for (JavaSampler javaSampler : TEAR_DOWN_SET) { + for (IdentityKey key : TEAR_DOWN_SET) { + JavaSampler javaSampler = key.getValue(); JavaSamplerClient client = javaSampler.javaClient; if (client != null) { client.teardownTest(javaSampler.context); diff --git a/xdocs/changes.xml b/xdocs/changes.xml index 449fa8f4818..ce782ec80be 100644 --- a/xdocs/changes.xml +++ b/xdocs/changes.xml @@ -130,6 +130,7 @@ Summary
  • 5763Updated Gradle to 7.3 (from 7.2)
  • Update lets-plot-jvm to 4.3.0 (from 3.1.1)
  • Update lets-plot-batik to 3.1.0 (from 2.1.1)
    • +
  • Added randomized test GitHub Actions matrix for better coverage of locales and time zones
    • @@ -190,6 +191,7 @@ Summary
    • 66157719Correct theme for darklaf on rsyntaxtextarea
    • 58725874Trim name in Argument objects.
      • +
    • 693Avoid wrong results when Object.hashCode() happen to collide. Use IdentityHashMap instead of HashMap when key is TestElement