Refactor: move part of initWithContext off main thread by jinliu9508 · Pull Request #2368 · OneSignal/OneSignal-Android-SDK

jinliu9508 · 2025-09-11T14:33:28Z

Description

One Line Summary

Move slow/IO-bound parts of initWithContext off the main thread and add an async overload with a completion callback for internal usage.

Details

Motivation

Avoid main-thread stalls and potential ANRs during initialization, especially when SharedPreferences or other disk/network operations are involved. Provide an async path so app components (Activities/Receivers) can trigger initialization without blocking UI or risking Receiver timeouts.

Scope

initWithContext(contex, appId): now move all initialization to background and return immediately.
initWithContext(contex) is now suspend and can only be called from a coroutine. This method should used by internal classes only. It will remain suspend until all initialization is completed.
Accessors remain blocked if called immediately after initWithContext and before initialization is fully completed. This is to ensure no breaking change or expectation on existing consumer use cases.
Receivers will now call initWithContext(contex) in background, and switch back to main on tasks that must be executed in main thread, eg, finish(), startActivity() and other app lifecycle or UI operations.
Workers run doWork() in background will now directly call initWithContext(contex)
Retain the existing synchronous overload for backward compatibility; the new overload provides a non-blocking path with a completion callback.
Introduces a centralized ANR timeout constant.
No behavioral change to notification delivery formats, APIs, or user-visible features.

Testing

Unit testing

New Robolectric/Kotest tests cover initWithContext in different scenarios. Ex:
- Accessors throw before initialization.
- Init does not block even when SharedPreferences access is artificially delayed.
- Tags, local onesignalId, push subscription, and immediate login right after init.
LatchAwaiterTests testing the genetic functionalities of the waiter.

Manual testing

Cold-start the sample app with logging at VERBOSE. Ensure all initial operations are created in the same order.
Trigger init from an Activity and verify UI remains responsive.
For Receivers: simulate a boot or FCM broadcast; verify the receiver returns promptly (no >10s wait), and OneSignal completes initialization shortly after.
Notification click when app is killed / backgrounded.
Login, logout, addTag, addTrigger, permission update, IAM interaction
Environment: Android Studio Hedgehog or later; Pixel 6 emulator API 33; fresh install of the demo app.

Affected code checklist

Checklist

Overview

I have filled out all REQUIRED sections above
PR does one thing
- If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
Any Public API changes are explained in the PR details and conform to existing APIs

Testing

I have included test coverage for these changes, or explained why they are not needed
All automated tests pass, or I explained why that is not possible
I have personally tested this on my device, or explained why that is not possible

Final pass

Code is as readable as possible.
- Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
I have reviewed this PR myself, ensuring it meets each checklist item
- WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is

abdulraqeeb33

a few nits and some must haves

abdulraqeeb33 · 2025-09-16T15:11:23Z

+    testImplementation("org.robolectric:robolectric:4.8.1")
+    // kotest-extensions-android allows Robolectric to work with Kotest via @RobolectricTest
+    testImplementation("br.com.colman:kotest-extensions-android:0.1.1")
+    testImplementation("androidx.test:core-ktx:1.4.0")


can we create variables for these version numbers? atleast until we move to kotlin dsl

These testImplementations are no longer there as we removed the app level tests out.

abdulraqeeb33 · 2025-09-18T18:02:25Z

+/**
+ * Create an awaiter for OneSignal SDK specifically.
+ */
+fun createOneSignalAwaiter() = LatchAwaiter("OneSignal SDK")


maybe we dont need these two methods?

Removed in the follow up commit

abdulraqeeb33

small nits but otherwise looks good

