diff --git a/docs/process/learnings.md b/docs/process/learnings.md new file mode 100644 index 0000000000..36f4a5ca08 --- /dev/null +++ b/docs/process/learnings.md @@ -0,0 +1,5 @@ +# Learnings + +## Kotlin LSP test harness +- Disposing the `KtLspTestEnvironment` in a unit test (`env.close()`, or `Disposer.dispose(env.project)`) throws `AssertionError: Write access is allowed inside write-action only`. IntelliJ requires model teardown to run inside a write action. This is why `KtLspTestRule`'s teardown has `env.close()` commented out as "fails in test cases". To dispose deterministically in a test, wrap it: `ApplicationManager.getApplication().runWriteAction { env.close() }`. +- The index/compilation environment lifecycle is racy: background `IndexWorker` coroutines call `PsiManager.findFile(project)` and will crash with `Project is already disposed` if the project is disposed before the workers are stopped. Always stop & join `KtSymbolIndex.close()` (and cancel related scopes) before `Disposer.dispose(...)`. diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/AbstractCompilationEnvironment.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/AbstractCompilationEnvironment.kt index 5bcba764b7..cc5550c987 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/AbstractCompilationEnvironment.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/AbstractCompilationEnvironment.kt @@ -10,6 +10,8 @@ import com.itsaky.androidide.lsp.kotlin.compiler.services.JavaModuleAnnotationsP import com.itsaky.androidide.lsp.kotlin.compiler.services.KtLspService import com.itsaky.androidide.lsp.kotlin.compiler.services.WriteAccessGuard import com.itsaky.androidide.lsp.kotlin.compiler.services.latestLanguageVersionSettings +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeoutOrNull import org.jetbrains.kotlin.K1Deprecation import org.jetbrains.kotlin.analysis.api.platform.declarations.KotlinAnnotationsResolverFactory import org.jetbrains.kotlin.analysis.api.platform.declarations.KotlinDeclarationProviderFactory @@ -96,6 +98,9 @@ internal abstract class AbstractCompilationEnvironment( ) : AutoCloseable { companion object { + /** Max time close() will block the (main) thread draining background workers before disposal. */ + const val CLOSE_DRAIN_TIMEOUT_MS = 2_000L + init { System.setProperty("java.awt.headless", "true") setupIdeaStandaloneExecution() @@ -333,6 +338,16 @@ internal abstract class AbstractCompilationEnvironment( } override fun close() { + // Stop and join the background index workers *before* the project is disposed. + // Otherwise IndexWorker's coroutine keeps calling PsiManager.findFile(project) on a + // disposed project and crashes with "AssertionError: Project is already disposed" + // (Sentry APPDEVFORALL-17R / ADFA-4384). close() runs on the main thread during editor + // teardown, so the join is bounded by a timeout to avoid an ANR if a read is slow; the + // project.isDisposed guards cover the rare case where the timeout fires before draining. + if (::ktSymbolIndex.isInitialized) { + runBlocking { withTimeoutOrNull(CLOSE_DRAIN_TIMEOUT_MS) { ktSymbolIndex.close() } } + } + Disposer.dispose(disposable) } } 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..0702aa0694 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 @@ -20,8 +20,12 @@ import io.sentry.Sentry import kotlinx.coroutines.CoroutineName import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.runBlocking import kotlinx.coroutines.withContext +import kotlinx.coroutines.withTimeoutOrNull import org.appdevforall.codeonthego.indexing.jvm.JvmSymbolIndex import org.appdevforall.codeonthego.indexing.jvm.KtFileMetadataIndex import org.jetbrains.kotlin.analysis.api.KaImplementationDetail @@ -298,6 +302,16 @@ internal class CompilationEnvironment( override fun close() { ktProject.removeListener(this) + + // fileAnalyzer reads the project (collectDiagnosticsFor). Cancel AND join it before + // super.close() disposes the project, so an in-flight read can't touch a disposed project + // (APPDEVFORALL-17R / ADFA-4384). Bounded so a slow read can't block shutdown indefinitely. + runBlocking { + withTimeoutOrNull(CLOSE_DRAIN_TIMEOUT_MS) { + coroutineScope.coroutineContext[Job]?.cancelAndJoin() + } + } + super.close() } 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 e27bd84f94..cf43f0e72c 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 @@ -59,6 +59,11 @@ internal class IndexWorker( } while (isActive) { + // Defensive guard: if the project was disposed out from under us (e.g. a disposal + // path that didn't first drain this worker), stop instead of calling PsiManager on a + // disposed project, which throws "Project is already disposed" (APPDEVFORALL-17R). + if (project.isDisposed) break + when (val cmd = queue.take()) { is IndexCommand.RemoveFromIndex -> { val filePath = cmd.path.pathString @@ -72,6 +77,8 @@ internal class IndexWorker( continue } + if (project.isDisposed) break + val ktFile = project.read { PsiManager.getInstance(project) .findFile(cmd.vf) as? KtFile @@ -111,6 +118,8 @@ internal class IndexWorker( } is IndexCommand.ScanSourceFile -> { + if (project.isDisposed) break + val ktFile = project.read { PsiManager.getInstance(project).findFile(cmd.vf) as? KtFile } diff --git a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt index e9e4fd1e3a..adcbd44f75 100644 --- a/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt +++ b/lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt @@ -202,6 +202,13 @@ internal class KtSymbolIndex( scanningJob?.cancelAndJoin() indexingJob?.join() + + // Cancel AND JOIN the index's own scope. Beyond the main worker loop drained above, the + // debounced modifiedFileIndexer and queueOnFileChangedAsync coroutines also run + // project.read { PsiManager … }. Joining guarantees none survive into the caller's + // Disposer.dispose(...), which would otherwise crash with "Project is already disposed" + // (APPDEVFORALL-17R). This index owns `scope`. + scope.coroutineContext[Job]?.cancelAndJoin() } } 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 6d38dee820..b03c205a92 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 @@ -74,7 +74,17 @@ internal suspend fun indexSourceFile( symbolsIndex: JvmSymbolIndex, cancelChecker: ICancelChecker, ) { - val newFile = ktFile.toMetadata(project, isIndexed = true) + // 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. + if (project.isDisposed) return + + // Re-check disposal *inside* the read lock so it is atomic with toMetadata()'s PSI access: + // the fast-path check above can race a concurrent disposal before toMetadata enters its read. + val newFile = project.read { + if (project.isDisposed) return@read null + ktFile.toMetadata(project, isIndexed = true) + } ?: return val existingFile = fileIndex.get(newFile.filePath) cancelChecker.abortIfCancelled() @@ -89,6 +99,9 @@ internal suspend fun indexSourceFile( } val symbols = project.read { + // Atomic w.r.t. the read lock: bail if the project was disposed before we acquired it. + if (project.isDisposed) return@read emptyList() + val list = mutableListOf() analyzeMaybeDangling(ktFile) { val session = this diff --git a/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorkerDisposalTest.kt b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorkerDisposalTest.kt new file mode 100644 index 0000000000..d84ffc2b83 --- /dev/null +++ b/lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorkerDisposalTest.kt @@ -0,0 +1,65 @@ +package com.itsaky.androidide.lsp.kotlin.compiler.index + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout +import org.appdevforall.codeonthego.indexing.InMemoryIndex +import org.appdevforall.codeonthego.indexing.jvm.JvmSymbolDescriptor +import org.appdevforall.codeonthego.indexing.jvm.JvmSymbolIndex +import org.appdevforall.codeonthego.indexing.jvm.KtFileMetadataDescriptor +import org.appdevforall.codeonthego.indexing.jvm.KtFileMetadataIndex +import org.appdevforall.codeonthego.indexing.util.BackgroundIndexer +import org.jetbrains.kotlin.com.intellij.openapi.application.ApplicationManager +import org.junit.Test + +/** + * Regression tests for APPDEVFORALL-17R / ADFA-4384: + * `AssertionError: Project is already disposed` thrown by [IndexWorker] when the IntelliJ + * [org.jetbrains.kotlin.com.intellij.openapi.project.Project] is disposed while the background + * index worker is still running and about to call `PsiManager.findFile(project)`. + */ +class IndexWorkerDisposalTest : KtLspTest() { + + private fun jvmIndex(): JvmSymbolIndex { + val backing = InMemoryIndex(JvmSymbolDescriptor) + return object : JvmSymbolIndex(backing, BackgroundIndexer(backing)) { + override fun isActive(sourceId: String) = true + } + } + + private fun fileIndex(): KtFileMetadataIndex = + KtFileMetadataIndex(InMemoryIndex(KtFileMetadataDescriptor)) + + @Test + fun `worker stops without crashing when project is already disposed`(): Unit = runBlocking { + // Capture a real VirtualFile while the project is still alive. + val vf = createSourceFile("Sample.kt", "fun foo() = 1").virtualFile + + val worker = IndexWorker( + project = env.project, + queue = WorkerQueue(), + fileIndex = fileIndex(), + sourceIndex = jvmIndex(), + scope = CoroutineScope(Dispatchers.Default + SupervisorJob()), + ) + + // Dispose the project out from under the worker, exactly as happens on LSP shutdown + // (env.close() disposes the project via its parent Disposable, the production path). + // IntelliJ requires model teardown to run inside a write action. + ApplicationManager.getApplication().runWriteAction { env.close() } + assertThat(env.project.isDisposed).isTrue() + + // Pre-fix, processing either command calls PsiManager.findFile on the disposed project + // and throws AssertionError("Project is already disposed"). Post-fix, the disposal guard + // breaks the loop, so start() returns cleanly instead of throwing. + worker.submitCommand(IndexCommand.ScanSourceFile(vf)) + worker.submitCommand(IndexCommand.IndexSourceFile(vf)) + worker.submitCommand(IndexCommand.Stop) + + withTimeout(5_000) { worker.start() } + } +}