Skip to content

ADFA-5052: Defer eager JavaCompilerService construction until a real .java file is touched - #1637

Open
davidschachterADFA wants to merge 4 commits into
stagefrom
task/ADFA-5052-lazy-load-java-compiler
Open

ADFA-5052: Defer eager JavaCompilerService construction until a real .java file is touched#1637
davidschachterADFA wants to merge 4 commits into
stagefrom
task/ADFA-5052-lazy-load-java-compiler

Conversation

@davidschachterADFA

@davidschachterADFA davidschachterADFA commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

DefaultLanguageServerRegistry.onProjectInitialized dispatches setupWithProject to every registered language server unconditionally, regardless of the project's actual language. JavaLanguageServer.setupWithProject() referenced JavaCompilerService.NO_MODULE_COMPILER and called SourceFileManager.clearCache(), both of which trigger class-init that eagerly constructs real javac Context/JavacFileManager machinery plus a full android.jar top-level-class scan -- on the first project open in the app's lifetime, Kotlin-only projects included. shutdown() had the mirror-image problem on every project close.

This is the same eager-load bug pattern ADFA-5010 fixed for the Kotlin Analysis API. While researching whether javac could get ADFA-5010's DexClassLoader/carrier-APK treatment too, I confirmed and sized the real analog (openjdk.tools.javac, ~2,238 classes, ~3.7MB of the dex) and found this identical bug independently of whether a module-split/carrier-APK is ever built. This PR is scoped to just that: no DexClassLoader split, javac/jdk-compiler stay in the main dex, just constructed lazily now.

Change

  • setupWithProject() now only stashes the Workspace; the actual reset (destroy NO_MODULE_COMPILER, clear file-manager/JAR-fs caches, index module classpaths) moved to a new ensureProjectReset(), called from getCompiler(), onContentChange(), and analyze() -- all gated on DocumentUtils.isJavaFile(), so the reset now only runs on genuine Java-file interaction.
  • shutdown() skips its javac-specific cleanup entirely if that reset never happened.
  • Per-file LSP dispatch methods (complete/findReferences/findDefinition/expandSelection/signatureHelp) needed no changes: the editor's IDELanguage already resolves one language server per file before calling any of them, so they were never the source of the cross-language trigger.
  • Concurrency: replaced the initial @Volatile + narrow synchronized claim with an explicit PENDING → RESETTING → INITIALIZED / SHUTDOWN state machine guarded by one ReentrantLock held for the entire reset or shutdown, not just the decision to run one. getCompiler()/onContentChange() hold the lock across both the reset and the subsequent JavaCompilerProvider lookup/use (reentrant, so no deadlock), closing a narrow window where a concurrent reset for a newer project could destroy() a compiler mid-use.
  • Robustness: an exception during the reset (e.g. a bad submodule) now re-queues the workspace and reverts to PENDING before rethrowing, instead of silently claiming INITIALIZED for a half-torn-down state with no retry path.
  • analyze() (diagnostics) now also triggers the deferred reset: JavaDiagnosticProvider.analyze() builds its own JavaCompilerService directly, bypassing getCompiler(), and diagnostics are often the first real Java-file interaction (auto-triggered on file open, ahead of any completion request) -- without this it could run against stale R.jar/classpath caches for an entire session.

