Skip to content

Commit

Permalink
fix(android): initialization issues
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@abhaysood
abhaysood committed Nov 28, 2024
1 parent 27b11b8 commit cd4ff34
Show file tree
Hide file tree
Showing 7 changed files with 62 additions and 48 deletions.
4 changes: 2 additions & 2 deletions android/measure/src/androidTest/java/sh/measure/android/NoopPeriodicEventExporter.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
27 changes: 17 additions & 10 deletions android/measure/src/main/java/sh/measure/android/MeasureInternal.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -75,7 +77,7 @@ internal class MeasureInternal(measureInitializer: MeasureInitializer) : AppLife
}

fun start() {
synchronized(startLock) {
startLock.withLock {
if (!isStarted) {
registerCollectors()
isStarted = true
Expand All @@ -84,7 +86,7 @@ internal class MeasureInternal(measureInitializer: MeasureInitializer) : AppLife
}

fun stop() {
synchronized(startLock) {
startLock.withLock {
if (isStarted) {
unregisterCollectors()
isStarted = false
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
}
}
4 changes: 2 additions & 2 deletions android/measure/src/main/java/sh/measure/android/events/EventProcessor.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand Down Expand Up @@ -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 {
Expand Down
41 changes: 24 additions & 17 deletions android/measure/src/main/java/sh/measure/android/exporter/Heartbeat.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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<HeartbeatListener>()
Expand All @@ -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
}
}
}
13 changes: 9 additions & 4 deletions android/measure/src/main/java/sh/measure/android/exporter/PeriodicEventExporter.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}

Expand Down Expand Up @@ -41,15 +42,19 @@ internal class PeriodicEventExporterImpl(
exportEvents()
}

override fun onAppForeground() {
override fun register() {
heartbeat.start(intervalMs = configProvider.eventsBatchingIntervalMs)
}

override fun resume() {
heartbeat.start(intervalMs = configProvider.eventsBatchingIntervalMs)
}

override fun unregister() {
heartbeat.stop()
}

override fun onAppBackground() {
override fun pause() {
heartbeat.stop()
exportEvents()
}
Expand Down
5 changes: 0 additions & 5 deletions android/measure/src/main/java/sh/measure/android/networkchange/NetworkChangesCollector.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
16 changes: 8 additions & 8 deletions android/measure/src/test/java/sh/measure/android/exporter/PeriodicEventExporterTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -57,7 +57,7 @@ class PeriodicEventExporterTest {

@Test
fun `stops heartbeat when app goes to background`() {
periodicEventExporter.onAppBackground()
periodicEventExporter.pause()

verify(heartbeat, atMostOnce()).stop()
}
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -129,7 +129,7 @@ class PeriodicEventExporterTest {
)

// When
periodicEventExporter.onAppBackground()
periodicEventExporter.pause()

// Then
verify(eventExporter).export("batchId", listOf("event1", "event2"))
Expand All @@ -146,7 +146,7 @@ class PeriodicEventExporterTest {
testClock.advance(Duration.ofSeconds(29))

// When
periodicEventExporter.onAppBackground()
periodicEventExporter.pause()

// Then
verify(eventExporter, never()).export(any<String>(), any<List<String>>())
Expand All @@ -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())
}
Expand Down

0 comments on commit cd4ff34

Please sign in to comment.