From cd4ff3473e780fde4c485c5132c2babc2bea7860 Mon Sep 17 00:00:00 2001 From: Abhay Sood Date: Thu, 28 Nov 2024 08:38:50 +0530 Subject: [PATCH] fix(android): initialization issues * update log statement formatting * add reentrant lock to heartbeat to avoid any potential race conditions * use reentrant lock for SDK start/stop * disable power state, network changes collection in background * standardize function names for register/unregister/pause/resume * unregister AppLifecycleCollector correctly --- .../android/NoopPeriodicEventExporter.kt | 4 +- .../sh/measure/android/MeasureInternal.kt | 27 +++++++----- .../measure/android/events/EventProcessor.kt | 4 +- .../sh/measure/android/exporter/Heartbeat.kt | 41 +++++++++++-------- .../android/exporter/PeriodicEventExporter.kt | 13 ++++-- .../networkchange/NetworkChangesCollector.kt | 5 --- .../exporter/PeriodicEventExporterTest.kt | 16 ++++---- 7 files changed, 62 insertions(+), 48 deletions(-) diff --git a/android/measure/src/androidTest/java/sh/measure/android/NoopPeriodicEventExporter.kt b/android/measure/src/androidTest/java/sh/measure/android/NoopPeriodicEventExporter.kt index 3aa354e44..8351f0f50 100644 --- a/android/measure/src/androidTest/java/sh/measure/android/NoopPeriodicEventExporter.kt +++ b/android/measure/src/androidTest/java/sh/measure/android/NoopPeriodicEventExporter.kt @@ -3,11 +3,11 @@ package sh.measure.android import sh.measure.android.exporter.PeriodicEventExporter internal class NoopPeriodicEventExporter : PeriodicEventExporter { - override fun onAppForeground() { + override fun resume() { // No-op } - override fun onAppBackground() { + override fun pause() { // No-op } diff --git a/android/measure/src/main/java/sh/measure/android/MeasureInternal.kt b/android/measure/src/main/java/sh/measure/android/MeasureInternal.kt index b19c8a6d9..22b78d78c 100644 --- a/android/measure/src/main/java/sh/measure/android/MeasureInternal.kt +++ b/android/measure/src/main/java/sh/measure/android/MeasureInternal.kt @@ -3,6 +3,8 @@ package sh.measure.android import android.os.Build import sh.measure.android.lifecycle.AppLifecycleListener import sh.measure.android.logger.LogLevel +import java.util.concurrent.locks.ReentrantLock +import kotlin.concurrent.withLock /** * Initializes the Measure SDK and hides the internal dependencies from public API. @@ -41,7 +43,7 @@ internal class MeasureInternal(measureInitializer: MeasureInitializer) : AppLife private val dataCleanupService by lazy { measureInitializer.dataCleanupService } private val powerStateProvider by lazy { measureInitializer.powerStateProvider } private var isStarted: Boolean = false - private var startLock = Any() + private var startLock = ReentrantLock() fun init() { logger.log(LogLevel.Debug, "Starting Measure SDK") @@ -75,7 +77,7 @@ internal class MeasureInternal(measureInitializer: MeasureInitializer) : AppLife } fun start() { - synchronized(startLock) { + startLock.withLock { if (!isStarted) { registerCollectors() isStarted = true @@ -84,7 +86,7 @@ internal class MeasureInternal(measureInitializer: MeasureInitializer) : AppLife } fun stop() { - synchronized(startLock) { + startLock.withLock { if (isStarted) { unregisterCollectors() isStarted = false @@ -114,30 +116,34 @@ internal class MeasureInternal(measureInitializer: MeasureInitializer) : AppLife gestureCollector.register() networkChangesCollector.register() httpEventCollector.register() + powerStateProvider.register() + periodicEventExporter.resume() } override fun onAppForeground() { // session manager must be the first to be notified about app foreground to ensure that // new session ID (if created) is reflected in all events collected after the launch. sessionManager.onAppForeground() - synchronized(startLock) { + startLock.withLock { if (isStarted) { powerStateProvider.register() + networkChangesCollector.register() cpuUsageCollector.resume() memoryUsageCollector.resume() - periodicEventExporter.onAppForeground() + periodicEventExporter.resume() } } } override fun onAppBackground() { sessionManager.onAppBackground() - synchronized(startLock) { + startLock.withLock { if (isStarted) { cpuUsageCollector.pause() memoryUsageCollector.pause() - periodicEventExporter.onAppBackground() + periodicEventExporter.pause() powerStateProvider.unregister() + networkChangesCollector.unregister() dataCleanupService.clearStaleData() if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) { appExitCollector.collect() @@ -195,15 +201,16 @@ internal class MeasureInternal(measureInitializer: MeasureInitializer) : AppLife private fun unregisterCollectors() { unhandledExceptionCollector.unregister() anrCollector.unregister() + userTriggeredEventCollector.unregister() activityLifecycleCollector.unregister() - appLifecycleCollector.register() + appLifecycleCollector.unregister() cpuUsageCollector.pause() memoryUsageCollector.pause() componentCallbacksCollector.unregister() gestureCollector.unregister() networkChangesCollector.unregister() - periodicEventExporter.unregister() - userTriggeredEventCollector.unregister() httpEventCollector.unregister() + powerStateProvider.unregister() + periodicEventExporter.unregister() } } diff --git a/android/measure/src/main/java/sh/measure/android/events/EventProcessor.kt b/android/measure/src/main/java/sh/measure/android/events/EventProcessor.kt index 7fe369a12..a12c9857b 100644 --- a/android/measure/src/main/java/sh/measure/android/events/EventProcessor.kt +++ b/android/measure/src/main/java/sh/measure/android/events/EventProcessor.kt @@ -162,7 +162,7 @@ internal class EventProcessorImpl( onEventTracked(event) sessionManager.markCrashedSession(event.sessionId) exceptionExporter.export(event.sessionId) - logger.log(LogLevel.Debug, "Event processed: $type, ${event.sessionId}") + logger.log(LogLevel.Debug, "Event processed: $type, ${event.id}") } ?: logger.log(LogLevel.Debug, "Event dropped: $type") } @@ -202,7 +202,7 @@ internal class EventProcessorImpl( onEventTracked(event) logger.log( LogLevel.Debug, - "Event processed: ${event.type}:${event.id}", + "Event processed: ${event.type}, ${event.id}", ) }) } else { diff --git a/android/measure/src/main/java/sh/measure/android/exporter/Heartbeat.kt b/android/measure/src/main/java/sh/measure/android/exporter/Heartbeat.kt index 75515ef41..eae387315 100644 --- a/android/measure/src/main/java/sh/measure/android/exporter/Heartbeat.kt +++ b/android/measure/src/main/java/sh/measure/android/exporter/Heartbeat.kt @@ -8,6 +8,8 @@ import java.util.concurrent.CopyOnWriteArrayList import java.util.concurrent.Future import java.util.concurrent.RejectedExecutionException import java.util.concurrent.TimeUnit +import java.util.concurrent.locks.ReentrantLock +import kotlin.concurrent.withLock internal interface HeartbeatListener { fun pulse() @@ -27,6 +29,7 @@ internal class HeartbeatImpl( private val scheduler: MeasureExecutorService, ) : Heartbeat { private var future: Future<*>? = null + private val lock = ReentrantLock() @VisibleForTesting internal val listeners = CopyOnWriteArrayList() @@ -36,26 +39,30 @@ internal class HeartbeatImpl( } override fun start(intervalMs: Long, initialDelayMs: Long) { - if (future != null) { - return - } - future = try { - scheduler.scheduleAtFixedRate( - { - listeners.forEach(HeartbeatListener::pulse) - }, - initialDelayMs, - intervalMs, - TimeUnit.MILLISECONDS, - ) - } catch (e: RejectedExecutionException) { - logger.log(LogLevel.Error, "Failed to start ExportHeartbeat", e) - return + lock.withLock { + if (future != null) { + return + } + try { + future = scheduler.scheduleAtFixedRate( + { + listeners.forEach(HeartbeatListener::pulse) + }, + initialDelayMs, + intervalMs, + TimeUnit.MILLISECONDS, + ) + } catch (e: RejectedExecutionException) { + logger.log(LogLevel.Error, "Failed to start ExportHeartbeat", e) + return + } } } override fun stop() { - future?.cancel(false) - future = null + lock.withLock { + future?.cancel(false) + future = null + } } } diff --git a/android/measure/src/main/java/sh/measure/android/exporter/PeriodicEventExporter.kt b/android/measure/src/main/java/sh/measure/android/exporter/PeriodicEventExporter.kt index 98fe690d7..43f49fb51 100644 --- a/android/measure/src/main/java/sh/measure/android/exporter/PeriodicEventExporter.kt +++ b/android/measure/src/main/java/sh/measure/android/exporter/PeriodicEventExporter.kt @@ -10,8 +10,9 @@ import java.util.concurrent.RejectedExecutionException import java.util.concurrent.atomic.AtomicBoolean internal interface PeriodicEventExporter { - fun onAppForeground() - fun onAppBackground() + fun register() + fun resume() + fun pause() fun unregister() } @@ -41,7 +42,11 @@ internal class PeriodicEventExporterImpl( exportEvents() } - override fun onAppForeground() { + override fun register() { + heartbeat.start(intervalMs = configProvider.eventsBatchingIntervalMs) + } + + override fun resume() { heartbeat.start(intervalMs = configProvider.eventsBatchingIntervalMs) } @@ -49,7 +54,7 @@ internal class PeriodicEventExporterImpl( heartbeat.stop() } - override fun onAppBackground() { + override fun pause() { heartbeat.stop() exportEvents() } diff --git a/android/measure/src/main/java/sh/measure/android/networkchange/NetworkChangesCollector.kt b/android/measure/src/main/java/sh/measure/android/networkchange/NetworkChangesCollector.kt index c4a920c5d..38c23242d 100644 --- a/android/measure/src/main/java/sh/measure/android/networkchange/NetworkChangesCollector.kt +++ b/android/measure/src/main/java/sh/measure/android/networkchange/NetworkChangesCollector.kt @@ -109,11 +109,6 @@ internal class NetworkChangesCollector( systemServiceProvider.connectivityManager?.unregisterNetworkCallback(callback) networkCallback = null } - currentNetworkType = NetworkType.UNKNOWN - currentNetworkGeneration = NetworkGeneration.UNKNOWN - networkStateProvider.setNetworkState( - NetworkState(NetworkType.UNKNOWN, NetworkGeneration.UNKNOWN, NetworkProvider.UNKNOWN), - ) } @RequiresApi(Build.VERSION_CODES.M) diff --git a/android/measure/src/test/java/sh/measure/android/exporter/PeriodicEventExporterTest.kt b/android/measure/src/test/java/sh/measure/android/exporter/PeriodicEventExporterTest.kt index 91568400f..1fce73dad 100644 --- a/android/measure/src/test/java/sh/measure/android/exporter/PeriodicEventExporterTest.kt +++ b/android/measure/src/test/java/sh/measure/android/exporter/PeriodicEventExporterTest.kt @@ -40,7 +40,7 @@ class PeriodicEventExporterTest { @Test fun `starts heartbeat when app comes to foreground with a delay`() { - periodicEventExporter.onAppForeground() + periodicEventExporter.resume() verify(heartbeat, atMostOnce()).start( configProvider.eventsBatchingIntervalMs, @@ -57,7 +57,7 @@ class PeriodicEventExporterTest { @Test fun `stops heartbeat when app goes to background`() { - periodicEventExporter.onAppBackground() + periodicEventExporter.pause() verify(heartbeat, atMostOnce()).stop() } @@ -71,7 +71,7 @@ class PeriodicEventExporterTest { batches[batch2.first] = batch2.second `when`(eventExporter.getExistingBatches()).thenReturn(batches) - periodicEventExporter.onAppBackground() + periodicEventExporter.pause() verify(eventExporter).export(batch1.first, batch1.second) verify(eventExporter).export(batch2.first, batch2.second) @@ -92,7 +92,7 @@ class PeriodicEventExporterTest { ), ).thenReturn(HttpResponse.Error.ServerError(500)) - periodicEventExporter.onAppBackground() + periodicEventExporter.pause() verify(eventExporter).export(batch1.first, batch1.second) verify(eventExporter, never()).export(batch2.first, batch2.second) @@ -113,7 +113,7 @@ class PeriodicEventExporterTest { ), ).thenReturn(HttpResponse.Error.RateLimitError()) - periodicEventExporter.onAppBackground() + periodicEventExporter.pause() verify(eventExporter).export(batch1.first, batch1.second) verify(eventExporter, never()).export(batch2.first, batch2.second) @@ -129,7 +129,7 @@ class PeriodicEventExporterTest { ) // When - periodicEventExporter.onAppBackground() + periodicEventExporter.pause() // Then verify(eventExporter).export("batchId", listOf("event1", "event2")) @@ -146,7 +146,7 @@ class PeriodicEventExporterTest { testClock.advance(Duration.ofSeconds(29)) // When - periodicEventExporter.onAppBackground() + periodicEventExporter.pause() // Then verify(eventExporter, never()).export(any(), any>()) @@ -163,7 +163,7 @@ class PeriodicEventExporterTest { // forcefully mark an export in progress periodicEventExporter.isExportInProgress.set(true) - periodicEventExporter.onAppBackground() + periodicEventExporter.pause() verify(eventExporter, never()).export(any(), any()) }