Skip to content

feat: [SDK-4993] add Future and minSdk-safe callback bridges for Java - #2711

Open
abdulraqeeb33 wants to merge 2 commits into
feature/async-first-public-apifrom
ar/sdk-4993
Open

feat: [SDK-4993] add Future and minSdk-safe callback bridges for Java#2711
abdulraqeeb33 wants to merge 2 commits into
feature/async-first-public-apifrom
ar/sdk-4993

Conversation

@abdulraqeeb33

@abdulraqeeb33 abdulraqeeb33 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Java reaches the SDK's suspending APIs through Continue, which today offers exactly one shape: a callback built on java.util.function.Consumer. This adds a Future style alongside it and fixes a latent crash in the callback style, by extending Continue rather than standing up a second bridge next to it — option 1 from the ticket.

Ticket: SDK-4993. Part of SDK-4783.

Independent of the rest of the stack

Targets the feature branch directly. The ticket asks to "preserve the same OneSignalResult<T> return model," which reads like a dependency on SDK-4988 — but the way to satisfy it is to make the helpers generic over the return value and hand it back unchanged, so they never name a result type at all. A generic bridge preserves the model by construction.

For future() that also settles the double-envelope problem raised in the #2710 review: the value passes through untouched, so Java is handed one success flag rather than two. To be precise about the limit of that claim — callback() still wraps in ContinueResult, so a new-style API through it yields ContinueResult<OneSignalResult<LogoutData>>, where the outer flag is always true because the new APIs never throw. Whether callback should gain a raw-value variant belongs to the ticket that changes the return types; it still has to serve the legacy suspending APIs that do throw.

Continue.future()

Returns a FutureContinuation, which is both a Continuation and a Future, so a Java caller hands it to a suspending function and collects the answer later.

FutureContinuation<Boolean> consent = Continue.future();
OneSignal.getConsentGivenSuspend(consent);

boolean given = consent.get();  // off the main thread

get() throws MainThreadException rather than logging, since blocking there is the stall this migration exists to remove. It refuses even when the value has already arrived — allowing that case would make the same line throw or not throw depending on whether the call happened to finish first, which is a worse contract than a rule that always holds. Cancellation surfaces as CancellationException and everything else as ExecutionException, matching what Future already specifies and keeping a cancelled call distinguishable from a failed one.

Continue.callback() — a latent-bug fix in ergonomics clothing

The same callback bridge on a Kotlin fun interface instead of Consumer.

This is scope the ticket did not name, and it changes how to read the helper. Consumer only exists from API 24, core library desugaring is not enabled in this project, and minSdk is 21 — so on API 21 through 23, inside the SDK's own supported range, Continue.with() fails at runtime, with nothing but a @RequiresApi lint annotation as warning. Continue.callback() works across the whole range and is otherwise identical.

with is deliberately not deprecated: the ticket warned against churn, and it stays correct on API 24 and above.

Tests are in Java, which needed new infrastructure

The acceptance criterion asked for a Java test source set, and it is also the only way to assert what a Java caller actually types — if a signature stops being usable from Java, the file stops compiling. Kotest was not an option, because a Kotest spec is a Kotlin DSL and a Kotlin test cannot prove Java usability: Kotlin resolves SAM conversion, nullability, and generics on its own terms.

Why the vintage engine. Worth stating plainly that this adds the old test framework, not a new one. Robolectric ships only a JUnit 4 runner, and the main-thread tests need Robolectric because the guard reaches Looper.getMainLooper(), an android.jar stub that throws in a plain JVM test. Every test-bearing module here already calls useJUnitPlatform(), since Kotest only runs there, and the platform cannot discover JUnit 4 tests on its own — junit-vintage-engine is purely that adapter, which is why its version tracks the platform (5.x) rather than JUnit 4. Jupiter appears nowhere in this repo, so reaching for it would have been the genuinely new framework; JUnit 4 plus vintage is the conservative end of the trade.

I verified this did not disturb anything:

  • Grepped every test source in :OneSignal:core for org.junit imports — the only JUnit 4 file in the module is the one this PR adds, so no pre-existing test changed status.
  • Full core suite: 987 tests, 0 failures, with both new suites present in the results.
  • Three consecutive --rerun-tasks runs of the 17 new tests to rule out flakiness in the concurrent paths.

Both KDoc examples are also duplicated in a never-executed method in the Java test, so javac checks them against the real public API. A doc comment is the one place an unusable Java signature can sit indefinitely without anything noticing, and a local review pass caught exactly that — the original class example could not have compiled.

One test pins the shape this bridge cannot handle: a suspending function that returns without ever suspending hands its value straight back and never resumes the continuation, so a Future waiting on it would wait forever. Every public suspending API in the SDK changes dispatchers and therefore does suspend, and Continue.with has carried the same constraint for years — the test exists so that stops being an accident rather than because anything hits it today.

Reviewer notes

The API dump will need regenerating, and neither branch can do it alone. This adds four public symbols — FutureContinuation, ContinueCallback, Continue.callback, Continue.future — but there is no .api dump on the feature branch yet, because the binary-compatibility gate lands in SDK-4986. Whichever of the two merges second leaves apiCheck failing until core.api is regenerated. Flagging it now because it will otherwise look like an unrelated CI break later.

The guard protects the wait, not the call. A suspending function runs on the thread that started it until it first suspends, so invoking one from the main thread still does that much work there — no continuation can change that. The class KDoc says so explicitly rather than implying a stronger promise.

