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
getsentry · Jun 23, 2022 · 5127b6e · 5127b6e
1 parent b5ef9c5
commit 5127b6e
Show file tree

Hide file tree

Showing 10 changed files with 122 additions and 48 deletions.
diff --git 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
@@ -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/...id-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java b/...id-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
@@ -181,4 +181,16 @@ class LifecycleWatcherTest {
         watcher.onStop(fixture.ownerMock)
         verify(fixture.hub, never()).addBreadcrumb(any<Breadcrumb>())
     }
+
+    @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/...rc/test/java/io/sentry/android/core/internal/gestures/SentryGestureListenerTracingTest.kt b/...rc/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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)
+    }
 }