diff --git a/dd-java-agent/instrumentation/spark/spark-common/src/main/java/datadog/trace/instrumentation/spark/AbstractDatadogSparkListener.java b/dd-java-agent/instrumentation/spark/spark-common/src/main/java/datadog/trace/instrumentation/spark/AbstractDatadogSparkListener.java index 817aa1f0d84..c1826f9e548 100644 --- a/dd-java-agent/instrumentation/spark/spark-common/src/main/java/datadog/trace/instrumentation/spark/AbstractDatadogSparkListener.java +++ b/dd-java-agent/instrumentation/spark/spark-common/src/main/java/datadog/trace/instrumentation/spark/AbstractDatadogSparkListener.java @@ -444,7 +444,7 @@ private void addDatabricksSpecificTags( AgentSpanContext parentContext = new DatabricksParentContext(databricksJobId, databricksJobRunId, databricksTaskRunId); - if (parentContext.getTraceId() != DDTraceId.ZERO) { + if (!parentContext.getTraceId().isZero()) { if (withParentContext) { builder.asChildOf(parentContext); } else { diff --git a/dd-trace-api/build.gradle.kts b/dd-trace-api/build.gradle.kts index bc05a8753a4..fc4924ba271 100644 --- a/dd-trace-api/build.gradle.kts +++ b/dd-trace-api/build.gradle.kts @@ -16,6 +16,7 @@ val excludedClassesCoverage by extra( "datadog.trace.api.DDTags", "datadog.trace.api.DDTraceApiInfo", "datadog.trace.api.DDTraceId", + "datadog.trace.api.DDTraceId.ConstantId", "datadog.trace.api.EventTracker", "datadog.trace.api.GlobalTracer*", "datadog.trace.api.PropagationStyle", diff --git a/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java b/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java index f40b47a89d4..958e45f93f9 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java @@ -59,11 +59,10 @@ public static DD64bTraceId fromHex(String s) throws NumberFormatException { } static DD64bTraceId create(long id, String str) { - // ZERO constant is created and stored by the parent class as part of its API contract - // But initialized by this 64-bit child class. Ensures uniqueness of ZERO once created. - if (id == 0 && ZERO != null) { - return (DD64bTraceId) ZERO; - } else if (id == -1) { + // -1 (all bits set) reuses the MAX singleton. 0 is not special-cased: DDTraceId.ZERO is a + // sibling type (not a DD64bTraceId), so it cannot be returned here; a zero 64-bit id is simply + // a DD64bTraceId(0), and callers detect zero via DDTraceId.isZero() rather than by identity. + if (id == -1) { return MAX; } else { return new DD64bTraceId(id, str); diff --git a/dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java b/dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java index 7ed5be915cc..03ec9e2538a 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java @@ -9,11 +9,25 @@ * The string representations are either kept from parsing, or generated on demand and cached. */ public abstract class DDTraceId { - /** Invalid TraceId value used to denote no TraceId. */ - public static final DDTraceId ZERO = from(0); + /** + * Invalid TraceId value used to denote no TraceId. + * + *

This is an instance of a private {@link DDTraceId} subtype (a sibling of {@link + * DD64bTraceId}), not a {@code DD64bTraceId}, and that is deliberate. {@code DD64bTraceId} is a + * subclass of {@code DDTraceId}, so the JVM must initialize {@code DDTraceId} before {@code + * DD64bTraceId}. If this constant were built via {@code DD64bTraceId.from(0)} (as it once was), + * {@code DDTraceId.} would initialize {@code DD64bTraceId} while holding the {@code + * DDTraceId} init lock, and two threads first touching the classes from opposite ends would + * deadlock on the two class-initialization locks. Building it from a sibling type keeps {@code + * DDTraceId.} free of any reference to the subclass. + * + *

To test whether an id is zero, prefer {@link #isZero()} over {@code == DDTraceId.ZERO}: a + * zero id parsed via the 64-bit factories is a distinct instance, not this singleton. + */ + public static final DDTraceId ZERO = new ConstantId(0, "0"); - /** Convenience constant used from tests */ - public static final DDTraceId ONE = from(1); + /** Convenience constant used from tests. See {@link #ZERO} for why this is a sibling type. */ + public static final DDTraceId ONE = new ConstantId(1, "1"); /** * 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 { * 0 for 64-bit {@link DDTraceId} only. */ public abstract long toHighOrderLong(); + + /** + * Returns whether this {@link DDTraceId} is zero, the value used to denote no/invalid TraceId. + * + *

Prefer this over {@code == DDTraceId.ZERO}: it is value-based, so it recognizes a zero id + * regardless of its concrete type or how it was created (e.g. a zero parsed via the 64-bit + * factories, which is a distinct instance from the {@link #ZERO} singleton). + * + * @return {@code true} if both the high- and low-order 64 bits are zero. + */ + public boolean isZero() { + return toHighOrderLong() == 0 && toLong() == 0; + } + + /** + * Minimal concrete {@link DDTraceId} backing the {@link #ZERO} and {@link #ONE} constants. It is + * a sibling of {@link DD64bTraceId} (it extends {@link DDTraceId} directly), so constructing + * these constants in {@code DDTraceId.} never initializes {@code DD64bTraceId} (which + * would create a class-initialization deadlock; see {@link #ZERO}). It represents a 64-bit id and + * formats identically to the equivalent {@link DD64bTraceId}. + */ + private static final class ConstantId extends DDTraceId { + private final long id; + private final String str; + private String hexStr; // cache for hex string representation + + private ConstantId(long id, String str) { + this.id = id; + this.str = str; + } + + @Override + public String toString() { + return this.str; + } + + @Override + public String toHexString() { + String hexStr = this.hexStr; + // This race condition is intentional and benign. + // The worst that can happen is that an identical value is produced and written into the + // field. + if (hexStr == null) { + this.hexStr = hexStr = LongStringUtils.toHexStringPadded(this.id, 32); + } + return hexStr; + } + + @Override + public String toHexStringPadded(int size) { + if (size > 16) { + return toHexString(); + } + return LongStringUtils.toHexStringPadded(this.id, size); + } + + @Override + public long toLong() { + return this.id; + } + + @Override + public long toHighOrderLong() { + return 0; + } + } } diff --git a/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdClinitDeadlockForkedTest.java b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdClinitDeadlockForkedTest.java new file mode 100644 index 00000000000..e838ef75177 --- /dev/null +++ b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdClinitDeadlockForkedTest.java @@ -0,0 +1,88 @@ +package datadog.trace.api; + +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.jupiter.api.Assertions.fail; + +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.atomic.AtomicReference; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +/** + * Regression test for the {@code DDTraceId} <-> {@code DD64bTraceId} class-initialization + * deadlock. + * + *

{@code DD64bTraceId} is a subclass of {@code DDTraceId}, so the JVM must initialize {@code + * DDTraceId} before {@code DD64bTraceId}. The bug was that {@code DDTraceId.} in turn + * initialized {@code DD64bTraceId} by building its {@code ZERO}/{@code ONE} constants via {@code + * DD64bTraceId.from(...)}. When the two classes were first touched concurrently from opposite ends + * (one thread initializing {@code DDTraceId}, another initializing {@code DD64bTraceId}), each + * thread held one class-initialization lock and waited for the other, hanging trace creation. This + * surfaced as 30s {@code LogInjectionSmokeTest} timeouts in CI. + * + *

{@code DDTraceId.ZERO}/{@code ONE} are now instances of a private sibling type (not {@code + * DD64bTraceId}), so {@code DDTraceId.} no longer references {@code DD64bTraceId} and the + * cycle is gone. This test initializes the two classes for the first time concurrently from + * opposite ends and asserts neither thread hangs. + * + *

Runs forked ({@code forkEvery = 1}) so it gets a fresh JVM in which these classes have not yet + * been initialized by another test. Without the fix it deadlocks and fails via the join check (and + * the {@code @Timeout} backstop); with the fix it completes immediately. + */ +class DDTraceIdClinitDeadlockForkedTest { + + @Test + @Timeout(value = 60, unit = SECONDS) // backstop; the join below is the primary guard + void traceIdClassPairInitializesConcurrentlyWithoutDeadlock() throws Exception { + final ClassLoader cl = getClass().getClassLoader(); + final CyclicBarrier barrier = new CyclicBarrier(2); + final AtomicReference error = new AtomicReference<>(); + + // One thread enters via the superclass (mirrors blackholeSpan() -> DDTraceId.ZERO), the other + // via the subclass (mirrors IdGenerationStrategy.generateTraceId() -> DD64bTraceId.from()). + Thread viaSuper = + new Thread( + () -> { + try { + barrier.await(); + Class.forName("datadog.trace.api.DDTraceId", true, cl); + } catch (Throwable t) { + error.compareAndSet(null, t); + } + }, + "init-DDTraceId"); + Thread viaSub = + new Thread( + () -> { + try { + barrier.await(); + Class.forName("datadog.trace.api.DD64bTraceId", true, cl); + } catch (Throwable t) { + error.compareAndSet(null, t); + } + }, + "init-DD64bTraceId"); + // Daemon so a deadlock cannot block forked-JVM shutdown. + viaSuper.setDaemon(true); + viaSub.setDaemon(true); + + viaSuper.start(); + viaSub.start(); + viaSuper.join(SECONDS.toMillis(15)); + viaSub.join(SECONDS.toMillis(15)); + + if (viaSuper.isAlive() || viaSub.isAlive()) { + fail( + "DDTraceId/DD64bTraceId class-initialization deadlock: DDTraceId. must not " + + "reference DD64bTraceId (init-DDTraceId.alive=" + + viaSuper.isAlive() + + ", init-DD64bTraceId.alive=" + + viaSub.isAlive() + + ")."); + } + if (error.get() != null) { + throw new AssertionError( + "Unexpected error during concurrent class initialization", error.get()); + } + } +} diff --git a/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdConstantsTest.java b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdConstantsTest.java new file mode 100644 index 00000000000..f65bc74853d --- /dev/null +++ b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdConstantsTest.java @@ -0,0 +1,40 @@ +package datadog.trace.api; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +/** Behavior of the {@link DDTraceId#ZERO}/{@link DDTraceId#ONE} sibling constants. */ +class DDTraceIdConstantsTest { + + @Test + void zeroAndOneFormatLikeTheEquivalentDD64bTraceId() { + assertEquals("0", DDTraceId.ZERO.toString()); + assertEquals("00000000000000000000000000000000", DDTraceId.ZERO.toHexString()); + assertEquals("0000000000000000", DDTraceId.ZERO.toHexStringPadded(16)); + assertEquals("00000000000000000000000000000000", DDTraceId.ZERO.toHexStringPadded(32)); + assertEquals(0L, DDTraceId.ZERO.toLong()); + assertEquals(0L, DDTraceId.ZERO.toHighOrderLong()); + + assertEquals("1", DDTraceId.ONE.toString()); + assertEquals("00000000000000000000000000000001", DDTraceId.ONE.toHexString()); + assertEquals("0000000000000001", DDTraceId.ONE.toHexStringPadded(16)); + assertEquals(1L, DDTraceId.ONE.toLong()); + assertEquals(0L, DDTraceId.ONE.toHighOrderLong()); + } + + @Test + void isZeroReflectsTheValue() { + assertTrue(DDTraceId.ZERO.isZero()); + assertTrue(DDTraceId.from(0).isZero()); + assertTrue(DDTraceId.from("0").isZero()); + assertTrue(DDTraceId.fromHex("0").isZero()); + assertTrue(DD64bTraceId.from(0).isZero()); + + assertFalse(DDTraceId.ONE.isZero()); + assertFalse(DDTraceId.from(1).isZero()); + assertFalse(DD64bTraceId.from(42).isZero()); + } +} diff --git a/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java index 3820b17c256..2aa58c07b22 100644 --- a/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java +++ b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java @@ -2,8 +2,8 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -175,10 +175,11 @@ void failParsingIllegal128BitIdHexadecimalStringRepresentationFromPartialString( @Test void checkZeroConstantInitialization() { DDTraceId zero = DDTraceId.ZERO; - DDTraceId fromZero = DDTraceId.from(0); assertNotNull(zero); - assertSame(fromZero, zero); + assertTrue(zero.isZero()); + // from(0) is a zero-valued id but, by design, no longer the ZERO singleton; use isZero(). + assertTrue(DDTraceId.from(0).isZero()); } private static String leftPadWithZeros(String value, int size) { diff --git a/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java b/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java index 73e3c2063e0..bc892e8b5c4 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java @@ -2025,7 +2025,7 @@ protected static final DDSpanContext buildSpanContext( propagationTags = extractedContext.getPropagationTags(); } else if (resolvedParentContext != null) { traceId = - resolvedParentContext.getTraceId() == DDTraceId.ZERO + resolvedParentContext.getTraceId().isZero() ? tracer.idGenerationStrategy.generateTraceId() : resolvedParentContext.getTraceId(); parentSpanId = resolvedParentContext.getSpanId(); diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java index 5afc104c675..17a78262f47 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java @@ -250,7 +250,7 @@ public ContextInterpreter reset(TraceConfig traceConfig) { protected TagContext build() { if (valid) { - if (fullContext && !DDTraceId.ZERO.equals(traceId)) { + if (fullContext && !traceId.isZero()) { if (propagationTags == null) { propagationTags = propagationTagsFactory.empty(); } @@ -305,7 +305,7 @@ private TagContext.HttpHeaders getHeaders() { } private int samplingPriorityOrDefault(DDTraceId traceId, int samplingPriority) { - return samplingPriority == PrioritySampling.UNSET || DDTraceId.ZERO.equals(traceId) + return samplingPriority == PrioritySampling.UNSET || traceId.isZero() ? defaultSamplingPriority() : samplingPriority; } diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java index 6723dfdbc8c..fe8c046934f 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java @@ -234,7 +234,7 @@ private long extractEndToEndStartTime(String value) { private void restore128bTraceId() { long highOrderBits; // Check if the low-order 64 bits of the TraceId, and propagation tags were parsed - if (traceId != DDTraceId.ZERO + if (!traceId.isZero() && propagationTags != null && (highOrderBits = propagationTags.getTraceIdHighOrderBits()) != 0) { // Restore the 128-bit TraceId diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java index f2396d83706..2f485ee230f 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java @@ -12,7 +12,6 @@ import datadog.trace.api.DD64bTraceId; import datadog.trace.api.DDSpanId; import datadog.trace.api.DDTags; -import datadog.trace.api.DDTraceId; import datadog.trace.api.TraceConfig; import datadog.trace.api.TracePropagationStyle; import datadog.trace.api.sampling.PrioritySampling; @@ -212,7 +211,10 @@ static void handleXRayTraceHeader(ContextInterpreter interpreter, String value) } String part = value.substring(startPart, endPart).trim(); if (part.startsWith(ROOT_PREFIX)) { - if (interpreter.traceId == null || interpreter.traceId == DDTraceId.ZERO) { + // isZero() (not == DDTraceId.ZERO): DD64bTraceId.fromHex below returns a non-singleton + // zero-valued id, so a zero Root must still be treated as "unset" and overwritten by a + // later valid Root in the same header. + if (interpreter.traceId == null || interpreter.traceId.isZero()) { interpreter.traceId = DD64bTraceId.fromHex(part.substring(ROOT_PREAMBLE + TRACE_ID_PADDING.length())); } diff --git a/dd-trace-core/src/test/java/datadog/trace/api/TraceIdIsZeroTest.java b/dd-trace-core/src/test/java/datadog/trace/api/TraceIdIsZeroTest.java new file mode 100644 index 00000000000..1e809dac66a --- /dev/null +++ b/dd-trace-core/src/test/java/datadog/trace/api/TraceIdIsZeroTest.java @@ -0,0 +1,40 @@ +package datadog.trace.api; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import datadog.trace.core.propagation.B3TraceId; +import org.junit.jupiter.api.Test; + +/** + * {@link DDTraceId#isZero()} across every {@link DDTraceId} subtype, including the {@code ZERO}/ + * {@code ONE} sibling constants and {@link B3TraceId} (defined in dd-trace-core). + * + *

{@code isZero()} is the value-based replacement for the old {@code == DDTraceId.ZERO} identity + * checks, so it must recognize a zero id regardless of concrete type or how it was created (a zero + * parsed via the 64-bit factories is a distinct instance from the {@code ZERO} singleton). + */ +class TraceIdIsZeroTest { + + @Test + void zeroValuedIdsOfEveryTypeAreZero() { + assertTrue(DDTraceId.ZERO.isZero()); + assertTrue(DDTraceId.from(0).isZero()); + assertTrue(DDTraceId.from("0").isZero()); + assertTrue(DDTraceId.fromHex("0").isZero()); + assertTrue(DD64bTraceId.from(0).isZero()); + assertTrue(DD128bTraceId.from(0, 0).isZero()); + assertTrue(B3TraceId.fromHex("0").isZero()); + } + + @Test + void nonZeroIdsAreNotZero() { + assertFalse(DDTraceId.ONE.isZero()); + assertFalse(DDTraceId.from(1).isZero()); + assertFalse(DD64bTraceId.from(42).isZero()); + assertFalse(DD128bTraceId.from(0, 1).isZero()); + // High-order bits set, low-order zero: still not a zero TraceId. + assertFalse(DD128bTraceId.from(7, 0).isZero()); + assertFalse(B3TraceId.fromHex("2a").isZero()); + } +} diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java index 99c90b53b30..07a75d774bd 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java @@ -62,7 +62,7 @@ static AgentSpan fromSpanContext(AgentSpanContext spanContext) { * @return {@code true} if the span is considered valid, {@code false} otherwise. */ default boolean isValid() { - return getTraceId() != DDTraceId.ZERO && getSpanId() != DDSpanId.ZERO; + return !getTraceId().isZero() && getSpanId() != DDSpanId.ZERO; } @Override