Merge pull request #2141 from OneSignal/rollback-init-optimization

Revert "Merge pull request #2127 from OneSignal/get-service-by-getter"
OneSignal · Jul 3, 2024 · 10d1507 · 10d1507
2 parents 6078d77 + c2db8c8
commit 10d1507
Show file tree

Hide file tree

Showing 5 changed files with 67 additions and 82 deletions.
diff --git a/...gnalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceRegistration.kt b/...gnalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceRegistration.kt
@@ -56,6 +56,7 @@ 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
@@ -32,6 +32,7 @@ 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
@@ -53,6 +54,7 @@ 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/...nalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt b/...nalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt
@@ -1,23 +1,18 @@
 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 services: ServiceProvider,
+    private val _bootstrapServices: List<IBootstrapService>,
+    private val _startableServices: List<IStartableService>,
 ) {
-    private val coroutineScope = CoroutineScope(newSingleThreadContext(name = "StartupService"))
-
     fun bootstrap() {
-        services.getAllServices<IBootstrapService>().forEach { it.bootstrap() }
+        // now that we have the params initialized, start everything else up
+        for (bootstrapService in _bootstrapServices)
+            bootstrapService.bootstrap()
     }
 
-    // schedule to start all startable services in a separate thread
-    fun scheduleStart() {
-        coroutineScope.launch {
-            services.getAllServices<IStartableService>().forEach { it.start() }
-        }
+    fun start() {
+        // now that we have the params initialized, start everything else up
+        for (startableService in _startableServices)
+            startableService.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
@@ -86,56 +86,50 @@ internal class OneSignalImp : IOneSignal, IServiceProvider {
     override val debug: IDebugManager = DebugManager()
     override val session: ISessionManager get() =
         if (isInitialized) {
-            services.getService()
+            _session!!
         } else {
             throw Exception(
                 "Must call 'initWithContext' before use",
             )
         }
     override val notifications: INotificationsManager get() =
         if (isInitialized) {
-            services.getService()
+            _notifications!!
         } else {
             throw Exception(
                 "Must call 'initWithContext' before use",
             )
         }
     override val location: ILocationManager get() =
         if (isInitialized) {
-            services.getService()
+            _location!!
         } else {
             throw Exception(
                 "Must call 'initWithContext' before use",
             )
         }
     override val inAppMessages: IInAppMessagesManager get() =
         if (isInitialized) {
-            services.getService()
-        } else {
-            throw Exception(
-                "Must call 'initWithContext' before use",
-            )
-        }
-    override val user: IUserManager get() =
-        if (isInitialized) {
-            services.getService()
+            iam!!
         } 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 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()
+    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
 
     // Other State
     private val services: ServiceProvider
@@ -240,10 +234,21 @@ internal class OneSignalImp : IOneSignal, IServiceProvider {
                 configModel!!.disableGMSMissingPrompt = _disableGMSMissingPrompt!!
             }
 
-            val startupService = StartupService(services)
-
-            // bootstrap services
-            startupService.bootstrap()
+            // "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()
 
             if (forceCreateUser || !identityModelStore!!.model.hasProperty(IdentityConstants.ONESIGNAL_ID)) {
                 val legacyPlayerId =
@@ -293,12 +298,8 @@ 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(
@@ -327,9 +328,10 @@ internal class OneSignalImp : IOneSignal, IServiceProvider {
                 Logging.debug("initWithContext: using cached user ${identityModelStore!!.model.onesignalId}")
             }
 
-            // schedule service starts out of main thread
-            startupService.scheduleStart()
+            startupService!!.start()
+
             isInitialized = true
+
             return true
         }
     }

diff --git a/...K/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt b/...K/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt
@@ -1,38 +1,24 @@
 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(setupServiceProvider(listOf(), listOf()))
+        val startupService = StartupService(listOf(), listOf())
 
         // When
         startupService.bootstrap()
@@ -45,7 +31,7 @@ class StartupServiceTests : FunSpec({
         val mockBootstrapService1 = spyk<IBootstrapService>()
         val mockBootstrapService2 = spyk<IBootstrapService>()
 
-        val startupService = StartupService(setupServiceProvider(listOf(mockBootstrapService1, mockBootstrapService2), listOf()))
+        val startupService = StartupService(listOf(mockBootstrapService1, mockBootstrapService2), listOf())
 
         // When
         startupService.bootstrap()
@@ -63,7 +49,7 @@ class StartupServiceTests : FunSpec({
         every { mockBootstrapService1.bootstrap() } throws exception
         val mockBootstrapService2 = spyk<IBootstrapService>()
 
-        val startupService = StartupService(setupServiceProvider(listOf(mockBootstrapService1, mockBootstrapService2), listOf()))
+        val startupService = StartupService(listOf(mockBootstrapService1, mockBootstrapService2), listOf())
 
         // When
         val actualException =
@@ -77,41 +63,40 @@ class StartupServiceTests : FunSpec({
         verify(exactly = 0) { mockBootstrapService2.bootstrap() }
     }
 
-    test("startup will call all IStartableService dependencies successfully after a short delay") {
+    test("startup will call all IStartableService dependencies successfully") {
         // Given
         val mockStartupService1 = spyk<IStartableService>()
         val mockStartupService2 = spyk<IStartableService>()
 
-        val startupService = StartupService(setupServiceProvider(listOf(), listOf(mockStartupService1, mockStartupService2)))
+        val startupService = StartupService(listOf(), listOf(mockStartupService1, mockStartupService2))
 
         // When
-        startupService.scheduleStart()
+        startupService.start()
 
         // Then
-        Thread.sleep(10)
         verify(exactly = 1) { mockStartupService1.start() }
         verify(exactly = 1) { mockStartupService2.start() }
     }
 
-    test("scheduleStart does not block main thread") {
+    test("startup will propagate exception when an IStartableService throws an exception") {
         // Given
-        val mockStartableService1 = spyk<IStartableService>()
+        val exception = Exception("SOMETHING BAD")
+
+        val mockStartableService1 = mockk<IStartableService>()
+        every { mockStartableService1.start() } throws exception
         val mockStartableService2 = spyk<IStartableService>()
-        val mockStartableService3 = spyk<IStartableService>()
 
-        val startupService = StartupService(setupServiceProvider(listOf(), listOf(mockStartableService1, mockStartableService2)))
+        val startupService = StartupService(listOf(), listOf(mockStartableService1, mockStartableService2))
 
         // When
-        startupService.scheduleStart()
-        mockStartableService3.start()
+        val actualException =
+            shouldThrowUnit<Exception> {
+                startupService.start()
+            }
 
         // Then
-        Thread.sleep(10)
-        coVerifyOrder {
-            // service3 will call start() first even though service1 and service2 are scheduled first
-            mockStartableService3.start()
-            mockStartableService1.start()
-            mockStartableService2.start()
-        }
+        actualException shouldBe exception
+        verify(exactly = 1) { mockStartableService1.start() }
+        verify(exactly = 0) { mockStartableService2.start() }
     }
 })