ADFA-4387: add global analysis lock for Kotlin analysis #1428
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,14 +11,46 @@ import org.jetbrains.kotlin.psi.KtElement | |
| import org.jetbrains.kotlin.psi.KtFile | ||
| import org.jetbrains.kotlin.psi.UserDataProperty | ||
| import java.nio.file.Path | ||
| import java.util.concurrent.locks.ReentrantLock | ||
| import kotlin.concurrent.withLock | ||
|
|
||
| private val KT_LSP_COMPLETION_BACKING_FILE = Key<Path>("KT_LSP_COMPLETION_BACKING_FILE") | ||
| var KtFile.backingFilePath by UserDataProperty(KT_LSP_COMPLETION_BACKING_FILE) | ||
|
|
||
| /** | ||
| * Serializes all Kotlin Analysis API access (`analyze` / `analyzeCopy`). | ||
| * | ||
| * 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 | ||
| * that this LSP replaces with a custom [com.itsaky.androidide.lsp.kotlin.compiler.read] lock. | ||
| * Indexing, diagnostics and completion all run analysis on `Dispatchers.Default` and frequently | ||
| * target the same edited file, so overlapping `analyze` calls corrupted the lifetime/session | ||
| * 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. | ||
| * | ||
| * **Footgun:** analysis runs under the *read* (shared) side of the global | ||
| * [com.itsaky.androidide.lsp.kotlin.compiler.read] lock, and that `ReentrantReadWriteLock` is | ||
| * non-upgradeable. Code running inside [withAnalysisLock] / an `analyze` block must therefore never | ||
| * call [com.itsaky.androidide.lsp.kotlin.compiler.write] — upgrading read → write on the same thread | ||
| * deadlocks. | ||
| */ | ||
| private val analysisLock = ReentrantLock() | ||
|
Collaborator
There was a problem hiding this comment. 🟨 Altitude — second global lock overlaps the existing one. |
||
|
|
||
| /** | ||
| * 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` | ||
|
Collaborator
There was a problem hiding this comment. 🟨 Convention isn't enforced. "Never call |
||
| * directly, or the serialization guarantee is lost. | ||
| */ | ||
| internal inline fun <R> withAnalysisLock(action: () -> R): R = analysisLock.withLock(action) | ||
|
Collaborator
There was a problem hiding this comment. 🟧 Responsiveness / pool starvation. This is a blocking |
||
|
|
||
| internal inline fun <R> analyzeMaybeDangling(useSiteElement: KtElement, crossinline action: KaSession.() -> R): R = | ||
| withAnalysisLock { | ||
| if (useSiteElement is KtFile && useSiteElement.isDangling && useSiteElement.copyOrigin != null) { | ||
| analyzeCopy(useSiteElement, KaDanglingFileResolutionMode.PREFER_SELF, action) | ||
| } else { | ||
| analyze(useSiteElement, action) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| 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 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.atomic.AtomicInteger | ||
| import kotlin.math.max | ||
|
|
||
| /** | ||
| * Regression tests for the `KaInaccessibleLifetimeOwnerAccessException: ... Called outside an | ||
| * `analyze` context.` reported in Sentry (APPDEVFORALL-VR / 7454434587). | ||
| * | ||
| * Root cause: indexing, diagnostics and completion all drove the stock Kotlin Analysis API | ||
| * concurrently from `Dispatchers.Default` threads (the modified-file indexer is debounced into | ||
| * independent coroutines), frequently against the same file. The Analysis API tracks its `analyze` | ||
| * lifetime context in a per-thread stack and is not safe to run concurrently without platform | ||
| * 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. | ||
| * | ||
| * Both tests fail before the fix (either by throwing the exception or by observing overlapping | ||
| * analyses) and pass after it. | ||
| */ | ||
| class AnalysisSerializationTest : KtLspTest() { | ||
|
|
||
| @Test | ||
| fun `concurrent analyzeMaybeDangling never throws lifetime exception`(): Unit = runBlocking { | ||
| val files = (0 until 8).map { i -> | ||
| createSourceFile( | ||
| "Concurrent$i.kt", | ||
| """ | ||
| class Klass$i { | ||
| fun member$i(p: Int): Int = p + $i | ||
| val prop$i: String = "v$i" | ||
| } | ||
|
|
||
| fun topLevel$i() = $i | ||
| """.trimIndent() | ||
| ) | ||
| } | ||
|
|
||
| val errors = Collections.synchronizedList(mutableListOf<Throwable>()) | ||
|
|
||
| // Many short, overlapping analyses on a high-parallelism dispatcher to reproduce the race. | ||
| coroutineScope { | ||
| repeat(240) { iter -> | ||
| launch(Dispatchers.IO) { | ||
| val file = files[iter % files.size] | ||
| try { | ||
| env.project.read { | ||
| analyzeMaybeDangling(file) { | ||
| // Touching declaration symbols is what triggered the lifetime check. | ||
| file.declarations.forEach { dcl -> | ||
| dcl.symbol | ||
| } | ||
| } | ||
| } | ||
| } catch (t: Throwable) { | ||
| errors.add(t) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| assertThat(errors).isEmpty() | ||
| } | ||
|
|
||
| @Test | ||
| fun `analyzeMaybeDangling serializes overlapping analyses`(): Unit = runBlocking { | ||
| val files = (0 until 8).map { i -> | ||
| createSourceFile("Serialized$i.kt", "class S$i { fun f$i() = $i }") | ||
| } | ||
|
|
||
| val inFlight = AtomicInteger(0) | ||
| val maxObserved = AtomicInteger(0) | ||
| val errors = Collections.synchronizedList(mutableListOf<Throwable>()) | ||
|
|
||
| coroutineScope { | ||
| repeat(64) { iter -> | ||
| launch(Dispatchers.IO) { | ||
| val file = files[iter % files.size] | ||
| try { | ||
| env.project.read { | ||
| analyzeMaybeDangling(file) { | ||
| val concurrent = inFlight.incrementAndGet() | ||
| maxObserved.updateAndGet { max(it, concurrent) } | ||
| try { | ||
| file.declarations.forEach { it.symbol } | ||
| // Widen the window so any real overlap is observed. | ||
| Thread.sleep(2) | ||
| } finally { | ||
| inFlight.decrementAndGet() | ||
| } | ||
| } | ||
| } | ||
| } catch (t: Throwable) { | ||
| errors.add(t) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| assertThat(errors).isEmpty() | ||
| // The shared analysis lock must prevent two analyses from running at once. | ||
| assertThat(maxObserved.get()).isEqualTo(1) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
Repository: appdevforall/CodeOnTheGo
Length of output: 13091
🏁 Script executed:
Repository: appdevforall/CodeOnTheGo
Length of output: 2228
🏁 Script executed:
Repository: appdevforall/CodeOnTheGo
Length of output: 189
🏁 Script executed:
Repository: appdevforall/CodeOnTheGo
Length of output: 1133
Wrap PSI/module resolution in a read lock. The calls to
psiManager.findFile(...)andstructureProvider.getModule(...)/findModuleForSourceId(...)at lines 214-219 run without protection, while the explicit comment at line 222-223 acknowledges that concurrentanalyzeoperations holding read locks can race against the subsequent write mutation. Compare withloadKtFile()(line 298), which correctly wrapspsiManager.findFile()inproject.read { }. Protect these PSI/module-structure reads by wrapping lines 213-219 inproject.read { ... }.🤖 Prompt for AI Agents