Skip to content

Commit

Permalink
Fixes thread safety issue in Tag.tag
Browse files Browse the repository at this point in the history
The current codebase carefully synchronizes all access to MutableSpan
where there can be concurrent use. We missed a spot where Tag.tag is not
synchronizing access. The only other place we don't guard is ScopedSpan,
which is documented explicitly as not thread safe.

Fixes #1384

Signed-off-by: Adrian Cole <adrian@tetrate.io>
  • Loading branch information
Adrian Cole committed Apr 10, 2024
1 parent 57f27e2 commit 4fe789a
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 1 deletion.
5 changes: 4 additions & 1 deletion brave/src/main/java/brave/Tag.java
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,10 @@ final void tag(Object span, I input, @Nullable TraceContext context) {
if (span instanceof SpanCustomizer) {
((SpanCustomizer) span).tag(key, value);
} else if (span instanceof MutableSpan) {
((MutableSpan) span).tag(key, value);
MutableSpan mSpan = (MutableSpan) span;
synchronized (mSpan) {
mSpan.tag(key, value);
}
}
}

Expand Down
24 changes: 24 additions & 0 deletions brave/src/test/java/brave/TagTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@

import brave.handler.MutableSpan;
import brave.propagation.TraceContext;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import java.util.function.BiFunction;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -257,6 +260,27 @@ class TagTest {
assertThat(mutableSpan).isEqualTo(expected);
}

@Test
public void tag_mutableSpan_threadSafe() throws InterruptedException {
int numThreads = 1000;
ExecutorService service = Executors.newFixedThreadPool(numThreads);
try {
for (int i = 0; i < numThreads; i++) {
String val = String.valueOf(i);
Tag<Object> tag = new Tag<Object>("key" + i) {
@Override protected String parseValue(Object input, TraceContext context) {
return val;
}
};
service.submit(() -> tag.tag(input, context, mutableSpan));
}
} finally {
service.shutdown();
service.awaitTermination(1, TimeUnit.MINUTES);
}
assertThat(mutableSpan.tagCount()).isEqualTo(numThreads);
}

@Test void tag_mutableSpan_nullContext() {
when(parseValue.apply(eq(input), isNull())).thenReturn("value");

Expand Down

0 comments on commit 4fe789a

Please sign in to comment.