From 3536934537c57b7e5975b004f3756bf6665d8618 Mon Sep 17 00:00:00 2001 From: Andy Lintner Date: Tue, 7 Sep 2021 16:39:00 -0400 Subject: [PATCH 1/3] Fixes race condition with toSpan and flush Fixes #1295 Previous to this fix, a call to Tracer::toSpan concurrent with a call to flush the span from pendingSpans could result in an assertion error. We now only fetch the pendingSpan once for a single call to toSpan. --- brave/src/main/java/brave/Tracer.java | 20 +++++++++++++------- brave/src/test/java/brave/TracerTest.java | 22 +++++++++++++++++++++- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/brave/src/main/java/brave/Tracer.java b/brave/src/main/java/brave/Tracer.java index 73de17b9a9..dca0b4b523 100644 --- a/brave/src/main/java/brave/Tracer.java +++ b/brave/src/main/java/brave/Tracer.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2020 The OpenZipkin Authors + * Copyright 2013-2021 The OpenZipkin Authors * * Licensed 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 @@ -193,9 +193,8 @@ public final Span joinSpan(TraceContext context) { } /** Returns an equivalent context if exists in the pending map */ - TraceContext swapForPendingContext(TraceContext context) { - PendingSpan pendingSpan = pendingSpans.get(context); - return pendingSpan != null ? pendingSpan.context() : null; + PendingSpan getPendingSpan(TraceContext context) { + return pendingSpans.get(context); } /** @@ -377,9 +376,12 @@ public Span toSpan(TraceContext context) { } Span toSpan(@Nullable TraceContext parent, TraceContext context) { - // Re-use a pending context if present: This ensures reference consistency on Span.context() - TraceContext pendingContext = swapForPendingContext(context); - if (pendingContext != null) return _toSpan(parent, pendingContext); + // Re-use a pending span if present: This ensures reference consistency on Span.context() + PendingSpan pendingSpan = getPendingSpan(context); + if (pendingSpan != null) { + if (isNoop(context)) return new NoopSpan(context); + return _toSpan(context, pendingSpan); + } // There are a few known scenarios for the context to be absent from the pending map: // * Created by a separate tracer (localRootId set) @@ -408,6 +410,10 @@ Span _toSpan(@Nullable TraceContext parent, TraceContext context) { // allocate a mutable span in case multiple threads call this method.. they'll use the same data PendingSpan pendingSpan = pendingSpans.getOrCreate(parent, context, false); + return _toSpan(context, pendingSpan); + } + + Span _toSpan(TraceContext context, PendingSpan pendingSpan) { TraceContext pendingContext = pendingSpan.context(); // A lost race of Tracer.toSpan(context) is the only known situation where "context" won't be // the same as pendingSpan.context() diff --git a/brave/src/test/java/brave/TracerTest.java b/brave/src/test/java/brave/TracerTest.java index 1a9dd78b81..ccde1206d2 100644 --- a/brave/src/test/java/brave/TracerTest.java +++ b/brave/src/test/java/brave/TracerTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2020 The OpenZipkin Authors + * Copyright 2013-2021 The OpenZipkin Authors * * Licensed 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 @@ -37,9 +37,12 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.ExecutionException; import java.util.function.Function; import org.junit.After; import org.junit.Test; +import org.mockito.Mockito; + import zipkin2.Endpoint; import zipkin2.reporter.Reporter; @@ -226,6 +229,7 @@ public class TracerTest { @Test public void join_createsChildWhenUnsupportedByPropagation() { tracer = Tracing.newBuilder() .propagationFactory(new Propagation.Factory() { + @Override @Deprecated public Propagation create(Propagation.KeyFactory keyFactory) { return B3Propagation.FACTORY.create(keyFactory); } @@ -1092,6 +1096,22 @@ private static void simulateInProcessPropagation(Tracer tracer1, Tracer tracer2) assertThat(toSpan.extra()).isNotEmpty(); } + @Test public void toSpan_flushedAfterFetch() throws InterruptedException, ExecutionException { + propagationFactory = baggageFactory; + TraceContext parent = TraceContext.newBuilder().traceId(1L).spanId(2L).sampled(true).build(); + TraceContext incoming = TraceContext.newBuilder().traceId(1L).spanId(3L).parentId(2L).sampled(true).build(); + + tracer.pendingSpans.getOrCreate(parent, incoming, false); + Tracer spiedTracer = Mockito.spy(tracer); + Mockito.doAnswer(i -> { + //Simulate a concurrent call flushing the span at the beginning of this method's execution + tracer.pendingSpans.flush(incoming); + return i.callRealMethod(); + }).when(spiedTracer)._toSpan(Mockito.any(TraceContext.class), Mockito.any(TraceContext.class)); + + spiedTracer.toSpan(incoming); + } + @Test public void currentSpan_sameContextReference() { Span span = tracer.newTrace(); try (SpanInScope ws = tracer.withSpanInScope(span)) { From c1b7446ad9ffb87b06a564f1b31a48ad961f19b5 Mon Sep 17 00:00:00 2001 From: Andy Lintner Date: Tue, 12 Oct 2021 12:23:42 -0400 Subject: [PATCH 2/3] Address review feedback --- brave/src/main/java/brave/Tracer.java | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/brave/src/main/java/brave/Tracer.java b/brave/src/main/java/brave/Tracer.java index dca0b4b523..1f9224f106 100644 --- a/brave/src/main/java/brave/Tracer.java +++ b/brave/src/main/java/brave/Tracer.java @@ -192,11 +192,6 @@ public final Span joinSpan(TraceContext context) { } } - /** Returns an equivalent context if exists in the pending map */ - PendingSpan getPendingSpan(TraceContext context) { - return pendingSpans.get(context); - } - /** * Explicitly creates a child within an existing trace. The result will be have its parent ID set * to the input's span ID. If a sampling decision has not yet been made, one will happen here. @@ -377,7 +372,7 @@ public Span toSpan(TraceContext context) { Span toSpan(@Nullable TraceContext parent, TraceContext context) { // Re-use a pending span if present: This ensures reference consistency on Span.context() - PendingSpan pendingSpan = getPendingSpan(context); + PendingSpan pendingSpan = pendingSpans.get(context); if (pendingSpan != null) { if (isNoop(context)) return new NoopSpan(context); return _toSpan(context, pendingSpan); From 42a870514b5cb4fbc98ec163989259c1dd8349e7 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Fri, 15 Dec 2023 18:53:54 +0700 Subject: [PATCH 3/3] licens Signed-off-by: Adrian Cole --- brave/src/main/java/brave/baggage/BaggagePropagationConfig.java | 2 +- brave/src/main/java/brave/handler/SpanHandler.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/brave/src/main/java/brave/baggage/BaggagePropagationConfig.java b/brave/src/main/java/brave/baggage/BaggagePropagationConfig.java index 7ec562a2da..81b6ec0433 100644 --- a/brave/src/main/java/brave/baggage/BaggagePropagationConfig.java +++ b/brave/src/main/java/brave/baggage/BaggagePropagationConfig.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2020 The OpenZipkin Authors + * Copyright 2013-2023 The OpenZipkin Authors * * Licensed 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 diff --git a/brave/src/main/java/brave/handler/SpanHandler.java b/brave/src/main/java/brave/handler/SpanHandler.java index 3e00db681f..01b21edf90 100644 --- a/brave/src/main/java/brave/handler/SpanHandler.java +++ b/brave/src/main/java/brave/handler/SpanHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2020 The OpenZipkin Authors + * Copyright 2013-2023 The OpenZipkin Authors * * Licensed 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