abdulraqeeb33 · 2025-09-25T16:44:47Z

        suspendifyOnThread {
+            // init OneSignal in background
+            if (!OneSignal.initWithContext(this)) {
+                jobFinished(jobParameters, false)


did we need this here @jinliu9508 ? looks like jobFinished was not called earlier when returning false

Yes, since now we always return true, jobFinished needs to be called to cancel the job if initialization is unsuccessful.

abdulraqeeb33 · 2025-09-25T16:48:36Z

+        // init OneSignal and enqueue restore work in background
+        suspendifyOnThread {
+            if (!OneSignal.initWithContext(context.applicationContext)) {
+                Logging.warn("NotificationRestoreReceiver skipped due to failed OneSignal init")


typo - upgradeReceiver

Addressed in the fixup

jkasten2

This PR is based on main, however we discussed we want to release it with JWT so it should be based on top of that.

jkasten2 · 2025-09-25T17:02:51Z

+ * A generic latch that allows waiting for asynchronous initialization or completion
+ * with timeout support and detailed logging.


Can we remove the word "generic" and explain in more detail the purpose of this class? Another sentence or two worth of specifics around the main / UI thread and ANR use-cases.

jkasten2 · 2025-09-25T17:12:03Z

+            try {
+                AndroidUtils.isRunningOnMainThread()
+            } catch (_: Throwable) {
+                false
+            }


Can we remove the try-catch here? I don't see how this could ever throw. Also pre-existing usages in the code base also don't have a try-catch.

jkasten2 · 2025-09-25T18:40:24Z

-        if (isOrderedBroadcast) {
-            resultCode = Activity.RESULT_OK
+    private suspend fun setSuccessfulResultCode() {
+        withContext(Dispatchers.Main) {


This doesn't need to run on the main thread, better if it doesn't

Addressed in the fixup

jkasten2 · 2025-09-25T18:40:36Z

        if (isOrderedBroadcast) {
-            // Prevents other BroadcastReceivers from firing
-            abortBroadcast()
+            withContext(Dispatchers.Main) {


This doesn't need to run on the main thread, better if it doesn't

Addressed in the fixup

jkasten2 · 2025-09-25T18:49:52Z

+        // run init in background
+        suspendifyOnThread {


I have concerns this is going to have side-effects. Mainly that notificationPayloadProcessorHMS.handleHMSNotificationOpenIntent will now run AFTER finish(). I recommend use the same logic from NotificationOpenedActivityBase to avoid changing order of execution.

Good point, the logic now runs in a suspendifyBlocking. This might cause ANR as we can no longer move it to the background.

The logic is now unchanged. However, ANR may still occur if initialization took too long

jkasten2 · 2025-09-25T19:20:12Z

+                withTimeout(LatchAwaiter.ANDROID_ANR_TIMEOUT_MS) {
+                    runBlocking {
+                        suspendInitInternal(context, null)
+                    }
                }


withTimeout will cause suspendInitInternal to stop executing. This could lead the SDK to being in an unpredictable state so I would rather we let it run.

addressed in the followup

jkasten2 · 2025-09-25T19:26:37Z

+            } catch (e: TimeoutCancellationException) {
+                Logging.log(LogLevel.ERROR, "initWithContext: Initialization timed out")
+                false
+            } catch (e: Exception) {
+                Logging.log(LogLevel.ERROR, "initWithContext: Initialization failed with exception", e)
+                false
            }


Don't catch here at all, it is going to hide root causes, as all we are going to get is a very generic throw IllegalStateException("Initialization failed. Cannot proceed.") later.

addressed in the followup

jkasten2 · 2025-09-25T19:29:21Z

+        context: Context,
+        appId: String?,
+    ): Boolean =
+        withContext(Dispatchers.Default) {


I see two issues with this withContext line

I don't think we should switch threads in this method, let the caller pick the thread / context to simply this.

This could cause delays in init based on what else the app might be putting on this generic context. We also don't use Dispatchers.Default anywhere else in the code base either.

addressed in the followup; the method is now no long in a thread

jkasten2 · 2025-09-25T19:41:44Z

+            } catch (e: Throwable) {
+                Logging.error("suspendInitInternal failed!", e)
+                initState = InitState.FAILED
+                latchAwaiter.release()
+                return@withContext false
+            }


Catching Throwable is going to hide root cause from crash reports, since we are not rethrowing here. If this was pre-existing before this PR it is ok to keep for now, but I don't think that was the case.

I do think the logic in the catch make sense to do if there is a failure, so let's keep doing that.

addressed in the followup; no longer in try-catch

jkasten2 · 2025-09-25T21:57:16Z

+        synchronized(initLock) {
+            if (initState.isSDKAccessible()) {
+                Logging.log(LogLevel.DEBUG, "initWithContext: SDK already initialized or in progress")
+                return true
+            }
+        }

-            // bootstrap services
-            startupService.bootstrap()
+        initState = InitState.IN_PROGRESS


synchronized(initLock) { } block needs to be around initState = InitState.IN_PROGRESS too, otherwise it doesn't solve anything.

Addressed in the fixup

jkasten2 · 2025-09-25T22:09:16Z

+ * Usage:
+ *   val awaiter = LatchAwaiter("OneSignal SDK Init")
+ *   awaiter.release() // when done
+ *   awaiter.awaitOrThrow() // or await() to just check


awaitOrThrow no longer exists, remove this in the comment

Addressed in the fixup

jkasten2 · 2025-09-25T22:14:31Z

+        // do not do this again if already initialized or init is in progress
+        synchronized(initLock) {
+            if (initState.isSDKAccessible()) {
+                Logging.log(LogLevel.DEBUG, "initWithContext: SDK already initialized or in progress")
+                return true
+            }
+        }

-            // bootstrap services
-            startupService.bootstrap()
+        initState = InitState.IN_PROGRESS


synchronized(initLock) { } block has to be around initState = InitState.IN_PROGRESS, as only around this read doesn't do anything on it's own.

Addressed in the fixup

jinliu9508 added the WIP Work In Progress label Sep 11, 2025

jinliu9508 force-pushed the refactor-init-in-background branch 2 times, most recently from 57aed46 to f048835 Compare September 15, 2025 05:25

jinliu9508 requested review from abdulraqeeb33, jkasten2 and nan-li September 15, 2025 14:34

jinliu9508 assigned abdulraqeeb33 Sep 15, 2025

jinliu9508 removed the WIP Work In Progress label Sep 15, 2025

jkasten2 requested changes Sep 15, 2025

View reviewed changes

abdulraqeeb33 force-pushed the refactor-init-in-background branch from 973461e to 38f9839 Compare September 18, 2025 16:29

jinliu9508 requested a review from jkasten2 September 18, 2025 17:37

abdulraqeeb33 reviewed Sep 18, 2025

View reviewed changes

jkasten2 requested changes Sep 19, 2025

View reviewed changes

jinliu9508 force-pushed the refactor-init-in-background branch 2 times, most recently from d8a1a50 to 97932bf Compare September 24, 2025 05:01

jinliu9508 requested review from abdulraqeeb33 and jkasten2 September 24, 2025 15:51

jinliu9508 force-pushed the refactor-init-in-background branch from 8e106eb to c80e0fb Compare September 24, 2025 23:29

abdulraqeeb33 approved these changes Sep 25, 2025

View reviewed changes

jkasten2 requested changes Sep 25, 2025

View reviewed changes

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

jinliu9508 force-pushed the refactor-init-in-background branch 2 times, most recently from 32acee4 to 695d830 Compare September 25, 2025 21:18

jinliu9508 requested a review from jkasten2 September 25, 2025 23:04

jkasten2 reviewed Sep 26, 2025

View reviewed changes

jkasten2 approved these changes Sep 26, 2025

View reviewed changes

jinliu9508 changed the base branch from main to 5.4-main September 26, 2025 20:54

refactor: move initialization process off main thread

808d1cc

jinliu9508 force-pushed the refactor-init-in-background branch from 23d672f to 808d1cc Compare September 26, 2025 21:02

jinliu9508 merged commit 6c3dfae into 5.4-main Sep 26, 2025
1 check passed

jinliu9508 deleted the refactor-init-in-background branch September 26, 2025 21:18

		* A generic latch that allows waiting for asynchronous initialization or completion
		* with timeout support and detailed logging.

Conversation

jinliu9508 commented Sep 11, 2025 • edited by abdulraqeeb33 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

One Line Summary

Details

Motivation

Scope

Testing

Unit testing

Manual testing

Affected code checklist

Checklist

Overview

Testing

Final pass

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abdulraqeeb33 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abdulraqeeb33 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkasten2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jinliu9508 commented Sep 11, 2025 •

edited by abdulraqeeb33

Loading

jkasten2 Sep 25, 2025 •

edited

Loading

jinliu9508 Sep 26, 2025 •

edited

Loading

jinliu9508 Sep 26, 2025 •

edited

Loading