-
-
Notifications
You must be signed in to change notification settings - Fork 42
ADFA-5052: Defer eager JavaCompilerService construction until a real .java file is touched #1637
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: stage
Are you sure you want to change the base?
Changes from all commits
0d9356d
ad09be1
e9a54b4
23094e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,6 +84,8 @@ import org.slf4j.LoggerFactory | |
| import java.nio.file.Files | ||
| import java.nio.file.Path | ||
| import java.util.Objects | ||
| import java.util.concurrent.locks.ReentrantLock | ||
| import kotlin.concurrent.withLock | ||
|
|
||
| class JavaLanguageServer : ILanguageServer { | ||
| private val completionProvider: CompletionProvider = CompletionProvider() | ||
|
|
@@ -96,6 +98,20 @@ class JavaLanguageServer : ILanguageServer { | |
| private val timer = AnalyzeTimer { analyzeSelected() } | ||
| private var cachedCompletion: CachedCompletion | ||
|
|
||
| // Lifecycle of the javac-backed compiler state (NO_MODULE_COMPILER, SourceFileManager, | ||
| // JavaCompilerProvider), which setupWithProject() defers instead of building eagerly | ||
| // (ADFA-5052). All reads/writes of pendingWorkspace and compilerLifecycle go through | ||
| // compilerLifecycleLock, held for the *entire* reset/shutdown, not just the decision to | ||
| // run one -- otherwise a concurrent getCompiler()/onContentChange() could use a compiler | ||
| // mid-teardown, or shutdown() could destroy state a reset is still rebuilding. | ||
| private enum class CompilerLifecycle { PENDING, RESETTING, INITIALIZED, SHUTDOWN } | ||
|
|
||
| private val compilerLifecycleLock = ReentrantLock() | ||
|
|
||
| // Guarded by compilerLifecycleLock. | ||
| private var pendingWorkspace: Workspace? = null | ||
| private var compilerLifecycle = CompilerLifecycle.PENDING | ||
|
|
||
| val settings: IServerSettings | ||
| get() { | ||
| return _settings ?: JavaServerSettings | ||
|
|
@@ -123,21 +139,29 @@ class JavaLanguageServer : ILanguageServer { | |
|
|
||
| val projectManager = ProjectManagerImpl.getInstance() | ||
| projectManager.indexingServiceManager.register( | ||
| service = JvmLibraryIndexingService(context = BaseApplication.baseInstance) | ||
| service = JvmLibraryIndexingService(context = BaseApplication.baseInstance), | ||
| ) | ||
| projectManager.indexingServiceManager.register( | ||
| service = JvmGeneratedIndexingService(context = BaseApplication.baseInstance) | ||
| service = JvmGeneratedIndexingService(context = BaseApplication.baseInstance), | ||
| ) | ||
|
|
||
| JavaSnippetRepository.init() | ||
| } | ||
|
|
||
| override fun shutdown() { | ||
| (this.debugAdapter as? AutoCloseable?)?.close() | ||
| JavaCompilerProvider.getInstance().destroy() | ||
| SourceFileManager.clearCache() | ||
| CacheFSInfoSingleton.clearCache() | ||
| clearCache() | ||
| compilerLifecycleLock.withLock { | ||
| // Blocks here if a reset is in flight (RESETTING can only be observed by another | ||
| // thread while the lock is held, never by us once we've acquired it), so this never | ||
| // races ensureProjectReset()'s own destroy/rebuild. | ||
| if (compilerLifecycle == CompilerLifecycle.INITIALIZED) { | ||
| JavaCompilerProvider.getInstance().destroy() | ||
| SourceFileManager.clearCache() | ||
| CacheFSInfoSingleton.clearCache() | ||
| clearCache() | ||
| } | ||
| compilerLifecycle = CompilerLifecycle.SHUTDOWN | ||
| } | ||
| EventBus.getDefault().unregister(this) | ||
| timer.cancel() | ||
| } | ||
|
|
@@ -163,41 +187,93 @@ class JavaLanguageServer : ILanguageServer { | |
| override fun setupWithProject(workspace: Workspace) { | ||
| LSPEditorActions.ensureActionsMenuRegistered(JavaCodeActionsMenu) | ||
|
|
||
| (ProjectManagerImpl.getInstance() | ||
| .indexingServiceManager | ||
| .getService(JvmLibraryIndexingService.ID) as? JvmLibraryIndexingService?) | ||
| ?.refresh() | ||
|
|
||
| // Once we have project initialized | ||
| // Destory the NO_MODULE_COMPILER instance | ||
| JavaCompilerService.NO_MODULE_COMPILER.destroy() | ||
|
|
||
| // Clear cached file managers | ||
| SourceFileManager.clearCache() | ||
|
|
||
| // Clear cached JAR file system for R.jar | ||
| // Using the cached instance will result in completions not being updated for updated resources | ||
| // TODO Clearing caches for JAR files ending with '/R.jar' is probably not a good idea | ||
| // Maybe this could be improved by using data from the AndroidModule project model | ||
| clearCachesForPaths { path: String -> path.endsWith("/R.jar") } | ||
| ( | ||
| ProjectManagerImpl | ||
| .getInstance() | ||
| .indexingServiceManager | ||
| .getService(JvmLibraryIndexingService.ID) as? JvmLibraryIndexingService? | ||
| )?.refresh() | ||
|
|
||
| // Deferred to ensureProjectReset(), run on the first real .java-file interaction instead | ||
| // of here -- this method runs for every project open regardless of language | ||
| // (DefaultLanguageServerRegistry dispatches to all registered servers unconditionally), | ||
| // and JavaCompilerService.NO_MODULE_COMPILER / SourceFileManager.NO_MODULE eagerly | ||
| // construct real javac machinery plus a full android.jar scan at class-init, merely by | ||
| // being referenced (ADFA-5052, mirrors ADFA-5010's KotlinLanguageServer fix). | ||
| compilerLifecycleLock.withLock { | ||
| pendingWorkspace = workspace | ||
| // Leave RESETTING alone: ensureProjectReset()'s own finally block re-checks | ||
| // pendingWorkspace once it re-acquires the lock, so a project switch mid-reset is | ||
| // picked up as another PENDING round rather than raced here. | ||
| if (compilerLifecycle != CompilerLifecycle.RESETTING) { | ||
| compilerLifecycle = CompilerLifecycle.PENDING | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Clear cached module-specific compilers | ||
| JavaCompilerProvider.getInstance().destroy() | ||
| /** | ||
| * Runs the javac-specific project reset deferred by [setupWithProject], for the most | ||
| * recently opened project, the first time a real Java file is actually interacted with. | ||
| * No-ops if already up to date. Blocks concurrent callers (and [shutdown]) for the entire | ||
| * reset, not just the decision to run one. | ||
| */ | ||
| private fun ensureProjectReset() { | ||
| compilerLifecycleLock.withLock { | ||
| if (compilerLifecycle != CompilerLifecycle.PENDING) return | ||
| val workspace = pendingWorkspace ?: return | ||
| pendingWorkspace = null | ||
| compilerLifecycle = CompilerLifecycle.RESETTING | ||
|
|
||
| try { | ||
| // Once we have project initialized | ||
| // Destory the NO_MODULE_COMPILER instance | ||
| JavaCompilerService.NO_MODULE_COMPILER.destroy() | ||
|
|
||
| // Clear cached file managers | ||
| SourceFileManager.clearCache() | ||
|
|
||
| // Clear cached JAR file system for R.jar | ||
| // Using the cached instance will result in completions not being updated for updated resources | ||
| // TODO Clearing caches for JAR files ending with '/R.jar' is probably not a good idea | ||
| // Maybe this could be improved by using data from the AndroidModule project model | ||
| clearCachesForPaths { path: String -> path.endsWith("/R.jar") } | ||
|
|
||
| // Clear cached module-specific compilers | ||
| JavaCompilerProvider.getInstance().destroy() | ||
|
|
||
| // Cache classpath locations | ||
| for (subModule in workspace.subProjects) { | ||
| if (subModule !is ModuleProject || subModule.path == workspace.rootProject.path) { | ||
| continue | ||
| // Cache classpath locations | ||
| for (subModule in workspace.subProjects) { | ||
| if (subModule !is ModuleProject || subModule.path == workspace.rootProject.path) { | ||
| continue | ||
| } | ||
| SourceFileManager.forModule(subModule) | ||
| } | ||
| startOrRestartAnalyzeTimer() | ||
| } catch (e: Exception) { | ||
| // Re-queue the workspace so the next real .java-file interaction retries the | ||
| // reset, instead of a half-destroyed/half-rebuilt state being silently claimed as | ||
| // INITIALIZED (pendingWorkspace is already null by this point). | ||
| log.warn("Failed to reset javac project state; will retry on next interaction", e) | ||
| pendingWorkspace = workspace | ||
| compilerLifecycle = CompilerLifecycle.PENDING | ||
| throw e | ||
| } | ||
| SourceFileManager.forModule(subModule) | ||
|
|
||
| // A newer setupWithProject() may have queued another workspace while we were | ||
| // resetting (see the RESETTING guard above); if so, go back to PENDING instead of | ||
| // claiming INITIALIZED for a project we didn't actually reset for. | ||
| compilerLifecycle = | ||
| if (pendingWorkspace != null) { | ||
| CompilerLifecycle.PENDING | ||
| } else { | ||
| CompilerLifecycle.INITIALIZED | ||
| } | ||
| } | ||
| startOrRestartAnalyzeTimer() | ||
| } | ||
|
Comment on lines
+214
to
272
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Locate lifecycle tests and their covered transitions.
rg -n \
-g '*Test*.kt' -g '*Test*.java' -g '*test*.kt' -g '*test*.java' \
'\b(ensureProjectReset|setupWithProject|getCompiler|onContentChange|shutdown|CompilerLifecycle)\b' \
lsp/javaRepository: appdevforall/CodeOnTheGo Length of output: 1679 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect the Java server lifecycle implementation and existing Java tests without running repository code.
printf '== JavaLanguageServer outline ==\n'
ast-grep outline lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt --match JavaLanguageServer --view expanded || true
printf '\n== JavaLanguageServer lifecycle section ==\n'
sed -n '150,290p' lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt | cat -n
printf '\n== Existing Java LSP tests size/content ==\n'
wc -l lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt lsp/java/src/test/java/com/itsaky/androidide/lsp/java/JavaLSPTest.kt lsp/java/src/test/java/com/itsaky/androidide/lsp/java/compiler/CompilerTest.kt lsp/java/src/test/java/com/itsaky/androidide/lsp/java/partial/PartialReparserImplTest.kt
printf '\nJavaLSPTest relevant content:\n'
sed -n '1,180p' lsp/java/src/test/java/com/itsaky/androidide/lsp/java/JavaLSPTest.kt | cat -n
printf '\nCompilerTest relevant content:\n'
sed -n '1,130p' lsp/java/src/test/java/com/itsaky/androidide/lsp/java/compiler/CompilerTest.kt | cat -n
printf '\nPartialReparserImplTest relevant content:\n'
sed -n '1,120p' lsp/java/src/test/java/com/itsaky/androidide/lsp/java/partial/PartialReparserImplTest.kt | cat -n
printf '\n== Deterministic coverage probe: tests mentioning lifecycle methods/states ==\n'
python3 - <<'PY'
import pathlib,re
dirs = [pathlib.Path('lsp/java/src/test')]
terms = ['ensureProjectReset','setupWithProject','CompilerLifecycle','getCompiler','onContentChange','shutdown']
for path in sorted(p for d in dirs for p in d.rglob('*') if p.name.endswith(('.kt','.java')) and 'Test' in p.name.lower()):
text = path.read_text(errors='ignore')
hits = [t for t in terms if t in text]
if hits:
print(path)
print('\n'.join(f' {t}' for t in hits))
PYRepository: appdevforall/CodeOnTheGo Length of output: 20074 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect lock usage and public lifecycle entry points to clarify the required test scenarios.
printf '== All JavaLanguageServer lifecycle symbols ==\n'
rg -n '\b(ensureProjectReset|setupWithProject|getCompiler|onContentChange|shutdown|compilerLifecycle|pendingWorkspace|compilerLifecycleLock|CompilerLifecycle)\b' lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt
printf '\n== Lock implementation nearby ==\n'
rg -n 'compilerLifecycleLock|lock\(|withLock\(' lsp/java/src/main/java/com/itsaky/androidide/lsp/javaRepository: appdevforall/CodeOnTheGo Length of output: 5311 Add lifecycle transition coverage. The new 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| override fun complete(params: CompletionParams?): CompletionResult { | ||
| val compiler = getCompiler(params!!.file) | ||
| if (!settings.completionsEnabled() || !completionProvider.canComplete(params.file) | ||
| ) { | ||
| if (!settings.completionsEnabled() || !completionProvider.canComplete(params.file)) { | ||
| return CompletionResult.EMPTY | ||
| } | ||
|
|
||
|
|
@@ -258,15 +334,21 @@ class JavaLanguageServer : ILanguageServer { | |
| return DiagnosticResult.NO_UPDATE | ||
| } | ||
|
|
||
| // diagnosticProvider.analyze() builds its own JavaCompilerService directly (bypassing | ||
| // getCompiler()), and analysis is often the first real .java-file interaction in a | ||
| // session (auto-triggered on file open, ahead of any completion request) -- without this, | ||
| // the R.jar/file-manager caches this reset clears would never get cleared for this | ||
| // project, and diagnostics could resolve against a stale previous project's classpath. | ||
| ensureProjectReset() | ||
|
|
||
| return if (!settings.codeAnalysisEnabled()) { | ||
| DiagnosticResult.NO_UPDATE | ||
| } else { | ||
| diagnosticProvider.analyze(file) | ||
| } | ||
| } | ||
|
|
||
| override fun formatCode(params: FormatCodeParams?): CodeFormatResult = | ||
| CodeFormatProvider(settings).format(params) | ||
| override fun formatCode(params: FormatCodeParams?): CodeFormatResult = CodeFormatProvider(settings).format(params) | ||
|
|
||
| override fun handleFailure(failure: LSPFailure?): Boolean { | ||
| return when (failure!!.type) { | ||
|
|
@@ -285,10 +367,17 @@ class JavaLanguageServer : ILanguageServer { | |
| if (!DocumentUtils.isJavaFile(file)) { | ||
| return JavaCompilerService.NO_MODULE_COMPILER | ||
| } | ||
| val module = | ||
| ProjectManagerImpl.getInstance().findModuleForFile(file!!) | ||
| ?: return JavaCompilerService.NO_MODULE_COMPILER | ||
| return JavaCompilerProvider.get(module) | ||
| // Held across ensureProjectReset() *and* the provider lookup (ReentrantLock is | ||
| // reentrant, so ensureProjectReset()'s own withLock nests fine): otherwise a concurrent | ||
| // reset for a newer project could destroy() the provider's compilers in the gap between | ||
| // this thread's reset finishing and its JavaCompilerProvider.get() call. | ||
| return compilerLifecycleLock.withLock { | ||
| ensureProjectReset() | ||
| val module = | ||
| ProjectManagerImpl.getInstance().findModuleForFile(file!!) | ||
| ?: return@withLock JavaCompilerService.NO_MODULE_COMPILER | ||
| JavaCompilerProvider.get(module) | ||
| } | ||
| } | ||
|
|
||
| private fun updateCachedCompletion(cachedCompletion: CachedCompletion) { | ||
|
|
@@ -314,14 +403,20 @@ class JavaLanguageServer : ILanguageServer { | |
| return | ||
| } | ||
|
|
||
| // TODO Find an alternative to efficiently update changeDelta in JavaCompilerService instance | ||
| JavaCompilerService.NO_MODULE_COMPILER.onDocumentChange(event) | ||
| val module = | ||
| getInstance() | ||
| .findModuleForFile(event.changedFile) | ||
| if (module != null) { | ||
| val compiler = JavaCompilerProvider.get(module) | ||
| compiler.onDocumentChange(event) | ||
| // See getCompiler(): held across the reset *and* the provider lookup/use so a concurrent | ||
| // reset can't destroy() these compilers in between. | ||
| compilerLifecycleLock.withLock { | ||
| ensureProjectReset() | ||
|
|
||
| // TODO Find an alternative to efficiently update changeDelta in JavaCompilerService instance | ||
| JavaCompilerService.NO_MODULE_COMPILER.onDocumentChange(event) | ||
| val module = | ||
| getInstance() | ||
| .findModuleForFile(event.changedFile) | ||
| if (module != null) { | ||
| val compiler = JavaCompilerProvider.get(module) | ||
| compiler.onDocumentChange(event) | ||
| } | ||
| } | ||
| startOrRestartAnalyzeTimer() | ||
| } | ||
|
|
||
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 | 🏗️ Heavy lift
Keep compiler use inside the lifecycle contract.
ensureProjectReset()releasescompilerLifecycleLockbeforegetCompiler()returns aJavaCompilerService.shutdown()can then acquire the lock and destroy that service while completion, navigation, or document-change work still uses it. After shutdown,getCompiler()andonContentChange()also continue to access compiler state, andsetupWithProject()can changeSHUTDOWNback toPENDING.Use an operation-scoped lifecycle lease or equivalent guarded execution API. Keep shutdown terminal. Route all compiler creation, use, and destruction through that API.
lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt#L109-L113: represent active compiler operations in the lifecycle mechanism.lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt#L153-L164: wait for active compiler operations before destruction.lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt#L203-L210: do not transitionSHUTDOWNtoPENDING.lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt#L355-L359: acquire and use the compiler through the lifecycle mechanism.lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt#L385-L396: execute Java document updates through the same mechanism.This conflicts with the PR objective that lifecycle locking prevents concurrent compiler use during teardown.
📍 Affects 1 file
lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt#L109-L113(this comment)lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt#L153-L164lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt#L203-L210lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt#L355-L359lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt#L385-L396🤖 Prompt for AI Agents