These last three items came from a /code-review high pass on the original version of this PR; two other findings from that pass were assessed and intentionally left as-is: shutdown() blocking on an in-flight reset with no cancellation checkpoint (real, but performance-only -- no crash/corruption -- and disproportionate to fix given the narrow, bounded-cost window); and KotlinLanguageServer still constructing eagerly (real, but already fixed by the separate, not-yet-merged ADFA-5010 (PR #1635) -- this branch just forked before that merge landed).

Test plan

  • :lsp:java:testV8DebugUnitTest passes
  • :app:assembleV8Debug builds clean
  • Manual on-device verification (Pixel 6 Pro), initial lazy-load pass: closed a mixed Java+Kotlin project, opened a Kotlin-only project (wizard-created, zero .java files) and its .kt file, opened a second Kotlin-only project's file -- greping the whole session's logcat for JavaCompilerService/SourceFileManager/openjdk.tools.javac/CacheFSInfoSingleton returns zero hits throughout. Then opened a Java project's .java file: SourceFileManager's Creating source file manager instance for module: AndroidModule: :app fires for the first time at that exact point, and live completion on a real field (binding: ActivityMainBinding) returns correct, type-resolved results. No crashes throughout.
  • Manual on-device re-verification (Pixel 6 Pro) after the concurrency/robustness fixes: repeated the same negative test (mixed project close -> Kotlin-only project + its .kt file -> still zero javac-related log hits) and confirmed shutdown() doesn't hang/crash when compilerLifecycle is still PENDING. For the positive test, opened a Java file and let diagnostics auto-fire via the file-open/analyze-timer path without ever touching completion first: logcat shows SourceFileManager's creation log firing before JavaDiagnosticProvider's "Analyzing:" log, confirming analyze()'s new ensureProjectReset() gate actually runs ahead of diagnostics. Live completion (which now holds the widened lock across the provider lookup) still returned correct, type-resolved results, naturally exercising concurrent getCompiler() + onContentChange() calls (one per keystroke) with no hang or deadlock. Closed the project again from the INITIALIZED state (this time javac really was built) to exercise shutdown()'s actual cleanup branch -- clean shutdown, process survived. Zero crashes/ANRs across the whole session.

🤖 Generated with Claude Code

…ntil a real .java file is touched

DefaultLanguageServerRegistry.onProjectInitialized dispatches setupWithProject
to every registered language server unconditionally, regardless of project
language. JavaLanguageServer.setupWithProject() referenced
JavaCompilerService.NO_MODULE_COMPILER and called SourceFileManager.clearCache(),
both of which trigger class-init that eagerly constructs real javac Context/
JavacFileManager machinery plus a full android.jar top-level-class scan --
on the first project open in the app's lifetime, Kotlin-only projects included.
shutdown() had the same problem in reverse, on every project close.

Same eager-load bug pattern ADFA-5010 fixed for the Kotlin Analysis API, and
independently confirmed and sized (openjdk.tools.javac ~2,238 classes, ~3.7MB)
while researching whether javac could get ADFA-5010's carrier-APK treatment.
This fix is scoped to just the eager-construction bug -- no DexClassLoader/
carrier-APK split; javac/jdk-compiler stay in the main dex, just constructed
lazily.

setupWithProject() now only stashes the workspace; the actual reset (destroy
NO_MODULE_COMPILER, clear file-manager/JAR-fs caches, index module classpaths)
is deferred to ensureProjectReset(), called from getCompiler() and
onContentChange() -- both already gated on DocumentUtils.isJavaFile(), so
this now only runs on genuine Java-file interaction. shutdown() skips its
javac-specific cleanup entirely if that never happened. Per-file LSP dispatch
methods (complete/findReferences/findDefinition/expandSelection/signatureHelp)
needed no changes: the editor's IDELanguage already resolves one language
server per file before calling any of them, so they were never the source of
the cross-language trigger.

Verified: :lsp:java unit tests pass, :app:assembleV8Debug builds clean.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough
  • Defer JavaCompilerService and SourceFileManager initialization until a real .java file is accessed.
  • Serialize compiler reset, initialization, use, and shutdown with a ReentrantLock.
  • Handle workspace changes during an in-progress reset.
  • Retry failed resets by preserving the pending workspace state.
  • Clear compiler and filesystem caches and preload module classpaths during project reset.
  • Restart analysis after a successful reset on the analysis path.
  • Skip javac cleanup during shutdown() when compiler initialization did not occur.
  • Keep Java LSP dispatch behavior unchanged.
  • Keep javac in the main dex. Do not add DexClassLoader or a carrier-APK split.
  • Java unit tests pass.
  • The V8 debug app builds successfully.
  • Manual verification confirms that Kotlin-only projects do not initialize javac and that Java files initialize the compiler and provide completion.
  • Risk: Deferred initialization and serialized lifecycle transitions require continued on-device verification.
  • Risk: Reset, compiler access, and provider use now share lifecycle locking. Review performance and deadlock behavior during concurrent analysis and document operations.

Walkthrough

JavaLanguageServer defers project reset and compiler initialization until compiler or Java document processing occurs. A lifecycle lock serializes reset, compiler lookup, document changes, and shutdown. Resets clear caches, preload module classpaths, restart analysis, and retry failures.

Changes

Java deferred initialization

Layer / File(s) Summary
Project reset state and initialization
lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt
The server records pending workspace state and performs synchronized resets. Resets destroy stale compiler state, clear caches, preload module classpaths, restart analysis, and preserve newer workspace state.
Interaction-triggered compiler setup
lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt
Analysis, compiler lookup, and Java document changes perform the deferred reset before compiler access or updates.
Shutdown lifecycle and formatting updates
lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt
Shutdown serializes cleanup and destroys compiler infrastructure only when initialized. formatCode behavior remains unchanged.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Workspace
  participant JavaLanguageServer
  participant Compiler
  participant Analysis
  Workspace->>JavaLanguageServer: setupWithProject
  JavaLanguageServer->>JavaLanguageServer: store pending workspace
  Workspace->>JavaLanguageServer: Java analysis or document change
  JavaLanguageServer->>Compiler: reset, clear caches, and preload classpaths
  JavaLanguageServer->>Analysis: restart analysis
  JavaLanguageServer->>Compiler: resolve or update compiler
Loading

Poem

A rabbit watched the compiler wait,
While workspace state stood at the gate.
A Java touch began the reset,
Caches cleared and paths were set.
Then analysis hopped back in.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely identifies the main change: deferring JavaCompilerService construction until a Java file is accessed.
Description check ✅ Passed The description accurately explains the lazy initialization, lifecycle synchronization, retry handling, testing, and scope of the changes.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch task/ADFA-5052-lazy-load-java-compiler

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt`:
- Around line 198-205: Serialize project reset, compiler access, and shutdown
through one lifecycle lock or explicit synchronized state. In
lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt
lines 198-205, keep reset-in-progress state private until compiler destruction,
cache cleanup, and module cache setup complete; lines 102-106 must represent
pending, resetting, initialized, and shutdown states under that mechanism; lines
144-151 must acquire it before compiler cleanup. Ensure concurrent requests and
shutdown block until reset finishes.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 120f2cf7-d110-4431-8ac1-a9620a9adb9c

📥 Commits

Reviewing files that changed from the base of the PR and between a80a8fa and 0d9356d.

📒 Files selected for processing (1)
  • lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt

Comment thread lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt Outdated
…ss and shutdown

The previous fix's synchronized(this) block only guarded the *decision* to
run ensureProjectReset() (claiming pendingWorkspace, flipping
javaCompilerInitialized to true) -- not the destroy/rebuild work that
followed, which ran unsynchronized. A concurrent getCompiler()/
onContentChange() call on another thread could see javaCompilerInitialized
already true and proceed to use JavaCompilerProvider/SourceFileManager while
the first thread was still mid-destroy or mid-rebuild. shutdown() didn't
synchronize on anything at all, so it could run its own destroy()/
clearCache() concurrently with an in-flight reset, racing two teardown/
rebuild sequences against each other.

Replaces the two ad-hoc @volatile fields with an explicit
PENDING/RESETTING/INITIALIZED/SHUTDOWN state machine guarded by a single
ReentrantLock (compilerLifecycleLock) held for the *entire* reset or
shutdown, not just the state transition. Concurrent callers now genuinely
block until an in-flight reset finishes (getCompiler()/onContentChange()
already route through ensureProjectReset(), which now can't return early
while another thread holds the lock), and shutdown() waits on the same
lock before deciding whether there's anything to tear down. setupWithProject()
also goes through the lock; if a new project arrives mid-reset, the
in-progress reset's own finally block detects the newer pendingWorkspace and
reverts to PENDING instead of incorrectly claiming INITIALIZED.

Verified: :lsp:java unit tests pass, :app:assembleV8Debug builds clean.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt`:
- Around line 109-113: Keep compiler lifecycle operations leased and
synchronized so shutdown cannot destroy a compiler while it is in use. In
JavaLanguageServer.kt:109-113, extend the lifecycle state with active-operation
tracking; at 153-164, make shutdown wait for active operations before destroying
the compiler and remain terminal; at 203-210, prevent setupWithProject() from
changing SHUTDOWN back to PENDING; at 355-359, acquire and use the compiler
through the lifecycle lease; and at 385-396, route onContentChange() through the
same guarded execution mechanism.
- Around line 252-261: Update the reset lifecycle flow around setupWithProject()
and its finally block so a failed reset never transitions compilerLifecycle to
INITIALIZED. Track whether the reset completed successfully, retain PENDING when
it fails or when pendingWorkspace exists, and narrowly catch known recoverable
reset failures to transition to the established explicit error state.
- Around line 214-264: Add focused tests covering the compiler lifecycle around
setupWithProject() and ensureProjectReset(): verify the initial reset is
deferred until Java interaction, a reset failure restores a recoverable
lifecycle state, a project queued during RESETTING becomes pending for a
subsequent reset, and shutdown() safely handles a pending reset before any Java
file interaction. Use existing test seams and lifecycle symbols rather than
relying only on compile tests.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8b58cba5-e4c7-439f-aa27-79459d354d36

📥 Commits

Reviewing files that changed from the base of the PR and between 0d9356d and ad09be1.

📒 Files selected for processing (1)
  • lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt

Comment on lines +109 to +113
private val compilerLifecycleLock = ReentrantLock()

// Guarded by compilerLifecycleLock.
private var pendingWorkspace: Workspace? = null
private var compilerLifecycle = CompilerLifecycle.PENDING

Copy link
Copy Markdown
Contributor

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() releases compilerLifecycleLock before getCompiler() returns a JavaCompilerService. shutdown() can then acquire the lock and destroy that service while completion, navigation, or document-change work still uses it. After shutdown, getCompiler() and onContentChange() also continue to access compiler state, and setupWithProject() can change SHUTDOWN back to PENDING.

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 transition SHUTDOWN to PENDING.
  • 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-L164
  • lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt#L203-L210
  • lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt#L355-L359
  • lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt#L385-L396
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt`
around lines 109 - 113, Keep compiler lifecycle operations leased and
synchronized so shutdown cannot destroy a compiler while it is in use. In
JavaLanguageServer.kt:109-113, extend the lifecycle state with active-operation
tracking; at 153-164, make shutdown wait for active operations before destroying
the compiler and remain terminal; at 203-210, prevent setupWithProject() from
changing SHUTDOWN back to PENDING; at 355-359, acquire and use the compiler
through the lifecycle lease; and at 385-396, route onContentChange() through the
same guarded execution mechanism.

Comment on lines +214 to 264
/**
* 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()
} finally {
// 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
}
}
SourceFileManager.forModule(subModule)
}
startOrRestartAnalyzeTimer()
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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/java

Repository: 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))
PY

Repository: 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/java

Repository: appdevforall/CodeOnTheGo

Length of output: 5311


Add lifecycle transition coverage.

The new setupWithProject() / ensureProjectReset() path and lifecycle guards are only exercised indirectly through compile tests. Add tests for the initial deferred reset, reset failure recovery, a new project enqueued during reset, and shutdown() called before the first Java-file interaction.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt`
around lines 214 - 264, Add focused tests covering the compiler lifecycle around
setupWithProject() and ensureProjectReset(): verify the initial reset is
deferred until Java interaction, a reset failure restores a recoverable
lifecycle state, a project queued during RESETTING becomes pending for a
subsequent reset, and shutdown() safely handles a pending reset before any Java
file interaction. Use existing test seams and lifecycle symbols rather than
relying only on compile tests.

Source: Coding guidelines

Comment thread lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt Outdated
…t-lock race

Three issues from /code-review high, verified against the current code:

1. ensureProjectReset() nulled pendingWorkspace before the try block, so any
   exception during destroy/rebuild (e.g. a bad submodule's classpath) still
   let the finally claim INITIALIZED -- silently treating a half-torn-down
   compiler as ready, with no retry, for the rest of the session. Now an
   exception re-queues the workspace, reverts to PENDING, and rethrows.

2. analyze() never called ensureProjectReset() at all. diagnosticProvider
   .analyze() builds its own JavaCompilerService directly, bypassing
   getCompiler(), and analysis is often the *first* real .java-file
   interaction (auto-triggered on file open, ahead of any completion
   request) -- so the R.jar/file-manager cache clear this reset performs
   could be skipped for an entire session, leaving diagnostics resolving
   against a stale previous project's classpath. Now gated the same way
   getCompiler()/onContentChange() already are.

3. getCompiler() and onContentChange() released compilerLifecycleLock as
   soon as ensureProjectReset() returned, then used JavaCompilerProvider
   unlocked -- a concurrent reset for a newer project could destroy() those
   compilers in the gap. Both now hold the lock across the reset and the
   subsequent provider lookup/use (safe: ReentrantLock is reentrant, so
   ensureProjectReset()'s own internal withLock nests without deadlocking).

Two other findings from the same pass were assessed and left as-is:
- shutdown() blocking on an in-flight reset with no cancellation is real but
  performance-only (no crash/corruption), requires disproportionate
  cancellation plumbing through SourceFileManager/JavaCompilerService for a
  narrow, bounded-cost edge case.
- KotlinLanguageServer's eager construction is a real observation about this
  branch's current state, but it's already fixed by the separate, not-yet-
  merged ADFA-5010 (PR #1635) -- out of scope here, not a gap in this PR.

Verified: :lsp:java unit tests pass, :app:assembleV8Debug builds clean.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant