Skip to content

Commit 0e15d6c

Browse files
bm1549claude
bm1549
and
claude
committed
Fix DDTraceId/DD64bTraceId class-init deadlock at the source
DD64bTraceId is a subclass of DDTraceId, so the JVM must initialize DDTraceId before DD64bTraceId. DDTraceId.<clinit> in turn initialized DD64bTraceId by building its ZERO/ONE constants via DD64bTraceId.from(), a circular initialization dependency. When the two classes were first touched concurrently from opposite ends -- the service-discovery task (muteTracing() -> blackholeSpan() -> DDTraceId.ZERO) racing the application's first span (IdGenerationStrategy.generateTraceId() -> DD64bTraceId.from()) -- each thread held one class-init lock and waited for the other, hanging trace creation. This surfaced as recurring 30s LogInjectionSmokeTest timeouts in CI (latent since #9705 added the scheduled muteTracing task). Break the cycle at its source while keeping DDTraceId.ZERO/ONE as public fields (preserving the API restored in #5021): ZERO/ONE are now instances of a private DDTraceId subtype (a sibling of DD64bTraceId), so DDTraceId.<clinit> no longer references the subclass. Replace the fragile "== DDTraceId.ZERO" identity checks with a value-based DDTraceId.isZero(). Those identity checks relied on every zero id being the single ZERO instance; isZero() recognizes a zero id of any concrete type, so the factories need not route 0 to the singleton and the propagation codecs no longer mishandle a zero parsed via the direct 64-bit factories. Add a forked regression test that initializes the two classes concurrently from opposite ends (deadlocks without the fix), plus isZero() coverage across the DDTraceId subtypes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 2bc2fc4 commit 0e15d6c

13 files changed

Lines changed: 271 additions & 20 deletions

File tree

