From 1346a3291335cd650575890f7e682e20f5e5fb89 Mon Sep 17 00:00:00 2001
From: jinliu9508 <jinliu9508@gmail.com>
Date: Fri, 14 Jun 2024 01:46:56 -0400
Subject: [PATCH] move unused services out of initWithContext process and
 schedule service start in background

---
 .../common/services/ServiceRegistration.kt    |  1 -
 .../java/com/onesignal/core/CoreModule.kt     |  2 -
 .../core/internal/startup/StartupService.kt   | 23 +++---
 .../com/onesignal/internal/OneSignalImp.kt    | 70 +++++++++----------
 .../internal/startup/StartupServiceTests.kt   | 53 +++++++++-----
 5 files changed, 82 insertions(+), 67 deletions(-)

diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceRegistration.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceRegistration.kt
index 0298381be9..871968ab11 100644
--- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceRegistration.kt
+++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceRegistration.kt
@@ -56,7 +56,6 @@ class ServiceRegistrationReflection<T>(
             Logging.debug("${ServiceProvider.indent}Already instantiated: $obj")
             return obj
         }
-
         // use reflection to try to instantiate the thing
         for (constructor in clazz.constructors) {
             if (doesHaveAllParameters(constructor, provider)) {
diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/CoreModule.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/CoreModule.kt
index efc06d54a4..6d36caaf9f 100644
--- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/CoreModule.kt
+++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/CoreModule.kt
@@ -32,7 +32,6 @@ import com.onesignal.core.internal.preferences.impl.PreferencesService
 import com.onesignal.core.internal.purchases.impl.TrackAmazonPurchase
 import com.onesignal.core.internal.purchases.impl.TrackGooglePurchase
 import com.onesignal.core.internal.startup.IStartableService
-import com.onesignal.core.internal.startup.StartupService
 import com.onesignal.core.internal.time.ITime
 import com.onesignal.core.internal.time.impl.Time
 import com.onesignal.inAppMessages.IInAppMessagesManager
@@ -54,7 +53,6 @@ internal class CoreModule : IModule {
         builder.register<DeviceService>().provides<IDeviceService>()
         builder.register<Time>().provides<ITime>()
         builder.register<DatabaseProvider>().provides<IDatabaseProvider>()
-        builder.register<StartupService>().provides<StartupService>()
         builder.register<InstallIdService>().provides<IInstallIdService>()
 
         // Params (Config)
diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt
index 60af1b8bc1..ec9075b88b 100644
--- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt
+++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt
@@ -1,18 +1,23 @@
 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 val coroutineScope = CoroutineScope(newSingleThreadContext(name = "StartupService"))
+
     fun bootstrap() {
-        // now that we have the params initialized, start everything else up
-        for (bootstrapService in _bootstrapServices)
-            bootstrapService.bootstrap()
+        services.getAllServices<IBootstrapService>().forEach { it.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 {
+            services.getAllServices<IStartableService>().forEach { it.start() }
+        }
     }
 }
diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt
index 2cb6c32656..b06a9bf81d 100644
--- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt
+++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt
@@ -86,7 +86,7 @@ internal class OneSignalImp : IOneSignal, IServiceProvider {
     override val debug: IDebugManager = DebugManager()
     override val session: ISessionManager get() =
         if (isInitialized) {
-            _session!!
+            services.getService()
         } else {
             throw Exception(
                 "Must call 'initWithContext' before use",
@@ -94,7 +94,7 @@ internal class OneSignalImp : IOneSignal, IServiceProvider {
         }
     override val notifications: INotificationsManager get() =
         if (isInitialized) {
-            _notifications!!
+            services.getService()
         } else {
             throw Exception(
                 "Must call 'initWithContext' before use",
@@ -102,7 +102,7 @@ internal class OneSignalImp : IOneSignal, IServiceProvider {
         }
     override val location: ILocationManager get() =
         if (isInitialized) {
-            _location!!
+            services.getService()
         } else {
             throw Exception(
                 "Must call 'initWithContext' before use",
@@ -110,26 +110,32 @@ internal class OneSignalImp : IOneSignal, IServiceProvider {
         }
     override val inAppMessages: IInAppMessagesManager get() =
         if (isInitialized) {
-            iam!!
+            services.getService()
+        } else {
+            throw Exception(
+                "Must call 'initWithContext' before use",
+            )
+        }
+    override val user: IUserManager get() =
+        if (isInitialized) {
+            services.getService()
         } else {
             throw Exception(
                 "Must call 'initWithContext' before use",
             )
         }
-    override val user: IUserManager get() = if (isInitialized) _user!! else throw Exception("Must call 'initWithContext' before use")
 
     // Services required by this class
-    private var _user: IUserManager? = null
-    private var _session: ISessionManager? = null
-    private var iam: IInAppMessagesManager? = null
-    private var _location: ILocationManager? = null
-    private var _notifications: INotificationsManager? = null
-    private var operationRepo: IOperationRepo? = null
-    private var identityModelStore: IdentityModelStore? = null
-    private var propertiesModelStore: PropertiesModelStore? = null
-    private var subscriptionModelStore: SubscriptionModelStore? = null
-    private var startupService: StartupService? = null
-    private var preferencesService: IPreferencesService? = null
+    private val operationRepo: IOperationRepo
+        get() = services.getService()
+    private val identityModelStore: IdentityModelStore
+        get() = services.getService()
+    private val propertiesModelStore: PropertiesModelStore
+        get() = services.getService()
+    private val subscriptionModelStore: SubscriptionModelStore
+        get() = services.getService()
+    private val preferencesService: IPreferencesService
+        get() = services.getService()
 
     // Other State
     private val services: ServiceProvider
@@ -234,21 +240,10 @@ internal class OneSignalImp : IOneSignal, IServiceProvider {
                 configModel!!.disableGMSMissingPrompt = _disableGMSMissingPrompt!!
             }
 
-            // "Inject" the services required by this main class
-            _location = services.getService()
-            _user = services.getService()
-            _session = services.getService()
-            iam = services.getService()
-            _notifications = services.getService()
-            operationRepo = services.getService()
-            propertiesModelStore = services.getService()
-            identityModelStore = services.getService()
-            subscriptionModelStore = services.getService()
-            preferencesService = services.getService()
-
-            // Instantiate and call the IStartableServices
-            startupService = services.getService()
-            startupService!!.bootstrap()
+            val startupService = StartupService(services)
+
+            // bootstrap services
+            startupService.bootstrap()
 
             if (forceCreateUser || !identityModelStore!!.model.hasProperty(IdentityConstants.ONESIGNAL_ID)) {
                 val legacyPlayerId =
@@ -298,8 +293,12 @@ internal class OneSignalImp : IOneSignal, IServiceProvider {
 
                         pushSubscriptionModel.sdk = OneSignalUtils.SDK_VERSION
                         pushSubscriptionModel.deviceOS = Build.VERSION.RELEASE
-                        pushSubscriptionModel.carrier = DeviceUtils.getCarrierName(services.getService<IApplicationService>().appContext) ?: ""
-                        pushSubscriptionModel.appVersion = AndroidUtils.getAppVersion(services.getService<IApplicationService>().appContext) ?: ""
+                        pushSubscriptionModel.carrier = DeviceUtils.getCarrierName(
+                            services.getService<IApplicationService>().appContext,
+                        ) ?: ""
+                        pushSubscriptionModel.appVersion = AndroidUtils.getAppVersion(
+                            services.getService<IApplicationService>().appContext,
+                        ) ?: ""
 
                         configModel!!.pushSubscriptionId = legacyPlayerId
                         subscriptionModelStore!!.add(
@@ -328,10 +327,9 @@ internal class OneSignalImp : IOneSignal, IServiceProvider {
                 Logging.debug("initWithContext: using cached user ${identityModelStore!!.model.onesignalId}")
             }
 
-            startupService!!.start()
-
+            // schedule service starts out of main thread
+            startupService.scheduleStart()
             isInitialized = true
-
             return true
         }
     }
diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt
index 71e72de07a..140b9400ed 100644
--- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt
+++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt
@@ -1,16 +1,30 @@
 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
@@ -18,7 +32,7 @@ class StartupServiceTests : FunSpec({
 
     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 =
@@ -63,40 +77,41 @@ class StartupServiceTests : FunSpec({
         verify(exactly = 0) { mockBootstrapService2.bootstrap() }
     }
 
-    test("startup will call all IStartableService dependencies successfully") {
+    test("startup will call all IStartableService dependencies successfully after a short delay") {
         // Given
         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
+        Thread.sleep(10)
         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() }
+        Thread.sleep(10)
+        coVerifyOrder {
+            // service3 will call start() first even though service1 and service2 are scheduled first
+            mockStartableService3.start()
+            mockStartableService1.start()
+            mockStartableService2.start()
+        }
     }
 })