Skip to content

Commit ee4081b

Browse files
committed
Make jmethodID-churn race-detection check a skip, not a fail
Fixes review comment: the counter-delta check is best-effort — a healthy host that doesn't race unload tightly enough shouldn't flake CI. Switch it to Assumptions.assumeTrue so a missed race reports as skipped rather than failed.
1 parent 07fe68f commit ee4081b

1 file changed

Lines changed: 16 additions & 7 deletions

File tree

ddprof-test/src/test/java/com/datadoghq/profiler/memleak/JMethodIDInvalidationStressTest.java

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
*/
1616
package com.datadoghq.profiler.memleak;
1717

18+
import org.junit.jupiter.api.Assumptions;
1819
import org.junit.jupiter.api.Test;
1920
import org.junit.jupiter.api.Timeout;
2021
import org.objectweb.asm.ClassWriter;
@@ -66,9 +67,13 @@
6667
* native whitebox counters ({@code JavaProfiler#getDebugCounters()}) for {@code
6768
* jmethodid_skipped_count} and {@code line_number_table_unreadable} -- both are incremented in
6869
* {@code fillJavaMethodInfo} exactly when {@code SafeAccess::isReadableRange} rejects a stale
69-
* jmethodID's class/method metadata or line-number table (see flightRecorder.cpp) -- and asserts
70-
* at least one of them increased during the churn window. A pass therefore means both "no crash"
71-
* and "a stale jmethodID was actually observed and safely guarded." We cannot run this under ASan
70+
* jmethodID's class/method metadata or line-number table (see flightRecorder.cpp) -- and checks
71+
* that at least one of them increased during the churn window. Whether the race is actually hit
72+
* within the window is JVM/host-discretionary, so that check is a JUnit assumption rather than an
73+
* assertion: if neither counter moved, the test is reported as skipped (not failed), since a
74+
* healthy host that simply didn't race tightly enough this run is not evidence of a regression. A
75+
* pass therefore means both "no crash" and "a stale jmethodID was actually observed and safely
76+
* guarded"; a skip means only the former was exercised. We cannot run this under ASan
7277
* here, since the profiler under test must run as a live JVMTI agent inside the same JVM process
7378
* the test is driving, not as a standalone ASan-instrumented binary the way the gtest-level native
7479
* unit tests can.
@@ -169,14 +174,18 @@ public void testProfilerSurvivesConcurrentClassUnloadDuringDump() throws Excepti
169174
long unreadableLineTableDelta = after.getOrDefault("line_number_table_unreadable", 0L)
170175
- before.getOrDefault("line_number_table_unreadable", 0L);
171176

172-
assertTrue(skippedDelta > 0 || unreadableLineTableDelta > 0,
177+
// Whether the stale-jmethodID race actually gets hit within the churn window is
178+
// JVM/host-discretionary (see class javadoc); treat "never observed" as an aborted
179+
// run rather than a failure so a healthy host that just didn't race tightly enough
180+
// doesn't flake CI.
181+
Assumptions.assumeTrue(skippedDelta > 0 || unreadableLineTableDelta > 0,
173182
"Churn window completed without the profiler ever encountering a stale/unmapped "
174183
+ "jmethodID (jmethodid_skipped_count delta=" + skippedDelta
175184
+ ", line_number_table_unreadable delta=" + unreadableLineTableDelta
176185
+ ") -- this test is meant to prove the guard actually fires under class-unload "
177-
+ "churn, not just that the JVM didn't crash; if this keeps failing, the churn "
178-
+ "isn't racing unload against resolveMethod/fillJavaMethodInfo tightly enough "
179-
+ "(consider more churn threads or a longer window).");
186+
+ "churn, not just that the JVM didn't crash; if this keeps getting skipped, the "
187+
+ "churn isn't racing unload against resolveMethod/fillJavaMethodInfo tightly "
188+
+ "enough (consider more churn threads or a longer window).");
180189
} finally {
181190
running.set(false);
182191
for (Thread t : churnThreads) {

0 commit comments

Comments
 (0)