Skip to content

Conversation

@abdulraqeeb33
Copy link
Contributor

@abdulraqeeb33 abdulraqeeb33 commented Nov 12, 2025

Description

One Line Summary

Fix race condition in initWithContext preventing concurrent initialization and premature SDK access.

Details

This PR fixes a race condition that could occur when initWithContext() was called multiple times concurrently, or when SDK accessors were accessed before initialization completed. The fix ensures thread-safe initialization state management and prevents inconsistent SDK state.

The Problem

The race condition could manifest in two scenarios:

  1. Concurrent Initialization: If multiple threads called initWithContext() simultaneously, both could attempt to initialize the SDK, leading to:

    • Duplicate service initialization
    • Inconsistent initState values
    • Potential crashes or undefined behavior
  2. Premature Access: If SDK accessors (like OneSignal.User, OneSignal.Notifications, etc.) were accessed before initWithContext() completed, they could:

    • Access uninitialized services
    • Return null or throw NullPointerException
    • Cause ANRs if blocking occurred on the main thread

The Solution

The fix implements proper synchronization and state management:

  1. Synchronized State Checks: Added synchronized(initLock) around initState checks to prevent concurrent initialization attempts
  2. State-Based Guarding: Early return if SDK is already initialized or initialization is in progress
  3. Consistent State Management: initState is properly set to IN_PROGRESS before starting initialization and SUCCESS/FAILED after completion
  4. Proper Waiting Mechanism: Accessors that need initialization wait using CompletableDeferred to ensure consistent state

Related Issues

  • Fixes race condition reported in #2192

Motivation

To address the race condition where concurrent initialization attempts or premature SDK access could lead to crashes or inconsistent behavior.

Scope

  • Internal race condition handling in OneSignalImp.initWithContext()
  • Thread-safe state management using synchronized blocks
  • Proper state transitions during initialization lifecycle
  • No public API changes - purely internal implementation improvement

Testing

Unit testing

  • Existing tests continue to pass, verifying backward compatibility
  • Race condition scenarios are implicitly tested through existing initialization tests
  • State management is verified through InitState enum transitions

Manual testing

  • Tested concurrent initWithContext() calls from multiple threads
  • Verified SDK accessors work correctly when called immediately after initWithContext()
  • Confirmed no crashes or inconsistent state when initialization is in progress
  • Smoke tested on various devices and Android versions

Affected code checklist

  • [] Notifications
    • [] Display (protected by initialization state checks)
    • [] Open (protected by initialization state checks)
    • [] Push Processing (protected by initialization state checks)
    • [] Confirm Deliveries (protected by initialization state checks)
  • [] Outcomes (protected by initialization state checks)
  • [] Sessions (protected by initialization state checks)
  • [] In-App Messaging (protected by initialization state checks)
  • [] REST API requests (initialization must complete before API calls)
  • Public API changes (no breaking changes)

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing - fixes race condition in initialization
  • 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.
    • Proper synchronization blocks with clear comments
    • State management follows clear lifecycle patterns
    • Thread-safety considerations are documented
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

Comment on lines 332 to 342
// Check state and provide appropriate error messages
when (initState) {
InitState.FAILED -> {
throw IllegalStateException("Initialization failed. Cannot proceed.")
}
InitState.NOT_STARTED -> {
throw IllegalStateException("Must call 'initWithContext' before 'logout'")
}
InitState.IN_PROGRESS, InitState.SUCCESS -> {
// Continue - these states allow proceeding (will wait if needed)
}
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate code, can we make a function for this?

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 callout, i centralized all of this. including the code path for suspend and blocking.

trigger.complete()

// Wait for initialization to complete (internalInit runs asynchronously)
var attempts = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the wait mechanism for the initialization seems repetitive in the test. Can we clean this up?

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

I would rather not have waitUntilInitInternal return, but initState == InitState.IN_PROGRESS. It creates an extra inconsistent state and doesn't provide any benefit I can see.

I would rather we just wait here as long as it takes so code doesn't have to consider both states. PR #2412 does this, and only logs if it is taking longer than expected.

@abdulraqeeb33 abdulraqeeb33 changed the title bug: Handling race condition fix: Remove throwing "initWithContext was not called or timed out", introduced in 5.4.0 Dec 5, 2025
@abdulraqeeb33 abdulraqeeb33 merged commit f63a583 into main Dec 8, 2025
1 of 2 checks passed
@abdulraqeeb33 abdulraqeeb33 deleted the bug/initWithContextRaceCondition branch December 8, 2025 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants