address review feedback

bugsnag · Jul 23, 2021 · 65aa21f · 65aa21f
1 parent c42f877
commit 65aa21f
Show file tree

Hide file tree

Showing 11 changed files with 53 additions and 86 deletions.
diff --git a/bugsnag-android-core/detekt-baseline.xml b/bugsnag-android-core/detekt-baseline.xml
@@ -30,11 +30,11 @@
     <ID>SwallowedException:ConnectivityCompat.kt$ConnectivityLegacy$catch (e: NullPointerException) { // in some rare cases we get a remote NullPointerException via Parcel.readException null }</ID>
     <ID>SwallowedException:ContextExtensions.kt$catch (exc: RuntimeException) { null }</ID>
     <ID>SwallowedException:DeviceDataCollector.kt$DeviceDataCollector$catch (exc: Exception) { false }</ID>
-    <ID>SwallowedException:DeviceDataCollector.kt$DeviceDataCollector$catch (exc: RejectedExecutionException) { null }</ID>
     <ID>SwallowedException:DeviceDataCollector.kt$DeviceDataCollector$catch (exception: Exception) { logger.w("Could not get battery status") }</ID>
     <ID>SwallowedException:DeviceDataCollector.kt$DeviceDataCollector$catch (exception: Exception) { logger.w("Could not get locationStatus") }</ID>
     <ID>SwallowedException:DeviceIdStore.kt$DeviceIdStore$catch (exc: OverlappingFileLockException) { Thread.sleep(FILE_LOCK_WAIT_MS) }</ID>
     <ID>SwallowedException:PluginClient.kt$PluginClient$catch (exc: ClassNotFoundException) { logger.d("Plugin '$clz' is not on the classpath - functionality will not be enabled.") null }</ID>
+    <ID>UnnecessaryAbstractClass:DependencyModule.kt$DependencyModule</ID>
     <ID>UnusedPrivateMember:ThreadStateTest.kt$ThreadStateTest$private val configuration = generateImmutableConfig()</ID>
     <ID>VariableNaming:EventInternal.kt$EventInternal$/** * @return user information associated with this Event */ internal var _user = User(null, null, null)</ID>
   </CurrentIssues>

diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/BugsnagStateModule.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/BugsnagStateModule.kt
@@ -10,7 +10,7 @@ import com.bugsnag.android.internal.dag.DependencyModule
 internal class BugsnagStateModule(
     configModule: ConfigModule,
     configuration: Configuration
-) : DependencyModule {
+) : DependencyModule() {
 
     private val cfg = configModule.config
 

diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/Client.java b/bugsnag-android-core/src/main/java/com/bugsnag/android/Client.java
@@ -138,7 +138,7 @@ public Unit invoke(Boolean hasConnection, String networkState) {
 
         // setup storage as soon as possible
         final StorageModule storageModule = new StorageModule(appContext,
-                immutableConfig, bgTaskService, logger);
+                immutableConfig, logger);
 
         // setup state trackers for bugsnag
         BugsnagStateModule bugsnagStateModule = new BugsnagStateModule(

diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/DataCollectionModule.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/DataCollectionModule.kt
@@ -5,8 +5,6 @@ import com.bugsnag.android.internal.dag.ConfigModule
 import com.bugsnag.android.internal.dag.ContextModule
 import com.bugsnag.android.internal.dag.DependencyModule
 import com.bugsnag.android.internal.dag.SystemServiceModule
-import com.bugsnag.android.internal.dag.loadDepModuleIoObjects
-import java.util.concurrent.Future
 
 /**
  * A dependency module which constructs the objects that collect data in Bugsnag. For example, this
@@ -20,15 +18,15 @@ internal class DataCollectionModule(
     bgTaskService: BackgroundTaskService,
     connectivity: Connectivity,
     deviceId: String?
-) : DependencyModule {
+) : DependencyModule() {
 
     private val ctx = contextModule.ctx
     private val cfg = configModule.config
     private val logger = cfg.logger
     private val deviceBuildInfo: DeviceBuildInfo = DeviceBuildInfo.defaultInfo()
     private val dataDir = Environment.getDataDirectory()
 
-    val appDataCollector by lazy {
+    val appDataCollector by future {
         AppDataCollector(
             ctx,
             ctx.packageManager,
@@ -40,25 +38,21 @@ internal class DataCollectionModule(
         )
     }
 
-    private val rootDetector by lazy {
+    private val rootDetector by future {
         RootDetector(logger = logger, deviceBuildInfo = deviceBuildInfo)
     }
 
-    val deviceDataCollector by lazy {
+    val deviceDataCollector by future {
         DeviceDataCollector(
-            connectivity, ctx,
-            ctx.resources, deviceId, deviceBuildInfo, dataDir,
-            rootDetector, bgTaskService, logger
+            connectivity,
+            ctx,
+            ctx.resources,
+            deviceId,
+            deviceBuildInfo,
+            dataDir,
+            rootDetector,
+            bgTaskService,
+            logger
         )
     }
-
-    // trigger initialization on a background thread. Client<init> will then block on the main
-    // thread with resolveDependencies() if these have not completed by the appropriate time.
-    private val future: Future<*>? = loadDepModuleIoObjects(bgTaskService) { rootDetector }
-
-    override fun resolveDependencies(bgTaskService: BackgroundTaskService, taskType: TaskType) {
-        runCatching {
-            future?.get()
-        }
-    }
 }
diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/EventStorageModule.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/EventStorageModule.kt
@@ -4,8 +4,6 @@ import com.bugsnag.android.internal.dag.ConfigModule
 import com.bugsnag.android.internal.dag.ContextModule
 import com.bugsnag.android.internal.dag.DependencyModule
 import com.bugsnag.android.internal.dag.SystemServiceModule
-import com.bugsnag.android.internal.dag.loadDepModuleIoObjects
-import java.util.concurrent.Future
 
 /**
  * A dependency module which constructs the objects that persist events to disk in Bugsnag.
@@ -18,11 +16,11 @@ internal class EventStorageModule(
     trackerModule: TrackerModule,
     systemServiceModule: SystemServiceModule,
     notifier: Notifier
-) : DependencyModule {
+) : DependencyModule() {
 
     private val cfg = configModule.config
 
-    private val delegate by lazy {
+    private val delegate by future {
         InternalReportDelegate(
             contextModule.ctx,
             cfg.logger,
@@ -36,15 +34,5 @@ internal class EventStorageModule(
         )
     }
 
-    val eventStore by lazy { EventStore(cfg, cfg.logger, notifier, bgTaskService, delegate) }
-
-    // trigger initialization on a background thread. Client<init> will then block on the main
-    // thread if these have not completed by the appropriate time.
-    private val future: Future<*>? = loadDepModuleIoObjects(bgTaskService) { eventStore }
-
-    override fun resolveDependencies(bgTaskService: BackgroundTaskService, taskType: TaskType) {
-        runCatching {
-            future?.get()
-        }
-    }
+    val eventStore by future { EventStore(cfg, cfg.logger, notifier, bgTaskService, delegate) }
 }
diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/StorageModule.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/StorageModule.kt
@@ -3,32 +3,29 @@ package com.bugsnag.android
 import android.content.Context
 import com.bugsnag.android.internal.ImmutableConfig
 import com.bugsnag.android.internal.dag.DependencyModule
-import com.bugsnag.android.internal.dag.loadDepModuleIoObjects
-import java.util.concurrent.Future
 
 /**
  * A dependency module which constructs the objects that store information to disk in Bugsnag.
  */
 internal class StorageModule(
     appContext: Context,
     immutableConfig: ImmutableConfig,
-    bgTaskService: BackgroundTaskService,
     logger: Logger
-) : DependencyModule {
+) : DependencyModule() {
 
-    val sharedPrefMigrator by lazy { SharedPrefMigrator(appContext) }
+    val sharedPrefMigrator by future { SharedPrefMigrator(appContext) }
 
-    private val deviceIdStore by lazy {
+    private val deviceIdStore by future {
         DeviceIdStore(
             appContext,
             sharedPrefMigrator = sharedPrefMigrator,
             logger = logger
         )
     }
 
-    val deviceId by lazy { deviceIdStore.loadDeviceId() }
+    val deviceId by future { deviceIdStore.loadDeviceId() }
 
-    val userStore by lazy {
+    val userStore by future {
         UserStore(
             immutableConfig,
             deviceId,
@@ -37,32 +34,14 @@ internal class StorageModule(
         )
     }
 
-    val lastRunInfoStore by lazy { LastRunInfoStore(immutableConfig) }
+    val lastRunInfoStore by future { LastRunInfoStore(immutableConfig) }
 
-    val sessionStore by lazy { SessionStore(immutableConfig, logger, null) }
+    val sessionStore by future { SessionStore(immutableConfig, logger, null) }
 
-    val lastRunInfo by lazy {
+    val lastRunInfo by future {
         val info = lastRunInfoStore.load()
         val currentRunInfo = LastRunInfo(0, crashed = false, crashedDuringLaunch = false)
         lastRunInfoStore.persist(currentRunInfo)
         info
     }
-
-    // trigger initialization on a background thread. Client<init> will then block on the main
-    // thread if these have not completed by the appropriate time.
-    private val future: Future<*>? = loadDepModuleIoObjects(bgTaskService) {
-        sharedPrefMigrator
-        deviceIdStore
-        deviceId
-        userStore
-        lastRunInfoStore
-        lastRunInfo
-        sessionStore
-    }
-
-    override fun resolveDependencies(bgTaskService: BackgroundTaskService, taskType: TaskType) {
-        runCatching {
-            future?.get()
-        }
-    }
 }
diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/TrackerModule.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/TrackerModule.kt
@@ -13,7 +13,7 @@ internal class TrackerModule(
     client: Client,
     bgTaskService: BackgroundTaskService,
     callbackState: CallbackState
-) : DependencyModule {
+) : DependencyModule() {
 
     private val config = configModule.config
 

diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/internal/dag/ConfigModule.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/internal/dag/ConfigModule.kt
@@ -12,7 +12,7 @@ internal class ConfigModule(
     contextModule: ContextModule,
     configuration: Configuration,
     connectivity: Connectivity
-) : DependencyModule {
+) : DependencyModule() {
 
     val config = sanitiseConfiguration(contextModule.ctx, configuration, connectivity)
 }
diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/internal/dag/ContextModule.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/internal/dag/ContextModule.kt
@@ -8,7 +8,7 @@ import android.content.Context
  */
 internal class ContextModule(
     appContext: Context
-) : DependencyModule {
+) : DependencyModule() {
 
     val ctx: Context = when (appContext.applicationContext) {
         null -> appContext

diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/internal/dag/DependencyModule.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/internal/dag/DependencyModule.kt
@@ -3,27 +3,33 @@ package com.bugsnag.android.internal.dag
 import com.bugsnag.android.BackgroundTaskService
 import com.bugsnag.android.TaskType
 
-/**
- * A collection of related objects which are used to inject dependencies. This is somewhat
- * analogous to Dagger's concept of modules - although this implementation is much simpler.
- */
-internal interface DependencyModule {
+internal abstract class DependencyModule {
+
+    private val properties = mutableListOf<Lazy<*>>()
+
+    /**
+     * Creates a new [Lazy] property that is marked as an object that should be resolved off the
+     * main thread when [resolveDependencies] is called.
+     */
+    fun <T> future(initializer: () -> T): Lazy<T> {
+        val lazy = lazy {
+            initializer()
+        }
+        properties.add(lazy)
+        return lazy
+    }
 
     /**
      * Blocks until all dependencies in the module have been constructed. This provides the option
      * for modules to construct objects in a background thread, then have a user block on another
      * thread until all the objects have been constructed.
      */
-    fun resolveDependencies(bgTaskService: BackgroundTaskService, taskType: TaskType) = Unit
-}
-
-/**
- * Creates a [Future] for loading objects on the IO thread.
- */
-internal fun loadDepModuleIoObjects(bgTaskService: BackgroundTaskService, action: () -> Unit) =
-    runCatching {
+    fun resolveDependencies(bgTaskService: BackgroundTaskService, taskType: TaskType) {
         bgTaskService.submitTask(
-            TaskType.IO,
-            Runnable(action)
+            taskType,
+            Runnable {
+                properties.forEach { it.value }
+            }
         )
-    }.getOrNull()
+    }
+}
diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/internal/dag/SystemServiceModule.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/internal/dag/SystemServiceModule.kt
@@ -8,7 +8,7 @@ import com.bugsnag.android.getStorageManager
  */
 internal class SystemServiceModule(
     contextModule: ContextModule
-) : DependencyModule {
+) : DependencyModule() {
 
     val storageManager = contextModule.ctx.getStorageManager()
     val activityManager = contextModule.ctx.getActivityManager()