Skip to content

[SDK-707] Replace request AsyncTask with SDK executor - #1096

Merged
franco-zalamena-iterable merged 7 commits into
masterfrom
feature/sdk-707-request-executor
Oct 8, 2026
Merged

franco-zalamena-iterable merged 7 commits into
masterfrom
feature/sdk-707-request-executor

Conversation

@franco-zalamena-iterable

@franco-zalamena-iterable franco-zalamena-iterable commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Replaces the remaining API-request AsyncTask execution with SDK-owned dispatchers while preserving the SDK's existing concurrency, retry ordering, callback threading, and offline-delivery behavior.

Jira: SDK-707

This stacked PR builds on SDK-706.

Behavior

  • Ordinary online API requests remain concurrent.
  • Automatic online request retries remain serial, matching the previous default AsyncTask.execute() behavior.
  • Offline-mode requests that cannot be stored execute serially.
  • Stored offline requests retain their own FIFO execution flow. Immediate and stored offline requests are independent; no cross-flow ordering is promised.
  • Push requests remain on the dedicated serial push lane introduced by SDK-705, and deep-link redirects remain on the SDK-706 lane.
  • Saturating the primary online pool moves excess work to a background overflow executor instead of dropping the request or running network work on the caller.
  • A persisted backlog continues draining if offline processing is disabled. On restart, the backlog runner is created only when stored rows exist, and logout clears those rows before identity changes.
  • API callbacks continue to be delivered on the Android main thread, and retries retain the request's callback contract.

Compatibility

  • No public API is added or changed.
  • This does not make all SDK requests single-threaded.
  • Normal online requests remain concurrent; ordering is retained only for flows that were serial before this migration.

Verification

Passed:

  • ./gradlew --no-daemon :iterableapi:testDebugUnitTest :iterableapi-ui:testDebugUnitTest
  • ./gradlew --no-daemon :iterableapi:lintDebug :iterableapi:checkstyle :iterableapi-ui:assembleDebug
  • ./gradlew --no-daemon :app:testDebugUnitTest
  • Focused routing, overflow, retry-ordering, offline-lane, persisted-storage, and task-runner tests

The required standalone inbox-customization sample still reaches iterableapi-ui compilation and fails on its pre-existing Kotlin language-version mismatch: the sample uses Kotlin 1.8 while the UI module contains Kotlin 1.9 data object declarations.

Changelog

Updated the Unreleased executor entry to describe concurrent online requests, serial automatic retries and offline-immediate work, and the independent stored-offline FIFO flow.

No documentation PR or GitHub issue is required.

@rtlsilva rtlsilva 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.

The ticket says the full suite should contain no ShadowPausedAsyncTask references, but this head still has six references in BaseTest, IterableApiIntegrationTest, and IterableApiGetAndTrackDeepLinkTest.

Suggest removing the remaining shadow dependencies and synchronizing those tests through the new request seam or the appropriate executor/looper primitives.

Comment thread iterableapi/src/main/java/com/iterable/iterableapi/OfflineRequestProcessor.java Outdated
@franco-zalamena-iterable

Copy link
Copy Markdown
Contributor Author

Review feedback is addressed in ffe5431:

  • Restored IterableApiIntegrationTest to the production dispatcher.
  • Synchronized the callback-driven automatic push-registration test through the main looper.
  • Migrated IterableApiResponseTest to IterableRequestDispatcher; Android test sources compile.
  • Documented the inline request seam callback contract.
  • Routed persisted offline requests through the same serial dispatcher as immediate requests, with stored-versus-immediate ordering coverage and an in-flight guard.
  • Removed all ShadowPausedAsyncTask references from the suite.

Verification on this commit:

  • SDK-707 behavior and regression tests pass in the 764-test core run. The sole local failure is the pre-existing external-image IterableNotificationTest.testNotificationImage; 21 tests are skipped.
  • iterableapi-ui unit tests pass.
  • lintDebug, checkstyle, iterableapi-ui assembleDebug, app unit tests, and Android-test compilation pass.
  • The standalone sample reaches iterableapi-ui compilation and remains blocked by its existing Kotlin 1.8 versus data object language-version mismatch.

