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() + } } })