refactor StartupService

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceProvider.kt

@@ -1,10 +1,6 @@
 package com.onesignal.common.services
 
-import com.onesignal.core.internal.startup.IStartableService
 import com.onesignal.debug.internal.logging.Logging
-import kotlinx.coroutines.CoroutineScope
-import kotlinx.coroutines.launch
-import kotlinx.coroutines.newSingleThreadContext
 
 /**
  * A service provider gives access to the implementations of a service.
@@ -13,7 +9,6 @@ class ServiceProvider(
     registrations: List<ServiceRegistration<*>>,
 ) : IServiceProvider {
     private val serviceMap = mutableMapOf<Class<*>, MutableList<ServiceRegistration<*>>>()
-    private var coroutineScope = CoroutineScope(newSingleThreadContext(name = "ServiceProvider"))
 
     init {
         // go through the registrations to create the service map for easier lookup post-build
@@ -85,15 +80,6 @@ class ServiceProvider(
         }
     }
 
-    // schedule to start all startable services in a separate thread
-    fun scheduleStartServices() {
-        coroutineScope.launch {
-            for (service in getAllServices<IStartableService>()) {
-                service.start()
-            }
-        }
-    }
-
     companion object {
         var indent: String = ""
     }

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt

@@ -1,18 +1,27 @@
 package com.onesignal.core.internal.startup
 
+import com.onesignal.common.services.ServiceProvider
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.launch
+import kotlinx.coroutines.newSingleThreadContext
+
 internal class StartupService(
-    private val _bootstrapServices: List<IBootstrapService>,
-    private val _startableServices: List<IStartableService>,
+    private val services: ServiceProvider,
 ) {
+    private var coroutineScope = CoroutineScope(newSingleThreadContext(name = "ServiceProvider"))
+
     fun bootstrap() {
-        // now that we have the params initialized, start everything else up
-        for (bootstrapService in _bootstrapServices)
+        for (bootstrapService in services.getAllServices<IBootstrapService>()) {
             bootstrapService.bootstrap()
+        }
     }
 
-    fun start() {
-        // now that we have the params initialized, start everything else up
-        for (startableService in _startableServices)
-            startableService.start()
+    // schedule to start all startable services in a separate thread
+    fun scheduleStart() {
+        coroutineScope.launch {
+            for (service in services.getAllServices<IStartableService>()) {
+                service.start()
+            }
+        }
     }
 }

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt

@@ -25,7 +25,7 @@ import com.onesignal.core.internal.preferences.IPreferencesService
 import com.onesignal.core.internal.preferences.PreferenceOneSignalKeys
 import com.onesignal.core.internal.preferences.PreferenceStoreFix
 import com.onesignal.core.internal.preferences.PreferenceStores
-import com.onesignal.core.internal.startup.IBootstrapService
+import com.onesignal.core.internal.startup.StartupService
 import com.onesignal.debug.IDebugManager
 import com.onesignal.debug.LogLevel
 import com.onesignal.debug.internal.DebugManager
@@ -240,10 +240,10 @@ internal class OneSignalImp : IOneSignal, IServiceProvider {
                 configModel!!.disableGMSMissingPrompt = _disableGMSMissingPrompt!!
             }
 
+            val startupService = StartupService(services)
+
             // bootstrap services
-            for (bootstrapService in services.getAllServices<IBootstrapService>()) {
-                bootstrapService.bootstrap()
-            }
+            startupService.bootstrap()
 
             if (forceCreateUser || !identityModelStore!!.model.hasProperty(IdentityConstants.ONESIGNAL_ID)) {
                 val legacyPlayerId =
@@ -327,7 +327,8 @@ internal class OneSignalImp : IOneSignal, IServiceProvider {
                 Logging.debug("initWithContext: using cached user ${identityModelStore!!.model.onesignalId}")
             }
 
-            services.scheduleStartServices()
+            // schedule service starts out of main thread
+            startupService.scheduleStart()
             isInitialized = true
             return true
         }

OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt

@@ -1,24 +1,38 @@
 package com.onesignal.core.internal.startup
 
+import com.onesignal.common.services.ServiceBuilder
+import com.onesignal.common.services.ServiceProvider
 import com.onesignal.debug.LogLevel
 import com.onesignal.debug.internal.logging.Logging
 import io.kotest.assertions.throwables.shouldThrowUnit
 import io.kotest.core.spec.style.FunSpec
 import io.kotest.matchers.shouldBe
+import io.mockk.coVerifyOrder
 import io.mockk.every
 import io.mockk.mockk
 import io.mockk.spyk
 import io.mockk.verify
 
 class StartupServiceTests : FunSpec({
+    fun setupServiceProvider(
+        bootstrapServices: List<IBootstrapService>,
+        startableServices: List<IStartableService>,
+    ): ServiceProvider {
+        val serviceBuilder = ServiceBuilder()
+        for (reg in bootstrapServices)
+            serviceBuilder.register(reg).provides<IBootstrapService>()
+        for (reg in startableServices)
+            serviceBuilder.register(reg).provides<IStartableService>()
+        return serviceBuilder.build()
+    }
 
     beforeAny {
         Logging.logLevel = LogLevel.NONE
     }
 
     test("bootstrap with no IBootstrapService dependencies is a no-op") {
         // Given
-        val startupService = StartupService(listOf(), listOf())
+        val startupService = StartupService(setupServiceProvider(listOf(), listOf()))
 
         // When
         startupService.bootstrap()
@@ -31,7 +45,7 @@ class StartupServiceTests : FunSpec({
         val mockBootstrapService1 = spyk<IBootstrapService>()
         val mockBootstrapService2 = spyk<IBootstrapService>()
 
-        val startupService = StartupService(listOf(mockBootstrapService1, mockBootstrapService2), listOf())
+        val startupService = StartupService(setupServiceProvider(listOf(mockBootstrapService1, mockBootstrapService2), listOf()))
 
         // When
         startupService.bootstrap()
@@ -49,7 +63,7 @@ class StartupServiceTests : FunSpec({
         every { mockBootstrapService1.bootstrap() } throws exception
         val mockBootstrapService2 = spyk<IBootstrapService>()
 
-        val startupService = StartupService(listOf(mockBootstrapService1, mockBootstrapService2), listOf())
+        val startupService = StartupService(setupServiceProvider(listOf(mockBootstrapService1, mockBootstrapService2), listOf()))
 
         // When
         val actualException =
@@ -68,35 +82,34 @@ class StartupServiceTests : FunSpec({
         val mockStartupService1 = spyk<IStartableService>()
         val mockStartupService2 = spyk<IStartableService>()
 
-        val startupService = StartupService(listOf(), listOf(mockStartupService1, mockStartupService2))
+        val startupService = StartupService(setupServiceProvider(listOf(), listOf(mockStartupService1, mockStartupService2)))
 
         // When
-        startupService.start()
+        startupService.scheduleStart()
 
         // Then
         verify(exactly = 1) { mockStartupService1.start() }
         verify(exactly = 1) { mockStartupService2.start() }
     }
 
-    test("startup will propagate exception when an IStartableService throws an exception") {
+    test("scheduleStart does not block main thread") {
         // Given
-        val exception = Exception("SOMETHING BAD")
-
-        val mockStartableService1 = mockk<IStartableService>()
-        every { mockStartableService1.start() } throws exception
+        val mockStartableService1 = spyk<IStartableService>()
         val mockStartableService2 = spyk<IStartableService>()
+        val mockStartableService3 = spyk<IStartableService>()
 
-        val startupService = StartupService(listOf(), listOf(mockStartableService1, mockStartableService2))
+        val startupService = StartupService(setupServiceProvider(listOf(), listOf(mockStartableService1, mockStartableService2)))
 
         // When
-        val actualException =
-            shouldThrowUnit<Exception> {
-                startupService.start()
-            }
+        startupService.scheduleStart()
+        mockStartableService3.start()
 
         // Then
-        actualException shouldBe exception
-        verify(exactly = 1) { mockStartableService1.start() }
-        verify(exactly = 0) { mockStartableService2.start() }
+        coVerifyOrder {
+            // service3 will call start() first even though service1 and service2 are scheduled first
+            mockStartableService3.start()
+            mockStartableService1.start()
+            mockStartableService2.start()
+        }
     }
 })