Requesting another review.

@franco-zalamena-iterable
franco-zalamena-iterable force-pushed the feature/sdk-707-request-executor branch from ffe5431 to e697ce6 Compare September 23, 2026 12:54
@franco-zalamena-iterable

Copy link
Copy Markdown
Contributor Author

Updated SDK-707 after the stack rebase, preserving all prior review fixes and addressing the concurrency concern:

  • Ordinary online API requests use a bounded concurrent SDK pool.
  • Push requests stay on the dedicated serial push lane.
  • Offline immediate and persisted requests share a dedicated serial offline lane.
  • Deep-link work remains on its separate SDK-706 lane.
  • Callbacks remain on main, and retries retain the originating dispatcher.

New focused coverage proves that the client routes ordinary and push requests to the correct lanes, a blocked online request does not serialize a second online request, and blocked online work does not delay push work. The synchronization uses latches with no sleeps.

Verification:

  • Focused executor, request, retry, offline, and task-runner suites pass.
  • Full core run: 770 passed, 21 skipped; only the pre-existing external IterableNotificationTest.testNotificationImage failed.
  • UI unit tests pass.
  • Lint, Checkstyle, UI assemble, root app unit tests, and Android-test source compilation pass.
  • The standalone sample remains blocked by its existing Kotlin 1.8 versus data-object language-version mismatch.

CI is currently running on e697ce6. Ready for another review.

@franco-zalamena-iterable

Copy link
Copy Markdown
Contributor Author

Follow-up from the first CI run: the new concurrency and routing tests passed. CI exposed a separate Mockito spy race in IterableFirebaseMessagingServiceTest, where a pending initialization callback could call getDebugMode while the test was stubbing getInAppManager. Commit 2f34558 now drains initialization callbacks before creating the spy and uses safe doReturn stubbing. The affected service tests and executor tests pass locally; replacement CI is running.

@franco-zalamena-iterable

Copy link
Copy Markdown
Contributor Author

Final CI follow-up for 2f34558:

  • Check, changelog, Java analysis, CodeQL, BCIT, and instrumentation all pass.
  • The callback-driven Mockito race no longer appears.
  • The unit job ran 771 tests with only the repository-wide external IterableNotificationTest.testNotificationImage failure; all SDK-707 executor, routing, request, retry, offline, and service tests passed.

The branch is now waiting only on reviewer approval and the known external image-test failure.

@rtlsilva rtlsilva 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.

No new comments on this PR, but waiting for ancestor PRs #1093 and #1094 for a final review before approving.

@jferrao-itrbl jferrao-itrbl 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.

Great work @franco-zalamena-iterable . LGTM.

It might be worth checking the failing test though it seems unrelated with these changes.

@joaodordio
joaodordio requested a review from a team September 25, 2026 10:18
@franco-zalamena-iterable
franco-zalamena-iterable force-pushed the feature/sdk-707-request-executor branch 2 times, most recently from 7ca9382 to b6d0d84 Compare October 6, 2026 09:00

@rtlsilva rtlsilva 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.

Reopened a discussion, no other findings.

@franco-zalamena-iterable
franco-zalamena-iterable force-pushed the feature/sdk-707-request-executor branch from a34b9e3 to 5cb0451 Compare October 7, 2026 09:16
Base automatically changed from feature/sdk-706-deeplink-redirect-executor to master October 7, 2026 09:30

@rtlsilva rtlsilva 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.

No new comments. LGTM

@franco-zalamena-iterable
franco-zalamena-iterable merged commit 7944446 into master Oct 8, 2026
8 checks passed
@franco-zalamena-iterable
franco-zalamena-iterable deleted the feature/sdk-707-request-executor branch October 8, 2026 11:00
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.

3 participants