diff --git a/.claude/.gitignore b/.claude/.gitignore new file mode 100644 index 0000000000..6d88c79ccb --- /dev/null +++ b/.claude/.gitignore @@ -0,0 +1,12 @@ +# Local, machine-specific settings (not shared) +settings.local.json + +# Claude Code worktrees +worktrees/ + +# Session/runtime artifacts +projects/ +todos/ +shell-snapshots/ +statsig/ +.credentials.json diff --git a/build.gradle.kts b/build.gradle.kts index 142e2c4ca4..8bb596384a 100755 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -97,11 +97,17 @@ subprojects { // setAccessible on java.lang.Class fields. // - java.base/java.io, java.util: needed by Robolectric/Gradle worker // reflection in the same test JVM. + // - java.base/java.util.concurrent: the embedded IntelliJ scheduler + // (BoundedTaskExecutor.info -> AppDelayQueue "Periodic tasks thread") + // reflectively reads FutureTask.callable. Without this the periodic + // thread dies, disabling CoreProgressManager's cancellation poll that + // makes the Kotlin Analysis API interruptible mid-`analyze`. jvmArgs( "--add-opens=java.base/java.lang=ALL-UNNAMED", "--add-opens=java.base/java.lang.reflect=ALL-UNNAMED", "--add-opens=java.base/java.io=ALL-UNNAMED", "--add-opens=java.base/java.util=ALL-UNNAMED", + "--add-opens=java.base/java.util.concurrent=ALL-UNNAMED", "--add-opens=jdk.unsupported/sun.misc=ALL-UNNAMED", ) diff --git a/editor/src/main/java/com/itsaky/androidide/editor/language/IDELanguage.kt b/editor/src/main/java/com/itsaky/androidide/editor/language/IDELanguage.kt index ddca102e71..f074ec747a 100644 --- a/editor/src/main/java/com/itsaky/androidide/editor/language/IDELanguage.kt +++ b/editor/src/main/java/com/itsaky/androidide/editor/language/IDELanguage.kt @@ -25,6 +25,7 @@ import com.itsaky.androidide.lsp.debug.model.BreakpointDefinition import com.itsaky.androidide.lsp.debug.model.BreakpointRequest import com.itsaky.androidide.preferences.internal.EditorPreferences import com.itsaky.androidide.progress.ICancelChecker +import com.itsaky.androidide.progress.ProgressManager import io.github.rosemoe.sora.lang.Language import io.github.rosemoe.sora.lang.completion.CompletionCancelledException import io.github.rosemoe.sora.lang.completion.CompletionPublisher @@ -67,11 +68,18 @@ abstract class IDELanguage : Language { publisher: CompletionPublisher, extraArguments: Bundle ) { + val completionThread = Thread.currentThread() try { val cancelChecker = CompletionCancelChecker(publisher) Lookup.getDefault().update(ICancelChecker::class.java, cancelChecker) + // Bind the checker to this completion thread. EditorCompletionWindow.cancelCompletion() + // cancels the (old) thread via ProgressManager.cancel(thread); routing that to *this* + // checker's cancel() lets the LSP's invokeOnCancel listener abort the running analysis + // mid-`analyze` immediately, instead of only at coarse checkpoints. + ProgressManager.instance.register(completionThread, cancelChecker) doComplete(content, position, publisher, cancelChecker, extraArguments) } finally { + ProgressManager.instance.unregister(completionThread) Lookup.getDefault().unregister( ICancelChecker::class.java ) diff --git a/editor/src/main/java/com/itsaky/androidide/editor/ui/EditorCompletionWindow.kt b/editor/src/main/java/com/itsaky/androidide/editor/ui/EditorCompletionWindow.kt index 8561b79de3..ad5e4f5709 100644 --- a/editor/src/main/java/com/itsaky/androidide/editor/ui/EditorCompletionWindow.kt +++ b/editor/src/main/java/com/itsaky/androidide/editor/ui/EditorCompletionWindow.kt @@ -41,9 +41,22 @@ class EditorCompletionWindow(val editor: IDEEditor) : EditorAutoCompletion(edito private var listView: ListView? = null private val items: MutableList = mutableListOf() + /** + * A scheduled-but-not-yet-started completion request, kept so a newer keystroke can cancel it. + * See [requireCompletion]. + */ + private var pendingCompletion: Runnable? = null + companion object { private val log = LoggerFactory.getLogger(EditorCompletionWindow::class.java) + + /** + * Quiet period used to coalesce a burst of keystrokes into a single completion request. Rapid + * typing (re)schedules the start; only after the user pauses this long does one analysis run, + * for the latest cursor position. Keeps at most one completion in flight. + */ + private const val COMPLETION_DEBOUNCE_MS = 80L } init { @@ -119,29 +132,57 @@ class EditorCompletionWindow(val editor: IDEEditor) : EditorAutoCompletion(edito } override fun cancelCompletion() { + // Drop any request that was scheduled but hasn't started yet. + pendingCompletion?.let { editor.handler.removeCallbacks(it) } + pendingCompletion = null if (completionThread != null) { ProgressManager.instance.cancel(completionThread) } super.cancelCompletion() } - override fun requireCompletion() { + /** + * Whether a completion may be shown for the current editor state. Hides the window (matching the + * prior inline behaviour) when the cursor is selected or completion is otherwise not applicable. + */ + private fun canStartCompletion(): Boolean { if (cancelShowUp || !isEnabled || !editor.isAttachedToWindow) { - return + return false } - - val text = editor.text - if (text.cursor.isSelected || checkNoCompletion()) { + if (editor.text.cursor.isSelected || checkNoCompletion()) { hide() - return + return false } + return true + } - if (System.nanoTime() - requestTime < editor.props.cancelCompletionNs) { - requestTime = System.nanoTime() + override fun requireCompletion() { + if (!canStartCompletion()) { return } + // Coalesce a burst of keystrokes into a single completion. Cancel the in-flight completion and + // any pending (not-yet-started) one, then (re)schedule one start after a short quiet period. + // This guarantees at most one completion analysis in flight and that only the latest cursor + // position is computed — preventing the CompletionThread/allocation pile-up that saturated the + // heap and froze the editor during fast typing. cancelCompletion() clears any pending request, + // so we always schedule exactly one. cancelCompletion() + + val request = Runnable { startCompletion() } + pendingCompletion = request + editor.handler.postDelayed(request, COMPLETION_DEBOUNCE_MS) + } + + /** Starts a single completion for the current cursor position. Runs on the UI thread. */ + private fun startCompletion() { + pendingCompletion = null + + // The editor state may have changed during the debounce delay; re-check the guards. + if (!canStartCompletion()) { + return + } + requestTime = System.nanoTime() currentSelection = -1 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 c9af5b7263..20512c20b0 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 @@ -3,6 +3,7 @@ package com.itsaky.androidide.lsp.kotlin.compiler import com.itsaky.androidide.lsp.api.ILanguageClient import com.itsaky.androidide.lsp.kotlin.compiler.index.KtSymbolIndex import com.itsaky.androidide.lsp.kotlin.compiler.modules.AbstractKtModule +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPreemptedException import com.itsaky.androidide.lsp.kotlin.compiler.modules.KtModule import com.itsaky.androidide.lsp.kotlin.compiler.modules.asFlatSequence import com.itsaky.androidide.lsp.kotlin.compiler.modules.backingFilePath @@ -179,9 +180,16 @@ internal class CompilationEnvironment( scope = coroutineScope, debounceDuration = DEFAULT_FILE_MOD_EVENT_DEBOUNCE_DURATION, ) { path, cancelChecker -> - val result = collectDiagnosticsFor(path, cancelChecker) - withContext(Dispatchers.Main.immediate) { - languageClient?.publishDiagnostics(result) + try { + val result = collectDiagnosticsFor(path, cancelChecker) + withContext(Dispatchers.Main.immediate) { + languageClient?.publishDiagnostics(result) + } + } catch (e: AnalysisPreemptedException) { + // A higher-priority analysis (completion) preempted this diagnostics run. + // Re-schedule so diagnostics still run once the higher-priority work finishes. + logger.debug("diagnostics for {} preempted; rescheduling", path) + fileAnalyzer.schedule(path) } } } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorker.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorker.kt index cf43f0e72c..b588ad4829 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorker.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorker.kt @@ -1,6 +1,7 @@ package com.itsaky.androidide.lsp.kotlin.compiler.index import com.itsaky.androidide.lsp.kotlin.compiler.CompilationEnvironment +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPreemptedException import com.itsaky.androidide.lsp.kotlin.compiler.modules.backingFilePath import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.progress.ICancelChecker @@ -8,6 +9,7 @@ import com.itsaky.androidide.utils.KeyedDebouncingAction import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.isActive +import kotlinx.coroutines.launch import org.appdevforall.codeonthego.indexing.jvm.JvmSymbolIndex import org.appdevforall.codeonthego.indexing.jvm.KtFileMetadata import org.appdevforall.codeonthego.indexing.jvm.KtFileMetadataIndex @@ -54,8 +56,14 @@ internal class IndexWorker( debounceDuration = CompilationEnvironment.DEFAULT_FILE_MOD_EVENT_DEBOUNCE_DURATION ) { (path, ktFile), cancelChecker -> logger.debug("Indexing modified file: {}", path) - indexSourceFile(project, ktFile, fileIndex, sourceIndex, cancelChecker) - sourceIndexCount++ + try { + indexSourceFile(project, ktFile, fileIndex, sourceIndex, cancelChecker) + sourceIndexCount++ + } catch (e: AnalysisPreemptedException) { + // Preempted by higher-priority analysis; re-queue so the edit still gets indexed. + logger.debug("Indexing of modified file {} preempted; re-queueing", path) + scope.launch { submitCommand(IndexCommand.IndexModifiedFile(ktFile)) } + } } while (isActive) { @@ -89,15 +97,23 @@ internal class IndexWorker( continue } - indexSourceFile( - project = project, - ktFile = ktFile, - fileIndex = fileIndex, - symbolsIndex = sourceIndex, - cancelChecker = ICancelChecker.NOOP - ) + try { + indexSourceFile( + project = project, + ktFile = ktFile, + fileIndex = fileIndex, + symbolsIndex = sourceIndex, + // A real (cancellable) checker so the scheduler can preempt this pass + // in favour of completion/diagnostics. + cancelChecker = ICancelChecker.Default() + ) - sourceIndexCount++ + sourceIndexCount++ + } catch (e: AnalysisPreemptedException) { + // Preempted by higher-priority analysis; re-queue so the file still gets indexed. + logger.debug("Indexing of {} preempted; re-queueing", cmd.vf.path) + scope.launch { submitCommand(cmd) } + } } is IndexCommand.IndexModifiedFile -> { diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/SourceFileIndexer.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/SourceFileIndexer.kt index b03c205a92..5765998a9c 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/SourceFileIndexer.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/SourceFileIndexer.kt @@ -1,5 +1,7 @@ package com.itsaky.androidide.lsp.kotlin.compiler.index +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.modules.backingFilePath import com.itsaky.androidide.lsp.kotlin.compiler.read @@ -74,6 +76,12 @@ internal suspend fun indexSourceFile( symbolsIndex: JvmSymbolIndex, cancelChecker: ICancelChecker, ) { + // Indexing runs at the lowest (INDEXING) priority: it yields to both completion and diagnostics. + // Wrapping the checker lets the scheduler preempt an in-progress index pass; the preemption + // surfaces as AnalysisPreemptedException at the abortIfCancelled() checkpoints below, which the + // IndexWorker catches to re-queue the file. + val checker = cancelChecker as? ScheduledCancelChecker ?: ScheduledCancelChecker(cancelChecker) + // Defensive backstop: this runs on the debounced/async index scope, so a disposal path that // didn't first drain & join the workers could otherwise touch PSI on a disposed project and // throw "Project is already disposed" (APPDEVFORALL-17R). Cheap fast-path before the reads below. @@ -86,7 +94,7 @@ internal suspend fun indexSourceFile( ktFile.toMetadata(project, isIndexed = true) } ?: return val existingFile = fileIndex.get(newFile.filePath) - cancelChecker.abortIfCancelled() + checker.abortIfCancelled() if (KtFileMetadata.shouldBeSkipped(existingFile, newFile) && existingFile?.isIndexed == true) { return @@ -95,7 +103,7 @@ internal suspend fun indexSourceFile( // Remove stale symbols written during the previous indexing pass. if (existingFile?.isIndexed == true) { symbolsIndex.removeBySource(newFile.filePath) - cancelChecker.abortIfCancelled() + checker.abortIfCancelled() } val symbols = project.read { @@ -103,13 +111,13 @@ internal suspend fun indexSourceFile( if (project.isDisposed) return@read emptyList() val list = mutableListOf() - analyzeMaybeDangling(ktFile) { + analyzeMaybeDangling(ktFile, AnalysisPriority.INDEXING, checker) { val session = this ktFile.accept(object : KtTreeVisitorVoid() { override fun visitDeclaration(dcl: KtDeclaration) { - cancelChecker.abortIfCancelled() + checker.abortIfCancelled() val symbol = with(session) { analyzeDeclaration(newFile.filePath, dcl) } - cancelChecker.abortIfCancelled() + checker.abortIfCancelled() symbol?.let { list.add(it) } super.visitDeclaration(dcl) } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt new file mode 100644 index 0000000000..73c3c26a63 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisScheduler.kt @@ -0,0 +1,208 @@ +package com.itsaky.androidide.lsp.kotlin.compiler.modules + +import com.itsaky.androidide.progress.ICancelChecker +import java.util.concurrent.CancellationException +import java.util.concurrent.CopyOnWriteArrayList +import java.util.concurrent.TimeUnit +import java.util.concurrent.locks.ReentrantLock +import kotlin.concurrent.withLock + +/** + * Priority of an Analysis API request. Higher [ordinal] wins: a request can preempt any strictly + * lower-priority analysis that is currently running, and is served before any lower-priority request + * that is merely waiting. + * + * Order: [INDEXING] < [DIAGNOSTICS] < [COMPLETION] — interactive completion beats background + * diagnostics, which beats bulk indexing. + * + * [supersedesSamePriority] additionally lets a *newer* request preempt an in-flight request of the + * **same** priority. This is enabled only for [COMPLETION]: when the user types fast, several + * completion requests fire in a row and the in-flight one is computing results for a now-stale cursor + * position, so the newer request cancels it and the fresh position is analysed immediately. A + * superseded completion is simply *discarded* — nothing reschedules it (see + * `KotlinCompletions.codeComplete`). It is intentionally off for [DIAGNOSTICS] and [INDEXING], whose + * preempted work is re-queued rather than dropped; same-priority preemption there would livelock, as + * two contenders would endlessly re-queue and re-preempt each other. + */ +internal enum class AnalysisPriority(val supersedesSamePriority: Boolean) { + INDEXING(supersedesSamePriority = false), + DIAGNOSTICS(supersedesSamePriority = false), + COMPLETION(supersedesSamePriority = true), +} + +/** + * Thrown at an `abortIfCancelled()` checkpoint when the running analysis has been preempted by a + * higher-priority request (see [AnalysisScheduler]). It is a [CancellationException] so it unwinds + * cleanly through the existing cancellation-aware `catch` blocks; callers that want the preempted work + * to run later catch this specific type and re-schedule it. + */ +internal class AnalysisPreemptedException : + CancellationException("analysis preempted by a higher-priority request") + +/** + * An [ICancelChecker] that adds a cooperative *preemption* signal on top of an existing [delegate] + * checker. [AnalysisScheduler] flags preemption here; the running analysis observes it both at its + * LSP-level [abortIfCancelled] checkpoints and, because [withAnalysisLock] installs a + * [CancelCheckerProgressIndicator] bridging [isCancelled] to the compiler's `ProgressManager`, + * mid-`analyze` at the compiler's own internal cancellation checks. + * + * Preemption is distinct from ordinary cancellation: [abortIfCancelled] throws + * [AnalysisPreemptedException] (so the source can re-schedule the work) while still honouring the + * delegate's own cancellation (e.g. a superseding edit or a closed file). + */ +internal class ScheduledCancelChecker( + private val delegate: ICancelChecker, +) : ICancelChecker { + + @Volatile + private var preempted = false + + private val onCancelListeners = CopyOnWriteArrayList<() -> Unit>() + + /** Marks this analysis as preempted; the next [abortIfCancelled] will throw. */ + fun preempt() { + preempted = true + // Push: preemption is a cancellation too, so notify [invokeOnCancel] listeners immediately. + onCancelListeners.forEach { it() } + onCancelListeners.clear() + } + + override fun cancel() { + delegate.cancel() + } + + override fun isCancelled(): Boolean = preempted || delegate.isCancelled() + + override fun abortIfCancelled() { + if (preempted) { + throw AnalysisPreemptedException() + } + delegate.abortIfCancelled() + } + + override fun invokeOnCancel(listener: () -> Unit) { + // Fire on either scheduler preemption (stored locally, run by preempt()) or the delegate's own + // cancellation (forwarded so the editor's CompletionCancelChecker.cancel() pushes it). The + // listener body is idempotent, so firing via both paths is harmless. + onCancelListeners.add(listener) + delegate.invokeOnCancel(listener) + // Guard the race where preempt() ran between add and now. + if (preempted && onCancelListeners.remove(listener)) { + listener() + } + } +} + +/** + * A process-global, priority-aware, preemptive lock that serializes all Kotlin Analysis API access. + * + * It replaces the plain FIFO lock that previously guarded `analyze` / `analyzeCopy`. Semantics: + * - only one analysis runs at a time (the Analysis API is not safe to drive concurrently); + * - a higher-priority requester **preempts** a strictly lower-priority holder by invoking its + * `onPreempt` callback once (cooperative — the holder bails at its next `abortIfCancelled()`); + * - a newer requester of the **same** priority likewise preempts the holder when that priority is + * [AnalysisPriority.supersedesSamePriority] (completion only — its superseded work is discarded, not + * rescheduled); + * - when the lock frees, the highest-priority waiter acquires it next; + * - it is **reentrant**: a nested analysis on the same thread re-enters without deadlocking. + * + * Access it through [withAnalysisLock] / [analyzeMaybeDangling] rather than directly. + */ +internal object AnalysisScheduler { + + /** Upper bound on how long a queued requester waits before re-checking its cancellation. */ + private const val WAIT_POLL_MILLIS = 25L + + private val mutex = ReentrantLock() + private val available = mutex.newCondition() + + private var holderThread: Thread? = null + private var holderPriority: AnalysisPriority? = null + private var holderReentry = 0 + private var holderPreempted = false + private var holderPreempt: (() -> Unit)? = null + + /** Number of threads currently waiting to acquire, per priority. */ + private val waiting = IntArray(AnalysisPriority.entries.size) + + /** + * Acquire the analysis lock at the given [priority]. Blocks until the current thread may run. If a + * preemptable analysis is in progress — strictly lower priority, or the same priority when that + * priority is [AnalysisPriority.supersedesSamePriority] — [onPreempt] of *that* holder is invoked so + * it yields; [onPreempt] passed here is stored and used if this acquisition is later preempted. + * + * [cancelChecker] is *this* requester's checker: while waiting for the lock, the wait is re-checked on + * a short timer, and if the requester has been cancelled — e.g. the editor superseded + * this completion — [acquire] throws instead of parking until the lock frees. This is what stops + * superseded completions from piling up holding heavy state (KtFile copies, symbol lists) while they + * wait, which on-device saturated the heap and triggered multi-second GC stalls. + */ + fun acquire(priority: AnalysisPriority, cancelChecker: ICancelChecker, onPreempt: () -> Unit) { + mutex.withLock { + val me = Thread.currentThread() + if (holderThread === me) { + // Reentrant: nested analysis on the same thread shares the outer hold. + holderReentry++ + return + } + + waiting[priority.ordinal]++ + try { + while (true) { + // Bail out (rather than keep waiting) if this requester was cancelled while queued. + cancelChecker.abortIfCancelled() + + val hp = holderPriority + if (holderThread != null && hp != null && !holderPreempted && + (hp.ordinal < priority.ordinal || + (hp == priority && priority.supersedesSamePriority)) + ) { + // Signal the holder to bail (once): either it is strictly lower priority, or a + // newer same-priority request supersedes it (completion only). + holderPreempted = true + holderPreempt?.invoke() + } + + if (holderThread == null && !higherPriorityWaiting(priority)) { + break + } + // Timed wait so a cancellation that arrives without a lock-state change (no signal) is + // still observed within WAIT_POLL_MILLIS, bounding how long a superseded waiter parks. + available.await(WAIT_POLL_MILLIS, TimeUnit.MILLISECONDS) + } + } finally { + waiting[priority.ordinal]-- + } + + holderThread = me + holderPriority = priority + holderPreempt = onPreempt + holderPreempted = false + holderReentry = 1 + } + } + + /** Release a hold acquired via [acquire]. Wakes waiters when the outermost hold is released. */ + fun release() { + mutex.withLock { + if (holderThread !== Thread.currentThread()) { + return + } + if (--holderReentry > 0) { + return + } + holderThread = null + holderPriority = null + holderPreempt = null + holderPreempted = false + available.signalAll() + } + } + + private fun higherPriorityWaiting(priority: AnalysisPriority): Boolean { + for (i in priority.ordinal + 1 until waiting.size) { + if (waiting[i] > 0) return true + } + return false + } +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisThreadContext.java b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisThreadContext.java new file mode 100644 index 0000000000..35aa3dde9c --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisThreadContext.java @@ -0,0 +1,37 @@ +package com.itsaky.androidide.lsp.kotlin.compiler.modules; + +import kotlin.coroutines.CoroutineContext; +import kotlinx.coroutines.Job; +import org.jetbrains.kotlin.com.intellij.concurrency.ThreadContext; +import org.jetbrains.kotlin.com.intellij.openapi.application.AccessToken; + +/** + * Bridge to the embeddable IntelliJ {@link ThreadContext} coroutine-context API. + * + *

