initWithContext synchronization fix

[brismithers]

Description

One Line Summary

Synchronize initWithContext so initialization occurs once, and prevent login/logout from being called before initWithContext.

Details

It is possible for an app to call initWithContext on different threads at the same time, causing the initialization logic to run more than once. If this were to happen, the internal state of the SDK will not be correct. For example, the initialization code drives calling registerActivityLifecycleCallbacks, we don't want to have 2 of those!

This change synchronizes the initialization logic so the SDK can only go through initWithContext once. Additionally, the login and logout functions now throw exceptions rather than log an error, if called before initWithContext. This is done to raise programming errors to the caller, rather than swallow them in the logs.

Motivation

No specific issues to point to, although the belief is this is a concurrency issue that needs to be resolved.

Scope

This change is limited to initialization and login/logout flows.

Testing

Unit testing

2 additional unit tests were added to demonstrate the expected behavior of calling login/logout before calling initWithContext.

Manual testing

Run the OneSignal Example App on an emulator to ensure initialization of the SDK continues to operate successfully.

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 

[shepherd-l]

Could you fix the linting errors in OneSignalImplTests?

[brismithers]

Done, also renamed OneSignalImplTests to OneSignalImpTests to align with OneSignalImp

[jennantilla]

Makes sense to add additional synchronization here, great work!

[jkasten2]

A if (isInitialized) check was added in this PR in the new synchronized block, so we can remove this one.

[brismithers]

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt line 175 at r2 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

A if (isInitialized) check was added in this PR in the new synchronized block, so we can remove this one.

The check is done twice intentionally. Most post-initialization callers will hit this if block and can do so without lock contention. We need to check isInitialized again after obtaining the lock to ensure there wasn't someone else (that had the lock, but didn't finish initialization).

[jkasten2]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brismithers, @emawby, @nan-li, and @shepherd-l)

---

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt line 175 at r2 (raw file):

Previously, brismithers (Brian Smith) wrote…

The check is done twice intentionally. Most post-initialization callers will hit this if block and can do so without lock contention. We need to check isInitialized again after obtaining the lock to ensure there wasn't someone else (that had the lock, but didn't finish initialization).

Gotcha, I have seen this before, it trades performance (in-most cases) over less code. Since init isn't normally called more than not sure if I like that trade off. However it's more of a nit so I'll unblock the PR and leave that up to you.

[brismithers]

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt line 175 at r2 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Gotcha, I have seen this before, it trades performance (in-most cases) over less code. Since init isn't normally called more than not sure if I like that trade off. However it's more of a nit so I'll unblock the PR and leave that up to you.

that makes sense. I've removed the extra check before obtaining the lock. If we notice high contention cases this can always be added in the future!