[Optimization] Init with context in background

[jinliu9508]

Description

One Line Summary

Move start call of unused services off the main thread to make the initWithContext more efficient.

Details

Motivation

Our SDK initialization is currently taking longer to finish compare to some other SDKs. One of the reason is we construct service components like Location Manager or IAM Manager during the initialization phrase when they are not really needed until later. This PR aims to move the initialization of these services to background thread so they don't slow down the overall SDK initialization.

Scope

The retrieval of services that were changed to initialize in background is not longer from a saved instance in OneSignalImp.kt. Instead, each of these components has a getter that retrieve its instance from the service provider.

Testing

Manual testing

• Bench mark tests: I measure how long the initialization takes by tracking the start and finish time when 'initWithContext' is called. In my testing in a pixel 3a API 34 emulator, it took an average of 600 ms to complete initialization before the change and around 300 ms after the change, which is around ~50% faster.
• Normal SDK behavior test: Send notification, add tags, and outcome work.
• Calling 'initWithContext' in a background thread: no error and the demo app works as normal. Other OneSignal calls can only work if they are called after 'initWithContext' and in the same thread.
• Calling 'requestPermission' in a background thread: a test case is added in MainApplication to ensure the app won't crash by calling requestPermission in a thread right after initWithContext.

Affected code checklist

• Notifications
  • Display
  • Open
  • Push Processing
  • Confirm Deliveries
• Outcomes
• Sessions
• In-App Messaging
• REST API requests
• Public API changes

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 

[jkasten2]

Since we only need to run a one off background task can we use a Thread instead of a CoroutineScope? This has a bit less overhead.

[jinliu9508]

Sure. By using thread, we can avoid creating coroutineScope.

But I think Coroutine is a simpler version of Thread as it does not use as much resources going down to the OS threads and only take an object in the JVM heap. In my test, the time it takes for the code inside the block to complete by using coroutineScope.launch vs using thread is very similar. Using thread is a tiny bit slower in average. This may be affected by the current os responsiveness as well.

Another reason I used coroutine is that this practice is also used in other places like operationRepo so there is some consistency. Do we think it is worth changing it to using thread?

[jkasten2]

In this specific case we were creating a newSingleThreadContext and just running one task, the OperationRepo stays alive forever and continually takes work. In this class we simply need to run something in the background and then we no longer need the thread.

We could optimize this in future by using any existing coroutineScope that is shared with other parts of the SDK, but since we don't have that it is more efficient to use a thread.

In my test, the time it takes for the code inside the block to complete by using coroutineScope.launch vs using thread is very similar.

Did you include CoroutineScope(newSingleThreadContext(name = "StartupService")) and coroutineScope.launch in your test vs Thread {}.start()?

[jinliu9508]

Ok the newest commit has changed to use 'Thread.start()' instead of using 'coroutineScope'. I placed a mark right before the initialization of 'StartupService', a mark right before 'launch'/'Thread.start', and the final mark at the completion. The diff is insignificant in general.

[jkasten2]

I looked over all the classes that implement IStartableService and can't see any issues with their start() method from being called delayed / called from the background.

[jkasten2]

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

[jinliu9508]

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.

[nan-li]

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.

[jinliu9508]

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.

[jinliu9508]

Force push: Consolidating the commits. Code is unchanged since last review.