Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Optimization] Init with context in background #2151

Merged
merged 2 commits into from
Aug 5, 2024
Merged

[Optimization] Init with context in background #2151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e18e4b6
move unused service initialization out of main thread
jinliu9508 Jul 16, 2024
4112dee
Add a requestPermission call immediately after initWithContext to ens…
jinliu9508 Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions ...es/OneSignalDemo/app/src/main/java/com/onesignal/sdktest/application/MainApplication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
package com.onesignal.sdktest.application;

import android.annotation.SuppressLint;
import android.os.StrictMode;
import android.util.Log;

import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.multidex.MultiDexApplication;

import com.onesignal.Continue;
import com.onesignal.OneSignal;
import com.onesignal.inAppMessages.IInAppMessageClickListener;
import com.onesignal.inAppMessages.IInAppMessageClickEvent;
Expand All @@ -28,8 +29,10 @@
import com.onesignal.user.state.IUserStateObserver;
import com.onesignal.user.state.UserChangedState;
import com.onesignal.user.state.UserState;

import org.json.JSONObject;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

public class MainApplication extends MultiDexApplication {
private static final int SLEEP_TIME_TO_MIMIC_ASYNC_OPERATION = 2000;
Expand All @@ -40,6 +43,7 @@ public MainApplication() {
StrictMode.enableDefaults();
}

@SuppressLint("NewApi")
@Override
public void onCreate() {
super.onCreate();
Expand All @@ -54,8 +58,18 @@ public void onCreate() {
}

OneSignalNotificationSender.setAppId(appId);

OneSignal.initWithContext(this, appId);

// Ensure calling requestPermission in a thread right after initWithContext does not crash
// This will reproduce result similar to Kotlin CouroutineScope.launch{}, which may potentially crash the app
ExecutorService executor = Executors.newSingleThreadExecutor();
@SuppressLint({"NewApi", "LocalSuppress"}) CompletableFuture<Void> future = CompletableFuture.runAsync(() -> {
OneSignal.getNotifications().requestPermission(true, Continue.none());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use a Thread instead for this example code?

Copy link
Contributor Author

@jinliu9508 jinliu9508 Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I check out the second commit and use this piece of code in MainApplication.java, I am not able to reproduce the crash anymore.

// ensure calling requestPermission in a thread right after initWithContext does not crash
        new Thread(new Runnable() {
            @Override
            public void run() {
                OneSignal.getNotifications().requestPermission(true, Continue.none());
            }
        }){}.start();

Reports from user are suggesting that

CoroutineScope(Dispatchers.IO).launch {
  OneSignal.Notifications.requestPermission(true)
}

is what causes the crash.

Although both Java Thread and Kotlin CoroutineScope are used for handling concurrency, they seem to operate differently. The only equivalence in Java I found to reproduce the same crash is by calling java8 'CompletableFuture.join()' I guess the reason is that 'CompletableFuture.join()' waits for the thread to complete, which is similar to Kotlin Coroutine that can be suspended.

I added a comment to further explain the reason CompletableFuture and its related annotation is added here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative is to add a Kotlin object file to the example app that has a function to run

CoroutineScope(Dispatchers.IO).launch { 
   OneSignal.Notifications.requestPermission(true) 
}

and then call it from MainApplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I have tried this approach as well, and they behave the same in my test. I chose the Java code here considering "broader" test coverage from other wrappers that run Java code over the bridge.

}, executor);
future.join(); // Waits for the task to complete
executor.shutdown();

OneSignal.getInAppMessages().addLifecycleListener(new IInAppMessageLifecycleListener() {
@Override
public void onWillDisplay(@NonNull IInAppMessageWillDisplayEvent event) {
Expand Down
2 changes: 0 additions & 2 deletions OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/CoreModule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion ...re/src/main/java/com/onesignal/core/internal/permissions/impl/RequestPermissionService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import com.onesignal.core.internal.permissions.IRequestPermissionService

internal class RequestPermissionService(
private val _application: IApplicationService,
) : Activity(), IRequestPermissionService {
) : IRequestPermissionService {
var waiting = false
var fallbackToSettings = false
var shouldShowRequestPermissionRationaleBeforeRequest = false
Expand Down
18 changes: 9 additions & 9 deletions ...nalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
package com.onesignal.core.internal.startup

import com.onesignal.common.services.ServiceProvider

internal class StartupService(
private val _bootstrapServices: List<IBootstrapService>,
private val _startableServices: List<IStartableService>,
private val services: ServiceProvider,
) {
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() {
Thread {
services.getAllServices<IStartableService>().forEach { it.start() }
}.start()
}
}
69 changes: 34 additions & 35 deletions OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,50 +86,56 @@ 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",
)
}
override val notifications: INotificationsManager get() =
if (isInitialized) {
_notifications!!
services.getService()
} else {
throw Exception(
"Must call 'initWithContext' before use",
)
}
override val location: ILocationManager get() =
if (isInitialized) {
_location!!
services.getService()
} else {
throw Exception(
"Must call 'initWithContext' before use",
)
}
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
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -328,10 +327,10 @@ 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
}
}
Expand Down
53 changes: 34 additions & 19 deletions ...K/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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()
Expand All @@ -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()
Expand All @@ -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 =
Expand All @@ -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()
}
}
})
Loading