Uh oh!
There was an error while loading. Please reload this page.
ADFA-5052: Defer eager JavaCompilerService construction until a real .java file is touched - #1637
ADFA-5052: Defer eager JavaCompilerService construction until a real .java file is touched#1637davidschachterADFA wants to merge 8 commits into
Conversation
…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.
There was a problem hiding this comment.
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.
📝 Walkthrough
WalkthroughJavaLanguageServer 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. ChangesJava deferred initialization
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
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
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
📒 Files selected for processing (1)
lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt
Uh oh!
There was an error while loading. Please reload this page.
…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.
There was a problem hiding this comment.
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
📒 Files selected for processing (1)
lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…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.
shutdown() destroyed the javac state but nothing kept it destroyed: a later setupWithProject() moved SHUTDOWN back to PENDING, and getCompiler() and onContentChange() would rebuild what had just been torn down. All four paths now treat SHUTDOWN as terminal -- getCompiler hands back NO_MODULE_COMPILER, the other two return, and setupWithProject logs why it is ignoring the project. Three tests cover the transitions that need no project fixture: shutdown before the first Java interaction, a project opened after shutdown, and a fresh server. The middle one fails without the guard. They need a seam, because every path returns NO_MODULE_COMPILER for its own reasons -- from outside, a test cannot tell a refusal after shutdown from a file with no module -- hence the @VisibleForTesting isShutDown. What this does not fix is the concurrent case. getCompiler() returns a service the caller uses after the lock is released, so a shutdown landing in that window can still destroy a compiler in use. Fixing that means leasing a compiler for the duration of an operation and draining in-flight leases before teardown, which converts all 18 call sites and needs its own testing. Filed as ADFA-5261 rather than appended to a PR about deferring construction that is already approved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ee gaps
The worst of these is mine and not in the code: my previous commit swept up
testing/resources/test-project/.cg/gradle-sync/{project,sync}.pb, replacing the
previous author's absolute paths with /home/david and a 12 MB binary with a
locally regenerated one. Nothing in this ticket needs them. Reverted to stage.
analyze() was the one javac entry point left without a shutdown guard. It calls
ensureProjectReset(), which no-ops after shutdown, and then goes on to
diagnosticProvider.analyze(), which constructs its own JavaCompilerService -- so
an analysis already in flight when shutdown() lands would rebuild what shutdown
had just destroyed, permanently, since SHUTDOWN is terminal. It is reachable:
analyzeSelected() launches on Dispatchers.Default, and a timer callback already
dispatched cannot be recalled.
ensureProjectReset() caught Exception, so an Error left the lifecycle stuck at
RESETTING with the workspace already discarded, and every later reset returned
early -- Java support dead for the session with no retry. The class init this
change defers is precisely what fails as an Error: OutOfMemoryError, or
ExceptionInInitializerError out of the android.jar scan. It catches Throwable
now.
setupWithProject() had stopped re-arming the analyze timer. A .java tab restored
from the tab cache opens before the sync posts this event, and AnalyzeTimer fires
once: that shot finds no module and returns NO_UPDATE, so the restored file
showed no diagnostics until the user typed. Arming the timer builds no javac.
The new test file gains the GPL header its siblings carry and an @after that
shuts down the servers it creates -- each registers on the global EventBus and
adds indexing services to a singleton, and Robolectric caches a sandbox across
test classes.
Recorded on ADFA-5261 rather than fixed here: returning the NO_MODULE_COMPILER
sentinel loads javac by class init, so the deferral leaks through every such
return -- including the SHUTDOWN branch -- and handleFailure() destroys every
compiler outside the lock. Both need the same signature change as the lease.
19 tests pass in :lsp:java.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Running :lsp:java's tests rewrites testing/resources/test-project/.cg/gradle-sync/
{project,sync}.pb with the local absolute paths -- which is how they reached the
previous commit in the first place, and why reverting them before running the
tests did not stick.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Summary
DefaultLanguageServerRegistry.onProjectInitializeddispatchessetupWithProjectto every registered language server unconditionally, regardless of the project's actual language.JavaLanguageServer.setupWithProject()referencedJavaCompilerService.NO_MODULE_COMPILERand calledSourceFileManager.clearCache(), both of which trigger class-init that eagerly constructs real javacContext/JavacFileManagermachinery plus a fullandroid.jartop-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 theWorkspace; the actual reset (destroyNO_MODULE_COMPILER, clear file-manager/JAR-fs caches, index module classpaths) moved to a newensureProjectReset(), called fromgetCompiler(),onContentChange(), andanalyze()-- all gated onDocumentUtils.isJavaFile(), so the reset now only runs on genuine Java-file interaction.shutdown()skips its javac-specific cleanup entirely if that reset never happened.complete/findReferences/findDefinition/expandSelection/signatureHelp) needed no changes: the editor'sIDELanguagealready resolves one language server per file before calling any of them, so they were never the source of the cross-language trigger.@Volatile+ narrowsynchronizedclaim with an explicitPENDING → RESETTING → INITIALIZED / SHUTDOWNstate machine guarded by oneReentrantLockheld for the entire reset or shutdown, not just the decision to run one.getCompiler()/onContentChange()hold the lock across both the reset and the subsequentJavaCompilerProviderlookup/use (reentrant, so no deadlock), closing a narrow window where a concurrent reset for a newer project coulddestroy()a compiler mid-use.PENDINGbefore rethrowing, instead of silently claimingINITIALIZEDfor a half-torn-down state with no retry path.analyze()(diagnostics) now also triggers the deferred reset:JavaDiagnosticProvider.analyze()builds its ownJavaCompilerServicedirectly, bypassinggetCompiler(), 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 highpass 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); andKotlinLanguageServerstill 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:testV8DebugUnitTestpasses:app:assembleV8Debugbuilds clean.javafiles) and its.ktfile, opened a second Kotlin-only project's file --greping the whole session's logcat forJavaCompilerService/SourceFileManager/openjdk.tools.javac/CacheFSInfoSingletonreturns zero hits throughout. Then opened a Java project's.javafile:SourceFileManager'sCreating source file manager instance for module: AndroidModule: :appfires 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..ktfile -> still zero javac-related log hits) and confirmedshutdown()doesn't hang/crash whencompilerLifecycleis stillPENDING. 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 showsSourceFileManager's creation log firing beforeJavaDiagnosticProvider's "Analyzing:" log, confirminganalyze()'s newensureProjectReset()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 concurrentgetCompiler()+onContentChange()calls (one per keystroke) with no hang or deadlock. Closed the project again from theINITIALIZEDstate (this time javac really was built) to exerciseshutdown()'s actual cleanup branch -- clean shutdown, process survived. Zero crashes/ANRs across the whole session.🤖 Generated with Claude Code