From 5127b6e1d7391b836c912531f344437d75f25a08 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Thu, 23 Jun 2022 17:27:36 +0200 Subject: [PATCH] Fix thread leak due to `Timer` being created and never cancelled (#2131) * Only create timer if idleTimeout is set on SentryTracer; also cancel timer on finish * Add changelog * Synchronize timer interaction; only create Timer if session tracking enabled * Move null init --- CHANGELOG.md | 6 ++ .../sentry/android/core/LifecycleWatcher.java | 50 ++++++++++----- .../gestures/SentryGestureListener.java | 2 +- .../android/core/LifecycleWatcherTest.kt | 12 ++++ .../SentryGestureListenerTracingTest.kt | 2 +- sentry/api/sentry.api | 6 +- .../src/main/java/io/sentry/ITransaction.java | 8 +-- .../main/java/io/sentry/NoOpTransaction.java | 2 +- .../src/main/java/io/sentry/SentryTracer.java | 62 +++++++++++++------ .../test/java/io/sentry/SentryTracerTest.kt | 20 ++++++ 10 files changed, 122 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d644ee973..44a7e95f55 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Fixes + +- Fix thread leak due to Timer being created and never cancelled ([#2131](https://github.com/getsentry/sentry-java/pull/2131)) + ## 6.1.2 ### Fixes diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java b/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java index b0a435f062..c2b100b7ff 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java @@ -22,7 +22,8 @@ final class LifecycleWatcher implements DefaultLifecycleObserver { private final long sessionIntervalMillis; private @Nullable TimerTask timerTask; - private final @NotNull Timer timer = new Timer(true); + private final @Nullable Timer timer; + private final @NotNull Object timerLock = new Object(); private final @NotNull IHub hub; private final boolean enableSessionTracking; private final boolean enableAppLifecycleBreadcrumbs; @@ -54,6 +55,11 @@ final class LifecycleWatcher implements DefaultLifecycleObserver { this.enableAppLifecycleBreadcrumbs = enableAppLifecycleBreadcrumbs; this.hub = hub; this.currentDateProvider = currentDateProvider; + if (enableSessionTracking) { + timer = new Timer(true); + } else { + timer = null; + } } // App goes to foreground @@ -95,24 +101,30 @@ public void onStop(final @NotNull LifecycleOwner owner) { } private void scheduleEndSession() { - cancelTask(); - timerTask = - new TimerTask() { - @Override - public void run() { - addSessionBreadcrumb("end"); - hub.endSession(); - runningSession.set(false); - } - }; - - timer.schedule(timerTask, sessionIntervalMillis); + synchronized (timerLock) { + cancelTask(); + if (timer != null) { + timerTask = + new TimerTask() { + @Override + public void run() { + addSessionBreadcrumb("end"); + hub.endSession(); + runningSession.set(false); + } + }; + + timer.schedule(timerTask, sessionIntervalMillis); + } + } } private void cancelTask() { - if (timerTask != null) { - timerTask.cancel(); - timerTask = null; + synchronized (timerLock) { + if (timerTask != null) { + timerTask.cancel(); + timerTask = null; + } } } @@ -147,4 +159,10 @@ AtomicBoolean isRunningSession() { TimerTask getTimerTask() { return timerTask; } + + @TestOnly + @Nullable + Timer getTimer() { + return timer; + } } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java b/sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java index d01db42ef9..3d55bbe789 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java @@ -236,7 +236,7 @@ private void startTracing(final @NotNull View target, final @NotNull String even if (idleTimeout != null) { // reschedule the finish task for the idle transaction, so it keeps running for the same // view - activeTransaction.scheduleFinish(idleTimeout); + activeTransaction.scheduleFinish(); } return; } else { diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/LifecycleWatcherTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/LifecycleWatcherTest.kt index a97e284f27..2a27ba4872 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/LifecycleWatcherTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/LifecycleWatcherTest.kt @@ -181,4 +181,16 @@ class LifecycleWatcherTest { watcher.onStop(fixture.ownerMock) verify(fixture.hub, never()).addBreadcrumb(any()) } + + @Test + fun `timer is created if session tracking is enabled`() { + val watcher = fixture.getSUT(enableAutoSessionTracking = true, enableAppLifecycleBreadcrumbs = false) + assertNotNull(watcher.timer) + } + + @Test + fun `timer is not created if session tracking is disabled`() { + val watcher = fixture.getSUT(enableAutoSessionTracking = false, enableAppLifecycleBreadcrumbs = false) + assertNull(watcher.timer) + } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/internal/gestures/SentryGestureListenerTracingTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/internal/gestures/SentryGestureListenerTracingTest.kt index c68ddb0a81..ecee5ca4ab 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/internal/gestures/SentryGestureListenerTracingTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/internal/gestures/SentryGestureListenerTracingTest.kt @@ -304,7 +304,7 @@ class SentryGestureListenerTracingTest { // second view interaction sut.onSingleTapUp(fixture.event) - verify(fixture.transaction).scheduleFinish(anyOrNull()) + verify(fixture.transaction).scheduleFinish() } internal open class ScrollableListView : AbsListView(mock()) { diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 8752c93f08..e5ae899b70 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -475,7 +475,7 @@ public abstract interface class io/sentry/ITransaction : io/sentry/ISpan { public abstract fun getName ()Ljava/lang/String; public abstract fun getSpans ()Ljava/util/List; public abstract fun isSampled ()Ljava/lang/Boolean; - public abstract fun scheduleFinish (Ljava/lang/Long;)V + public abstract fun scheduleFinish ()V public abstract fun setName (Ljava/lang/String;)V } @@ -667,7 +667,7 @@ public final class io/sentry/NoOpTransaction : io/sentry/ITransaction { public fun getThrowable ()Ljava/lang/Throwable; public fun isFinished ()Z public fun isSampled ()Ljava/lang/Boolean; - public fun scheduleFinish (Ljava/lang/Long;)V + public fun scheduleFinish ()V public fun setData (Ljava/lang/String;Ljava/lang/Object;)V public fun setDescription (Ljava/lang/String;)V public fun setName (Ljava/lang/String;)V @@ -1395,7 +1395,7 @@ public final class io/sentry/SentryTracer : io/sentry/ITransaction { public fun getTimestamp ()Ljava/lang/Double; public fun isFinished ()Z public fun isSampled ()Ljava/lang/Boolean; - public fun scheduleFinish (Ljava/lang/Long;)V + public fun scheduleFinish ()V public fun setData (Ljava/lang/String;Ljava/lang/Object;)V public fun setDescription (Ljava/lang/String;)V public fun setName (Ljava/lang/String;)V diff --git a/sentry/src/main/java/io/sentry/ITransaction.java b/sentry/src/main/java/io/sentry/ITransaction.java index 8006bf4179..2718f8988e 100644 --- a/sentry/src/main/java/io/sentry/ITransaction.java +++ b/sentry/src/main/java/io/sentry/ITransaction.java @@ -51,10 +51,6 @@ public interface ITransaction extends ISpan { @NotNull SentryId getEventId(); - /** - * Schedules when transaction should be automatically finished. - * - * @param idleTimeout - the time to wait before finishing the transaction - */ - void scheduleFinish(final @NotNull Long idleTimeout); + /** Schedules when transaction should be automatically finished. */ + void scheduleFinish(); } diff --git a/sentry/src/main/java/io/sentry/NoOpTransaction.java b/sentry/src/main/java/io/sentry/NoOpTransaction.java index 3ff192c667..0c415449bb 100644 --- a/sentry/src/main/java/io/sentry/NoOpTransaction.java +++ b/sentry/src/main/java/io/sentry/NoOpTransaction.java @@ -63,7 +63,7 @@ public void setName(@NotNull String name) {} } @Override - public void scheduleFinish(@NotNull Long idleTimeout) {} + public void scheduleFinish() {} @Override public boolean isFinished() { diff --git a/sentry/src/main/java/io/sentry/SentryTracer.java b/sentry/src/main/java/io/sentry/SentryTracer.java index 0f6803aabe..e941dc7ba5 100644 --- a/sentry/src/main/java/io/sentry/SentryTracer.java +++ b/sentry/src/main/java/io/sentry/SentryTracer.java @@ -65,7 +65,8 @@ public final class SentryTracer implements ITransaction { private final @Nullable Long idleTimeout; private @Nullable TimerTask timerTask; - private final @NotNull Timer timer = new Timer(true); + private @Nullable Timer timer = null; + private final @NotNull Object timerLock = new Object(); private final @NotNull SpanByTimestampComparator spanByTimestampComparator = new SpanByTimestampComparator(); private final @NotNull AtomicBoolean isFinishTimerRunning = new AtomicBoolean(false); @@ -110,32 +111,39 @@ public SentryTracer( this.transactionFinishedCallback = transactionFinishedCallback; if (idleTimeout != null) { - scheduleFinish(idleTimeout); + timer = new Timer(true); + scheduleFinish(); } } @Override - public void scheduleFinish(final @NotNull Long idleTimeout) { - cancelTimer(); - isFinishTimerRunning.set(true); - timerTask = - new TimerTask() { - @Override - public void run() { - final SpanStatus status = getStatus(); - finish((status != null) ? status : SpanStatus.OK); - isFinishTimerRunning.set(false); - } - }; + public void scheduleFinish() { + synchronized (timerLock) { + cancelTimer(); + if (timer != null) { + isFinishTimerRunning.set(true); + timerTask = + new TimerTask() { + @Override + public void run() { + final SpanStatus status = getStatus(); + finish((status != null) ? status : SpanStatus.OK); + isFinishTimerRunning.set(false); + } + }; - timer.schedule(timerTask, idleTimeout); + timer.schedule(timerTask, idleTimeout); + } + } } private void cancelTimer() { - if (timerTask != null) { - timerTask.cancel(); - isFinishTimerRunning.set(false); - timerTask = null; + synchronized (timerLock) { + if (timerTask != null) { + timerTask.cancel(); + isFinishTimerRunning.set(false); + timerTask = null; + } } } @@ -217,7 +225,7 @@ private ISpan createChild( // so the transaction will either idle and finish itself, or a new child will be // added and we'll wait for it again if (!waitForChildren || hasAllChildrenFinished()) { - scheduleFinish(idleTimeout); + scheduleFinish(); } } else if (finishStatus.isFinishing) { finish(finishStatus.spanStatus); @@ -333,6 +341,14 @@ public void finish(@Nullable SpanStatus status) { if (transactionFinishedCallback != null) { transactionFinishedCallback.execute(this); } + + if (timer != null) { + synchronized (timerLock) { + timer.cancel(); + timer = null; + } + } + if (children.isEmpty() && idleTimeout != null) { // if it's an idle transaction which has no children, we drop it to save user's quota return; @@ -538,6 +554,12 @@ TimerTask getTimerTask() { return timerTask; } + @TestOnly + @Nullable + Timer getTimer() { + return timer; + } + @TestOnly @NotNull AtomicBoolean isFinishTimerRunning() { diff --git a/sentry/src/test/java/io/sentry/SentryTracerTest.kt b/sentry/src/test/java/io/sentry/SentryTracerTest.kt index 79e8823b1b..a38fffe98f 100644 --- a/sentry/src/test/java/io/sentry/SentryTracerTest.kt +++ b/sentry/src/test/java/io/sentry/SentryTracerTest.kt @@ -690,4 +690,24 @@ class SentryTracerTest { anyOrNull() ) } + + @Test + fun `timer is created if idle timeout is set`() { + val transaction = fixture.getSut(waitForChildren = true, idleTimeout = 50, trimEnd = true, sampled = true) + assertNotNull(transaction.timer) + } + + @Test + fun `timer is not created if idle timeout is not set`() { + val transaction = fixture.getSut(waitForChildren = true, idleTimeout = null, trimEnd = true, sampled = true) + assertNull(transaction.timer) + } + + @Test + fun `timer is cancelled on finish`() { + val transaction = fixture.getSut(waitForChildren = true, idleTimeout = 50, trimEnd = true, sampled = true) + assertNotNull(transaction.timer) + transaction.finish(SpanStatus.OK) + assertNull(transaction.timer) + } }