Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes race condition with toSpan and flush #1306

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 11 additions & 10 deletions brave/src/main/java/brave/Tracer.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2013-2020 The OpenZipkin Authors
* Copyright 2013-2021 The OpenZipkin Authors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2022

*
* 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
Expand Down Expand Up @@ -192,12 +192,6 @@ public final Span joinSpan(TraceContext context) {
}
}

/** Returns an equivalent context if exists in the pending map */
TraceContext swapForPendingContext(TraceContext context) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why this was removed.

PendingSpan pendingSpan = pendingSpans.get(context);
return pendingSpan != null ? pendingSpan.context() : null;
}

/**
* 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.
Expand Down Expand Up @@ -377,9 +371,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 = pendingSpans.get(context);
if (pendingSpan != null) {
if (isNoop(context)) return new NoopSpan(context);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be moved into the _toSpan method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could, but it would mean either a redundant check with the call from the overloaded _toSpan, or eliminating the shortcut taken there that avoids unnecessary pendingSpan lookup.

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)
Expand Down Expand Up @@ -408,6 +405,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()
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion brave/src/main/java/brave/handler/SpanHandler.java
Original file line number Diff line number Diff line change
@@ -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
Expand Down
22 changes: 21 additions & 1 deletion brave/src/test/java/brave/TracerTest.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2013-2020 The OpenZipkin Authors
* Copyright 2013-2021 The OpenZipkin Authors
codefromthecrypt marked this conversation as resolved.
Show resolved Hide resolved
*
* 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
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -226,6 +229,7 @@ public class TracerTest {
@Test public void join_createsChildWhenUnsupportedByPropagation() {
tracer = Tracing.newBuilder()
.propagationFactory(new Propagation.Factory() {
@Override
@Deprecated public <K> Propagation<K> create(Propagation.KeyFactory<K> keyFactory) {
return B3Propagation.FACTORY.create(keyFactory);
}
Expand Down Expand Up @@ -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)) {
Expand Down
Loading