Also applies to hasAllChildrenFinished() and probably others.

We should check for all places where we use the CopyOnWriteArrayList

These are the two locations the Tracer copies the list

Aren't those locations simply copying references to Span objects? They're not actually cloning the objects are they?

As far as I know, new ArrayList<>(this.children) is not doing a deep copy, yes, but it will still allocate a new ArrayList for the references.

OK so this shouldn't be the cause of OOM just where it surfaced. It should only allocate little memory and can be garbage collected right away, no risk of anyone holding on to references as we don't return the list. Shall we close this then?

@adinauer yes you're right, the elements of the array are not cloned / deep copied. According to SDK console this issue is still our no. 1 overall issue. I can imagine situations where there are many spans or SentryTracer.getLatestActiveSpan is called very frequently causing OOMs.

We could just forgoe the copying of the array since CopyOnWriteArrayList creates an Iterator that does not throw ConcurrentModificationException.

We need to test to make sure that's actually the case and it doesn't throw when under load.

We need to make sure to use the iterator instead of iterating based on the index. To get reverse we can use:

  ListIterator<Span> iterator = this.children.listIterator();
  while (iterator.hasPrevious()) {
    Span s = iterator.previous();
    ...
  }

Both getLatestActiveSpan and hasAllChildrenFinished should be updated.

For Testing this:
We can replace: private final @NotNull List<Span> children = new CopyOnWriteArrayList<>(); with a normal ArrayList.

Create a setup that concurrently from multiple threads starts new spans and invokes the affected methods.

With ArrayList in place this should throw ConcurrentModificationException and with CopyOnWriteArrayList it should not.

In a loop check affected methods.
In another loop on another thread create spans with a sleep in between.

I've ran some tests

package io.sentry.samples.console;

import io.sentry.ISpan;
import io.sentry.ITransaction;
import io.sentry.Sentry;
import io.sentry.Span;
import io.sentry.TransactionOptions;
import java.util.List;
import java.util.Random;

public class Main {

  public static int ITERATIONS = 1000000000;

  public static void main(String[] args) throws InterruptedException {
    Sentry.init(
        options -> {
          options.setDsn("https://[email protected]/4508683222843393");
          options.setTracesSampleRate(1.0);
        });

    TransactionOptions opts = new TransactionOptions();
    opts.setBindToScope(true);
    ITransaction tx = Sentry.startTransaction("lol", "op", opts);

    Thread thread1 = new Thread(new SpanStarter());
    Thread thread2 = new Thread(new LatestSpanGetter());
    thread1.start();
    thread2.start();
    try {
      thread1.join();
      thread2.join();
    } catch (InterruptedException e) {
      e.printStackTrace();
    }
  }
}

class SpanStarter implements Runnable {

  @Override
  public void run() {
    try {
      Random random = new Random();
      ITransaction tx = Sentry.getCurrentScopes().getTransaction();
      for (int i = 0; i < Main.ITERATIONS; i++) {
        List<Span> spans = tx.getSpans();
        if (!spans.isEmpty() & i % 3 == 0) {
          Integer w = random.nextInt(spans.size());
          spans.remove(w);
        }
        Integer r = random.nextInt(Integer.MAX_VALUE);
        tx.startChild(r.toString());
      }
    } catch (Exception e) {
      System.out.println(e.toString());
    }
    System.out.println("SpanStarter finished");
  }
}

class LatestSpanGetter implements Runnable {
  @Override
  public void run() {
    Integer x = 0;
    try {
      ITransaction tx = Sentry.getCurrentScopes().getTransaction();
      for (int i = 0; i < 100 * Main.ITERATIONS; i++) {
        ISpan span = tx.getLatestActiveSpan();
        if (span != null) {
          x += Integer.parseInt(span.getOperation());
        }
      }
    } catch (Exception e) {
      System.out.println(e.toString());
    }
    System.out.println(x);
    System.out.println("LatestSpanGetter finished");
  }
}

When replacing the current implementation of SentryTracer.children with an ArrayList, I've been able to trigger a ConcurrentModificationException only if I also modify SentryTracer.getLatestActiveSpan to use an iterator instead of index-based accessing.
Index-based accessing could also be problematic though. It could happen that one thread deletes some element and the other thread which is executing getLatestActiveSpan tries to access it (as it's relying on the old length of the list when iterating) causing an NPE.

CopyOnWriteArrayList prevents ConcurrentModificationException when using the iterator (tested with the above example code).

Therefore we will move forward with not copying and also accessing the list using an iterator.

To perform the checks starting from the last span we need to "consume" the iterator (calling .next() until the end) and only then start checking, as CopyOnWriteArrayList doesn't provide a direct way to get a reverse iterator in a thread-safe and copyless manner (see e.g. https://stackoverflow.com/questions/42045206/thread-safe-copyonwritearraylist-reverse-iteration).