improve

Author: lcian

sentry/api/sentry.api

@@ -6087,6 +6087,7 @@ public final class io/sentry/util/CollectionUtils {
 	public static fun newArrayList (Ljava/util/List;)Ljava/util/List;
 	public static fun newConcurrentHashMap (Ljava/util/Map;)Ljava/util/Map;
 	public static fun newHashMap (Ljava/util/Map;)Ljava/util/Map;
+	public static fun reverseListIterator (Ljava/util/concurrent/CopyOnWriteArrayList;)Ljava/util/ListIterator;
 	public static fun size (Ljava/lang/Iterable;)I
 }
 

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

@@ -1,5 +1,7 @@
 package io.sentry;
 
+import static io.sentry.util.CollectionUtils.reverseListIterator;
+
 import io.sentry.protocol.Contexts;
 import io.sentry.protocol.SentryId;
 import io.sentry.protocol.SentryTransaction;
@@ -154,10 +156,9 @@ private void onDeadlineTimeoutReached() {
     // abort all child-spans first, this ensures the transaction can be finished,
     // even if waitForChildren is true
     // iterate in reverse order to ensure leaf spans are processed before their parents
-    @NotNull final ListIterator<Span> iterator = this.children.listIterator();
-    while (iterator.hasNext()) {
-      iterator.next();
-    }
+    @NotNull
+    final ListIterator<Span> iterator =
+        reverseListIterator((CopyOnWriteArrayList<Span>) this.children);
     while (iterator.hasPrevious()) {
       @NotNull final Span span = iterator.previous();
       span.setSpanFinishedCallback(null);
@@ -910,10 +911,9 @@ public void setName(@NotNull String name, @NotNull TransactionNameSource transac
 
   @Override
   public @Nullable ISpan getLatestActiveSpan() {
-    @NotNull final ListIterator<Span> iterator = this.children.listIterator();
-    while (iterator.hasNext()) {
-      iterator.next();
-    }
+    @NotNull
+    final ListIterator<Span> iterator =
+        reverseListIterator((CopyOnWriteArrayList<Span>) this.children);
     while (iterator.hasPrevious()) {
       @NotNull final Span span = iterator.previous();
       if (!span.isFinished()) {

sentry/src/main/java/io/sentry/util/CollectionUtils.java

@@ -4,8 +4,10 @@
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
+import java.util.ListIterator;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CopyOnWriteArrayList;
 import org.jetbrains.annotations.ApiStatus;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -179,4 +181,23 @@ public interface Predicate<T> {
   public interface Mapper<T, R> {
     R map(T t);
   }
+
+  /**
+   * Returns a reverse iterator, where the first (resp. last) valid call to `prev` returns the last
+   * (resp. first) element that would be returned when iterating forwards. Note that this differs
+   * from the behavior of e.g. `org.apache.commons.collections4.iterators.ReverseListIterator`,
+   * where you need to iterate using `next` instead. We use the concrete type `CopyOnWriteArrayList`
+   * here as we are relying on the fact that its copy constructor only copies the reference to an
+   * internal array. We don't want to use this for other `List` implementations, as it could lead to
+   * an unnecessary copy of the elements instead.
+   *
+   * @param list the `CopyOnWriteArrayList` to get the reverse iterator for
+   * @param <T> the type
+   * @return a reverse iterator over `list`
+   */
+  public static @NotNull <T> ListIterator<T> reverseListIterator(
+      final @NotNull CopyOnWriteArrayList<T> list) {
+    final @NotNull CopyOnWriteArrayList<T> copy = new CopyOnWriteArrayList<>(list);
+    return copy.listIterator(copy.size());
+  }
 }

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

@@ -906,6 +906,18 @@ class ScopeTest {
         assertTrue(cloned.attachments is CopyOnWriteArrayList)
     }
 
+    @Test
+    fun `getAttachments returns new instance`() {
+        val scope = Scope(SentryOptions())
+        scope.addAttachment(Attachment(""))
+
+        assertNotSame(
+            scope.attachments,
+            scope.attachments,
+            "Scope.attachments must return a new instance on each call."
+        )
+    }
+
     @Test
     fun `clearAttachments clears all attachments`() {
         val scope = Scope(SentryOptions())

sentry/src/test/java/io/sentry/util/CollectionUtilsTest.kt

@@ -2,6 +2,7 @@ package io.sentry.util
 
 import io.sentry.JsonObjectReader
 import java.io.StringReader
+import java.util.concurrent.CopyOnWriteArrayList
 import kotlin.test.Test
 import kotlin.test.assertEquals
 import kotlin.test.assertFalse
@@ -79,4 +80,26 @@ class CollectionUtilsTest {
     fun `contains returns false if element is not present`() {
         assertFalse(CollectionUtils.contains(arrayOf("one", "two", "three"), "four"))
     }
+
+    @Test
+    fun `reverseListIterator returns empty iterator if list is empty`() {
+        val list = CopyOnWriteArrayList<String>()
+        val iterator = CollectionUtils.reverseListIterator(list)
+        assertFalse(iterator.hasNext())
+        assertFalse(iterator.hasPrevious())
+    }
+
+    @Test
+    fun `reverseListIterator returns reversed iterator if list is not empty`() {
+        val elements = listOf("one", "two", "three")
+        val list = CopyOnWriteArrayList(elements)
+        val iterator = CollectionUtils.reverseListIterator(list)
+        assertFalse(iterator.hasNext())
+        assertTrue(iterator.hasPrevious())
+        val reversedElements = mutableListOf<String>()
+        while (iterator.hasPrevious()) {
+            reversedElements.add(iterator.previous())
+        }
+        assertEquals(elements.reversed(), reversedElements)
+    }
 }