From 08e8e39509bfb927229c51db77a3daa7f2abeea1 Mon Sep 17 00:00:00 2001
From: Adrian Cole <64215+codefromthecrypt@users.noreply.github.com>
Date: Wed, 10 Apr 2024 14:10:43 -1000
Subject: [PATCH] Fixes thread safety issue in Tag.tag (#1434)

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>
---
 brave/src/main/java/brave/Tag.java     |  5 ++++-
 brave/src/test/java/brave/TagTest.java | 24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/brave/src/main/java/brave/Tag.java b/brave/src/main/java/brave/Tag.java
index e0984f02f..941357b09 100644
--- a/brave/src/main/java/brave/Tag.java
+++ b/brave/src/main/java/brave/Tag.java
@@ -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);
+      }
     }
   }
 
diff --git a/brave/src/test/java/brave/TagTest.java b/brave/src/test/java/brave/TagTest.java
index b3e6df825..2ee28d24d 100644
--- a/brave/src/test/java/brave/TagTest.java
+++ b/brave/src/test/java/brave/TagTest.java
@@ -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;
@@ -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");