Skip to content

Commit 0734abf

Browse files
itsaky-adfaitsaky-adfa
authored
ADFA-4387: add global analysis lock for Kotlin analysis (#1428)
1 parent 1d27a8a commit 0734abf

6 files changed

Lines changed: 195 additions & 33 deletions

File tree

‎lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import com.itsaky.androidide.lsp.models.DocumentChange
1818
import com.itsaky.androidide.lsp.models.TextEdit
1919
import com.itsaky.androidide.resources.R
2020
import org.appdevforall.codeonthego.indexing.jvm.JvmSymbol
21-
import org.jetbrains.kotlin.analysis.api.fir.diagnostics.KaFirDiagnostic
2221
import org.slf4j.LoggerFactory
2322

2423
class AddImportAction : BaseKotlinCodeAction() {
@@ -46,14 +45,13 @@ class AddImportAction : BaseKotlinCodeAction() {
4645
return
4746
}
4847

49-
val diagnostic = extra.diagnostic as? KaFirDiagnostic.UnresolvedReference?
50-
if (diagnostic == null) {
48+
val reference = extra.unresolvedReference
49+
if (reference == null) {
5150
markInvisible()
5251
return
5352
}
5453

5554
val env = extra.compilationEnv
56-
val reference = diagnostic.reference
5755
val hasImportableSymbols = env.ktSymbolIndex
5856
.findSymbolBySimpleName(reference, limit = 0)
5957
.any { it.kind.isClassifier }
@@ -65,10 +63,10 @@ class AddImportAction : BaseKotlinCodeAction() {
6563
}
6664

6765
override suspend fun execAction(data: ActionData): Map<JvmSymbol, List<TextEdit>> {
68-
val (diagnostic, env) = data.require<DiagnosticItem>().extra as? KotlinDiagnosticExtra
66+
val (reference, env) = data.require<DiagnosticItem>().extra as? KotlinDiagnosticExtra
6967
?: return emptyMap()
7068

71-
diagnostic as KaFirDiagnostic.UnresolvedReference
69+
if (reference == null) return emptyMap()
7270

7371
val file = data.requireFile()
7472
val nioPath = file.toPath()
@@ -77,7 +75,7 @@ class AddImportAction : BaseKotlinCodeAction() {
7775
?: return emptyMap()
7876

7977
return env.ktSymbolIndex
80-
.findSymbolBySimpleName(diagnostic.reference, limit = 0)
78+
.findSymbolBySimpleName(reference, limit = 0)
8179
.filter { it.kind.isClassifier }
8280
.associateWith { symbol -> insertImport(ktFile, symbol.fqName) }
8381
}

‎lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt‎

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -221,22 +221,31 @@ internal class CompilationEnvironment(
221221
@OptIn(KaImplementationDetail::class)
222222
private inline fun notifyElementModifiedForPath(
223223
path: Path,
224-
typeProvider: (KtFile) -> KaElementModificationType,
224+
crossinline typeProvider: (KtFile) -> KaElementModificationType,
225225
) {
226-
val structureProvider = ProjectStructureProvider.getInstance(project)
227-
val ktFile = path.toVirtualFileOrNull()?.let {
228-
psiManager.findFile(it) as? KtFile
229-
}
226+
// Resolve PSI/module structure under the read lock, mirroring loadKtFile(); driving
227+
// psiManager.findFile / structureProvider concurrently with an `analyze` read section
228+
// otherwise races.
229+
val (ktFile, module) = project.read {
230+
val structureProvider = ProjectStructureProvider.getInstance(project)
231+
val ktFile = path.toVirtualFileOrNull()?.let {
232+
psiManager.findFile(it) as? KtFile
233+
}
230234

231-
if (ktFile != null) {
232-
KaSourceModificationService.getInstance(project)
233-
.handleElementModification(ktFile, typeProvider(ktFile))
234-
}
235+
val module = (ktFile?.let { structureProvider.getModule(it, null) }
236+
?: structureProvider.findModuleForSourceId(path.pathString)) as? AbstractKtModule
235237

236-
val module = (ktFile?.let { structureProvider.getModule(it, null) }
237-
?: structureProvider.findModuleForSourceId(path.pathString)) as? AbstractKtModule
238+
ktFile to module
239+
}
238240

239241
project.write {
242+
// Must run under the write lock so the session mutation can't race a concurrent
243+
// `analyze` (which only holds the read lock); see onFileContentChanged.
244+
if (ktFile != null) {
245+
KaSourceModificationService.getInstance(project)
246+
.handleElementModification(ktFile, typeProvider(ktFile))
247+
}
248+
240249
if (module != null) {
241250
module.invalidateSearchScope()
242251
project.publishModificationEvent(

‎lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.kt‎

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,46 @@ import org.jetbrains.kotlin.psi.KtElement
1111
import org.jetbrains.kotlin.psi.KtFile
1212
import org.jetbrains.kotlin.psi.UserDataProperty
1313
import java.nio.file.Path
14+
import java.util.concurrent.locks.ReentrantLock
15+
import kotlin.concurrent.withLock
1416

1517
private val KT_LSP_COMPLETION_BACKING_FILE = Key<Path>("KT_LSP_COMPLETION_BACKING_FILE")
1618
var KtFile.backingFilePath by UserDataProperty(KT_LSP_COMPLETION_BACKING_FILE)
1719

18-
internal inline fun <R> analyzeMaybeDangling(useSiteElement: KtElement, crossinline action: KaSession.() -> R): R {
19-
if (useSiteElement is KtFile && useSiteElement.isDangling && useSiteElement.copyOrigin != null) {
20-
return analyzeCopy(useSiteElement, KaDanglingFileResolutionMode.PREFER_SELF, action)
21-
}
20+
/**
21+
* Serializes all Kotlin Analysis API access (`analyze` / `analyzeCopy`).
22+
*
23+
* The Analysis API tracks its `analyze` lifetime context in a per-thread stack and is not safe to
24+
* drive concurrently from multiple background threads without the platform read-action coordination
25+
* that this LSP replaces with a custom [com.itsaky.androidide.lsp.kotlin.compiler.read] lock.
26+
* Indexing, diagnostics and completion all run analysis on `Dispatchers.Default` and frequently
27+
* target the same edited file, so overlapping `analyze` calls corrupted the lifetime/session
28+
* lifecycle and surfaced as
29+
* `KaInaccessibleLifetimeOwnerAccessException: ... Called outside an \`analyze\` context.`
30+
*
31+
* Holding this lock around every analysis entry point makes analyses mutually exclusive. It is a
32+
* [ReentrantLock] so an (indirect) nested analysis on the same thread cannot deadlock.
33+
*
34+
* **Footgun:** analysis runs under the *read* (shared) side of the global
35+
* [com.itsaky.androidide.lsp.kotlin.compiler.read] lock, and that `ReentrantReadWriteLock` is
36+
* non-upgradeable. Code running inside [withAnalysisLock] / an `analyze` block must therefore never
37+
* call [com.itsaky.androidide.lsp.kotlin.compiler.write] — upgrading read → write on the same thread
38+
* deadlocks.
39+
*/
40+
private val analysisLock = ReentrantLock()
41+
42+
/**
43+
* Runs [action] while holding the shared [analysisLock]. **All** Analysis API access must go through
44+
* this helper (or [analyzeMaybeDangling], which already does); never call `analyze` / `analyzeCopy`
45+
* directly, or the serialization guarantee is lost.
46+
*/
47+
internal inline fun <R> withAnalysisLock(action: () -> R): R = analysisLock.withLock(action)
2248

23-
return analyze(useSiteElement, action)
24-
}
49+
internal inline fun <R> analyzeMaybeDangling(useSiteElement: KtElement, crossinline action: KaSession.() -> R): R =
50+
withAnalysisLock {
51+
if (useSiteElement is KtFile && useSiteElement.isDangling && useSiteElement.copyOrigin != null) {
52+
analyzeCopy(useSiteElement, KaDanglingFileResolutionMode.PREFER_SELF, action)
53+
} else {
54+
analyze(useSiteElement, action)
55+
}
56+
}

‎lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/completion/KotlinCompletions.kt‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package com.itsaky.androidide.lsp.kotlin.completion
33
import com.itsaky.androidide.lookup.Lookup
44
import com.itsaky.androidide.lsp.api.describeSnippet
55
import com.itsaky.androidide.lsp.kotlin.compiler.CompilationEnvironment
6+
import com.itsaky.androidide.lsp.kotlin.compiler.modules.analyzeMaybeDangling
67
import com.itsaky.androidide.lsp.kotlin.compiler.read
78
import com.itsaky.androidide.lsp.kotlin.utils.AnalysisContext
89
import com.itsaky.androidide.lsp.kotlin.utils.ContextKeywords
@@ -30,8 +31,6 @@ import org.jetbrains.kotlin.analysis.api.KaContextParameterApi
3031
import org.jetbrains.kotlin.analysis.api.KaExperimentalApi
3132
import org.jetbrains.kotlin.analysis.api.KaIdeApi
3233
import org.jetbrains.kotlin.analysis.api.KaSession
33-
import org.jetbrains.kotlin.analysis.api.analyzeCopy
34-
import org.jetbrains.kotlin.analysis.api.projectStructure.KaDanglingFileResolutionMode
3534
import org.jetbrains.kotlin.analysis.api.renderer.types.KaTypeRenderer
3635
import org.jetbrains.kotlin.analysis.api.renderer.types.impl.KaTypeRendererForSource
3736
import org.jetbrains.kotlin.analysis.api.symbols.KaCallableSymbol
@@ -149,10 +148,7 @@ internal fun doComplete(params: CompletionParams): CompletionResult {
149148
env.project.read {
150149
abortIfCancelled()
151150

152-
analyzeCopy(
153-
useSiteElement = completionKtFile,
154-
resolutionMode = KaDanglingFileResolutionMode.PREFER_SELF,
155-
) {
151+
analyzeMaybeDangling(completionKtFile) {
156152
val ctx =
157153
resolveAnalysisContext(
158154
env = env,
@@ -168,7 +164,7 @@ internal fun doComplete(params: CompletionParams): CompletionResult {
168164
completionOffset,
169165
params.file
170166
)
171-
return@analyzeCopy CompletionResult.EMPTY
167+
return@analyzeMaybeDangling CompletionResult.EMPTY
172168
}
173169

174170
abortIfCancelled()

‎lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.kt‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import org.jetbrains.kotlin.analysis.api.KaExperimentalApi
1313
import org.jetbrains.kotlin.analysis.api.components.KaDiagnosticCheckerFilter
1414
import org.jetbrains.kotlin.analysis.api.diagnostics.KaDiagnosticWithPsi
1515
import org.jetbrains.kotlin.analysis.api.diagnostics.KaSeverity
16+
import org.jetbrains.kotlin.analysis.api.fir.diagnostics.KaFirDiagnostic
1617
import org.jetbrains.kotlin.com.intellij.openapi.util.TextRange
1718
import org.jetbrains.kotlin.com.intellij.psi.PsiErrorElement
1819
import org.jetbrains.kotlin.com.intellij.psi.PsiFile
@@ -23,7 +24,14 @@ import java.nio.file.Path
2324
private val logger = LoggerFactory.getLogger("KotlinDiagnosticProvider")
2425

2526
internal data class KotlinDiagnosticExtra(
26-
val diagnostic: KaDiagnosticWithPsi<*>,
27+
/**
28+
* The unresolved-reference name extracted from an [KaFirDiagnostic.UnresolvedReference]
29+
* diagnostic, or `null` for any other diagnostic. This is plain data extracted *inside* the
30+
* `analyze` block on purpose: storing the [KaDiagnosticWithPsi] (a `KaLifetimeOwner`) here and
31+
* reading its members later from a code action would access it outside an `analyze` context and
32+
* crash with `KaInaccessibleLifetimeOwnerAccessException`.
33+
*/
34+
val unresolvedReference: String?,
2735
val compilationEnv: CompilationEnvironment,
2836
)
2937

@@ -79,8 +87,12 @@ private fun doAnalyze(file: Path, cancelChecker: ICancelChecker): DiagnosticResu
7987
ktFile.collectDiagnostics(KaDiagnosticCheckerFilter.EXTENDED_AND_COMMON_CHECKERS)
8088
.forEach { diagnostic ->
8189
cancelChecker.abortIfCancelled()
90+
// Extract plain data while still inside the analyze context; never let
91+
// the KaLifetimeOwner diagnostic escape (see KotlinDiagnosticExtra).
92+
val unresolvedReference =
93+
(diagnostic as? KaFirDiagnostic.UnresolvedReference)?.reference
8294
add(diagnostic.toDiagnosticItem().apply {
83-
extra = KotlinDiagnosticExtra(diagnostic, env)
95+
extra = KotlinDiagnosticExtra(unresolvedReference, env)
8496
})
8597
}
8698
}

‎lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.kt‎

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
package com.itsaky.androidide.lsp.kotlin.compiler.modules
2+
3+
import com.google.common.truth.Truth.assertThat
4+
import com.itsaky.androidide.lsp.kotlin.compiler.read
5+
import com.itsaky.androidide.lsp.kotlin.fixtures.KtLspTest
6+
import kotlinx.coroutines.Dispatchers
7+
import kotlinx.coroutines.coroutineScope
8+
import kotlinx.coroutines.launch
9+
import kotlinx.coroutines.runBlocking
10+
import org.junit.Test
11+
import java.util.Collections
12+
import java.util.concurrent.atomic.AtomicInteger
13+
import kotlin.math.max
14+
15+
/**
16+
* Regression tests for the `KaInaccessibleLifetimeOwnerAccessException: ... Called outside an
17+
* `analyze` context.` reported in Sentry (APPDEVFORALL-VR / 7454434587).
18+
*
19+
* Root cause: indexing, diagnostics and completion all drove the stock Kotlin Analysis API
20+
* concurrently from `Dispatchers.Default` threads (the modified-file indexer is debounced into
21+
* independent coroutines), frequently against the same file. The Analysis API tracks its `analyze`
22+
* lifetime context in a per-thread stack and is not safe to run concurrently without platform
23+
* read-action coordination (which this LSP replaces with a shared read lock that does not serialize
24+
* analysis). Overlapping `analyze` calls corrupted the lifetime/session lifecycle.
25+
*
26+
* Fix: [analyzeMaybeDangling] / [withAnalysisLock] hold a process-wide reentrant lock so analyses
27+
* are mutually exclusive.
28+
*
29+
* Both tests fail before the fix (either by throwing the exception or by observing overlapping
30+
* analyses) and pass after it.
31+
*/
32+
class AnalysisSerializationTest : KtLspTest() {
33+
34+
@Test
35+
fun `concurrent analyzeMaybeDangling never throws lifetime exception`(): Unit = runBlocking {
36+
val files = (0 until 8).map { i ->
37+
createSourceFile(
38+
"Concurrent$i.kt",
39+
"""
40+
class Klass$i {
41+
fun member$i(p: Int): Int = p + $i
42+
val prop$i: String = "v$i"
43+
}
44+
45+
fun topLevel$i() = $i
46+
""".trimIndent()
47+
)
48+
}
49+
50+
val errors = Collections.synchronizedList(mutableListOf<Throwable>())
51+
52+
// Many short, overlapping analyses on a high-parallelism dispatcher to reproduce the race.
53+
coroutineScope {
54+
repeat(240) { iter ->
55+
launch(Dispatchers.IO) {
56+
val file = files[iter % files.size]
57+
try {
58+
env.project.read {
59+
analyzeMaybeDangling(file) {
60+
// Touching declaration symbols is what triggered the lifetime check.
61+
file.declarations.forEach { dcl ->
62+
dcl.symbol
63+
}
64+
}
65+
}
66+
} catch (t: Throwable) {
67+
errors.add(t)
68+
}
69+
}
70+
}
71+
}
72+
73+
assertThat(errors).isEmpty()
74+
}
75+
76+
@Test
77+
fun `analyzeMaybeDangling serializes overlapping analyses`(): Unit = runBlocking {
78+
val files = (0 until 8).map { i ->
79+
createSourceFile("Serialized$i.kt", "class S$i { fun f$i() = $i }")
80+
}
81+
82+
val inFlight = AtomicInteger(0)
83+
val maxObserved = AtomicInteger(0)
84+
val errors = Collections.synchronizedList(mutableListOf<Throwable>())
85+
86+
coroutineScope {
87+
repeat(64) { iter ->
88+
launch(Dispatchers.IO) {
89+
val file = files[iter % files.size]
90+
try {
91+
env.project.read {
92+
analyzeMaybeDangling(file) {
93+
val concurrent = inFlight.incrementAndGet()
94+
maxObserved.updateAndGet { max(it, concurrent) }
95+
try {
96+
file.declarations.forEach { it.symbol }
97+
// Widen the window so any real overlap is observed.
98+
Thread.sleep(2)
99+
} finally {
100+
inFlight.decrementAndGet()
101+
}
102+
}
103+
}
104+
} catch (t: Throwable) {
105+
errors.add(t)
106+
}
107+
}
108+
}
109+
}
110+
111+
assertThat(errors).isEmpty()
112+
// The shared analysis lock must prevent two analyses from running at once.
113+
assertThat(maxObserved.get()).isEqualTo(1)
114+
}
115+
}

0 commit comments

Comments
 (0)