Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions common/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -29,9 +31,13 @@ class KeyedDebouncingAction<T: Any>(
val channel: Channel<T>,
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()
}
}

Expand All @@ -50,38 +56,50 @@ class KeyedDebouncingAction<T: Any>(
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<T> {
val channel = Channel<T>(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<Unit> {
actionJob.onJoin {}
channel.onReceive { newerKey ->
actionJob.cancel()
channel.trySend(newerKey)
select<Unit> {
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
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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<Throwable?>(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<String>(
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()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
Loading