Java still cannot initiate cancellation. Future.cancel returns false, because the caller started the suspending function directly and this bridge never had a Job to cancel. Upstream cancellation is reported correctly. If Java-initiated cancellation is ever wanted, that means Kotlin-side async wrappers per method that own a Job — a materially larger design, and one the ticket steered away from.

Not here: the MIGRATION_GUIDE.md Java section, which the ticket assigns to the docs sub-issue. Worth knowing its existing examples are already stale — they use OneSignal.getNotifications(), which is no longer the API — so that pass should correct rather than build on them.

Test plan

  • :OneSignal:core:testDebugUnitTest — 987 tests, 0 failures
  • Three repeat runs of both new suites, no flakes
  • :OneSignal:core:detekt and spotlessCheck
  • Java: callback as a method reference, Future value, Unit return, failure as ExecutionException, main-thread refusal, timeout, and the non-suspending case
  • Kotlin: refusal even when complete, cancellation versus failure, timeout then late completion, cancel() honesty

@abdulraqeeb33
abdulraqeeb33 requested a review from a team as a code owner August 7, 2026 21:11
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

📊 Diff Coverage Report

Diff Coverage Report (Changed Lines Only)

Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff).

Changed Files Coverage

  • Continue.kt: 5/7 touched executable lines (71.4%) (70 touched lines in diff)
    • 2 uncovered touched lines in this file
  • FutureContinuation.kt: 26/27 touched executable lines (96.3%) (134 touched lines in diff)

Overall (aggregate gate)

31/34 touched executable lines covered (91.2% — requires ≥ 80%)

Per-file detail (informational; gate is aggregate above):

  • Continue.kt: 71.4% (2 uncovered touched lines)

📥 View workflow run

Java reaches the SDK's suspending APIs through Continue, which today offers
one shape: a callback built on java.util.function.Consumer. Two gaps follow
from that, and both are addressed by extending Continue rather than standing
up a second bridge beside it.

Continue.future() returns a FutureContinuation, which is both a Continuation
and a Future, so a Java caller can hand it to a suspending function and
collect the answer later. get() refuses to run on the main thread with
MainThreadException rather than logging, since blocking there is the stall the
async migration exists to remove. It is refused even when the value has
already arrived, because a rule that depended on whether the call happened to
finish first would throw or not throw depending on timing. Cancellation
surfaces as CancellationException and everything else as ExecutionException,
matching what Future already specifies and keeping a cancelled call
distinguishable from a failed one.

Continue.callback() is the existing callback bridge on a Kotlin fun interface
instead of Consumer. Consumer only exists from API 24 and core library
desugaring is not enabled here, so on API 21 through 23 — inside the SDK's
own supported range — Continue.with() fails at runtime rather than at compile
time. The two behave identically otherwise.

Neither helper names a result type of its own. They are generic over the
return value and hand it back unchanged, so whatever the suspending API
returns flows through untouched and Java is not handed a second success flag
to disambiguate.

Tests are in Java, because that is the only way to assert what a Java caller
actually types; if a signature stops being usable from Java, the file stops
compiling. Robolectric ships only a JUnit 4 runner and this module runs on
the JUnit Platform for Kotest, so the vintage engine lets both live in one
source set. One test pins the shape this bridge cannot handle: a suspending
function that returns without ever suspending never resumes its continuation,
so a Future waiting on it would wait forever. Every public suspending API in
the SDK hops dispatchers and therefore does suspend, but that is now recorded
rather than left to be discovered.

Co-authored-by: Cursor <cursoragent@cursor.com>
@fadi-george

Copy link
Copy Markdown
Contributor

Two blockers from my read, rest is minor.

FutureContinuation.get() needs @Throws(InterruptedException::class, ExecutionException::class) (plus TimeoutException on the timed overload). Without it Kotlin emits no throws clause, so the catch (ExecutionException e) in your own KDoc example doesn't compile. The Java test misses it because every get() goes through Callable.call() throws Exception.

resumeWith has no single-completion guard despite the KDoc promising one instance per call. A second resume can clear failure, and concurrent resumes can leave both fields null, where value as R returns null for a non-null R. A CAS plus IllegalStateException("Already resumed") matches the stdlib.

Also: the class KDoc example (getNotificationsSuspend(permission).requestPermission(...)) can't compile from Java and reuses one continuation for two calls. And I think the never-suspends hang is reachable, not just theoretical: Continue.future(OneSignalDispatchers.IO) against an internal withContext(OneSignalDispatchers.IO) hits the undispatched path and never resumes. Dropping the context param from future() would close that.

Two defects from review, both invisible to the tests as written.

get() carried no throws clause, since Kotlin emits none unless asked. A Java
caller could not catch ExecutionException at all — javac rejects it as
unreachable — which made the entire documented failure path unusable from the
language these helpers exist for. The tests missed it because every get() went
through Callable.call() throws Exception, which satisfies the compiler on its
own. The new test hand-rolls the try/catch so the catch clause itself is the
assertion.

resumeWith recorded into two separate fields with no completion guard, so a
second resume could replace the outcome of the call the caller was waiting on,
and two interleaved resumes could leave both fields unset — returning null for a
non-null R. Replaced with a single compare-and-set slot, which makes the
one-instance-per-call rule in the KDoc enforced rather than merely requested and
removes the unchecked cast as a side effect.

Co-authored-by: Cursor <cursoragent@cursor.com>
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