{@code currentThreadContext}/{@code installThreadContext} live in a Kotlin file facade whose + * metadata the Kotlin compiler cannot resolve against from this module (they surface as + * "unresolved reference" from Kotlin). At the bytecode level, though, they are plain + * {@code public static} methods, so Java can call them directly. + * + *

This is used by {@code withAnalysisLock} to install a cancellable {@link Job} into the analysis + * thread's context: the embeddable {@code CoreProgressManager.checkCanceled()} routes through + * {@code Cancellation.checkCancelled()}, which throws as soon as that Job is cancelled, aborting the + * running analysis mid-{@code analyze}. + */ +public final class AnalysisThreadContext { + + private AnalysisThreadContext() { + } + + /** + * Installs {@code job} into the current thread's IntelliJ coroutine context (preserving any + * context already present) and returns a token that restores the previous context when closed. + * + *

Public because it is referenced from the {@code internal inline} {@code withAnalysisLock}; + * an inline function may only reference declarations at least as accessible as itself. + */ + public static AccessToken installJob(Job job) { + CoroutineContext context = ThreadContext.currentThreadContext().plus(job); + return ThreadContext.installThreadContext(context, true); + } +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/CancelCheckerProgressIndicator.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/CancelCheckerProgressIndicator.kt new file mode 100644 index 0000000000..7c47d33034 --- /dev/null +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/CancelCheckerProgressIndicator.kt @@ -0,0 +1,37 @@ +package com.itsaky.androidide.lsp.kotlin.compiler.modules + +import com.itsaky.androidide.progress.ICancelChecker +import org.jetbrains.kotlin.com.intellij.openapi.progress.ProcessCanceledException +import org.jetbrains.kotlin.com.intellij.openapi.progress.util.AbstractProgressIndicatorBase + +/** + * A [com.intellij.openapi.progress.ProgressIndicator] whose cancellation state is driven by an + * [ICancelChecker]. Installing it as the analysis thread's indicator (via + * [withAnalysisLock]) is what makes the Kotlin Analysis API actually interruptible *mid*-`analyze`. + * + * The embeddable analysis API ships the full IntelliJ `CoreProgressManager`, whose + * `ProgressManager.checkCanceled()` (called densely throughout FIR resolution) re-fetches the + * current thread's indicator and rethrows its [checkCanceled]. By bridging that to [checker], a + * preemption or ordinary cancellation aborts the running analysis at the compiler's next internal + * checkpoint instead of only at the coarse LSP-level [ICancelChecker.abortIfCancelled] checks. + * + * This extends [AbstractProgressIndicatorBase] rather than `EmptyProgressIndicator` on purpose: + * the base is a *non-standard* indicator, so `CoreProgressManager` runs a background task that + * polls [checkCanceled] every ~10ms. That poll flips the manager's internal "should check + * cancelled" flag to active once [checker] reports cancellation, which is what arms the in-`analyze` + * checks. [cancel] (invoked synchronously when this analysis is preempted) flips the same flag + * immediately, so preemption does not have to wait for the poll. + */ +internal class CancelCheckerProgressIndicator( + private val checker: ICancelChecker, +) : AbstractProgressIndicatorBase() { + + override fun isCanceled(): Boolean = super.isCanceled() || checker.isCancelled() + + override fun checkCanceled() { + if (checker.isCancelled()) { + throw ProcessCanceledException() + } + super.checkCanceled() + } +} diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt index db20d93e91..58813a34f0 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt @@ -6,19 +6,23 @@ import org.jetbrains.kotlin.analysis.api.analyzeCopy import org.jetbrains.kotlin.analysis.api.projectStructure.KaDanglingFileResolutionMode import org.jetbrains.kotlin.analysis.api.projectStructure.copyOrigin import org.jetbrains.kotlin.analysis.api.projectStructure.isDangling +import kotlinx.coroutines.Job +import org.jetbrains.kotlin.com.intellij.openapi.progress.ProcessCanceledException +import org.jetbrains.kotlin.com.intellij.openapi.progress.ProgressManager import org.jetbrains.kotlin.com.intellij.openapi.util.Key import org.jetbrains.kotlin.psi.KtElement import org.jetbrains.kotlin.psi.KtFile import org.jetbrains.kotlin.psi.UserDataProperty +import org.slf4j.LoggerFactory import java.nio.file.Path -import java.util.concurrent.locks.ReentrantLock -import kotlin.concurrent.withLock private val KT_LSP_COMPLETION_BACKING_FILE = Key("KT_LSP_COMPLETION_BACKING_FILE") var KtFile.backingFilePath by UserDataProperty(KT_LSP_COMPLETION_BACKING_FILE) +private val logger = LoggerFactory.getLogger("KtFileExts") + /** - * Serializes all Kotlin Analysis API access (`analyze` / `analyzeCopy`). + * Runs [action] while holding the global analysis lock at the given [priority]. * * The Analysis API tracks its `analyze` lifetime context in a per-thread stack and is not safe to * drive concurrently from multiple background threads without the platform read-action coordination @@ -28,8 +32,23 @@ var KtFile.backingFilePath by UserDataProperty(KT_LSP_COMPLETION_BACKING_FILE) * lifecycle and surfaced as * `KaInaccessibleLifetimeOwnerAccessException: ... Called outside an \`analyze\` context.` * - * Holding this lock around every analysis entry point makes analyses mutually exclusive. It is a - * [ReentrantLock] so an (indirect) nested analysis on the same thread cannot deadlock. + * Serialization is handled by [AnalysisScheduler], which is priority-aware and preemptive: a + * higher-priority request preempts a strictly lower-priority one (cooperatively, via [cancelChecker]). + * The scheduler is reentrant, so an (indirect) nested analysis on the same thread cannot deadlock. + * + * **All** Analysis API access must go through this helper (or [analyzeMaybeDangling], which already + * does); never call `analyze` / `analyzeCopy` directly, or the serialization guarantee is lost. + * + * **Cancellation.** The [action] runs with a [kotlinx.coroutines.Job] installed in the thread's + * IntelliJ coroutine context. The compiler's own `ProgressManager.checkCanceled()` calls (dense + * throughout FIR resolution) route through `Cancellation.checkCancelled()`, which throws as soon as + * that Job is cancelled — aborting the analysis *mid*-`analyze`, not just at the coarse LSP + * [ScheduledCancelChecker.abortIfCancelled] checkpoints between work units. (A + * [CancelCheckerProgressIndicator] is also installed as a fallback, but its poll-driven arming does + * not run in this embeddable environment; see the body.) The compiler signals cancellation by + * throwing [ProcessCanceledException]; we translate it back into the typed exception the callers + * already handle ([AnalysisPreemptedException] when preempted, else the delegate's + * `CancellationException`) so preempted work is rescheduled rather than silently dropped. * * **Footgun:** analysis runs under the *read* (shared) side of the global * [com.itsaky.androidide.lsp.kotlin.compiler.read] lock, and that `ReentrantReadWriteLock` is @@ -37,17 +56,64 @@ var KtFile.backingFilePath by UserDataProperty(KT_LSP_COMPLETION_BACKING_FILE) * call [com.itsaky.androidide.lsp.kotlin.compiler.write] — upgrading read → write on the same thread * deadlocks. */ -private val analysisLock = ReentrantLock() +internal inline fun withAnalysisLock( + priority: AnalysisPriority, + cancelChecker: ScheduledCancelChecker, + crossinline action: () -> R, +): R { + val indicator = CancelCheckerProgressIndicator(cancelChecker) -/** - * Runs [action] while holding the shared [analysisLock]. **All** Analysis API access must go through - * this helper (or [analyzeMaybeDangling], which already does); never call `analyze` / `analyzeCopy` - * directly, or the serialization guarantee is lost. - */ -internal inline fun withAnalysisLock(action: () -> R): R = analysisLock.withLock(action) + // Cancelling [job] is what actually aborts the running analysis *mid*-`analyze`: the embeddable + // `CoreProgressManager.checkCanceled()` (called densely throughout FIR resolution) invokes + // `Cancellation.checkCancelled()` *unconditionally* — before any indicator/`CheckCanceledBehavior` + // gating — and that throws a `CeProcessCanceledException` (a `ProcessCanceledException`) the moment + // the [Job] installed in this thread's context is cancelled. + // + // The [indicator] is kept only as a fallback for environments whose `CoreProgressManager` runs its + // ~10ms non-standard-indicator poll; that poll does not run in this (Android, embeddable) one. + val job = Job() + + AnalysisScheduler.acquire(priority, cancelChecker) { + // Preemption is signalled by flipping the checker; the invokeOnCancel listener below turns that + // (and ordinary editor cancellation) into the actual mid-`analyze` abort. + cancelChecker.preempt() + } + // Single push path for *both* preemption and ordinary editor cancellation: [ScheduledCancelChecker] + // fires this on preempt() and on its delegate's cancel(), so the running analyze aborts immediately + // — no polling, and unaffected by GC pauses that would stall a poll thread. + cancelChecker.invokeOnCancel { + indicator.cancel() + job.cancel() + } + try { + val holder = arrayOfNulls(1) + try { + // Install the Job for the duration of the analysis and restore the previous context after. + AnalysisThreadContext.installJob(job).use { + ProgressManager.getInstance() + .executeProcessUnderProgress({ holder[0] = action() }, indicator) + } + } catch (e: ProcessCanceledException) { + logger.debug("process cancelled: prio={}", priority) + // Re-derive the semantically-correct exception callers expect (re-throws + // AnalysisPreemptedException when preempted, or the delegate's CancellationException). + cancelChecker.abortIfCancelled() + throw e + } + @Suppress("UNCHECKED_CAST") + return holder[0] as R + } finally { + AnalysisScheduler.release() + } +} -internal inline fun analyzeMaybeDangling(useSiteElement: KtElement, crossinline action: KaSession.() -> R): R = - withAnalysisLock { +internal inline fun analyzeMaybeDangling( + useSiteElement: KtElement, + priority: AnalysisPriority, + cancelChecker: ScheduledCancelChecker, + crossinline action: KaSession.() -> R, +): R = + withAnalysisLock(priority, cancelChecker) { if (useSiteElement is KtFile && useSiteElement.isDangling && useSiteElement.copyOrigin != null) { analyzeCopy(useSiteElement, KaDanglingFileResolutionMode.PREFER_SELF, action) } else { diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt index d7e012f2ff..7b760991ce 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt @@ -3,6 +3,9 @@ package com.itsaky.androidide.lsp.kotlin.completion import com.itsaky.androidide.lookup.Lookup import com.itsaky.androidide.lsp.api.describeSnippet import com.itsaky.androidide.lsp.kotlin.compiler.CompilationEnvironment +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPreemptedException +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.utils.AnalysisContext @@ -21,7 +24,9 @@ import com.itsaky.androidide.preferences.utils.indentationString import com.itsaky.androidide.progress.ICancelChecker import com.itsaky.androidide.progress.ProgressManager import com.itsaky.androidide.projects.FileManager +import io.github.rosemoe.sora.lang.completion.CompletionCancelledException import kotlinx.coroutines.CancellationException +import org.jetbrains.kotlin.com.intellij.openapi.progress.ProcessCanceledException import org.appdevforall.codeonthego.indexing.jvm.JvmClassInfo import org.appdevforall.codeonthego.indexing.jvm.JvmFunctionInfo import org.appdevforall.codeonthego.indexing.jvm.JvmSymbol @@ -75,13 +80,49 @@ private const val KT_COMPLETION_PLACEHOLDER = "KT_COMPLETION_PLACEHOLDER" private val logger = LoggerFactory.getLogger("KotlinCompletions") +/** Max unimported symbols pulled from each index for scope completion (see [collectUnimportedSymbols]). */ +private const val UNIMPORTED_SYMBOL_LIMIT = 100 + +/** + * The [ScheduledCancelChecker] for the completion request running on this thread, set for the + * duration of [doComplete]. The LSP-level [abortIfCancelled] checkpoints consult it so they observe + * scheduler *preemption* (a newer completion superseding this one, or this one yielding to a + * higher-priority request) — not just the editor's own request cancellation. Without this, those + * checkpoints only saw the raw delegate from [Lookup] and every preemption was misreported as an + * ordinary cancellation. + */ +private val currentCancelChecker = ThreadLocal() + private fun abortIfCancelled() { ProgressManager.abortIfCancelled() - Lookup.getDefault() - .lookup(ICancelChecker::class.java) - ?.abortIfCancelled() + val checker = currentCancelChecker.get() + if (checker != null) { + checker.abortIfCancelled() + } else { + Lookup.getDefault() + .lookup(ICancelChecker::class.java) + ?.abortIfCancelled() + } } +/** + * A cancelled completion surfaces as one of several exception types depending on *where* it was + * observed, and none of them is an error: + * - [CancellationException]/[AnalysisPreemptedException] — a coarse [abortIfCancelled] checkpoint; + * - [ProcessCanceledException] — the compiler's mid-`analyze` cancellation (our `job` was cancelled); + * - [CompletionCancelledException] — the editor's sora publisher was cancelled (surfaced via the + * [com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker] delegate); + * - [InterruptedException] — the sora `CompletionThread` was interrupted mid-resolution. + * + * All of them mean "this completion was superseded/cancelled"; treat them uniformly so mid-`analyze` + * cancellations are reported cleanly instead of logged as spurious errors with a stack trace. + */ +private fun Throwable.isCancellation(): Boolean = + this is CancellationException || + this is InterruptedException || + this is ProcessCanceledException || + this is CompletionCancelledException + /** * Provide code completion for the given completion parameters. * @@ -94,8 +135,9 @@ internal fun codeComplete(params: CompletionParams): CompletionResult { return try { doComplete(params) } catch (error: Throwable) { - if (error is CancellationException || error is InterruptedException) { - logger.info("completion cancelled") + if (error.isCancellation()) { + val isPreempted = error is AnalysisPreemptedException + logger.info("completion cancelled (preempted={})", isPreempted) if (error is InterruptedException) { Thread.interrupted() } @@ -144,11 +186,26 @@ internal fun doComplete(params: CompletionParams): CompletionResult { abortIfCancelled() + // Completion is the highest-priority analysis: it preempts in-progress diagnostics/indexing and is + // never preempted by lower-priority analysis. It can, however, be superseded by a *newer* completion + // request (the user typing on) — that in-flight completion is then cancelled and simply discarded. + // + // Use the request-scoped checker carried on [params] rather than the global Lookup: Lookup holds a + // single ICancelChecker updated per request, so with concurrent completion threads an older request + // could read a newer request's checker and never observe its own cancellation. [params.cancelChecker] + // is the editor's own CompletionCancelChecker for *this* request. Fall back to Lookup only if a + // caller supplied the NOOP checker (e.g. tests that don't set one). + val delegate = params.cancelChecker.takeUnless { it === ICancelChecker.NOOP } + ?: Lookup.getDefault().lookup(ICancelChecker::class.java) + ?: ICancelChecker.NOOP + val cancelChecker = ScheduledCancelChecker(delegate) + currentCancelChecker.set(cancelChecker) + return try { env.project.read { abortIfCancelled() - analyzeMaybeDangling(completionKtFile) { + analyzeMaybeDangling(completionKtFile, AnalysisPriority.COMPLETION, cancelChecker) { val ctx = resolveAnalysisContext( env = env, @@ -184,12 +241,17 @@ internal fun doComplete(params: CompletionParams): CompletionResult { } } } catch (e: Throwable) { - if (e is CancellationException) { + // Let cancellation (incl. mid-`analyze` ProcessCanceledException / sora + // CompletionCancelledException / InterruptedException) propagate to codeComplete's uniform + // handler rather than logging it as an error. + if (e.isCancellation()) { throw e } logger.warn("An error occurred while computing completions for {}", params.file, e) return CompletionResult.EMPTY + } finally { + currentCancelChecker.remove() } } @@ -352,13 +414,16 @@ private fun KaSession.collectUnimportedSymbols( buildUnimportedSymbolItem(symbol)?.let { to += it } } - env.libraryIndex?.findByPrefix(ctx.partial, limit = 0) + // Bounded: limit = 0 means "unlimited", which pulled every matching symbol from all three + // indexes on every keystroke — a major allocation/GC source. A capped result set is more than + // enough for a completion popup and keeps the per-request footprint small. + env.libraryIndex?.findByPrefix(ctx.partial, limit = UNIMPORTED_SYMBOL_LIMIT) ?.forEach(::addCompletionItem) - env.sourceIndex?.findByPrefix(ctx.partial, limit = 0) + env.sourceIndex?.findByPrefix(ctx.partial, limit = UNIMPORTED_SYMBOL_LIMIT) ?.forEach(::addCompletionItem) - env.generatedIndex?.findByPrefix(ctx.partial, limit = 0) + env.generatedIndex?.findByPrefix(ctx.partial, limit = UNIMPORTED_SYMBOL_LIMIT) ?.forEach(::addCompletionItem) } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt index f9304a9230..c60bece3fe 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt @@ -1,6 +1,8 @@ package com.itsaky.androidide.lsp.kotlin.diagnostic import com.itsaky.androidide.lsp.kotlin.compiler.CompilationEnvironment +import com.itsaky.androidide.lsp.kotlin.compiler.modules.AnalysisPriority +import com.itsaky.androidide.lsp.kotlin.compiler.modules.ScheduledCancelChecker import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.utils.toRange @@ -64,11 +66,17 @@ private fun doAnalyze(file: Path, cancelChecker: ICancelChecker): DiagnosticResu return DiagnosticResult.NO_UPDATE } + // Diagnostics run at DIAGNOSTICS priority: they yield to completion but preempt indexing. The + // wrapped checker turns a scheduler preemption into an AnalysisPreemptedException, which + // CompilationEnvironment's fileAnalyzer catches to re-schedule this run after the higher-priority + // work finishes. + val checker = ScheduledCancelChecker(cancelChecker) + val diagnostics = env.project.read { buildList { PsiTreeUtil.collectElementsOfType(ktFile, PsiErrorElement::class.java) .forEach { errorElement -> - cancelChecker.abortIfCancelled() + checker.abortIfCancelled() add( diagnosticItem( file = ktFile, @@ -79,14 +87,13 @@ private fun doAnalyze(file: Path, cancelChecker: ICancelChecker): DiagnosticResu ) } - // This should be canceled as well - // The analysis API uses a no-op implementation of - // Intellij's ProgressManager for cancellations, so the following - // isn't really cancellable at the moment - analyzeMaybeDangling(ktFile) { + // analyzeMaybeDangling installs a CancelCheckerProgressIndicator, so this analysis is + // cancellable mid-`analyze`: it aborts at the compiler's internal checkCanceled() once + // `checker` reports preemption/cancellation (in addition to the abortIfCancelled() below). + analyzeMaybeDangling(ktFile, AnalysisPriority.DIAGNOSTICS, checker) { ktFile.collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS) .forEach { diagnostic -> - cancelChecker.abortIfCancelled() + checker.abortIfCancelled() // Extract plain data while still inside the analyze context; never let // the KaLifetimeOwner diagnostic escape (see KotlinDiagnosticExtra). val unresolvedReference = diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt index e2ffcd966c..29156f98d2 100644 --- a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt @@ -3,13 +3,20 @@ package com.itsaky.androidide.lsp.kotlin.compiler.modules import com.google.common.truth.Truth.assertThat import com.itsaky.androidide.lsp.kotlin.compiler.read import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import com.itsaky.androidide.progress.ICancelChecker +import org.jetbrains.kotlin.com.intellij.openapi.progress.ProgressManager import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import org.junit.Test import java.util.Collections +import java.util.concurrent.CancellationException +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.atomic.AtomicInteger +import java.util.concurrent.atomic.AtomicReference import kotlin.math.max /** @@ -23,11 +30,12 @@ import kotlin.math.max * read-action coordination (which this LSP replaces with a shared read lock that does not serialize * analysis). Overlapping `analyze` calls corrupted the lifetime/session lifecycle. * - * Fix: [analyzeMaybeDangling] / [withAnalysisLock] hold a process-wide reentrant lock so analyses - * are mutually exclusive. + * Fix: [analyzeMaybeDangling] / [withAnalysisLock] route through [AnalysisScheduler], a process-wide + * priority-aware, preemptive, reentrant lock so analyses are mutually exclusive. * - * Both tests fail before the fix (either by throwing the exception or by observing overlapping - * analyses) and pass after it. + * The first two tests cover serialization (they fail before the fix, either by throwing the exception + * or by observing overlapping analyses). The remaining tests cover the scheduler's priority, + * preemption, and reentrancy behaviour. */ class AnalysisSerializationTest : KtLspTest() { @@ -56,7 +64,11 @@ class AnalysisSerializationTest : KtLspTest() { val file = files[iter % files.size] try { env.project.read { - analyzeMaybeDangling(file) { + analyzeMaybeDangling( + file, + AnalysisPriority.DIAGNOSTICS, + ScheduledCancelChecker(ICancelChecker.NOOP), + ) { // Touching declaration symbols is what triggered the lifetime check. file.declarations.forEach { dcl -> dcl.symbol @@ -89,7 +101,11 @@ class AnalysisSerializationTest : KtLspTest() { val file = files[iter % files.size] try { env.project.read { - analyzeMaybeDangling(file) { + analyzeMaybeDangling( + file, + AnalysisPriority.DIAGNOSTICS, + ScheduledCancelChecker(ICancelChecker.NOOP), + ) { val concurrent = inFlight.incrementAndGet() maxObserved.updateAndGet { max(it, concurrent) } try { @@ -112,4 +128,314 @@ class AnalysisSerializationTest : KtLspTest() { // The shared analysis lock must prevent two analyses from running at once. assertThat(maxObserved.get()).isEqualTo(1) } + + @Test(timeout = 10_000) + fun `reentrant withAnalysisLock on the same thread does not deadlock`() { + var innerRan = false + withAnalysisLock(AnalysisPriority.DIAGNOSTICS, ScheduledCancelChecker(ICancelChecker.NOOP)) { + withAnalysisLock(AnalysisPriority.COMPLETION, ScheduledCancelChecker(ICancelChecker.NOOP)) { + innerRan = true + } + } + assertThat(innerRan).isTrue() + } + + @Test(timeout = 10_000) + fun `higher priority request preempts a lower priority holder`() { + val holderChecker = ScheduledCancelChecker(ICancelChecker.NOOP) + val holding = CountDownLatch(1) + val preempted = AtomicBoolean(false) + val higherRan = AtomicBoolean(false) + + // Low-priority (indexing) holder runs a long, cooperatively-cancellable analysis. + val lower = Thread { + try { + withAnalysisLock(AnalysisPriority.INDEXING, holderChecker) { + holding.countDown() + repeat(2_000) { + holderChecker.abortIfCancelled() + Thread.sleep(5) + } + } + } catch (e: AnalysisPreemptedException) { + preempted.set(true) + } + } + lower.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + // A completion request must preempt the in-progress indexing. + val higher = Thread { + withAnalysisLock(AnalysisPriority.COMPLETION, ScheduledCancelChecker(ICancelChecker.NOOP)) { + higherRan.set(true) + } + } + higher.start() + higher.join(5_000) + lower.join(5_000) + + assertThat(preempted.get()).isTrue() + assertThat(higherRan.get()).isTrue() + } + + @Test(timeout = 10_000) + fun `analysis is interrupted mid-analyze at the compiler's ProgressManager checkpoint`() { + // The Kotlin Analysis API calls ProgressManager.checkCanceled() densely during FIR + // resolution, but never the LSP-level ICancelChecker.abortIfCancelled(). This body mimics + // that: it only polls ProgressManager.checkCanceled(). Before withAnalysisLock installed a + // CancelCheckerProgressIndicator, that call was inert (no indicator => the manager's + // check-cancelled behaviour stayed disabled) and the work ran to completion regardless of + // preemption. It must now be interruptible. + val holderChecker = ScheduledCancelChecker(ICancelChecker.NOOP) + val holding = CountDownLatch(1) + val preempted = AtomicBoolean(false) + val ranToCompletion = AtomicBoolean(false) + + val lower = Thread { + try { + withAnalysisLock(AnalysisPriority.INDEXING, holderChecker) { + holding.countDown() + repeat(2_000) { + // Compiler-level checkpoint only — no abortIfCancelled() here. + ProgressManager.checkCanceled() + Thread.sleep(5) + } + ranToCompletion.set(true) + } + } catch (e: AnalysisPreemptedException) { + preempted.set(true) + } + } + lower.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + // A completion request preempts the in-progress (indexing) analysis. + val higher = Thread { + withAnalysisLock(AnalysisPriority.COMPLETION, ScheduledCancelChecker(ICancelChecker.NOOP)) {} + } + higher.start() + higher.join(5_000) + lower.join(5_000) + + assertThat(preempted.get()).isTrue() + assertThat(ranToCompletion.get()).isFalse() + } + + @Test(timeout = 10_000) + fun `ordinary cancellation aborts an in-flight analysis mid-analyze`() { + // Regression for ADFA-4174. Unlike preemption (a competing higher/same-priority request), an + // *ordinary* cancellation — the editor cancelling because the user typed on / moved the cursor + // / dismissed the popup — flips the request's ICancelChecker. Via ScheduledCancelChecker's + // invokeOnCancel push, cancelling the delegate must abort the compiler's mid-`analyze` FIR + // resolution promptly, rather than letting it run to completion (observed on-device as ~900ms + // stalls and piled-up completion threads). + val delegate = ICancelChecker.Default() + val file = createSourceFile("OrdinaryCancel.kt", "class C { fun f(): Int = 1 }") + val holding = CountDownLatch(1) + val ranToCompletion = AtomicBoolean(false) + val caught = AtomicReference(null) + + // Run inside a real analyze under the read lock: this also guards against a read/write-lock + // upgrade deadlock regression (withAnalysisLock runs under the shared read lock). + val worker = Thread { + try { + env.project.read { + analyzeMaybeDangling( + file, + AnalysisPriority.COMPLETION, + ScheduledCancelChecker(delegate), + ) { + holding.countDown() + repeat(2_000) { + // Compiler-level checkpoint only — mirrors FIR resolution, which never calls + // the LSP-level abortIfCancelled(). + ProgressManager.checkCanceled() + Thread.sleep(5) + } + ranToCompletion.set(true) + } + } + } catch (t: Throwable) { + caught.set(t) + } + } + worker.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + val startNs = System.nanoTime() + // Simulates EditorCompletionWindow.cancelCompletion() -> ProgressManager.cancel(thread) -> + // CompletionCancelChecker.cancel(). + delegate.cancel() + worker.join(2_000) + val elapsedMs = (System.nanoTime() - startNs) / 1_000_000 + + assertThat(worker.isAlive).isFalse() + assertThat(ranToCompletion.get()).isFalse() + // Ordinary cancellation, not preemption: the delegate's CancellationException, not + // AnalysisPreemptedException. + assertThat(caught.get()).isInstanceOf(CancellationException::class.java) + assertThat(caught.get()).isNotInstanceOf(AnalysisPreemptedException::class.java) + assertThat(elapsedMs).isLessThan(500) + } + + @Test(timeout = 10_000) + fun `a waiting requester bails when cancelled instead of waiting for the lock`() { + // Regression for ADFA-4174: a superseded completion that is queued behind another analysis must + // abort as soon as it is cancelled, rather than parking (holding heavy state) until the lock + // frees. On-device, parked-until-release completions piled up and saturated the heap. + val holding = CountDownLatch(1) + val release = CountDownLatch(1) + val waiterDelegate = ICancelChecker.Default() + val waiterThrew = AtomicReference(null) + val waiterEntered = AtomicBoolean(false) + + // A completion holder keeps the lock until released. + val holder = Thread { + withAnalysisLock(AnalysisPriority.COMPLETION, ScheduledCancelChecker(ICancelChecker.NOOP)) { + holding.countDown() + release.await() + } + } + holder.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + // A lower-priority (diagnostics) requester must wait behind the completion holder (it does not + // preempt). It is cancelled while waiting and must bail from acquire() promptly. + val waiter = Thread { + try { + withAnalysisLock(AnalysisPriority.DIAGNOSTICS, ScheduledCancelChecker(waiterDelegate)) { + waiterEntered.set(true) + } + } catch (t: Throwable) { + waiterThrew.set(t) + } + } + waiter.start() + + // Let it enter the wait loop, then cancel it. + Thread.sleep(200) + waiterDelegate.cancel() + + // The waiter must finish (bail) while the holder still holds the lock. + waiter.join(2_000) + val bailedWhileHeld = !waiter.isAlive + + release.countDown() + holder.join(5_000) + + assertThat(bailedWhileHeld).isTrue() + assertThat(waiterEntered.get()).isFalse() + assertThat(waiterThrew.get()).isInstanceOf(CancellationException::class.java) + } + + @Test(timeout = 10_000) + fun `lower priority request waits while a higher priority holder runs`() { + val holding = CountDownLatch(1) + val release = CountDownLatch(1) + val lowerEntered = AtomicBoolean(false) + + // High-priority (completion) holder holds the lock until released. + val higher = Thread { + withAnalysisLock(AnalysisPriority.COMPLETION, ScheduledCancelChecker(ICancelChecker.NOOP)) { + holding.countDown() + release.await() + } + } + higher.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + // A diagnostics request is strictly lower priority: it must not preempt completion. + val lower = Thread { + withAnalysisLock(AnalysisPriority.DIAGNOSTICS, ScheduledCancelChecker(ICancelChecker.NOOP)) { + lowerEntered.set(true) + } + } + lower.start() + + // Give the lower-priority request time to (incorrectly) barge in. + Thread.sleep(300) + val enteredWhileHeld = lowerEntered.get() + + release.countDown() + higher.join(5_000) + lower.join(5_000) + + assertThat(enteredWhileHeld).isFalse() + assertThat(lowerEntered.get()).isTrue() + } + + @Test(timeout = 10_000) + fun `same priority completion supersedes an in-flight completion`() { + val holderChecker = ScheduledCancelChecker(ICancelChecker.NOOP) + val holding = CountDownLatch(1) + val preempted = AtomicBoolean(false) + val newerRan = AtomicBoolean(false) + + // An in-flight completion runs a long, cooperatively-cancellable analysis. + val older = Thread { + try { + withAnalysisLock(AnalysisPriority.COMPLETION, holderChecker) { + holding.countDown() + repeat(2_000) { + holderChecker.abortIfCancelled() + Thread.sleep(5) + } + } + } catch (e: AnalysisPreemptedException) { + preempted.set(true) + } + } + older.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + // A newer completion request (user typed on) must supersede the in-flight one. + val newer = Thread { + withAnalysisLock(AnalysisPriority.COMPLETION, ScheduledCancelChecker(ICancelChecker.NOOP)) { + newerRan.set(true) + } + } + newer.start() + newer.join(5_000) + older.join(5_000) + + assertThat(preempted.get()).isTrue() + assertThat(newerRan.get()).isTrue() + } + + @Test(timeout = 10_000) + fun `same priority diagnostics does not preempt an in-flight diagnostics`() { + val holding = CountDownLatch(1) + val release = CountDownLatch(1) + val secondEntered = AtomicBoolean(false) + + // A diagnostics holder holds the lock until released. + val first = Thread { + withAnalysisLock(AnalysisPriority.DIAGNOSTICS, ScheduledCancelChecker(ICancelChecker.NOOP)) { + holding.countDown() + release.await() + } + } + first.start() + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue() + + // A second diagnostics request is the same priority but must NOT supersede the holder + // (only completion supersedes same-priority work); it waits until the holder releases. + val second = Thread { + withAnalysisLock(AnalysisPriority.DIAGNOSTICS, ScheduledCancelChecker(ICancelChecker.NOOP)) { + secondEntered.set(true) + } + } + second.start() + + // Give the second request time to (incorrectly) barge in. + Thread.sleep(300) + val enteredWhileHeld = secondEntered.get() + + release.countDown() + first.join(5_000) + second.join(5_000) + + assertThat(enteredWhileHeld).isFalse() + assertThat(secondEntered.get()).isTrue() + } } diff --git a/shared/src/main/java/com/itsaky/androidide/progress/ICancelChecker.kt b/shared/src/main/java/com/itsaky/androidide/progress/ICancelChecker.kt index 059824966f..ce0bcb1974 100644 --- a/shared/src/main/java/com/itsaky/androidide/progress/ICancelChecker.kt +++ b/shared/src/main/java/com/itsaky/androidide/progress/ICancelChecker.kt @@ -18,6 +18,7 @@ package com.itsaky.androidide.progress import java.util.concurrent.CancellationException +import java.util.concurrent.CopyOnWriteArrayList import java.util.concurrent.atomic.AtomicBoolean /** @@ -45,12 +46,31 @@ interface ICancelChecker { @Throws(CancellationException::class) fun abortIfCancelled() + /** + * Register [listener] to run when this process is cancelled — a *push* notification, so a consumer + * can react to cancellation immediately instead of polling [isCancelled]. If already cancelled, + * [listener] runs synchronously now. [listener] runs at most once. + * + * The default implementation only fires when already cancelled; an implementation that can transition + * to cancelled after registration (e.g. [Default]) overrides this to fire on the transition. + */ + fun invokeOnCancel(listener: () -> Unit) { + if (isCancelled()) { + listener() + } + } + open class Default(cancelled: Boolean = false) : ICancelChecker { private val cancelled = AtomicBoolean(cancelled) + private val onCancelListeners = CopyOnWriteArrayList<() -> Unit>() override fun cancel() { - cancelled.set(true) + // Fire listeners once, on the false -> true transition only. + if (cancelled.compareAndSet(false, true)) { + onCancelListeners.forEach { it() } + onCancelListeners.clear() + } } override fun isCancelled(): Boolean { @@ -62,6 +82,19 @@ interface ICancelChecker { throw CancellationException() } } + + override fun invokeOnCancel(listener: () -> Unit) { + if (isCancelled()) { + listener() + return + } + onCancelListeners.add(listener) + // Guard the race where cancel() ran between the check above and the add: if we now observe + // cancellation, run the listener ourselves (removing it so cancel() can't also run it). + if (isCancelled() && onCancelListeners.remove(listener)) { + listener() + } + } } companion object { @@ -70,7 +103,10 @@ interface ICancelChecker { * A no-op cancel checker. The task is never cancelled. */ @JvmField - val NOOP = Default(false) + val NOOP = object : Default(false) { + // Never transitions to cancelled, so retaining listeners would only leak them. + override fun invokeOnCancel(listener: () -> Unit) = Unit + } /** * An already cancelled cancel checker. diff --git a/shared/src/main/java/com/itsaky/androidide/progress/ProgressManager.kt b/shared/src/main/java/com/itsaky/androidide/progress/ProgressManager.kt index b977b9ac33..def9416f1e 100644 --- a/shared/src/main/java/com/itsaky/androidide/progress/ProgressManager.kt +++ b/shared/src/main/java/com/itsaky/androidide/progress/ProgressManager.kt @@ -40,21 +40,42 @@ class ProgressManager private constructor() { } } + /** + * Associate an existing [checker] with [thread]. A subsequent [cancel] of [thread] then flips + * *this* checker (rather than a throwaway [Default]), so a caller that also polls [checker] + * observes the cancellation. Used by the editor to make a completion's cancel checker cancellable + * via the thread it runs on. Pair with [unregister]. + */ + fun register(thread: Thread, checker: ICancelChecker) { + synchronized(threads) { + threads[thread] = checker + } + } + + /** Remove any checker previously associated with [thread] via [register]. */ + fun unregister(thread: Thread) { + synchronized(threads) { + threads.remove(thread) + } + } + fun cancel(thread: Thread) { - var checker = threads[thread] - if (checker == null) { - checker = Default() + synchronized(threads) { + var checker = threads[thread] + if (checker == null) { + checker = Default() + threads[thread] = checker + } + checker.cancel() } - checker.cancel() - threads[thread] = checker } @JvmName("internalAbortIfCancelled") private fun abortIfCancelled() { val thisThread = Thread.currentThread() - val checker = threads[thisThread] + val checker = synchronized(threads) { threads[thisThread] } if (checker != null && checker.isCancelled()) { - threads.remove(thisThread) + synchronized(threads) { threads.remove(thisThread) } throw CancellationException() } } diff --git a/shared/src/test/java/com/itsaky/androidide/progress/ICancelCheckerTest.kt b/shared/src/test/java/com/itsaky/androidide/progress/ICancelCheckerTest.kt new file mode 100644 index 0000000000..571fec354b --- /dev/null +++ b/shared/src/test/java/com/itsaky/androidide/progress/ICancelCheckerTest.kt @@ -0,0 +1,98 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.progress + +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import java.util.concurrent.atomic.AtomicInteger + +/** + * Tests for the push-based [ICancelChecker.invokeOnCancel] added for ADFA-4174, which lets the Kotlin + * LSP abort an in-flight `analyze` the moment cancellation happens instead of polling. + */ +class ICancelCheckerTest { + + @Test + fun `invokeOnCancel fires when cancel is called`() { + val checker = ICancelChecker.Default() + val fired = AtomicInteger(0) + + checker.invokeOnCancel { fired.incrementAndGet() } + assertThat(fired.get()).isEqualTo(0) + + checker.cancel() + assertThat(fired.get()).isEqualTo(1) + } + + @Test + fun `invokeOnCancel fires immediately when already cancelled`() { + val checker = ICancelChecker.Default(cancelled = true) + val fired = AtomicInteger(0) + + checker.invokeOnCancel { fired.incrementAndGet() } + + assertThat(fired.get()).isEqualTo(1) + } + + @Test + fun `invokeOnCancel fires at most once across repeated cancel calls`() { + val checker = ICancelChecker.Default() + val fired = AtomicInteger(0) + + checker.invokeOnCancel { fired.incrementAndGet() } + checker.cancel() + checker.cancel() + checker.cancel() + + assertThat(fired.get()).isEqualTo(1) + } + + @Test + fun `multiple listeners all fire on cancel`() { + val checker = ICancelChecker.Default() + val fired = AtomicInteger(0) + + checker.invokeOnCancel { fired.incrementAndGet() } + checker.invokeOnCancel { fired.incrementAndGet() } + + checker.cancel() + + assertThat(fired.get()).isEqualTo(2) + } + + @Test + fun `NOOP invokeOnCancel is a no-op`() { + val fired = AtomicInteger(0) + + // NOOP is a shared singleton that is never cancelled; registering must be a no-op so listeners + // (which may capture large objects) do not accumulate on it forever. We deliberately do NOT call + // NOOP.cancel() — flipping the shared singleton would corrupt every other user of it. + ICancelChecker.NOOP.invokeOnCancel { fired.incrementAndGet() } + + assertThat(fired.get()).isEqualTo(0) + } + + @Test + fun `CANCELLED fires immediately`() { + val fired = AtomicInteger(0) + + ICancelChecker.CANCELLED.invokeOnCancel { fired.incrementAndGet() } + + assertThat(fired.get()).isEqualTo(1) + } +} diff --git a/shared/src/test/java/com/itsaky/androidide/progress/ProgressManagerTest.kt b/shared/src/test/java/com/itsaky/androidide/progress/ProgressManagerTest.kt new file mode 100644 index 0000000000..41edc171c7 --- /dev/null +++ b/shared/src/test/java/com/itsaky/androidide/progress/ProgressManagerTest.kt @@ -0,0 +1,58 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.progress + +import com.google.common.truth.Truth.assertThat +import org.junit.Test + +class ProgressManagerTest { + + @Test + fun `cancel flips a registered checker`() { + // Regression for ADFA-4174: cancel(thread) must act on the *registered* checker so a caller that + // polls that same checker (the completion cancel checker driving mid-analyze abort) observes the + // cancellation — before, cancel() always stored a throwaway Default and the registered checker + // never became cancelled. + val checker = ICancelChecker.Default() + val thread = Thread.currentThread() + + ProgressManager.instance.register(thread, checker) + try { + assertThat(checker.isCancelled()).isFalse() + + ProgressManager.instance.cancel(thread) + + assertThat(checker.isCancelled()).isTrue() + } finally { + ProgressManager.instance.unregister(thread) + } + } + + @Test + fun `unregister detaches the checker so cancel no longer affects it`() { + val checker = ICancelChecker.Default() + val thread = Thread.currentThread() + + ProgressManager.instance.register(thread, checker) + ProgressManager.instance.unregister(thread) + + ProgressManager.instance.cancel(thread) + + assertThat(checker.isCancelled()).isFalse() + } +}