Skip to content

refactor: [SDK-4995] bootstrap dispatchers off the caller thread - #2766

Open
fadi-george wants to merge 1 commit into
mainfrom
fadi/sdk-4995-dispatcher-bootstrap
Open

fadi-george wants to merge 1 commit into
mainfrom
fadi/sdk-4995-dispatcher-bootstrap

Conversation

@fadi-george

Copy link
Copy Markdown
Contributor

Description

One Line Summary

Build dispatcher executors on a background bootstrap thread so the first launchOn* call never constructs a pool on the caller (often main) thread.

Details

Motivation

Split out of #2712. prewarm() is best-effort: a cold-start entry point that dispatches right after it can still win the lazy-init race and pay executor construction on the main thread (SDK-4995 Play Vitals data showed no improvement after the 5.9.5 prewarm).

Scope

  • Each lane (IO, Default, SerialIO) is a GateDispatcher that queues work while cold and requests bootstrap from a single BootstrapCoordinator thread.
  • Once the executor is built and core threads are prestarted, queued work drains in order and later dispatches go straight to the executor.
  • If primary construction fails, the lane falls back to Dispatchers.IO / Default / IO.limitedParallelism(1). If fallback also fails, queued jobs are cancelled and later lanes are not wedged.
  • Rejected or cancelled work is completed on Dispatchers.IO, never inline on the caller thread.
  • SerialIO keeps an unbounded queue, matching the previous newSingleThreadExecutor.
  • prewarm() now just requests warmup for every lane. No public API changes.

Testing

Unit testing

  • First launch returns while its lane is still being created, and creation happens off the caller thread.
  • A cold queued launch completes when its generation is reset.
  • Primary and fallback bootstrap failure does not wedge later lanes.

Manual testing

Not tested on device separately; covered by the full core and notifications unit suites.

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
  • 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.
  • I have reviewed this PR myself, ensuring it meets each checklist item

Co-authored-by: Cursor <cursoragent@cursor.com>
@fadi-george
fadi-george requested a review from a team as a code owner September 23, 2026 21:17

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Multi-model review (Opus 5.5, GPT 5.6 Sol, Grok 4.7)

Intent: first launchOn* / dispatch must queue and return without building a pool on the caller (often main).

Act on

  1. failPending reopens CLOSED (3/3). close() is not terminal if in-flight initialize() then fails — the old gate goes back to COLD and can leak a new executor after resetForTest has already dropped that generation.
  2. createTarget leaks workers on prestart failure (3/3). prestartAllCoreThreads() can throw after some cores are alive; allowCoreThreadTimeOut(false) and no shutdownNow() on that path, so retries leak more threads.
  3. Error from thread creation skips fallback (2/3, Opus + Grok). Only catch (Exception) tries Dispatchers.IO / Default. prestartAllCoreThreads() throwing OutOfMemoryError is the realistic primary-failure case.

Consider

  • Cold pending is unbounded; drain then burst-submits into the 200-slot IO/Default queues, so work queued during a slow bootstrap can be cancelled at birth (3/3).
  • prewarm() tests no longer assert off-caller construction; a synchronous prewarm would still pass (3/3).
  • Coordinator running + ConcurrentLinkedQueue exit vs request() can leave a lane stuck in STARTING (Grok).
  • prewarmStarted stays true if the bootstrap thread fails to start, so later prewarm() no-ops (Opus, Grok).

Noted

  • cancelAndComplete on Dispatchers.IO can run SerialIO finally off the serial thread (Opus).
  • close() during DRAINING/READY shutdownNow()s without completing those Jobs (Opus).

Dismissed

  • Public API contract is intact. SerialIO remaining unbounded is intentional.
Open in Web View Automation 

Sent by Cursor Automation: PR Reviews

synchronized(lock) {
val copy = pending.toList()
pending.clear()
state = LaneState.COLD

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Act on (3/3): failPending always sets COLD, including after close() has set CLOSED. If resetForTest / shutdown races an in-flight initialize(), the discarded generation can bootstrap again and leak an executor that later resets will not tear down. Keep CLOSED terminal.

OptimizedThreadFactory(config.threadName, config.priority),
)
executor.allowCoreThreadTimeOut(false)
executor.prestartAllCoreThreads()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Act on (3/3): If prestartAllCoreThreads() throws after starting some workers, this executor is never shutdownNow()’d (allowCoreThreadTimeOut(false)). Also 2/3 (Opus + Grok): that throw is typically OutOfMemoryError, so createTargetOrFallback skips the Dispatchers.* fallback and only catch (Exception) would have used it.

@abdulraqeeb33 abdulraqeeb33 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Requesting changes. CI is still red (NotificationGenerationProcessorTests > processNotificationData should not display notification when external callback indicates not to).

prestartAllCoreThreads throwing Error skips the Dispatchers fallback and leaks the half-built executor. That is the failure the fallback is for.

failPending("OneSignal $lane dispatcher fallback failed")
null
}
} catch (t: Throwable) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

prestartAllCoreThreads (line 365) throws OutOfMemoryError after some core threads exist. That misses catch (Exception) and lands here, which cancels queued work and never calls createFallbackTarget. Shut the partial executor down in a finally, then fall back. Only failPending if the fallback also fails. failPending also sets the lane back to COLD, which reopens a CLOSED generation during resetForTest.

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.

2 participants