Fix: If transaction or span is finished, do not allow to mutate

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+* Fix: If transaction or span is finished, do not allow to mutate (#1940)
+
 ## 5.6.2
 
 * Ref: Make ActivityFramesTracker public to be used by Hybrid SDKs (#1931)

sentry/src/main/java/io/sentry/SentryTracer.java

@@ -137,6 +137,10 @@ private ISpan createChild(
       final @NotNull String operation,
       final @Nullable String description,
       @Nullable Date timestamp) {
+    if (root.isFinished()) {
+      return NoOpSpan.getInstance();
+    }
+
     Objects.requireNonNull(parentSpanId, "parentSpanId is required");
     Objects.requireNonNull(operation, "operation is required");
     final Span span =
@@ -179,6 +183,10 @@ private ISpan createChild(
       final @NotNull String operation,
       final @Nullable String description,
       @Nullable Date timestamp) {
+    if (root.isFinished()) {
+      return NoOpSpan.getInstance();
+    }
+
     if (children.size() < hub.getOptions().getMaxSpans()) {
       return root.startChild(operation, description, timestamp);
     } else {
@@ -288,6 +296,10 @@ private boolean hasAllChildrenFinished() {
 
   @Override
   public void setOperation(final @NotNull String operation) {
+    if (root.isFinished()) {
+      return;
+    }
+
     this.root.setOperation(operation);
   }
 
@@ -298,6 +310,10 @@ public void setOperation(final @NotNull String operation) {
 
   @Override
   public void setDescription(final @Nullable String description) {
+    if (root.isFinished()) {
+      return;
+    }
+
     this.root.setDescription(description);
   }
 
@@ -308,6 +324,10 @@ public void setDescription(final @Nullable String description) {
 
   @Override
   public void setStatus(final @Nullable SpanStatus status) {
+    if (root.isFinished()) {
+      return;
+    }
+
     this.root.setStatus(status);
   }
 
@@ -318,6 +338,10 @@ public void setStatus(final @Nullable SpanStatus status) {
 
   @Override
   public void setThrowable(final @Nullable Throwable throwable) {
+    if (root.isFinished()) {
+      return;
+    }
+
     this.root.setThrowable(throwable);
   }
 
@@ -333,6 +357,10 @@ public void setThrowable(final @Nullable Throwable throwable) {
 
   @Override
   public void setTag(final @NotNull String key, final @NotNull String value) {
+    if (root.isFinished()) {
+      return;
+    }
+
     this.root.setTag(key, value);
   }
 
@@ -348,6 +376,10 @@ public boolean isFinished() {
 
   @Override
   public void setData(@NotNull String key, @NotNull Object value) {
+    if (root.isFinished()) {
+      return;
+    }
+
     this.root.setData(key, value);
   }
 
@@ -367,6 +399,10 @@ public void setData(@NotNull String key, @NotNull Object value) {
 
   @Override
   public void setName(@NotNull String name) {
+    if (root.isFinished()) {
+      return;
+    }
+
     this.name = name;
   }
 
@@ -379,6 +415,10 @@ public void setName(@NotNull String name) {
   @Deprecated
   @ApiStatus.ScheduledForRemoval
   public void setRequest(final @Nullable Request request) {
+    if (root.isFinished()) {
+      return;
+    }
+
     this.request = request;
   }
 

sentry/src/main/java/io/sentry/Span.java

@@ -94,12 +94,20 @@ public Span(
       final @NotNull String operation,
       final @Nullable String description,
       final @Nullable Date timestamp) {
+    if (finished.get()) {
+      return NoOpSpan.getInstance();
+    }
+
     return transaction.startChild(context.getSpanId(), operation, description, timestamp);
   }
 
   @Override
   public @NotNull ISpan startChild(
       final @NotNull String operation, final @Nullable String description) {
+    if (finished.get()) {
+      return NoOpSpan.getInstance();
+    }
+
     return transaction.startChild(context.getSpanId(), operation, description);
   }
 
@@ -152,6 +160,10 @@ void finish(@Nullable SpanStatus status, Date timestamp) {
 
   @Override
   public void setOperation(final @NotNull String operation) {
+    if (finished.get()) {
+      return;
+    }
+
     this.context.setOperation(operation);
   }
 
@@ -162,6 +174,10 @@ public void setOperation(final @NotNull String operation) {
 
   @Override
   public void setDescription(final @Nullable String description) {
+    if (finished.get()) {
+      return;
+    }
+
     this.context.setDescription(description);
   }
 
@@ -172,6 +188,10 @@ public void setDescription(final @Nullable String description) {
 
   @Override
   public void setStatus(final @Nullable SpanStatus status) {
+    if (finished.get()) {
+      return;
+    }
+
     this.context.setStatus(status);
   }
 
@@ -187,6 +207,10 @@ public void setStatus(final @Nullable SpanStatus status) {
 
   @Override
   public void setTag(final @NotNull String key, final @NotNull String value) {
+    if (finished.get()) {
+      return;
+    }
+
     this.context.setTag(key, value);
   }
 
@@ -210,6 +234,10 @@ public boolean isFinished() {
 
   @Override
   public void setThrowable(final @Nullable Throwable throwable) {
+    if (finished.get()) {
+      return;
+    }
+
     this.throwable = throwable;
   }
 
@@ -237,6 +265,10 @@ public Map<String, String> getTags() {
 
   @Override
   public void setData(@NotNull String key, @NotNull Object value) {
+    if (finished.get()) {
+      return;
+    }
+
     data.put(key, value);
   }
 

sentry/src/test/java/io/sentry/SentryTracerTest.kt

@@ -389,6 +389,42 @@ class SentryTracerTest {
         assertTrue(transaction.isFinished)
     }
 
+    @Test
+    fun `when span is finished, do nothing`() {
+        val transaction = fixture.getSut()
+        transaction.description = "desc"
+        transaction.setTag("myTag", "myValue")
+        transaction.setData("myData", "myValue")
+        val ex = RuntimeException()
+        transaction.throwable = ex
+        val req = Request()
+        transaction.request = req
+
+        transaction.finish(SpanStatus.OK)
+        assertTrue(transaction.isFinished)
+
+        assertEquals(NoOpSpan.getInstance(), transaction.startChild("op", "desc"))
+
+        transaction.finish(SpanStatus.UNKNOWN_ERROR)
+        transaction.operation = "newOp"
+        transaction.description = "newDesc"
+        transaction.status = SpanStatus.ABORTED
+        transaction.setTag("myTag", "myNewValue")
+        transaction.throwable = RuntimeException()
+        transaction.setData("myData", "myNewValue")
+        transaction.name = "newName"
+        transaction.request = Request()
+
+        assertEquals(SpanStatus.OK, transaction.status)
+        assertEquals("op", transaction.operation)
+        assertEquals("desc", transaction.description)
+        assertEquals("myValue", transaction.getTag("myTag"))
+        assertEquals("myValue", transaction.getData("myData"))
+        assertEquals("name", transaction.name)
+        assertEquals(req, transaction.request)
+        assertEquals(ex, transaction.throwable)
+    }
+
     @Test
     fun `when startTimestamp is given, use it as startTimestamp`() {
         val date = Date(0)

sentry/src/test/java/io/sentry/SpanTest.kt

@@ -154,6 +154,37 @@ class SpanTest {
         assertEquals(timestamp, span.timestamp)
     }
 
+    @Test
+    fun `when span is finished, do nothing`() {
+        val span = fixture.getSut()
+        span.description = "desc"
+        span.setTag("myTag", "myValue")
+        span.setData("myData", "myValue")
+        val ex = RuntimeException()
+        span.throwable = ex
+
+        span.finish(SpanStatus.OK)
+        assertTrue(span.isFinished)
+
+        assertEquals(NoOpSpan.getInstance(), span.startChild("op", "desc", null))
+        assertEquals(NoOpSpan.getInstance(), span.startChild("op", "desc"))
+
+        span.finish(SpanStatus.UNKNOWN_ERROR)
+        span.operation = "newOp"
+        span.description = "newDesc"
+        span.status = SpanStatus.ABORTED
+        span.setTag("myTag", "myNewValue")
+        span.throwable = RuntimeException()
+        span.setData("myData", "myNewValue")
+
+        assertEquals(SpanStatus.OK, span.status)
+        assertEquals("op", span.operation)
+        assertEquals("desc", span.description)
+        assertEquals("myValue", span.tags["myTag"])
+        assertEquals("myValue", span.data["myData"])
+        assertEquals(ex, span.throwable)
+    }
+
     @Test
     fun `child trace state is equal to transaction trace state`() {
         val transaction = getTransaction()