From 83bde85ecdaaebfcd87bcd06e1486741b57ae773 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Fri, 7 Aug 2026 16:02:06 -0500 Subject: [PATCH 1/2] feat: [SDK-4993] add Future and minSdk-safe callback bridges for Java MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- OneSignalSDK/build.gradle | 4 + OneSignalSDK/onesignal/core/build.gradle | 6 + .../src/main/java/com/onesignal/Continue.kt | 70 ++++++ .../java/com/onesignal/FutureContinuation.kt | 134 +++++++++++ .../java/com/onesignal/ContinueJavaTest.java | 210 ++++++++++++++++++ .../com/onesignal/FutureContinuationTests.kt | 130 +++++++++++ .../java/com/onesignal/JavaInteropFixture.kt | 40 ++++ 7 files changed, 594 insertions(+) create mode 100644 OneSignalSDK/onesignal/core/src/main/java/com/onesignal/FutureContinuation.kt create mode 100644 OneSignalSDK/onesignal/core/src/test/java/com/onesignal/ContinueJavaTest.java create mode 100644 OneSignalSDK/onesignal/core/src/test/java/com/onesignal/FutureContinuationTests.kt create mode 100644 OneSignalSDK/onesignal/core/src/test/java/com/onesignal/JavaInteropFixture.kt diff --git a/OneSignalSDK/build.gradle b/OneSignalSDK/build.gradle index ff3fa2ec59..8640defc19 100644 --- a/OneSignalSDK/build.gradle +++ b/OneSignalSDK/build.gradle @@ -20,6 +20,10 @@ buildscript { kotlinxSerializationJsonVersion = '1.6.3' kotestVersion = '5.8.0' ioMockVersion = '1.13.2' + // JUnit 4 for the Java interop tests. The vintage engine runs them on the JUnit Platform + // that Kotest already uses, so its version tracks that platform (5.x) rather than JUnit 4. + junit4Version = '4.13.2' + junitVintageVersion = '5.8.2' // AndroidX Lifecycle and Activity versions lifecycleVersion = '2.6.2' activityVersion = '1.7.2' diff --git a/OneSignalSDK/onesignal/core/build.gradle b/OneSignalSDK/onesignal/core/build.gradle index d465a354c5..23b92307ce 100644 --- a/OneSignalSDK/onesignal/core/build.gradle +++ b/OneSignalSDK/onesignal/core/build.gradle @@ -114,6 +114,12 @@ dependencies { // com.tdunning:json is needed for non-Robolectric tests. testImplementation("com.tdunning:json:$tdunningJsonForTest") + + // The Java interop tests are the one place we assert what a Java caller sees, so they have to + // be written in Java. Robolectric only ships a JUnit 4 runner, and this module runs on the + // JUnit Platform for Kotest, so the vintage engine is what lets both live in one source set. + testImplementation("junit:junit:$junit4Version") + testRuntimeOnly("org.junit.vintage:junit-vintage-engine:$junitVintageVersion") } apply from: '../detekt.gradle' diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/Continue.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/Continue.kt index 9be203f2fc..c0ae6f90e4 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/Continue.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/Continue.kt @@ -7,6 +7,23 @@ import java.util.function.Consumer import kotlin.coroutines.Continuation import kotlin.coroutines.CoroutineContext +/** + * Receives the outcome of a coroutine on behalf of a Java caller. + * + * A Kotlin `fun interface` rather than a `java.util.function.Consumer` so that it is usable on the + * SDK's whole supported range. `Consumer` only exists from API 24, which is what confines + * [Continue.with] to `@RequiresApi(N)`; this one is a plain interface and works from API 21. Java + * still writes it as a lambda either way. + */ +fun interface ContinueCallback { + /** + * Called once the coroutine has completed, whether it succeeded or failed. + * + * @param result The outcome of the coroutine, to continue processing from. + */ + fun onFinished(result: ContinueResult) +} + /** * The result provided by [Continue.with] when the Java user wants to inspect the results * of a Kotlin coroutine completing. @@ -74,6 +91,59 @@ object Continue { } } + /** + * The same callback bridge as [with], usable on every API level the SDK supports. + * + * ```java + * OneSignal.getConsentGivenSuspend(Continue.callback(r -> { + * if (r.isSuccess()) render(r.getData()); + * else Log.e("app", "could not read consent", r.getThrowable()); + * })); + * ``` + * + * Prefer this over [with]: the two behave identically, but [with] takes a + * `java.util.function.Consumer`, which does not exist below API 24 and is not desugared here, so + * on API 21 through 23 it fails at runtime rather than at compile time. + * + * @param onFinished Called when the coroutine has completed, with its [ContinueResult]. + * @param context The optional coroutine context to run [onFinished] under. Defaults to the main + * thread, matching [with]. + */ + @JvmOverloads + @JvmStatic + fun callback( + onFinished: ContinueCallback, + context: CoroutineContext = Dispatchers.Main, + ): Continuation { + return object : Continuation { + override val context: CoroutineContext + get() = context + + override fun resumeWith(result: Result) { + onFinished.onFinished(ContinueResult(result.isSuccess, result.getOrNull(), result.exceptionOrNull())) + } + } + } + + /** + * Bridges a suspending call into a [Future], for Java callers that would rather collect the + * answer than be called back. + * + * ```java + * FutureContinuation consent = Continue.future(); + * OneSignal.getConsentGivenSuspend(consent); + * boolean given = consent.get(); // off the main thread + * ``` + * + * Each instance backs one call. [FutureContinuation.get] refuses to run on the main thread, so + * reach for [callback] when the answer is needed there. + * + * Unlike [with] and [callback] this takes no context, because there would be nothing for it to + * govern — see [FutureContinuation.context]. + */ + @JvmStatic + fun future(): FutureContinuation = FutureContinuation() + /** * Allows java code to indicate they have no follow-up to a Kotlin coroutine. */ diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/FutureContinuation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/FutureContinuation.kt new file mode 100644 index 0000000000..1a7495a669 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/FutureContinuation.kt @@ -0,0 +1,134 @@ +package com.onesignal + +import com.onesignal.common.AndroidUtils +import com.onesignal.common.exceptions.MainThreadException +import kotlinx.coroutines.Dispatchers +import java.util.concurrent.CancellationException +import java.util.concurrent.CountDownLatch +import java.util.concurrent.ExecutionException +import java.util.concurrent.Future +import java.util.concurrent.TimeUnit +import java.util.concurrent.TimeoutException +import kotlin.coroutines.Continuation +import kotlin.coroutines.CoroutineContext + +/** + * A [Continuation] that is also a [Future], letting Java call a suspending function and collect the + * answer later: + * + * ```java + * FutureContinuation consent = Continue.future(); + * OneSignal.getConsentGivenSuspend(consent); + * + * // ...on any thread that is not the main thread: + * boolean given = consent.get(); + * ``` + * + * Obtain one from [Continue.future]. Each instance backs a single call: it can be resumed once, so + * a second call needs a second instance. + * + * The value is whatever the suspending function returns, handed back unchanged. This bridge + * deliberately does not wrap it in a result type of its own — a suspending function that reports + * failure by returning something is left to say so in its own vocabulary, and one that reports + * failure by throwing surfaces here the way [Future] already specifies. + * + * What [get] guards is the blocking 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 SDK's suspending APIs change dispatchers almost + * immediately, so in practice the work leaves the main thread right away. + */ +class FutureContinuation internal constructor() : Continuation, Future { + /** + * Fixed at [Dispatchers.Unconfined], so the call resumes on whichever thread finished the work. + * Resuming records a value and releases a latch, so there is no user code here to place on a + * particular thread — dispatching elsewhere would only add latency, and dispatching to a busy + * main thread would add an unbounded amount of it. + */ + override val context: CoroutineContext + get() = Dispatchers.Unconfined + + private val completed = CountDownLatch(1) + + @Volatile + private var value: R? = null + + @Volatile + private var failure: Throwable? = null + + override fun resumeWith(result: Result) { + failure = result.exceptionOrNull() + if (failure == null) { + value = result.getOrNull() + } + completed.countDown() + } + + /** + * Blocks until the call completes and returns its value. + * + * @throws MainThreadException if called on the main thread. Blocking there is the stall this + * bridge exists to avoid, so it is refused rather than logged. Use [Continue.callback] when the + * answer is needed on the main thread. + * @throws ExecutionException if the call threw. The original throwable is the [Throwable.cause]. + * @throws CancellationException if the call was cancelled. + */ + override fun get(): R { + refuseMainThread() + completed.await() + return valueOrThrow() + } + + /** + * Blocks for at most [timeout] and returns the call's value. + * + * Throws as [get] does, plus [TimeoutException] if the call had not completed in time. A + * timeout leaves the underlying call running — this bridge has no handle on it, see [cancel]. + */ + override fun get( + timeout: Long, + unit: TimeUnit, + ): R { + refuseMainThread() + if (!completed.await(timeout, unit)) { + throw TimeoutException("OneSignal call did not complete within $timeout ${unit.name.lowercase()}.") + } + return valueOrThrow() + } + + override fun isDone(): Boolean = completed.count == 0L + + override fun isCancelled(): Boolean = isDone && failure is CancellationException + + /** + * Always returns `false`: this continuation is handed to a suspending function that the caller + * started directly, so there is no job here to cancel. Cancellation that happens upstream is + * still reported — it arrives as a [CancellationException] and surfaces through [get] and + * [isCancelled]. + */ + override fun cancel(mayInterruptIfRunning: Boolean): Boolean = false + + private fun valueOrThrow(): R { + when (val thrown = failure) { + null -> { + @Suppress("UNCHECKED_CAST") + return value as R + } + // Future specifies CancellationException directly and everything else wrapped, which + // also keeps a cancelled call from being mistaken for a failed one. + is CancellationException -> throw thrown + else -> throw ExecutionException(thrown) + } + } + + // Refused whether or not the value has already arrived. Allowing the already-complete case + // would make the same line throw or not depending on timing, which is a worse contract than a + // rule that always holds. + private fun refuseMainThread() { + if (AndroidUtils.isRunningOnMainThread()) { + throw MainThreadException( + "Blocking on a OneSignal call from the main thread is not allowed. " + + "Call get() from a background thread, or use Continue.callback() to be notified instead.", + ) + } + } +} diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/ContinueJavaTest.java b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/ContinueJavaTest.java new file mode 100644 index 0000000000..c734e77911 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/ContinueJavaTest.java @@ -0,0 +1,210 @@ +package com.onesignal; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import com.onesignal.common.exceptions.MainThreadException; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; + +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicReference; + +import kotlinx.coroutines.Dispatchers; + +/** + * Written in Java on purpose. The Kotlin tests cover what the helpers do; this file is the only + * place that can show what a Java caller actually has to type, which is the whole point of the + * helpers. If a signature stops being usable from Java, this file stops compiling. + * + *

Robolectric because the blocking guard reads the real Looper, and JUnit 4 because that is the + * only runner Robolectric ships — the vintage engine bridges it onto the JUnit Platform that Kotest + * uses for the rest of the module. + */ +@RunWith(RobolectricTestRunner.class) +public class ContinueJavaTest { + + /** Robolectric runs the test body on the main thread, so blocking has to happen off it. */ + private static T offMainThread(java.util.concurrent.Callable block) throws Exception { + AtomicReference outcome = new AtomicReference<>(); + AtomicReference threw = new AtomicReference<>(false); + Thread thread = new Thread(() -> { + try { + outcome.set(block.call()); + } catch (Throwable t) { + threw.set(true); + outcome.set(t); + } + }); + thread.start(); + thread.join(5_000); + // Without this the helper would return null on a hung thread, which reads as a pass for any + // test whose expected value happens to be null. + assertFalse("the background thread never finished", thread.isAlive()); + + if (threw.get()) { + throw new ExecutionOnOtherThread((Throwable) outcome.get()); + } + @SuppressWarnings("unchecked") + T value = (T) outcome.get(); + return value; + } + + private static final class ExecutionOnOtherThread extends RuntimeException { + ExecutionOnOtherThread(Throwable cause) { + super(cause); + } + } + + /** + * Collects a callback's single invocation. The fixture hops dispatchers, so the callback lands + * on another thread — waiting on a latch keeps that deterministic instead of sleeping and hoping. + */ + private static final class CallbackProbe implements ContinueCallback { + private final java.util.concurrent.CountDownLatch fired = + new java.util.concurrent.CountDownLatch(1); + private final AtomicReference> result = new AtomicReference<>(); + + @Override + public void onFinished(ContinueResult value) { + result.set(value); + fired.countDown(); + } + + ContinueResult await() throws InterruptedException { + assertTrue("callback never fired", fired.await(5, TimeUnit.SECONDS)); + return result.get(); + } + } + + /** + * Never executed — it exists so that javac checks the examples in the {@link Continue#future()} + * and {@link Continue#callback} KDoc against the real public API. A doc comment is the one place + * an unusable Java signature can sit indefinitely without anything noticing, so the examples are + * repeated here where a compiler sees them. + */ + @SuppressWarnings("unused") + private static void documentedExamplesStillCompile() throws Exception { + FutureContinuation consent = Continue.future(); + OneSignal.getConsentGivenSuspend(consent); + boolean given = consent.get(); + + OneSignal.getConsentGivenSuspend(Continue.callback(r -> { + if (r.isSuccess()) { + Boolean value = r.getData(); + } else { + Throwable failure = r.getThrowable(); + } + })); + } + + @Test + public void callbackReceivesTheValueAsALambda() throws Exception { + CallbackProbe probe = new CallbackProbe<>(); + + // Written as a method reference to prove the interface is a usable SAM from Java. + JavaInteropFixture.echo("os-1", Continue.callback(probe::onFinished, Dispatchers.getUnconfined())); + + ContinueResult result = probe.await(); + assertTrue(result.isSuccess()); + assertEquals("os-1", result.getData()); + assertNull(result.getThrowable()); + } + + @Test + public void callbackReportsAFailure() throws Exception { + CallbackProbe probe = new CallbackProbe<>(); + + JavaInteropFixture.boom(Continue.callback(probe::onFinished, Dispatchers.getUnconfined())); + + ContinueResult result = probe.await(); + assertFalse(result.isSuccess()); + assertNull(result.getData()); + assertEquals("boom", result.getThrowable().getMessage()); + } + + @Test + public void futureHandsBackTheValue() throws Exception { + FutureContinuation future = Continue.future(); + + JavaInteropFixture.echo("os-1", future); + + assertEquals("os-1", offMainThread(future::get)); + } + + @Test + public void futureCompletesForAUnitReturningCall() throws Exception { + FutureContinuation future = Continue.future(); + + JavaInteropFixture.nothing(future); + + assertSame(kotlin.Unit.INSTANCE, offMainThread(future::get)); + } + + @Test + public void futureReportsAFailureAsAnExecutionException() throws Exception { + FutureContinuation future = Continue.future(); + + JavaInteropFixture.boom(future); + + try { + offMainThread(future::get); + fail("expected the failure to surface"); + } catch (ExecutionOnOtherThread wrapper) { + ExecutionException thrown = (ExecutionException) wrapper.getCause(); + assertEquals("boom", thrown.getCause().getMessage()); + } + } + + @Test + public void blockingOnTheMainThreadIsRefused() { + FutureContinuation future = Continue.future(); + JavaInteropFixture.echo("os-1", future); + + try { + future.get(); + fail("expected blocking on the main thread to be refused"); + } catch (Exception e) { + assertTrue( + "expected MainThreadException but got " + e, + e instanceof MainThreadException); + } + } + + @Test + public void futureTimesOutRatherThanBlockingForever() throws Exception { + FutureContinuation future = Continue.future(); + + try { + offMainThread(() -> future.get(50, TimeUnit.MILLISECONDS)); + fail("expected a timeout"); + } catch (ExecutionOnOtherThread wrapper) { + assertTrue(wrapper.getCause() instanceof TimeoutException); + } + assertFalse(future.isDone()); + } + + /** + * Pins the one 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 hops dispatchers and + * so does suspend; this test exists so that stops being an accident. + */ + @Test + public void aCallThatNeverSuspendsReturnsDirectlyAndLeavesTheFutureWaiting() { + FutureContinuation future = Continue.future(); + + Object returnedDirectly = JavaInteropFixture.returnsWithoutSuspending(future); + + assertEquals("immediate", returnedDirectly); + assertFalse("the continuation was never resumed", future.isDone()); + } +} diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/FutureContinuationTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/FutureContinuationTests.kt new file mode 100644 index 0000000000..b06c3d89c1 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/FutureContinuationTests.kt @@ -0,0 +1,130 @@ +package com.onesignal + +import br.com.colman.kotest.android.extensions.robolectric.RobolectricTest +import com.onesignal.common.exceptions.MainThreadException +import io.kotest.assertions.throwables.shouldThrow +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.booleans.shouldBeFalse +import io.kotest.matchers.booleans.shouldBeTrue +import io.kotest.matchers.shouldBe +import kotlinx.coroutines.CancellationException +import java.util.concurrent.ExecutionException +import java.util.concurrent.TimeUnit +import java.util.concurrent.TimeoutException + +/** + * Robolectric because the main-thread guard reads the real [android.os.Looper]. + * + * Under Robolectric the spec body itself runs on the main thread, which is what makes the refusal + * test read as directly as it does — and why every test that expects a value has to go through + * [offMainThread]. + */ +@RobolectricTest +class FutureContinuationTests : FunSpec({ + + test("a resumed call hands back its value") { + val future = Continue.future() + + future.resumeWith(Result.success("os-1")) + + offMainThread { future.get() } shouldBe "os-1" + } + + test("isDone flips only once the call has completed") { + val future = Continue.future() + + future.isDone.shouldBeFalse() + + future.resumeWith(Result.success("os-1")) + + future.isDone.shouldBeTrue() + } + + // Blocking here is the stall the async migration exists to remove, so it is refused outright + // rather than logged and allowed through. + test("blocking on the main thread is refused") { + val future = Continue.future() + future.resumeWith(Result.success("os-1")) + + val thrown = shouldThrow { future.get() } + + thrown.message shouldBe + "Blocking on a OneSignal call from the main thread is not allowed. " + + "Call get() from a background thread, or use Continue.callback() to be notified instead." + } + + // Refused even when the value is already sitting there: a rule that depends on whether the call + // happened to finish first would throw or not throw depending on timing. + test("blocking on the main thread is refused even when the value has already arrived") { + val future = Continue.future() + future.resumeWith(Result.success("os-1")) + future.isDone.shouldBeTrue() + + shouldThrow { future.get() } + } + + test("a thrown failure surfaces wrapped, with the original attached as the cause") { + val boom = IllegalStateException("boom") + val future = Continue.future() + + future.resumeWith(Result.failure(boom)) + + val thrown = shouldThrow { offMainThread { future.get() } } + thrown.cause shouldBe boom + future.isCancelled.shouldBeFalse() + } + + // Future specifies cancellation as its own signal rather than an execution failure, which is + // also what keeps a cancelled call from being mistaken for a failed one. + test("cancellation surfaces as cancellation rather than as a failure") { + val future = Continue.future() + + future.resumeWith(Result.failure(CancellationException("parent scope went away"))) + + shouldThrow { offMainThread { future.get() } } + future.isCancelled.shouldBeTrue() + } + + test("a call that has not completed times out without disturbing the call") { + val future = Continue.future() + + shouldThrow { offMainThread { future.get(50, TimeUnit.MILLISECONDS) } } + + future.isDone.shouldBeFalse() + future.resumeWith(Result.success("late")) + offMainThread { future.get() } shouldBe "late" + } + + // The caller starts the suspending function directly, so this bridge never has a job to cancel. + // Reporting that honestly is better than a cancel() that quietly does nothing. + test("cancel reports that it did not cancel anything") { + val future = Continue.future() + + future.cancel(true).shouldBeFalse() + future.isCancelled.shouldBeFalse() + } + + test("a Unit-returning call completes rather than hanging") { + val future = Continue.future() + + future.resumeWith(Result.success(Unit)) + + offMainThread { future.get() } shouldBe Unit + } +}) + +/** + * Runs [block] on a background thread and returns its value, so a test can block on a future the + * way a real Java caller would. Failures are rethrown on the calling thread so `shouldThrow` still + * sees the exception the future produced. + */ +private fun offMainThread(block: () -> T): T { + var result: Result? = null + val thread = Thread { result = runCatching(block) } + thread.start() + thread.join(5_000) + // Reported explicitly rather than left to fail as a null dereference below, so a hung thread + // says so instead of surfacing as a confusing NPE. + check(!thread.isAlive) { "the background thread never finished" } + return result!!.getOrThrow() +} diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/JavaInteropFixture.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/JavaInteropFixture.kt new file mode 100644 index 0000000000..ee06a6f026 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/JavaInteropFixture.kt @@ -0,0 +1,40 @@ +package com.onesignal + +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext + +/** + * Suspending functions for the Java interop tests to call. + * + * These stand in for the SDK's public suspending API rather than mocking the bridge, so the Java + * tests exercise the real thing a Java caller faces: a `Continuation` parameter appended to the + * signature, and a return type of `Object`. + * + * [echo] and [boom] hop dispatchers, which is what every public suspending API in the SDK does + * before it returns — see [returnsWithoutSuspending] for why that distinction matters. + */ +object JavaInteropFixture { + /** Suspends, then completes with [value]. */ + @JvmStatic + suspend fun echo(value: String): String = withContext(Dispatchers.Default) { value } + + /** Suspends, then fails. */ + @JvmStatic + suspend fun boom(): String = withContext(Dispatchers.Default) { throw IllegalStateException("boom") } + + /** Suspends, then completes with no value, standing in for the `Unit`-returning APIs. */ + @JvmStatic + suspend fun nothing() = withContext(Dispatchers.Default) { } + + /** + * Completes without ever suspending. + * + * Kotlin compiles this to a plain return of the value, so the continuation passed to it is + * never resumed. Every public suspending API in the SDK hops dispatchers and therefore does + * suspend, but the distinction is load-bearing for anything built on continuation passing, so + * the tests pin it rather than leaving it to be discovered. + */ + @JvmStatic + @Suppress("RedundantSuspendModifier") + suspend fun returnsWithoutSuspending(): String = "immediate" +} From f6c11691b838572f65ed1eeffb5fc7b9cc119e86 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Fri, 7 Aug 2026 21:57:34 -0500 Subject: [PATCH 2/2] fix: [SDK-4993] declare checked exceptions and enforce single resume MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../java/com/onesignal/FutureContinuation.kt | 47 +++++++++++-------- .../java/com/onesignal/ContinueJavaTest.java | 33 +++++++++++++ .../com/onesignal/FutureContinuationTests.kt | 26 ++++++++++ 3 files changed, 87 insertions(+), 19 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/FutureContinuation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/FutureContinuation.kt index 1a7495a669..8cf830fd32 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/FutureContinuation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/FutureContinuation.kt @@ -9,6 +9,7 @@ import java.util.concurrent.ExecutionException import java.util.concurrent.Future import java.util.concurrent.TimeUnit import java.util.concurrent.TimeoutException +import java.util.concurrent.atomic.AtomicReference import kotlin.coroutines.Continuation import kotlin.coroutines.CoroutineContext @@ -49,16 +50,24 @@ class FutureContinuation internal constructor() : Continuation, Future private val completed = CountDownLatch(1) - @Volatile - private var value: R? = null - - @Volatile - private var failure: Throwable? = null + /** + * The one outcome, claimed by compare-and-set. + * + * A single slot rather than separate value and failure fields so that resuming once is enforced + * rather than merely asked for, and so that two resumes cannot interleave into a state that is + * neither of the results which arrived. + */ + private val outcome = AtomicReference?>(null) + /** + * @throws IllegalStateException if this instance has already been resumed. Accepting a second + * result quietly would leave the [Future] reporting an outcome belonging to some other call, + * which is worse than failing where the mistake was made. + */ override fun resumeWith(result: Result) { - failure = result.exceptionOrNull() - if (failure == null) { - value = result.getOrNull() + check(outcome.compareAndSet(null, result)) { + "This FutureContinuation has already been resumed. Each one backs a single call, so use " + + "a fresh Continue.future() for every suspending call." } completed.countDown() } @@ -71,7 +80,9 @@ class FutureContinuation internal constructor() : Continuation, Future * answer is needed on the main thread. * @throws ExecutionException if the call threw. The original throwable is the [Throwable.cause]. * @throws CancellationException if the call was cancelled. + * @throws InterruptedException if the waiting thread is interrupted. */ + @Throws(InterruptedException::class, ExecutionException::class) override fun get(): R { refuseMainThread() completed.await() @@ -84,6 +95,7 @@ class FutureContinuation internal constructor() : Continuation, Future * Throws as [get] does, plus [TimeoutException] if the call had not completed in time. A * timeout leaves the underlying call running — this bridge has no handle on it, see [cancel]. */ + @Throws(InterruptedException::class, ExecutionException::class, TimeoutException::class) override fun get( timeout: Long, unit: TimeUnit, @@ -97,7 +109,7 @@ class FutureContinuation internal constructor() : Continuation, Future override fun isDone(): Boolean = completed.count == 0L - override fun isCancelled(): Boolean = isDone && failure is CancellationException + override fun isCancelled(): Boolean = outcome.get()?.exceptionOrNull() is CancellationException /** * Always returns `false`: this continuation is handed to a suspending function that the caller @@ -108,16 +120,13 @@ class FutureContinuation internal constructor() : Continuation, Future override fun cancel(mayInterruptIfRunning: Boolean): Boolean = false private fun valueOrThrow(): R { - when (val thrown = failure) { - null -> { - @Suppress("UNCHECKED_CAST") - return value as R - } - // Future specifies CancellationException directly and everything else wrapped, which - // also keeps a cancelled call from being mistaken for a failed one. - is CancellationException -> throw thrown - else -> throw ExecutionException(thrown) - } + // Only reached once the latch has opened, which cannot happen before the slot is filled. + val result = outcome.get()!! + val thrown = result.exceptionOrNull() ?: return result.getOrThrow() + // Future specifies CancellationException directly and everything else wrapped, which also + // keeps a cancelled call from being mistaken for a failed one. + if (thrown is CancellationException) throw thrown + throw ExecutionException(thrown) } // Refused whether or not the value has already arrived. Allowing the already-complete case diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/ContinueJavaTest.java b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/ContinueJavaTest.java index c734e77911..89b486c742 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/ContinueJavaTest.java +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/ContinueJavaTest.java @@ -2,6 +2,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; @@ -164,6 +165,38 @@ public void futureReportsAFailureAsAnExecutionException() throws Exception { } } + /** + * The point of this test is the {@code catch} clause, not the assertion. Kotlin emits no throws + * clause unless asked, and without one javac rejects catching {@link ExecutionException} here as + * unreachable — so this compiling is the guarantee that a Java caller can handle a failed call. + * + *

Deliberately hand-rolled rather than going through {@link #offMainThread}: routing get() + * through {@code Callable.call() throws Exception} satisfies the compiler on its own and would + * hide a missing throws clause entirely, which is how this went unnoticed the first time. + */ + @Test + public void javaCanCatchExecutionExceptionFromGet() throws Exception { + FutureContinuation future = Continue.future(); + JavaInteropFixture.boom(future); + + AtomicReference caught = new AtomicReference<>(); + Thread thread = new Thread(() -> { + try { + future.get(); + } catch (ExecutionException e) { + caught.set(e); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }); + thread.start(); + thread.join(5_000); + assertFalse("the background thread never finished", thread.isAlive()); + + assertNotNull("get() did not report the failure", caught.get()); + assertEquals("boom", caught.get().getCause().getMessage()); + } + @Test public void blockingOnTheMainThreadIsRefused() { FutureContinuation future = Continue.future(); diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/FutureContinuationTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/FutureContinuationTests.kt index b06c3d89c1..efe875624e 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/FutureContinuationTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/FutureContinuationTests.kt @@ -30,6 +30,32 @@ class FutureContinuationTests : FunSpec({ offMainThread { future.get() } shouldBe "os-1" } + // The KDoc asks for one instance per call. Enforced rather than trusted, because a second result + // would otherwise silently replace the outcome of the call the caller is actually waiting on — + // and the class KDoc originally demonstrated exactly that mistake. + test("a second resume is refused instead of replacing the first outcome") { + val future = Continue.future() + future.resumeWith(Result.success("first")) + + shouldThrow { + future.resumeWith(Result.success("second")) + } + + offMainThread { future.get() } shouldBe "first" + } + + test("a failure cannot be overwritten by a later success") { + val future = Continue.future() + future.resumeWith(Result.failure(IllegalArgumentException("boom"))) + + shouldThrow { + future.resumeWith(Result.success("late")) + } + + val thrown = shouldThrow { offMainThread { future.get() } } + thrown.cause!!.message shouldBe "boom" + } + test("isDone flips only once the call has completed") { val future = Continue.future()