feat: demand-driven module prioritization for jdtls requests#47
Merged
aviadshiber merged 8 commits intomainfrom Apr 11, 2026
Merged
feat: demand-driven module prioritization for jdtls requests#47aviadshiber merged 8 commits intomainfrom
aviadshiber merged 8 commits intomainfrom
Conversation
The default 30s REQUEST_TIMEOUT was too short for jdtls to initialize on large Maven monorepos (e.g., products with hundreds of modules). jdtls needs to scan pom.xml files, resolve classpaths, and build its index before responding to the initialize handshake. Added INITIALIZE_TIMEOUT = 120s used only for the initialize request. Normal request timeout stays at 30s. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jdtls no longer starts at on_initialized. Instead: 1. on_initialized: lightweight PATH check only (check_available) 2. First didOpen: start jdtls scoped to the nearest Maven/Gradle module via find_module_root() — fast init (2-3s vs 30-120s for full monorepo) 3. Subsequent didOpen: add new modules incrementally via workspace/didChangeWorkspaceFolders (add_module_if_new) 4. Background: expand to full workspace root (expand_full_workspace) so cross-module references work for all files Key design decisions: - Non-blocking: lazy start runs as background task so on_did_open returns immediately with custom diagnostics (never delayed by jdtls cold-start) - asyncio.Lock prevents double-start from rapid didOpen calls - _start_failed flag prevents retry loops after failure - Data-dir hash based on original monorepo root (stable across restarts) - Notification queue: didOpen/didChange/didSave/didClose buffered during jdtls startup, replayed after initialization completes - INITIALIZE_TIMEOUT removed (30s REQUEST_TIMEOUT sufficient for single module) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…, helpers Correctness: - Fix didOpen during startup silently dropped: now queued like didChange/save - Use _lazy_start_fired flag to prevent TOCTOU race on task creation - Deep copy init_params before mutation (ws['workspaceFolders'] = True) - expand_full_workspace removes initial module folder to avoid double-indexing - Return early in add_module_if_new when from_fs_path returns None Performance: - Cap notification queue at 200 entries (drop oldest on overflow) - Cache find_module_root results with lru_cache (avoid repeated stat walks) Quality: - Extract _resolve_module_uri helper (DRY: was duplicated 3 times) - Extract _forward_or_queue helper (DRY: was duplicated in 3 handlers) - Extract _WORKSPACE_DID_CHANGE_FOLDERS constant Tests: - Assert flush/expand called in test_lazy_start_jdtls_success - Add test_lazy_start_jdtls_silent_failure (ensure_started returns False) - Convert test_queue_and_flush to async (fix deprecated get_event_loop) - Add test_queue_caps_at_max - Add test_expand_full_workspace_noop_when_not_available - Assert queue cleared in test_ensure_started_no_retry_after_failure Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… requests When an LSP operation (hover, goToDefinition, findReferences, completion, documentSymbol) targets a file whose module isn't loaded in jdtls yet: 1. add_module_if_new() sends workspace/didChangeWorkspaceFolders immediately 2. Forwards the request to jdtls 3. If jdtls returns null AND the module was just added → waits 3s → retries This makes tool usage itself drive what gets prioritized — no config needed. Agents calling goToDefinition on a new module will see it auto-load and the result come back after a single retry. Changes: - add_module_if_new returns bool (True if module was new) - Extract _ensure_module_and_forward helper (DRY across 5 handlers) - _MODULE_IMPORT_WAIT_SEC = 3.0 for retry delay Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…provements Correctness: - Fix notification drop window: _forward_or_queue now uses _lazy_start_fired (set synchronously) instead of _starting (set inside lock) as queue gate - Set _start_failed on any exception in ensure_started (not just start() failures) - Document lru_cache staleness as known limitation Performance: - Replace list.pop(0) with collections.deque(maxlen=200) for O(1) overflow Tests: - Mock asyncio.sleep in retry test (was waiting 3 real seconds) - Assert oldest surviving entry in queue cap test - Add test for expand_full_workspace removal logic (initial module removed) - Add cache_clear autouse fixture to TestFindModuleRoot - Add URI scheme assertion in ensure_started_with_build_file test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace flat set + fixed 3s sleep with a proper state machine: ModuleRegistry tracks UNKNOWN → ADDED → READY per module using: - dict[str, ModuleState] for O(1) hot-path lookups (is_ready) - dict[str, asyncio.Event] for adaptive waiting (no fixed sleep) _ensure_module_and_forward now: - READY: forward immediately (zero overhead — identity comparison) - UNKNOWN: add_module → mark_added before await (atomic) → try request → if null, wait_until_ready (adaptive) → retry - ADDED: try request → if null, wait_until_ready → retry On first success, mark_ready() wakes all waiting coroutines instantly via Event.set(). Subsequent requests skip waiting entirely. Fixes bug where ADDED modules returned null on second request without retrying (add_module_if_new returned False → no retry path). No locks needed: asyncio is single-threaded, dict mutations before await are atomic. asyncio.Event.wait() suspends without blocking. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix is_ready: use == instead of `is` for string comparison (portable) - Fix single-caller deadlock: always retry after wait (5s timeout), even on timeout the module may be ready from jdtls background import - Clean up Events on mark_ready: pop from _ready_events after set() - Add ModuleRegistry.clear() method for test cleanup - Replace internal _states/_ready_events access in tests with clear() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Demand-driven jdtls module loading with adaptive waiting via
ModuleRegistry.Architecture
ModuleRegistry (new)
Tracks per-module import state:
UNKNOWN → ADDED → READYdict[str, ModuleState]is_ready)dict[str, asyncio.Event]Request flow (
_ensure_module_and_forward)On first successful response,
mark_ready()firesEvent.set()— waking all waiting coroutines instantly. All subsequent requests skip waiting entirely.Key properties
awaitare atomicasyncio.Event.wait()with 30s timeout replaces the old 3sasyncio.sleepEvent.wait()suspends without blocking the event loopmark_added()before anyawaitprevents duplicatedidChangeWorkspaceFoldersAlso includes (from earlier commits)
didOpen→ nearest pom.xml)add_module_if_newon eachdidOpen)deque(maxlen=200))find_module_root(@lru_cache)Bug fixes
_lazy_start_firedand_starting_start_failednot set on unexpected exceptionsNumbers
🤖 Generated with Claude Code