diff --git a/common/build.gradle.kts b/common/build.gradle.kts index ed723738b4..86f6715fee 100755 --- a/common/build.gradle.kts +++ b/common/build.gradle.kts @@ -43,6 +43,8 @@ dependencies { implementation(libs.monitor) testImplementation(projects.testing.common) + testImplementation(libs.tests.kotlinx.coroutines) + testImplementation(libs.tests.google.truth) androidTestImplementation(projects.testing.android) // brotli4j diff --git a/common/src/main/java/com/itsaky/androidide/utils/KeyedDebouncingAction.kt b/common/src/main/java/com/itsaky/androidide/utils/KeyedDebouncingAction.kt index 637c531239..987808fe7b 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/KeyedDebouncingAction.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/KeyedDebouncingAction.kt @@ -2,11 +2,13 @@ package com.itsaky.androidide.utils import com.itsaky.androidide.progress.ICancelChecker import com.itsaky.androidide.tasks.JobCancelChecker +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.Job import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.channels.ClosedReceiveChannelException import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.ensureActive import kotlinx.coroutines.isActive @@ -29,9 +31,13 @@ class KeyedDebouncingAction( val channel: Channel, val job: Job, ) { + /** Cancels this entry's worker job and closes its channel, in that order. */ fun cancel() { - channel.close() + // Cancel the job FIRST, then close the channel. Closing the channel first + // wakes a parked receive() with a ClosedReceiveChannelException before the + // job is cancelled, which can crash a worker that has no exception handling. job.cancel() + channel.close() } } @@ -50,38 +56,50 @@ class KeyedDebouncingAction( entry.channel.trySend(key) } + /** + * Creates a new [ActionEntry]: a CONFLATED channel plus a worker coroutine that debounces + * incoming keys and runs [action] for the latest one, stopping cleanly when the channel is closed. + */ @OptIn(ExperimentalCoroutinesApi::class) private fun createEntry(): ActionEntry { val channel = Channel(Channel.CONFLATED) val job = scope.launch(actionContext) { while (isActive) { - var latestKey = channel.receive() - var debouncing = true - while (debouncing) { - debouncing = select { - onTimeout(debounceDuration) { false } - channel.onReceive { newKey -> - latestKey = newKey - true + try { + var latestKey = channel.receive() + var debouncing = true + while (debouncing) { + debouncing = select { + onTimeout(debounceDuration) { false } + channel.onReceive { newKey -> + latestKey = newKey + true + } } } - } - ensureActive() - val actionJob = launch { - val cancelChecker = JobCancelChecker(currentCoroutineContext()[Job]) - action(latestKey, cancelChecker) - } + ensureActive() + val actionJob = launch { + val cancelChecker = JobCancelChecker(currentCoroutineContext()[Job]) + action(latestKey, cancelChecker) + } - select { - actionJob.onJoin {} - channel.onReceive { newerKey -> - actionJob.cancel() - channel.trySend(newerKey) + select { + actionJob.onJoin {} + channel.onReceive { newerKey -> + actionJob.cancel() + channel.trySend(newerKey) + } } - } - actionJob.join() + actionJob.join() + } catch (e: ClosedReceiveChannelException) { + // The channel was closed (entry cancelled). Stop the worker cleanly + // instead of letting the exception propagate to an uncaught handler. + break + } catch (e: CancellationException) { + throw e + } } } diff --git a/common/src/test/java/com/itsaky/androidide/utils/KeyedDebouncingActionCancelTest.kt b/common/src/test/java/com/itsaky/androidide/utils/KeyedDebouncingActionCancelTest.kt new file mode 100644 index 0000000000..2576d46490 --- /dev/null +++ b/common/src/test/java/com/itsaky/androidide/utils/KeyedDebouncingActionCancelTest.kt @@ -0,0 +1,66 @@ +package com.itsaky.androidide.utils + +import com.google.common.truth.Truth.assertThat +import kotlinx.coroutines.CoroutineExceptionHandler +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.channels.ClosedReceiveChannelException +import kotlinx.coroutines.delay +import kotlinx.coroutines.runBlocking +import org.junit.Test +import java.util.concurrent.atomic.AtomicReference +import kotlin.coroutines.CoroutineContext +import kotlin.time.Duration.Companion.milliseconds + +/** + * Repro for ADFA-4328: cancelling a [KeyedDebouncingAction] entry whose worker is + * parked on `channel.receive()` must NOT let a [ClosedReceiveChannelException] + * escape to the scope's uncaught-exception handler. + * + * On the pre-fix baseline, `ActionEntry.cancel()` did `channel.close()` BEFORE + * `job.cancel()`. Closing the channel wakes the parked `receive()` with a + * [ClosedReceiveChannelException] (NOT a CancellationException), which propagates + * uncaught to the [CoroutineExceptionHandler] -> the Sentry crash this ticket fixes. + * + * The fix swaps the order (job.cancel() first) AND wraps the worker loop in a + * try/catch that swallows ClosedReceiveChannelException, so no uncaught exception fires. + */ +class KeyedDebouncingActionCancelTest { + + /** Cancelling an entry whose worker is parked on receive() must not surface an uncaught exception. */ + @Test + fun `cancelling a parked worker does not leak a ClosedReceiveChannelException`() = runBlocking { + val uncaught = AtomicReference(null) + // A plain Job (not Supervisor of the worker) + a handler that records anything + // that escapes the debounce worker coroutine. + val handler = CoroutineExceptionHandler { _, t -> uncaught.set(t) } + val scope = CoroutineScope(SupervisorJob() + handler) + + val ctx: CoroutineContext = scope.coroutineContext + + val debouncer = KeyedDebouncingAction( + scope = scope, + debounceDuration = 50.milliseconds, + actionContext = ctx, + action = { _, _ -> /* never invoked: we cancel while parked on receive */ }, + ) + + // schedule() creates the entry + launches the worker. With a CONFLATED channel and + // no further sends, the worker debounces the single key, runs the (empty) action, + // then loops back and parks on channel.receive() waiting for the next key. + debouncer.schedule("k") + + // Give the worker time to: receive "k", run the empty action, loop, and PARK on + // the next channel.receive(). 200ms >> 50ms debounce window. + delay(200) + + // Cancel the entry while the worker is parked on receive(). + debouncer.cancelPending("k") + + // Let any uncaught exception propagate to the handler. + delay(200) + + val leaked = uncaught.get() + assertThat(leaked).isNull() + } +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt index 92c01e5d73..88eb7b3a94 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt @@ -17,6 +17,8 @@ import com.itsaky.androidide.projects.FileManager import com.itsaky.androidide.projects.api.Workspace import com.itsaky.androidide.utils.KeyedDebouncingAction import io.sentry.Sentry +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CoroutineExceptionHandler import kotlinx.coroutines.CoroutineName import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers @@ -66,7 +68,14 @@ internal class CompilationEnvironment( languageVersion: LanguageVersion = DEFAULT_LANGUAGE_VERSION, enableParserEventSystem: Boolean = true, val coroutineScope: CoroutineScope = CoroutineScope( - SupervisorJob() + CoroutineName("CompilationEnv[$name]") + SupervisorJob() + CoroutineName("CompilationEnv[$name]") + + CoroutineExceptionHandler { _, t -> + // Defense in depth: swallow (but log) non-cancellation failures from the + // debounce worker so a ClosedReceiveChannelException can never crash the app. + if (t !is CancellationException) { + logger.warn("Uncaught exception in compilation environment coroutine", t) + } + } ), ) : AbstractCompilationEnvironment( name = name,