‎dd-java-agent/instrumentation/spark/spark-common/src/main/java/datadog/trace/instrumentation/spark/AbstractDatadogSparkListener.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -444,7 +444,7 @@ private void addDatabricksSpecificTags(
444444
AgentSpanContext parentContext =
445445
new DatabricksParentContext(databricksJobId, databricksJobRunId, databricksTaskRunId);
446446

447-
if (parentContext.getTraceId() != DDTraceId.ZERO) {
447+
if (!parentContext.getTraceId().isZero()) {
448448
if (withParentContext) {
449449
builder.asChildOf(parentContext);
450450
} else {

‎dd-trace-api/build.gradle.kts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ val excludedClassesCoverage by extra(
1616
"datadog.trace.api.DDTags",
1717
"datadog.trace.api.DDTraceApiInfo",
1818
"datadog.trace.api.DDTraceId",
19+
"datadog.trace.api.DDTraceId.ConstantId",
1920
"datadog.trace.api.EventTracker",
2021
"datadog.trace.api.GlobalTracer*",
2122
"datadog.trace.api.PropagationStyle",

‎dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,10 @@ public static DD64bTraceId fromHex(String s) throws NumberFormatException {
5959
}
6060

6161
static DD64bTraceId create(long id, String str) {
62-
// ZERO constant is created and stored by the parent class as part of its API contract
63-
// But initialized by this 64-bit child class. Ensures uniqueness of ZERO once created.
64-
if (id == 0 && ZERO != null) {
65-
return (DD64bTraceId) ZERO;
66-
} else if (id == -1) {
62+
// -1 (all bits set) reuses the MAX singleton. 0 is not special-cased: DDTraceId.ZERO is a
63+
// sibling type (not a DD64bTraceId), so it cannot be returned here; a zero 64-bit id is simply
64+
// a DD64bTraceId(0), and callers detect zero via DDTraceId.isZero() rather than by identity.
65+
if (id == -1) {
6766
return MAX;
6867
} else {
6968
return new DD64bTraceId(id, str);

‎dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java‎

Lines changed: 84 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,25 @@
99
* The string representations are either kept from parsing, or generated on demand and cached.
1010
*/
1111
public abstract class DDTraceId {
12-
/** Invalid TraceId value used to denote no TraceId. */
13-
public static final DDTraceId ZERO = from(0);
12+
/**
13+
* Invalid TraceId value used to denote no TraceId.
14+
*
15+
* <p>This is an instance of a private {@link DDTraceId} subtype (a sibling of {@link
16+
* DD64bTraceId}), not a {@code DD64bTraceId}, and that is deliberate. {@code DD64bTraceId} is a
17+
* subclass of {@code DDTraceId}, so the JVM must initialize {@code DDTraceId} before {@code
18+
* DD64bTraceId}. If this constant were built via {@code DD64bTraceId.from(0)} (as it once was),
19+
* {@code DDTraceId.<clinit>} would initialize {@code DD64bTraceId} while holding the {@code
20+
* DDTraceId} init lock, and two threads first touching the classes from opposite ends would
21+
* deadlock on the two class-initialization locks. Building it from a sibling type keeps {@code
22+
* DDTraceId.<clinit>} free of any reference to the subclass.
23+
*
24+
* <p>To test whether an id is zero, prefer {@link #isZero()} over {@code == DDTraceId.ZERO}: a
25+
* zero id parsed via the 64-bit factories is a distinct instance, not this singleton.
26+
*/
27+
public static final DDTraceId ZERO = new ConstantId(0, "0");
1428

15-
/** Convenience constant used from tests */
16-
public static final DDTraceId ONE = from(1);
29+
/** Convenience constant used from tests. See {@link #ZERO} for why this is a sibling type. */
30+
public static final DDTraceId ONE = new ConstantId(1, "1");
1731

1832
/**
1933
* Creates a new {@link DD64bTraceId 64-bit TraceId} from the given {@code long} interpreted as
@@ -102,4 +116,70 @@ public static DDTraceId fromHex(String s) throws NumberFormatException {
102116
* <code>0</code> for 64-bit {@link DDTraceId} only.
103117
*/
104118
public abstract long toHighOrderLong();
119+
120+
/**
121+
* Returns whether this {@link DDTraceId} is zero, the value used to denote no/invalid TraceId.
122+
*
123+
* <p>Prefer this over {@code == DDTraceId.ZERO}: it is value-based, so it recognizes a zero id
124+
* regardless of its concrete type or how it was created (e.g. a zero parsed via the 64-bit
125+
* factories, which is a distinct instance from the {@link #ZERO} singleton).
126+
*
127+
* @return {@code true} if both the high- and low-order 64 bits are zero.
128+
*/
129+
public boolean isZero() {
130+
return toHighOrderLong() == 0 && toLong() == 0;
131+
}
132+
133+
/**
134+
* Minimal concrete {@link DDTraceId} backing the {@link #ZERO} and {@link #ONE} constants. It is
135+
* a sibling of {@link DD64bTraceId} (it extends {@link DDTraceId} directly), so constructing
136+
* these constants in {@code DDTraceId.<clinit>} never initializes {@code DD64bTraceId} (which
137+
* would create a class-initialization deadlock; see {@link #ZERO}). It represents a 64-bit id and
138+
* formats identically to the equivalent {@link DD64bTraceId}.
139+
*/
140+
private static final class ConstantId extends DDTraceId {
141+
private final long id;
142+
private final String str;
143+
private String hexStr; // cache for hex string representation
144+
145+
private ConstantId(long id, String str) {
146+
this.id = id;
147+
this.str = str;
148+
}
149+
150+
@Override
151+
public String toString() {
152+
return this.str;
153+
}
154+
155+
@Override
156+
public String toHexString() {
157+
String hexStr = this.hexStr;
158+
// This race condition is intentional and benign.
159+
// The worst that can happen is that an identical value is produced and written into the
160+
// field.
161+
if (hexStr == null) {
162+
this.hexStr = hexStr = LongStringUtils.toHexStringPadded(this.id, 32);
163+
}
164+
return hexStr;
165+
}
166+
167+
@Override
168+
public String toHexStringPadded(int size) {
169+
if (size > 16) {
170+
return toHexString();
171+
}
172+
return LongStringUtils.toHexStringPadded(this.id, size);
173+
}
174+
175+
@Override
176+
public long toLong() {
177+
return this.id;
178+
}
179+
180+
@Override
181+
public long toHighOrderLong() {
182+
return 0;
183+
}
184+
}
105185
}

‎dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdClinitDeadlockForkedTest.java‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
package datadog.trace.api;
2+
3+
import static java.util.concurrent.TimeUnit.SECONDS;
4+
import static org.junit.jupiter.api.Assertions.fail;
5+
6+
import java.util.concurrent.CyclicBarrier;
7+
import java.util.concurrent.atomic.AtomicReference;
8+
import org.junit.jupiter.api.Test;
9+
import org.junit.jupiter.api.Timeout;
10+
11+
/**
12+
* Regression test for the {@code DDTraceId} &lt;-&gt; {@code DD64bTraceId} class-initialization
13+
* deadlock.
14+
*
15+
* <p>{@code DD64bTraceId} is a subclass of {@code DDTraceId}, so the JVM must initialize {@code
16+
* DDTraceId} before {@code DD64bTraceId}. The bug was that {@code DDTraceId.<clinit>} in turn
17+
* initialized {@code DD64bTraceId} by building its {@code ZERO}/{@code ONE} constants via {@code
18+
* DD64bTraceId.from(...)}. When the two classes were first touched concurrently from opposite ends
19+
* (one thread initializing {@code DDTraceId}, another initializing {@code DD64bTraceId}), each
20+
* thread held one class-initialization lock and waited for the other, hanging trace creation. This
21+
* surfaced as 30s {@code LogInjectionSmokeTest} timeouts in CI.
22+
*
23+
* <p>{@code DDTraceId.ZERO}/{@code ONE} are now instances of a private sibling type (not {@code
24+
* DD64bTraceId}), so {@code DDTraceId.<clinit>} no longer references {@code DD64bTraceId} and the
25+
* cycle is gone. This test initializes the two classes for the first time concurrently from
26+
* opposite ends and asserts neither thread hangs.
27+
*
28+
* <p>Runs forked ({@code forkEvery = 1}) so it gets a fresh JVM in which these classes have not yet
29+
* been initialized by another test. Without the fix it deadlocks and fails via the join check (and
30+
* the {@code @Timeout} backstop); with the fix it completes immediately.
31+
*/
32+
class DDTraceIdClinitDeadlockForkedTest {
33+
34+
@Test
35+
@Timeout(value = 60, unit = SECONDS) // backstop; the join below is the primary guard
36+
void traceIdClassPairInitializesConcurrentlyWithoutDeadlock() throws Exception {
37+
final ClassLoader cl = getClass().getClassLoader();
38+
final CyclicBarrier barrier = new CyclicBarrier(2);
39+
final AtomicReference<Throwable> error = new AtomicReference<>();
40+
41+
// One thread enters via the superclass (mirrors blackholeSpan() -> DDTraceId.ZERO), the other
42+
// via the subclass (mirrors IdGenerationStrategy.generateTraceId() -> DD64bTraceId.from()).
43+
Thread viaSuper =
44+
new Thread(
45+
() -> {
46+
try {
47+
barrier.await();
48+
Class.forName("datadog.trace.api.DDTraceId", true, cl);
49+
} catch (Throwable t) {
50+
error.compareAndSet(null, t);
51+
}
52+
},
53+
"init-DDTraceId");
54+
Thread viaSub =
55+
new Thread(
56+
() -> {
57+
try {
58+
barrier.await();
59+
Class.forName("datadog.trace.api.DD64bTraceId", true, cl);
60+
} catch (Throwable t) {
61+
error.compareAndSet(null, t);
62+
}
63+
},
64+
"init-DD64bTraceId");
65+
// Daemon so a deadlock cannot block forked-JVM shutdown.
66+
viaSuper.setDaemon(true);
67+
viaSub.setDaemon(true);
68+
69+
viaSuper.start();
70+
viaSub.start();
71+
viaSuper.join(SECONDS.toMillis(15));
72+
viaSub.join(SECONDS.toMillis(15));
73+
74+
if (viaSuper.isAlive() || viaSub.isAlive()) {
75+
fail(
76+
"DDTraceId/DD64bTraceId class-initialization deadlock: DDTraceId.<clinit> must not "
77+
+ "reference DD64bTraceId (init-DDTraceId.alive="
78+
+ viaSuper.isAlive()
79+
+ ", init-DD64bTraceId.alive="
80+
+ viaSub.isAlive()
81+
+ ").");
82+
}
83+
if (error.get() != null) {
84+
throw new AssertionError(
85+
"Unexpected error during concurrent class initialization", error.get());
86+
}
87+
}
88+
}

‎dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdConstantsTest.java‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
package datadog.trace.api;
2+
3+
import static org.junit.jupiter.api.Assertions.assertEquals;
4+
import static org.junit.jupiter.api.Assertions.assertFalse;
5+
import static org.junit.jupiter.api.Assertions.assertTrue;
6+
7+
import org.junit.jupiter.api.Test;
8+
9+
/** Behavior of the {@link DDTraceId#ZERO}/{@link DDTraceId#ONE} sibling constants. */
10+
class DDTraceIdConstantsTest {
11+
12+
@Test
13+
void zeroAndOneFormatLikeTheEquivalentDD64bTraceId() {
14+
assertEquals("0", DDTraceId.ZERO.toString());
15+
assertEquals("00000000000000000000000000000000", DDTraceId.ZERO.toHexString());
16+
assertEquals("0000000000000000", DDTraceId.ZERO.toHexStringPadded(16));
17+
assertEquals("00000000000000000000000000000000", DDTraceId.ZERO.toHexStringPadded(32));
18+
assertEquals(0L, DDTraceId.ZERO.toLong());
19+
assertEquals(0L, DDTraceId.ZERO.toHighOrderLong());
20+
21+
assertEquals("1", DDTraceId.ONE.toString());
22+
assertEquals("00000000000000000000000000000001", DDTraceId.ONE.toHexString());
23+
assertEquals("0000000000000001", DDTraceId.ONE.toHexStringPadded(16));
24+
assertEquals(1L, DDTraceId.ONE.toLong());
25+
assertEquals(0L, DDTraceId.ONE.toHighOrderLong());
26+
}
27+
28+
@Test
29+
void isZeroReflectsTheValue() {
30+
assertTrue(DDTraceId.ZERO.isZero());
31+
assertTrue(DDTraceId.from(0).isZero());
32+
assertTrue(DDTraceId.from("0").isZero());
33+
assertTrue(DDTraceId.fromHex("0").isZero());
34+
assertTrue(DD64bTraceId.from(0).isZero());
35+
36+
assertFalse(DDTraceId.ONE.isZero());
37+
assertFalse(DDTraceId.from(1).isZero());
38+
assertFalse(DD64bTraceId.from(42).isZero());
39+
}
40+
}

‎dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@
22

33
import static org.junit.jupiter.api.Assertions.assertEquals;
44
import static org.junit.jupiter.api.Assertions.assertNotNull;
5-
import static org.junit.jupiter.api.Assertions.assertSame;
65
import static org.junit.jupiter.api.Assertions.assertThrows;
6+
import static org.junit.jupiter.api.Assertions.assertTrue;
77

88
import org.junit.jupiter.api.Test;
99
import org.junit.jupiter.params.ParameterizedTest;
@@ -175,10 +175,11 @@ void failParsingIllegal128BitIdHexadecimalStringRepresentationFromPartialString(
175175
@Test
176176
void checkZeroConstantInitialization() {
177177
DDTraceId zero = DDTraceId.ZERO;
178-
DDTraceId fromZero = DDTraceId.from(0);
179178

180179
assertNotNull(zero);
181-
assertSame(fromZero, zero);
180+
assertTrue(zero.isZero());
181+
// from(0) is a zero-valued id but, by design, no longer the ZERO singleton; use isZero().
182+
assertTrue(DDTraceId.from(0).isZero());
182183
}
183184

184185
private static String leftPadWithZeros(String value, int size) {

‎dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2025,7 +2025,7 @@ protected static final DDSpanContext buildSpanContext(
20252025
propagationTags = extractedContext.getPropagationTags();
20262026
} else if (resolvedParentContext != null) {
20272027
traceId =
2028-
resolvedParentContext.getTraceId() == DDTraceId.ZERO
2028+
resolvedParentContext.getTraceId().isZero()
20292029
? tracer.idGenerationStrategy.generateTraceId()
20302030
: resolvedParentContext.getTraceId();
20312031
parentSpanId = resolvedParentContext.getSpanId();

‎dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,7 @@ public ContextInterpreter reset(TraceConfig traceConfig) {
250250

251251
protected TagContext build() {
252252
if (valid) {
253-
if (fullContext && !DDTraceId.ZERO.equals(traceId)) {
253+
if (fullContext && !traceId.isZero()) {
254254
if (propagationTags == null) {
255255
propagationTags = propagationTagsFactory.empty();
256256
}
@@ -305,7 +305,7 @@ private TagContext.HttpHeaders getHeaders() {
305305
}
306306

307307
private int samplingPriorityOrDefault(DDTraceId traceId, int samplingPriority) {
308-
return samplingPriority == PrioritySampling.UNSET || DDTraceId.ZERO.equals(traceId)
308+
return samplingPriority == PrioritySampling.UNSET || traceId.isZero()
309309
? defaultSamplingPriority()
310310
: samplingPriority;
311311
}

‎dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ private long extractEndToEndStartTime(String value) {
234234
private void restore128bTraceId() {
235235
long highOrderBits;
236236
// Check if the low-order 64 bits of the TraceId, and propagation tags were parsed
237-
if (traceId != DDTraceId.ZERO
237+
if (!traceId.isZero()
238238
&& propagationTags != null
239239
&& (highOrderBits = propagationTags.getTraceIdHighOrderBits()) != 0) {
240240
// Restore the 128-bit TraceId

0 commit comments

Comments
 (0)