Fix misuses of CopyOnWriteArrayList (#4247)

* Avoid copying and iterate correctly on `SentryTracer.children`

* another place

* changelog

* fix

* remove unnecessary test

* wip

* improve

* improve

* Update CHANGELOG.md

Co-authored-by: Alexander Dinauer <adinauer@users.noreply.github.com>

---------

Co-authored-by: Alexander Dinauer <adinauer@users.noreply.github.com>

Author: lcian

CHANGELOG.md

@@ -19,6 +19,8 @@
 - Pass OpenTelemetry span attributes into TracesSampler callback ([#4253](https://github.com/getsentry/sentry-java/pull/4253))
   - `SamplingContext` now has a `getAttribute` method that grants access to OpenTelemetry span attributes via their String key (e.g. `http.request.method`)
 - Fix AbstractMethodError when using SentryTraced for Jetpack Compose ([#4255](https://github.com/getsentry/sentry-java/pull/4255))
+- Avoid unnecessary copies when using `CopyOnWriteArrayList` ([#4247](https://github.com/getsentry/sentry-java/pull/4247))
+  - This affects in particular `SentryTracer.getLatestActiveSpan` which would have previously copied all child span references. This may have caused `OutOfMemoryError` on certain devices due to high frequency of calling the method.
 
 ### Features
 

sentry/api/sentry.api

@@ -6095,6 +6095,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

@@ -5,9 +5,9 @@
 import io.sentry.protocol.SentryTransaction;
 import io.sentry.protocol.TransactionNameSource;
 import io.sentry.util.AutoClosableReentrantLock;
+import io.sentry.util.CollectionUtils;
 import io.sentry.util.Objects;
 import io.sentry.util.SpanUtils;
-import java.util.ArrayList;
 import java.util.List;
 import java.util.ListIterator;
 import java.util.Map;
@@ -155,7 +155,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 = children.listIterator(children.size());
+    @NotNull
+    final ListIterator<Span> iterator =
+        CollectionUtils.reverseListIterator((CopyOnWriteArrayList<Span>) this.children);
     while (iterator.hasPrevious()) {
       @NotNull final Span span = iterator.previous();
       span.setSpanFinishedCallback(null);
@@ -677,14 +679,13 @@ private void updateBaggageValues(final @NotNull Baggage baggage) {
   }
 
   private boolean hasAllChildrenFinished() {
-    final List<Span> spans = new ArrayList<>(this.children);
-    if (!spans.isEmpty()) {
-      for (final Span span : spans) {
-        // This is used in the spanFinishCallback, when the span isn't finished, but has a finish
-        // date
-        if (!span.isFinished() && span.getFinishDate() == null) {
-          return false;
-        }
+    @NotNull final ListIterator<Span> iterator = this.children.listIterator();
+    while (iterator.hasNext()) {
+      @NotNull final Span span = iterator.next();
+      // This is used in the spanFinishCallback, when the span isn't finished, but has a finish
+      // date
+      if (!span.isFinished() && span.getFinishDate() == null) {
+        return false;
       }
     }
     return true;
@@ -909,12 +910,13 @@ public void setName(@NotNull String name, @NotNull TransactionNameSource transac
 
   @Override
   public @Nullable ISpan getLatestActiveSpan() {
-    final List<Span> spans = new ArrayList<>(this.children);
-    if (!spans.isEmpty()) {
-      for (int i = spans.size() - 1; i >= 0; i--) {
-        if (!spans.get(i).isFinished()) {
-          return spans.get(i);
-        }
+    @NotNull
+    final ListIterator<Span> iterator =
+        CollectionUtils.reverseListIterator((CopyOnWriteArrayList<Span>) this.children);
+    while (iterator.hasPrevious()) {
+      @NotNull final Span span = iterator.previous();
+      if (!span.isFinished()) {
+        return span;
       }
     }
     return null;

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/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)
+    }
 }