Merge pull request #310 from bugsnag/PLAT-13071/strict-mode-fixes

Strict mode fixes

CHANGELOG.md

@@ -1,3 +1,10 @@
+## TBD
+
+### Bug fixes
+
+* Move remaining I/O to the worker thread in `BugsnagPerformance.start` - avoiding StrictMode violations
+  [#310](https://github.com/bugsnag/bugsnag-android-performance/pull/310)
+
 ## 1.10.0 (2024-11-14)
 
 ### Bug fixes

bugsnag-android-performance/src/main/kotlin/com/bugsnag/android/performance/BugsnagPerformance.kt

@@ -106,69 +106,66 @@ public object BugsnagPerformance {
             it.copy(isInForeground = isInForeground(application))
         }
 
-        val connectivity =
-            Connectivity.newInstance(application) { status ->
-                if (status.hasConnection && this::worker.isInitialized) {
-                    worker.wake()
+        val bsgWorker = Worker {
+            val resourceAttributes = createResourceAttributes(configuration)
+            LoadDeviceId(application, resourceAttributes).run()
+
+            val connectivity =
+                Connectivity.newInstance(application) { status ->
+                    if (status.hasConnection && this::worker.isInitialized) {
+                        worker.wake()
+                    }
+
+                    instrumentedAppState.defaultAttributeSource.update {
+                        it.copy(
+                            networkType = status.networkType,
+                            networkSubType = status.networkSubType,
+                        )
+                    }
                 }
 
-                instrumentedAppState.defaultAttributeSource.update {
-                    it.copy(
-                        networkType = status.networkType,
-                        networkSubType = status.networkSubType,
+            connectivity.registerForNetworkChanges()
+
+            val httpDelivery = HttpDelivery(
+                configuration.endpoint,
+                requireNotNull(configuration.apiKey) {
+                    "PerformanceConfiguration.apiKey may not be null"
+                },
+                connectivity,
+                configuration.samplingProbability != null,
+                configuration,
+            )
+
+            val persistence = Persistence(application)
+            val delivery = RetryDelivery(persistence.retryQueue, httpDelivery)
+
+            val workerTasks = ArrayList<Task>()
+            if (configuration.isReleaseStageEnabled) {
+                val sampler: ProbabilitySampler
+                if (configuration.samplingProbability == null) {
+                    sampler = ProbabilitySampler(1.0)
+
+                    val samplerTask = SamplerTask(
+                        delivery,
+                        sampler,
+                        persistence.persistentState,
                     )
+                    delivery.newProbabilityCallback = samplerTask
+                    workerTasks.add(samplerTask)
+                } else {
+                    sampler = ProbabilitySampler(configuration.samplingProbability)
                 }
-            }
 
-        connectivity.registerForNetworkChanges()
-
-        val httpDelivery = HttpDelivery(
-            configuration.endpoint,
-            requireNotNull(configuration.apiKey) {
-                "PerformanceConfiguration.apiKey may not be null"
-            },
-            connectivity,
-            configuration.samplingProbability != null,
-            configuration,
-        )
-
-        val persistence = Persistence(application)
-        val delivery = RetryDelivery(persistence.retryQueue, httpDelivery)
-
-        val workerTasks = ArrayList<Task>()
-
-        if (configuration.isReleaseStageEnabled) {
-            val sampler: ProbabilitySampler
-            if (configuration.samplingProbability == null) {
-                sampler = ProbabilitySampler(1.0)
-
-                val samplerTask = SamplerTask(
-                    delivery,
-                    sampler,
-                    persistence.persistentState,
-                )
-                delivery.newProbabilityCallback = samplerTask
-                workerTasks.add(samplerTask)
+                tracer.sampler = sampler
             } else {
-                sampler = ProbabilitySampler(configuration.samplingProbability)
+                tracer.sampler = DiscardingSampler
             }
 
-            tracer.sampler = sampler
-        } else {
-            tracer.sampler = DiscardingSampler
-        }
+            workerTasks.add(SendBatchTask(delivery, tracer, resourceAttributes))
+            workerTasks.add(RetryDeliveryTask(persistence.retryQueue, httpDelivery, connectivity))
 
-        val resourceAttributes = createResourceAttributes(configuration)
-        workerTasks.add(SendBatchTask(delivery, tracer, resourceAttributes))
-        workerTasks.add(RetryDeliveryTask(persistence.retryQueue, httpDelivery, connectivity))
-
-        val bsgWorker = Worker(
-            startupTasks =
-            listOf(
-                LoadDeviceId(application, resourceAttributes),
-            ),
-            tasks = workerTasks,
-        )
+            return@Worker workerTasks
+        }
 
         // register the Worker with the components that depend on it
         tracer.worker = bsgWorker

bugsnag-android-performance/src/main/kotlin/com/bugsnag/android/performance/internal/Worker.kt

@@ -32,14 +32,13 @@ internal abstract class AbstractTask : Task {
 
 internal class Worker(
     /**
-     * A list of `Runnable`s that need to be run (once each) before the worker tasks are run.
-     * These are typically IO related startup tasks that need to be completed before any spans
-     * are actually sent.
+     * The initiator function to create and return all of the `Task`s to be run by the `Worker`.
+     * This function will be run on the `Worker` thread.
      */
-    startupTasks: List<Runnable>,
-    private val tasks: List<Task>,
+    private var startup: () -> List<Task> = { emptyList() },
 ) : Runnable {
-    private var startupTasks: List<Runnable>? = startupTasks
+
+    private lateinit var tasks: List<Task>
 
     private val lock = ReentrantLock(false)
     private val wakeWorker = lock.newCondition()
@@ -103,10 +102,10 @@ internal class Worker(
     }
 
     private fun runStartupTasks() {
-        startupTasks?.forEach { it.run() }
+        tasks = startup()
 
-        // release the startup tasks so we don't hog the memory for ever
-        startupTasks = null
+        // release the startup task so we don't hog the memory for ever
+        startup = { emptyList() }
     }
 
     private fun attachTasks() {

bugsnag-android-performance/src/test/java/com/bugsnag/android/performance/internal/WorkerTest.kt

@@ -22,7 +22,7 @@ class WorkerTest {
     fun testWaitAndWake() {
         val count1 = CountingTask(1)
         val count2 = CountingTask(5)
-        val worker = Worker(emptyList(), listOf(count1, count2))
+        val worker = Worker { listOf(count1, count2) }
 
         worker.start()
         try {
@@ -40,7 +40,7 @@ class WorkerTest {
     fun testWaitAndAwake() {
         val count1 = CountingTask(1)
         val count2 = CountingTask(5)
-        val worker = Worker(emptyList(), listOf(count1, count2))
+        val worker = Worker { listOf(count1, count2) }
 
         worker.start()
         try {
@@ -69,8 +69,7 @@ class WorkerTest {
     @Test
     fun testExceptionHandling() {
         val latch = CountDownLatch(1)
-        val worker = Worker(
-            emptyList(),
+        val worker = Worker {
             listOf(
                 object : Task {
                     override fun execute(): Boolean {
@@ -83,8 +82,8 @@ class WorkerTest {
                         return false
                     }
                 },
-            ),
-        )
+            )
+        }
 
         // this test succeeds if the latch releases, as it means that the exception in the first
         // task was correctly handled without blocking the second task

features/fixtures/mazeracer/app/detekt-baseline.xml

@@ -6,7 +6,6 @@
     <ID>MagicNumber:AppBackgroundedScenario.kt$AppBackgroundedScenario$120_000L</ID>
     <ID>MagicNumber:AppStartScenario.kt$AppStartScenario$5000L</ID>
     <ID>MagicNumber:AppStartScenario.kt$AppStartScenario$500L</ID>
-    <ID>MagicNumber:AppStartScenario.kt$AppStartScenario$6</ID>
     <ID>MagicNumber:BackgroundAppStartScenario.kt$BackgroundAppStartScenario$100</ID>
     <ID>MagicNumber:BackgroundAppStartScenario.kt$BackgroundAppStartScenario$1000L</ID>
     <ID>MagicNumber:BackgroundAppStartScenario.kt$BackgroundAppStartScenario$100L</ID>

features/fixtures/mazeracer/app/src/main/java/com/bugsnag/mazeracer/Log.kt

@@ -9,7 +9,7 @@ fun log(msg: String) {
 
 fun log(
     msg: String,
-    e: Exception,
+    e: Throwable,
 ) {
     Log.e("BugsnagMazeRacer", msg, e)
 }

features/fixtures/mazeracer/app/src/main/java/com/bugsnag/mazeracer/MainActivity.kt

@@ -11,11 +11,16 @@ import android.view.Window
 import android.widget.Button
 import android.widget.EditText
 import androidx.appcompat.app.AppCompatActivity
+import androidx.lifecycle.coroutineScope
+import androidx.lifecycle.withResumed
 import com.bugsnag.android.Bugsnag
 import com.bugsnag.android.Configuration
 import com.bugsnag.android.EndpointConfiguration
 import com.bugsnag.android.performance.AutoInstrument
 import com.bugsnag.android.performance.PerformanceConfiguration
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.launch
+import kotlinx.coroutines.withContext
 import org.json.JSONObject
 import java.io.File
 import java.io.IOException
@@ -42,7 +47,6 @@ class MainActivity : AppCompatActivity() {
         log("MainActivity.onCreate called")
         requestWindowFeature(Window.FEATURE_NO_TITLE)
         setContentView(R.layout.activity_main)
-        prefs = getPreferences(Context.MODE_PRIVATE)
 
         // Attempt to dismiss any system dialogs (such as "MazeRunner crashed")
         if (Build.VERSION.SDK_INT < Build.VERSION_CODES.S) {
@@ -55,30 +59,37 @@ class MainActivity : AppCompatActivity() {
         log("Set up clearUserData click handler")
         val clearUserData = findViewById<Button>(R.id.clearUserData)
         clearUserData.setOnClickListener {
-            clearStoredApiKey()
-            val apiKeyField = findViewById<EditText>(R.id.manualApiKey)
-            apiKeyField.text.clear()
-            log("Cleared user data")
-        }
+            lifecycle.coroutineScope.launch {
+                withContext(Dispatchers.IO) {
+                    clearStoredApiKey()
+                }
 
-        if (apiKeyStored()) {
-            log("Using stored API key")
-            val apiKey = getStoredApiKey()
-            val apiKeyField = findViewById<EditText>(R.id.manualApiKey)
-            apiKeyField.text.clear()
-            apiKeyField.text.append(apiKey)
+                val apiKeyField = findViewById<EditText>(R.id.manualApiKey)
+                apiKeyField.text.clear()
+                log("Cleared user data")
+            }
         }
-        log("MainActivity.onCreate complete")
-    }
 
-    override fun onResume() {
-        super.onResume()
-        log("MainActivity.onResume called")
+        lifecycle.coroutineScope.launch {
+            prefs = withContext(Dispatchers.IO) { getPreferences(Context.MODE_PRIVATE) }
+
+            if (apiKeyStored()) {
+                log("Using stored API key")
+                val apiKey = getStoredApiKey()
+                val apiKeyField = findViewById<EditText>(R.id.manualApiKey)
+                apiKeyField.text.clear()
+                apiKeyField.text.append(apiKey)
+            }
 
-        if (!polling) {
-            startCommandRunner()
+            withResumed {
+                log("MainActivity is resumed - startCommandRunner if ${!polling}")
+                if (!polling) {
+                    startCommandRunner()
+                }
+            }
         }
-        log("MainActivity.onResume complete")
+
+        log("MainActivity.onCreate complete")
     }
 
     override fun onActivityResult(
@@ -158,6 +169,11 @@ class MainActivity : AppCompatActivity() {
                         sessions = "http://$mazeAddress/session",
                     )
 
+                addOnError { event ->
+                    event.originalError?.let { log("Reporting error", it) }
+                    true
+                }
+
                 logger = BugsnagLogger
             }
         Bugsnag.start(this, config)
@@ -281,11 +297,12 @@ class MainActivity : AppCompatActivity() {
             try {
                 val errorMessage = urlConnection.errorStream.use { it.reader().readText() }
                 log(
-                    "Failed to GET $commandUrl (HTTP ${urlConnection.responseCode} " +
-                        "${urlConnection.responseMessage}):\n" +
-                        "${"-".repeat(errorMessage.width)}\n" +
-                        "$errorMessage\n" +
-                        "-".repeat(errorMessage.width),
+                    """
+                    Failed to GET $commandUrl (HTTP ${urlConnection.responseCode} ${urlConnection.responseMessage}):
+                    ${"-".repeat(errorMessage.width)}
+                    $errorMessage
+                    ${"-".repeat(errorMessage.width)}
+                    """.trimIndent(),
                 )
             } catch (e: Exception) {
                 log("Failed to retrieve error message from connection", e)

features/fixtures/mazeracer/app/src/main/java/com/bugsnag/mazeracer/MazeRacerAlarmReceiver.kt

@@ -20,5 +20,7 @@ class MazeRacerAlarmReceiver : BroadcastReceiver() {
         BugsnagPerformance.startSpan("AlarmReceiver", SpanOptions.setFirstClass(true)).use {
             Thread.sleep(250L)
         }
+
+        PerformanceTestUtils.flushBatch()
     }
 }

features/fixtures/mazeracer/app/src/main/java/com/bugsnag/mazeracer/MazeRacerApplication.kt

@@ -25,19 +25,19 @@ class MazeRacerApplication : Application() {
     override fun onCreate() {
         super.onCreate()
 
+        // if there is stored "startup" config then we start BugsnagPerformance before the scenario
+        // this is used to test things like app-start instrumentation
+        readStartupConfig()?.let { config ->
+            InternalDebug.workerSleepMs = 2000L
+            BugsnagPerformance.start(config)
+        }
+
         StrictMode.setThreadPolicy(
             StrictMode.ThreadPolicy.Builder()
                 .detectNetwork()
                 .penaltyLog()
                 .penaltyDeath()
                 .build(),
         )
-
-        // if there is stored "startup" config then we start BugsnagPerformance before the scenario
-        // this is used to test things like app-start instrumentation
-        readStartupConfig()?.let { config ->
-            InternalDebug.workerSleepMs = 2000L
-            BugsnagPerformance.start(config)
-        }
     }
 }

features/fixtures/mazeracer/app/src/main/java/com/bugsnag/mazeracer/PerformanceTestUtils.java

@@ -0,0 +1,28 @@
+package com.bugsnag.mazeracer;
+
+import android.annotation.SuppressLint;
+
+import com.bugsnag.android.performance.BugsnagPerformance;
+import com.bugsnag.android.performance.internal.InstrumentedAppState;
+import com.bugsnag.android.performance.internal.SpanProcessor;
+import com.bugsnag.android.performance.internal.processing.Tracer;
+
+public class PerformanceTestUtils {
+    private PerformanceTestUtils() {
+    }
+
+    /**
+     * @noinspection KotlinInternalInJava
+     */
+    @SuppressLint("RestrictedApi")
+    public static void flushBatch() {
+        LogKt.log("PerformanceTestUtils.flushBatch()");
+        InstrumentedAppState appState = BugsnagPerformance.INSTANCE.getInstrumentedAppState$internal();
+        SpanProcessor processor = appState.getSpanProcessor();
+
+        if (processor instanceof Tracer) {
+            Tracer tracer = (Tracer) processor;
+            tracer.forceCurrentBatch();
+        }
+    }
+}