From 9859aa5297b339f76719796a2f495823deccdc82 Mon Sep 17 00:00:00 2001 From: lcian Date: Mon, 10 Mar 2025 17:27:29 +0100 Subject: [PATCH 1/4] Avoid copying and iterate correctly on `SentryTracer.children` --- .../src/main/java/io/sentry/SentryTracer.java | 35 ++++++++++--------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/sentry/src/main/java/io/sentry/SentryTracer.java b/sentry/src/main/java/io/sentry/SentryTracer.java index 36329db7a7..52a5018283 100644 --- a/sentry/src/main/java/io/sentry/SentryTracer.java +++ b/sentry/src/main/java/io/sentry/SentryTracer.java @@ -7,7 +7,6 @@ import io.sentry.util.AutoClosableReentrantLock; 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 +154,10 @@ 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 iterator = children.listIterator(children.size()); + @NotNull final ListIterator iterator = this.children.listIterator(); + while (iterator.hasNext()) { + iterator.next(); + } 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 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 iterator = this.children.listIterator(); + while (iterator.hasNext()) { + @NotNull final Span span = iterator.previous(); + // 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,14 @@ public void setName(@NotNull String name, @NotNull TransactionNameSource transac @Override public @Nullable ISpan getLatestActiveSpan() { - final List 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 iterator = this.children.listIterator(); + while (iterator.hasNext()) { + iterator.next(); + } + while (iterator.hasPrevious()) { + @NotNull final Span span = iterator.previous(); + if (!span.isFinished()) { + return span; } } return null; From 4ef26722d4683257147119ef6574f47f411da64d Mon Sep 17 00:00:00 2001 From: lcian Date: Mon, 10 Mar 2025 18:01:12 +0100 Subject: [PATCH 2/4] another place --- sentry/src/main/java/io/sentry/Scope.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry/src/main/java/io/sentry/Scope.java b/sentry/src/main/java/io/sentry/Scope.java index 4dacc8e4da..d8eecb4708 100644 --- a/sentry/src/main/java/io/sentry/Scope.java +++ b/sentry/src/main/java/io/sentry/Scope.java @@ -759,7 +759,7 @@ public void removeContexts(final @NotNull String key) { @NotNull @Override public List getAttachments() { - return new CopyOnWriteArrayList<>(attachments); + return attachments; } /** From 0ad1efb7582780d0efb29275faf7b842a090aec2 Mon Sep 17 00:00:00 2001 From: lcian Date: Mon, 10 Mar 2025 18:08:43 +0100 Subject: [PATCH 3/4] changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 440881b594..1dc5db2f5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ ### Fixes - Add support for setting in-app-includes/in-app-excludes via AndroidManifest.xml ([#4240](https://github.com/getsentry/sentry-java/pull/4240)) +- Avoid unnecessary copies and ensure thread-safety when using `CopyOnWriteArrayList` ([#4247](https://github.com/getsentry/sentry-java/pull/4247)) + - This affects in particular `SentryTracer.getLatestActiveSpan` which would have previously unnecessarily copied all children spans, with high impact on memory usage ### Features From a0872d0b6e3ad6d65fddeb2d24cd5c8fa6597a10 Mon Sep 17 00:00:00 2001 From: lcian Date: Mon, 10 Mar 2025 22:06:18 +0100 Subject: [PATCH 4/4] fix --- sentry/src/main/java/io/sentry/SentryTracer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry/src/main/java/io/sentry/SentryTracer.java b/sentry/src/main/java/io/sentry/SentryTracer.java index 52a5018283..7e9de25fbf 100644 --- a/sentry/src/main/java/io/sentry/SentryTracer.java +++ b/sentry/src/main/java/io/sentry/SentryTracer.java @@ -681,7 +681,7 @@ private void updateBaggageValues(final @NotNull Baggage baggage) { private boolean hasAllChildrenFinished() { @NotNull final ListIterator iterator = this.children.listIterator(); while (iterator.hasNext()) { - @NotNull final Span span = iterator.previous(); + @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) {