EmbraceAnrService cleanup (#2247)

* EmbraceAnrService cleanup

* Add stacktraceSampler as a private dependency for EmbraceAnrService.kt

* Stop exposing listeners in anrService, they were only used in tests

* make processAnrTick private in EmbraceAnrService.kt, as it's only used in tests

Author: priettt

embrace-android-features/src/main/kotlin/io/embrace/android/embracesdk/internal/anr/EmbraceAnrService.kt

@@ -19,47 +19,31 @@ import java.util.concurrent.TimeUnit
  * Checks whether the target thread is still responding by using the following strategy:
  *
  *  1. Creating a [android.os.Handler], on the target thread, and an executor on a monitor thread
- *  1. Using the 'monitoring' thread to message the target thread with a heartbeat
- *  1. Determining whether the target thread responds in time, and if not logging an ANR
+ *  2. Using the 'monitoring' thread to message the target thread with a heartbeat
+ *  3. Determining whether the target thread responds in time, and if not logging an ANR
  */
 internal class EmbraceAnrService(
     private val configService: ConfigService,
-    looper: Looper,
-    logger: EmbLogger,
-    livenessCheckScheduler: LivenessCheckScheduler,
+    private val looper: Looper,
+    private val logger: EmbLogger,
+    private val livenessCheckScheduler: LivenessCheckScheduler,
     private val anrMonitorWorker: BackgroundWorker,
-    state: ThreadMonitoringState,
-    val clock: Clock,
+    private val state: ThreadMonitoringState,
+    private val clock: Clock,
+    private val stacktraceSampler: AnrStacktraceSampler,
 ) : AnrService, MemoryCleanerListener, ProcessStateListener, BlockedThreadListener {
 
-    private val state: ThreadMonitoringState
-    private val targetThread: Thread
-    val stacktraceSampler: AnrStacktraceSampler
-    private val logger: EmbLogger
-    private val targetThreadHeartbeatScheduler: LivenessCheckScheduler
-
-    val listeners: CopyOnWriteArrayList<BlockedThreadListener> = CopyOnWriteArrayList<BlockedThreadListener>()
+    private val listeners: CopyOnWriteArrayList<BlockedThreadListener> = CopyOnWriteArrayList<BlockedThreadListener>()
 
     init {
-        targetThread = looper.thread
-        this.logger = logger
-        this.state = state
-        targetThreadHeartbeatScheduler = livenessCheckScheduler
-        stacktraceSampler = AnrStacktraceSampler(
-            configService,
-            clock,
-            targetThread,
-            anrMonitorWorker
-        )
-
         // add listeners
         listeners.add(stacktraceSampler)
         livenessCheckScheduler.listener = this
     }
 
     override fun startAnrCapture() {
         this.anrMonitorWorker.submit {
-            targetThreadHeartbeatScheduler.startMonitoringThread()
+            livenessCheckScheduler.startMonitoringThread()
         }
     }
 
@@ -89,7 +73,7 @@ internal class EmbraceAnrService(
 
     override fun handleCrash(crashId: String) {
         this.anrMonitorWorker.submit {
-            targetThreadHeartbeatScheduler.stopMonitoringThread()
+            livenessCheckScheduler.stopMonitoringThread()
         }
     }
 
@@ -116,26 +100,14 @@ internal class EmbraceAnrService(
         }
     }
 
-    fun processAnrTick(timestamp: Long) {
-        // Check if ANR capture is enabled
-        if (!configService.anrBehavior.isAnrCaptureEnabled()) {
-            return
-        }
-
-        // Invoke callbacks
-        for (listener in listeners) {
-            listener.onThreadBlockedInterval(targetThread, timestamp)
-        }
-    }
-
     /**
      * When app goes to foreground, we need to monitor the target thread again to
      * capture ANRs.
      */
     override fun onForeground(coldStart: Boolean, timestamp: Long) {
         this.anrMonitorWorker.submit {
             state.resetState()
-            targetThreadHeartbeatScheduler.startMonitoringThread()
+            livenessCheckScheduler.startMonitoringThread()
         }
     }
 
@@ -146,7 +118,19 @@ internal class EmbraceAnrService(
      */
     override fun onBackground(timestamp: Long) {
         this.anrMonitorWorker.submit {
-            targetThreadHeartbeatScheduler.stopMonitoringThread()
+            livenessCheckScheduler.stopMonitoringThread()
+        }
+    }
+
+    private fun processAnrTick(timestamp: Long) {
+        // Check if ANR capture is enabled
+        if (!configService.anrBehavior.isAnrCaptureEnabled()) {
+            return
+        }
+
+        // Invoke callbacks
+        for (listener in listeners) {
+            listener.onThreadBlockedInterval(looper.thread, timestamp)
         }
     }
 

embrace-android-features/src/main/kotlin/io/embrace/android/embracesdk/internal/injection/AnrModuleImpl.kt

@@ -3,6 +3,7 @@ package io.embrace.android.embracesdk.internal.injection
 import android.os.Looper
 import io.embrace.android.embracesdk.internal.anr.AnrOtelMapper
 import io.embrace.android.embracesdk.internal.anr.AnrService
+import io.embrace.android.embracesdk.internal.anr.AnrStacktraceSampler
 import io.embrace.android.embracesdk.internal.anr.EmbraceAnrService
 import io.embrace.android.embracesdk.internal.anr.detection.BlockedThreadDetector
 import io.embrace.android.embracesdk.internal.anr.detection.LivenessCheckScheduler
@@ -31,7 +32,8 @@ internal class AnrModuleImpl(
                 livenessCheckScheduler = livenessCheckScheduler,
                 anrMonitorWorker = anrMonitorWorker,
                 state = state,
-                clock = initModule.clock
+                clock = initModule.clock,
+                stacktraceSampler = stacktraceSampler
             )
         } else {
             null
@@ -50,6 +52,15 @@ internal class AnrModuleImpl(
 
     private val state by singleton { ThreadMonitoringState(initModule.clock) }
 
+    private val stacktraceSampler by singleton {
+        AnrStacktraceSampler(
+            configService = configService,
+            clock = initModule.clock,
+            targetThread = looper.thread,
+            anrMonitorWorker = anrMonitorWorker
+        )
+    }
+
     private val targetThreadHandler by singleton {
         TargetThreadHandler(
             looper = looper,

embrace-android-features/src/test/java/io/embrace/android/embracesdk/internal/anr/EmbraceAnrServiceRule.kt

@@ -37,6 +37,7 @@ internal class EmbraceAnrServiceRule<T : ScheduledExecutorService>(
     lateinit var anrExecutorService: T
     lateinit var targetThreadHandler: TargetThreadHandler
     lateinit var anrMonitorThread: AtomicReference<Thread>
+    lateinit var stacktraceSampler: AnrStacktraceSampler
 
     override fun before() {
         clock.setCurrentTime(0)
@@ -68,14 +69,21 @@ internal class EmbraceAnrServiceRule<T : ScheduledExecutorService>(
             blockedThreadDetector = blockedThreadDetector,
             logger = logger
         )
+        stacktraceSampler = AnrStacktraceSampler(
+            configService = fakeConfigService,
+            clock = clock,
+            targetThread = looper.thread,
+            anrMonitorWorker = worker
+        )
         anrService = EmbraceAnrService(
             configService = fakeConfigService,
             looper = looper,
             logger = logger,
             livenessCheckScheduler = livenessCheckScheduler,
             anrMonitorWorker = worker,
             state = state,
-            clock = clock
+            clock = clock,
+            stacktraceSampler = stacktraceSampler
         )
     }
 }

embrace-android-features/src/test/java/io/embrace/android/embracesdk/internal/anr/EmbraceAnrServiceTest.kt

@@ -62,8 +62,13 @@ internal class EmbraceAnrServiceTest {
         with(rule) {
             val listener = FakeBlockedThreadListener()
             anrService.addBlockedThreadListener(listener)
-            assertEquals(anrService, blockedThreadDetector.listener)
-            assertTrue(anrService.listeners.contains(listener))
+
+            // Test that the listener actually gets notified when thread blocking events occur
+            anrService.onThreadBlocked(currentThread(), 1000L)
+            assertEquals(1, listener.blockedCount)
+
+            anrService.onThreadUnblocked(currentThread(), 2000L)
+            assertEquals(1, listener.unblockedCount)
         }
     }
 
@@ -75,15 +80,15 @@ internal class EmbraceAnrServiceTest {
             anrService.onForeground(true, 0L)
 
             // assert no ANR interval was added
-            assertEquals(0, anrService.stacktraceSampler.anrIntervals.size)
+            assertEquals(0, stacktraceSampler.anrIntervals.size)
         }
     }
 
     @Test
     fun testCleanCollections() {
         with(rule) {
             // assert the ANR interval was added
-            val anrIntervals = anrService.stacktraceSampler.anrIntervals
+            val anrIntervals = stacktraceSampler.anrIntervals
             anrIntervals.add(AnrInterval(startTime = 15000000, endTime = 15000100))
             val inProgressInterval = AnrInterval(startTime = 15000000, lastKnownTime = 15000100)
             anrIntervals.add(inProgressInterval)
@@ -101,7 +106,7 @@ internal class EmbraceAnrServiceTest {
     @Test
     fun testGetIntervals() {
         with(rule) {
-            populateAnrIntervals(anrService)
+            populateAnrIntervals()
 
             val anrIntervals = anrService.getCapturedData()
             assertEquals(5, anrIntervals.size)
@@ -153,8 +158,8 @@ internal class EmbraceAnrServiceTest {
             blockedThreadDetector.listener = anrService
             state.anrInProgress = true
             state.lastTargetThreadResponseMs = 15000000L
-            anrService.processAnrTick(clock.now())
-            assertEquals(1, anrService.stacktraceSampler.size())
+            anrService.onThreadBlockedInterval(currentThread(), clock.now())
+            assertEquals(1, stacktraceSampler.size())
 
             // assert only one anr interval was added from the anrInProgress flag
             val anrIntervals = anrService.getCapturedData()
@@ -257,10 +262,9 @@ internal class EmbraceAnrServiceTest {
                 }
                 anrService.onThreadUnblocked(currentThread(), clock.now())
 
-                val sampler = anrService.stacktraceSampler
-                assertEquals(1, sampler.anrIntervals.size)
+                assertEquals(1, stacktraceSampler.anrIntervals.size)
 
-                val interval = checkNotNull(sampler.anrIntervals.first())
+                val interval = checkNotNull(stacktraceSampler.anrIntervals.first())
                 val samples = checkNotNull(interval.anrSampleList).samples
                 assertEquals(count, samples.size)
 
@@ -287,8 +291,8 @@ internal class EmbraceAnrServiceTest {
             // create an ANR service with config that disables ANR capture
             rule.anrBehavior.anrCaptureEnabled = false
             clock.setCurrentTime(15020000L)
-            anrService.processAnrTick(clock.now())
-            assertEquals(0, anrService.stacktraceSampler.size())
+            anrService.onThreadBlockedInterval(currentThread(), clock.now())
+            assertEquals(0, stacktraceSampler.size())
 
             // assert no anr intervals were added
             val anrIntervals = anrService.getCapturedData()
@@ -300,16 +304,15 @@ internal class EmbraceAnrServiceTest {
     fun testReachedAnrCaptureLimit() {
         with(rule) {
             rule.anrBehavior.anrPerSessionImpl = 3
-            val state = anrService.stacktraceSampler
-            assertFalse(state.reachedAnrStacktraceCaptureLimit())
+            assertFalse(stacktraceSampler.reachedAnrStacktraceCaptureLimit())
 
-            state.anrIntervals.add(AnrInterval(0, anrSampleList = AnrSampleList(listOf())))
-            state.anrIntervals.add(AnrInterval(0, anrSampleList = AnrSampleList(listOf())))
-            state.anrIntervals.add(AnrInterval(0, anrSampleList = AnrSampleList(listOf())))
-            assertFalse(state.reachedAnrStacktraceCaptureLimit())
+            stacktraceSampler.anrIntervals.add(AnrInterval(0, anrSampleList = AnrSampleList(listOf())))
+            stacktraceSampler.anrIntervals.add(AnrInterval(0, anrSampleList = AnrSampleList(listOf())))
+            stacktraceSampler.anrIntervals.add(AnrInterval(0, anrSampleList = AnrSampleList(listOf())))
+            assertFalse(stacktraceSampler.reachedAnrStacktraceCaptureLimit())
 
-            state.anrIntervals.add(AnrInterval(0, anrSampleList = AnrSampleList(listOf())))
-            assertTrue(state.reachedAnrStacktraceCaptureLimit())
+            stacktraceSampler.anrIntervals.add(AnrInterval(0, anrSampleList = AnrSampleList(listOf())))
+            assertTrue(stacktraceSampler.reachedAnrStacktraceCaptureLimit())
         }
     }
 
@@ -401,21 +404,23 @@ internal class EmbraceAnrServiceTest {
             anrExecutorService.submit {
                 assertTrue(state.started.get())
             }
-            populateAnrIntervals(anrService)
+            populateAnrIntervals()
             anrService.handleCrash("")
             val anrIntervals = anrService.getCapturedData()
             assertEquals(5, anrIntervals.size)
             assertFalse(state.started.get())
         }
     }
 
-    private fun populateAnrIntervals(anrService: EmbraceAnrService) {
-        val state = anrService.stacktraceSampler
-        state.anrIntervals.add(AnrInterval(startTime = 14000000L))
-        state.anrIntervals.add(AnrInterval(startTime = 15000000L))
-        state.anrIntervals.add(AnrInterval(startTime = 15000500L))
-        state.anrIntervals.add(AnrInterval(startTime = 15001000L))
-        state.anrIntervals.add(AnrInterval(startTime = 16000000L))
+    private fun populateAnrIntervals() {
+        with(rule) {
+            val state = stacktraceSampler
+            state.anrIntervals.add(AnrInterval(startTime = 14000000L))
+            state.anrIntervals.add(AnrInterval(startTime = 15000000L))
+            state.anrIntervals.add(AnrInterval(startTime = 15000500L))
+            state.anrIntervals.add(AnrInterval(startTime = 15001000L))
+            state.anrIntervals.add(AnrInterval(startTime = 16000000L))
+        }
     }
 
     /**