From bf1a0465d2ff369475bc5b99f033bc21aecb3a69 Mon Sep 17 00:00:00 2001 From: Aviad Shiber Date: Fri, 10 Apr 2026 21:38:26 +0300 Subject: [PATCH 1/7] fix: increase jdtls initialize timeout to 120s for large monorepos 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) --- pyproject.toml | 2 +- src/java_functional_lsp/__init__.py | 2 +- src/java_functional_lsp/proxy.py | 5 +++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index ce82353..270de4d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "java-functional-lsp" -version = "0.7.3" +version = "0.7.4" description = "Java LSP server enforcing functional programming best practices — null safety, immutability, no exceptions" readme = "README.md" license = { text = "MIT" } diff --git a/src/java_functional_lsp/__init__.py b/src/java_functional_lsp/__init__.py index d4fd9aa..f1a7966 100644 --- a/src/java_functional_lsp/__init__.py +++ b/src/java_functional_lsp/__init__.py @@ -1,3 +1,3 @@ """java-functional-lsp: A Java LSP server enforcing functional programming best practices.""" -__version__ = "0.7.3" +__version__ = "0.7.4" diff --git a/src/java_functional_lsp/proxy.py b/src/java_functional_lsp/proxy.py index 23a3117..0683251 100644 --- a/src/java_functional_lsp/proxy.py +++ b/src/java_functional_lsp/proxy.py @@ -17,7 +17,8 @@ logger = logging.getLogger(__name__) -REQUEST_TIMEOUT = 30.0 # seconds +REQUEST_TIMEOUT = 30.0 # seconds — per-request timeout for normal operations +INITIALIZE_TIMEOUT = 120.0 # seconds — jdtls initialize can be slow on large monorepos DEFAULT_JVM_MAX_HEAP = "4g" _STDERR_LINE_MAX = 1000 @@ -392,7 +393,7 @@ async def start(self, init_params: dict[str, Any]) -> bool: self._stderr_task = asyncio.create_task(self._stderr_reader(self._process.stderr)) # Send initialize request - result = await self.send_request("initialize", init_params) + result = await self.send_request("initialize", init_params, timeout=INITIALIZE_TIMEOUT) if result is None: logger.error("jdtls initialize request failed or timed out") await self.stop() From 1430c082b675b5bd3f9f28fb93c42b3f06a4bd52 Mon Sep 17 00:00:00 2001 From: Aviad Shiber Date: Fri, 10 Apr 2026 22:31:53 +0300 Subject: [PATCH 2/7] feat: lazy module-scoped jdtls initialization with incremental loading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/java_functional_lsp/proxy.py | 175 +++++++++++++++++++--- src/java_functional_lsp/server.py | 83 +++++++++-- tests/test_proxy.py | 238 ++++++++++++++++++++++++++++++ tests/test_server.py | 40 +++++ 4 files changed, 500 insertions(+), 36 deletions(-) diff --git a/src/java_functional_lsp/proxy.py b/src/java_functional_lsp/proxy.py index 0683251..08a1032 100644 --- a/src/java_functional_lsp/proxy.py +++ b/src/java_functional_lsp/proxy.py @@ -17,8 +17,7 @@ logger = logging.getLogger(__name__) -REQUEST_TIMEOUT = 30.0 # seconds — per-request timeout for normal operations -INITIALIZE_TIMEOUT = 120.0 # seconds — jdtls initialize can be slow on large monorepos +REQUEST_TIMEOUT = 30.0 # seconds DEFAULT_JVM_MAX_HEAP = "4g" _STDERR_LINE_MAX = 1000 @@ -312,6 +311,26 @@ async def read_message(reader: asyncio.StreamReader) -> dict[str, Any] | None: return None +_BUILD_FILES = ("pom.xml", "build.gradle", "build.gradle.kts") + + +def find_module_root(file_path: str) -> str | None: + """Walk up from *file_path* to find the nearest directory containing a build file. + + Returns the directory path, or ``None`` if no build file is found before + reaching the filesystem root. Used to scope jdtls initialization to a + single Maven/Gradle module for fast startup. + """ + current = Path(file_path).parent + while True: + if any((current / bf).is_file() for bf in _BUILD_FILES): + return str(current) + parent = current.parent + if parent == current: + return None + current = parent + + class JdtlsProxy: """Manages a jdtls subprocess and provides async request/notification forwarding.""" @@ -325,6 +344,15 @@ def __init__(self, on_diagnostics: Callable[[str, list[Any]], None] | None = Non self._on_diagnostics = on_diagnostics self._available = False self._jdtls_capabilities: dict[str, Any] = {} + # Lazy-start state + self._start_lock = asyncio.Lock() + self._starting = False + self._start_failed = False + self._jdtls_on_path = False + self._queued_notifications: list[tuple[str, Any]] = [] + self._original_root_uri: str | None = None + self._added_module_uris: set[str] = set() + self._workspace_expanded = False @property def is_available(self) -> bool: @@ -340,31 +368,53 @@ def get_cached_diagnostics(self, uri: str) -> list[Any]: """Get the latest jdtls diagnostics for a URI.""" return list(self._diagnostics_cache.get(uri, [])) - async def start(self, init_params: dict[str, Any]) -> bool: - """Start jdtls subprocess and initialize it.""" + def check_available(self) -> bool: + """Check if jdtls is on PATH (lightweight, no subprocess started).""" + self._jdtls_on_path = shutil.which("jdtls") is not None + if not self._jdtls_on_path: + logger.warning("jdtls not found on PATH — running in standalone mode (custom rules only)") + return self._jdtls_on_path + + async def start(self, init_params: dict[str, Any], *, module_root_uri: str | None = None) -> bool: + """Start jdtls subprocess and initialize it. + + If *module_root_uri* is provided, jdtls is scoped to that module for + fast startup. The data-directory hash is always based on the original + workspace root (from init_params) so the index persists across restarts. + """ jdtls_path = shutil.which("jdtls") if not jdtls_path: - logger.warning("jdtls not found on PATH — running in standalone mode (custom rules only)") return False - # jdtls requires a -data directory for workspace metadata (index, classpath, build state). - # Use ~/.cache/jdtls-data/ so it persists across reboots and LSP restarts. - # Fallback order mirrors LSP spec: rootUri → rootPath → cwd. - root_uri = init_params.get("rootUri") or init_params.get("rootPath") or str(Path.cwd()) - workspace_hash = hashlib.sha256(root_uri.encode()).hexdigest()[:12] + # Data-dir hash based on original workspace root (stable across module changes). + self._original_root_uri = init_params.get("rootUri") or init_params.get("rootPath") or str(Path.cwd()) + workspace_hash = hashlib.sha256(self._original_root_uri.encode()).hexdigest()[:12] data_dir = Path.home() / ".cache" / "jdtls-data" / workspace_hash data_dir.mkdir(parents=True, exist_ok=True) - # Build a clean environment for jdtls: detect Java 21+ and set JAVA_HOME - # explicitly, or strip JAVA_HOME if the inherited value points at an older - # Java (e.g. an IDE launched us with a project SDK of Java 8). Without this, - # jdtls 1.57+ fails with "jdtls requires at least Java 21" during its - # Python launcher's version check. - # - # build_jdtls_env() issues several blocking subprocess calls (java -version, - # /usr/libexec/java_home) to detect a suitable JDK. Run it in a thread pool - # so those calls don't block the asyncio event loop — the IDE's LSP handshake - # messages would otherwise stall for up to a few seconds during startup. + # If module_root_uri is provided, scope jdtls to that module. + effective_params = dict(init_params) + effective_root_uri = module_root_uri or self._original_root_uri + if module_root_uri: + effective_params["rootUri"] = module_root_uri + from pygls.uris import to_fs_path + + effective_params["rootPath"] = to_fs_path(module_root_uri) + logger.info( + "jdtls: scoping to module %s (full root: %s)", + _redact_path(module_root_uri), + _redact_path(self._original_root_uri), + ) + + # Inject workspaceFolders capability for later expansion. + caps = effective_params.setdefault("capabilities", {}) + ws = caps.setdefault("workspace", {}) + ws["workspaceFolders"] = True + + # Track the initial module as already loaded. + self._added_module_uris.add(effective_root_uri) + + # Build a clean environment for jdtls. loop = asyncio.get_running_loop() jdtls_env = await loop.run_in_executor(None, build_jdtls_env) @@ -386,14 +436,12 @@ async def start(self, init_params: dict[str, Any]) -> bool: _redact_path(jdtls_env.get("JAVA_HOME")), ) - # Start background readers for stdout (JSON-RPC) and stderr (diagnostics/errors) assert self._process.stdout is not None self._reader_task = asyncio.create_task(self._reader_loop(self._process.stdout)) if self._process.stderr is not None: self._stderr_task = asyncio.create_task(self._stderr_reader(self._process.stderr)) - # Send initialize request - result = await self.send_request("initialize", init_params, timeout=INITIALIZE_TIMEOUT) + result = await self.send_request("initialize", effective_params) if result is None: logger.error("jdtls initialize request failed or timed out") await self.stop() @@ -402,7 +450,6 @@ async def start(self, init_params: dict[str, Any]) -> bool: self._jdtls_capabilities = result.get("capabilities", {}) logger.info("jdtls initialized (capabilities: %s)", list(self._jdtls_capabilities.keys())) - # Send initialized notification await self.send_notification("initialized", {}) self._available = True return True @@ -411,6 +458,86 @@ async def start(self, init_params: dict[str, Any]) -> bool: logger.error("Failed to start jdtls: %s", e) return False + async def ensure_started(self, init_params: dict[str, Any], file_uri: str) -> bool: + """Start jdtls lazily, scoped to the module containing *file_uri*. + + Thread-safe: uses asyncio.Lock to prevent double-start from rapid + didOpen calls. Sets ``_start_failed`` on failure to prevent retries. + """ + if self._available: + return True + if self._start_failed or not self._jdtls_on_path: + return False + + async with self._start_lock: + if self._available: + return True + + self._starting = True + try: + from pygls.uris import from_fs_path, to_fs_path + + file_path = to_fs_path(file_uri) or file_uri + module_root = find_module_root(file_path) + module_uri = from_fs_path(module_root) if module_root else None + + started = await self.start(init_params, module_root_uri=module_uri) + if not started: + self._start_failed = True + self._queued_notifications.clear() + return started + finally: + self._starting = False + + def queue_notification(self, method: str, params: Any) -> None: + """Buffer a notification for replay after jdtls starts.""" + self._queued_notifications.append((method, params)) + + async def flush_queued_notifications(self) -> None: + """Send all queued notifications to jdtls.""" + queue, self._queued_notifications = self._queued_notifications, [] + for method, params in queue: + await self.send_notification(method, params) + + async def add_module_if_new(self, file_uri: str) -> None: + """Add the module containing *file_uri* to jdtls if not already added.""" + if not self._available: + return + from pygls.uris import from_fs_path, to_fs_path + + file_path = to_fs_path(file_uri) or file_uri + module_root = find_module_root(file_path) + if module_root is None: + return + module_uri = from_fs_path(module_root) or file_uri + if module_uri in self._added_module_uris: + return + self._added_module_uris.add(module_uri) + logger.info("jdtls: adding module %s", _redact_path(module_root)) + await self.send_notification( + "workspace/didChangeWorkspaceFolders", + {"event": {"added": [{"uri": module_uri, "name": Path(module_root).name}], "removed": []}}, + ) + + async def expand_full_workspace(self) -> None: + """Expand jdtls workspace to the full monorepo root (background task).""" + if self._workspace_expanded or not self._available or not self._original_root_uri: + return + from pygls.uris import from_fs_path, to_fs_path + + root_path = to_fs_path(self._original_root_uri) or self._original_root_uri + root_uri = from_fs_path(root_path) or self._original_root_uri + if root_uri in self._added_module_uris: + self._workspace_expanded = True + return + self._added_module_uris.add(root_uri) + logger.info("jdtls: expanding to full workspace %s", _redact_path(root_path)) + await self.send_notification( + "workspace/didChangeWorkspaceFolders", + {"event": {"added": [{"uri": root_uri, "name": Path(root_path).name}], "removed": []}}, + ) + self._workspace_expanded = True + async def stop(self) -> None: """Shutdown jdtls subprocess gracefully.""" self._available = False diff --git a/src/java_functional_lsp/server.py b/src/java_functional_lsp/server.py index 648535b..adcf372 100644 --- a/src/java_functional_lsp/server.py +++ b/src/java_functional_lsp/server.py @@ -77,6 +77,8 @@ def _on_jdtls_diagnostics(self, uri: str, diagnostics: list[Any]) -> None: # Debounce state for didChange events (only affects human typing in IDEs, not agents) _pending: dict[str, asyncio.Task[None]] = {} +# Background tasks (prevent GC of fire-and-forget tasks) +_bg_tasks: set[asyncio.Task[None]] = set() _DEBOUNCE_SECONDS = 0.15 @@ -242,17 +244,15 @@ def on_initialize(params: lsp.InitializeParams) -> lsp.InitializeResult: @server.feature(lsp.INITIALIZED) async def on_initialized(params: lsp.InitializedParams) -> None: - """Start jdtls proxy after initialization.""" + """Check jdtls availability; actual start deferred to first didOpen.""" logger.info( "java-functional-lsp initialized (rules: %s)", list(server._config.get("rules", {}).keys()) or "all defaults", ) - started = await server._proxy.start(server._init_params) - if started: - logger.info("jdtls proxy active — full Java language support enabled") - await _register_jdtls_capabilities() + if server._proxy.check_available(): + logger.info("jdtls found on PATH — will start lazily on first file open") else: - logger.info("jdtls proxy unavailable — running with custom rules only") + logger.info("jdtls not on PATH — running with custom rules only") _JAVA_SELECTOR = [lsp.TextDocumentFilterLanguage(language="java")] @@ -333,18 +333,39 @@ async def _deferred_validate(uri: str) -> None: @server.feature(lsp.TEXT_DOCUMENT_DID_OPEN) async def on_did_open(params: lsp.DidOpenTextDocumentParams) -> None: - """Forward to jdtls and analyze immediately.""" + """Forward to jdtls (starting lazily if needed) and analyze immediately. + + Custom diagnostics always publish immediately regardless of jdtls state. + jdtls startup is non-blocking — it runs in the background so the first + didOpen response isn't delayed by jdtls cold-start. + """ + uri = params.text_document.uri + serialized = _serialize_params(params) + if server._proxy.is_available: - await server._proxy.send_notification("textDocument/didOpen", _serialize_params(params)) - _analyze_and_publish(params.text_document.uri) + # Fast path: jdtls running. Forward didOpen + add module if new. + await server._proxy.send_notification("textDocument/didOpen", serialized) + await server._proxy.add_module_if_new(uri) + elif server._proxy._jdtls_on_path and not server._proxy._start_failed and not server._proxy._starting: + # First file: kick off lazy start in background. + server._proxy.queue_notification("textDocument/didOpen", serialized) + task = asyncio.create_task(_lazy_start_jdtls(uri)) + _bg_tasks.add(task) + task.add_done_callback(_bg_tasks.discard) + + # Custom diagnostics always publish immediately — never blocked by jdtls. + _analyze_and_publish(uri) @server.feature(lsp.TEXT_DOCUMENT_DID_CHANGE) async def on_did_change(params: lsp.DidChangeTextDocumentParams) -> None: """Forward to jdtls and schedule debounced re-analysis.""" uri = params.text_document.uri + serialized = _serialize_params(params) if server._proxy.is_available: - await server._proxy.send_notification("textDocument/didChange", _serialize_params(params)) + await server._proxy.send_notification("textDocument/didChange", serialized) + elif server._proxy._starting: + server._proxy.queue_notification("textDocument/didChange", serialized) # Cancel pending validation, schedule new one (150ms debounce for IDE typing) if uri in _pending: _pending[uri].cancel() @@ -354,8 +375,11 @@ async def on_did_change(params: lsp.DidChangeTextDocumentParams) -> None: @server.feature(lsp.TEXT_DOCUMENT_DID_SAVE) async def on_did_save(params: lsp.DidSaveTextDocumentParams) -> None: """Forward to jdtls and re-analyze immediately (no debounce on save).""" + serialized = _serialize_params(params) if server._proxy.is_available: - await server._proxy.send_notification("textDocument/didSave", _serialize_params(params)) + await server._proxy.send_notification("textDocument/didSave", serialized) + elif server._proxy._starting: + server._proxy.queue_notification("textDocument/didSave", serialized) _analyze_and_publish(params.text_document.uri) @@ -368,8 +392,43 @@ async def on_did_close(params: lsp.DidCloseTextDocumentParams) -> None: del _pending[uri] # Clear diagnostics for the closed document (LSP best practice) server.text_document_publish_diagnostics(lsp.PublishDiagnosticsParams(uri=uri, diagnostics=[])) + serialized = _serialize_params(params) if server._proxy.is_available: - await server._proxy.send_notification("textDocument/didClose", _serialize_params(params)) + await server._proxy.send_notification("textDocument/didClose", serialized) + elif server._proxy._starting: + server._proxy.queue_notification("textDocument/didClose", serialized) + + +async def _lazy_start_jdtls(file_uri: str) -> None: + """Background task: start jdtls scoped to the module containing *file_uri*. + + Runs in the background so ``on_did_open`` returns immediately with custom + diagnostics. After jdtls initializes, registers capabilities, flushes + queued notifications, and schedules workspace expansion. + """ + try: + started = await server._proxy.ensure_started(server._init_params, file_uri) + if started: + logger.info("jdtls proxy active — full Java language support enabled") + await _register_jdtls_capabilities() + await server._proxy.flush_queued_notifications() + await _expand_workspace_background() + except Exception: + logger.warning("jdtls lazy start failed", exc_info=True) + + +async def _expand_workspace_background() -> None: + """Background task: expand jdtls workspace to full monorepo root. + + Runs after jdtls finishes initializing with the first module scope. + The user's actively-opened modules are loaded immediately via + ``add_module_if_new()`` in ``on_did_open``; this adds the full root + so cross-module references for unopened files also work. + """ + try: + await server._proxy.expand_full_workspace() + except Exception: + logger.warning("Failed to expand jdtls workspace", exc_info=True) # --- jdtls passthrough handlers (registered dynamically, NOT at module level) --- diff --git a/tests/test_proxy.py b/tests/test_proxy.py index 446245b..64c5ea0 100644 --- a/tests/test_proxy.py +++ b/tests/test_proxy.py @@ -857,3 +857,241 @@ async def fake_send_request(*_args: Any, **_kwargs: Any) -> None: assert ok is False # The crucial assertion: env= was passed through to the subprocess call. assert captured["env"] == sentinel_env + + +class TestFindModuleRoot: + """Tests for find_module_root — build-file detection for module scoping.""" + + def test_finds_pom_xml(self, tmp_path: Any) -> None: + from java_functional_lsp.proxy import find_module_root + + (tmp_path / "pom.xml").touch() + java_file = tmp_path / "src" / "Main.java" + java_file.parent.mkdir() + java_file.touch() + assert find_module_root(str(java_file)) == str(tmp_path) + + def test_finds_build_gradle(self, tmp_path: Any) -> None: + from java_functional_lsp.proxy import find_module_root + + (tmp_path / "build.gradle").touch() + java_file = tmp_path / "src" / "Main.java" + java_file.parent.mkdir() + java_file.touch() + assert find_module_root(str(java_file)) == str(tmp_path) + + def test_finds_build_gradle_kts(self, tmp_path: Any) -> None: + from java_functional_lsp.proxy import find_module_root + + (tmp_path / "build.gradle.kts").touch() + java_file = tmp_path / "src" / "Main.java" + java_file.parent.mkdir() + java_file.touch() + assert find_module_root(str(java_file)) == str(tmp_path) + + def test_finds_nearest_not_parent(self, tmp_path: Any) -> None: + """Nested modules: should find the innermost module root.""" + from java_functional_lsp.proxy import find_module_root + + (tmp_path / "pom.xml").touch() # parent module + child = tmp_path / "child-module" + child.mkdir() + (child / "pom.xml").touch() # child module + java_file = child / "src" / "Main.java" + java_file.parent.mkdir() + java_file.touch() + assert find_module_root(str(java_file)) == str(child) + + def test_returns_none_when_no_build_file(self, tmp_path: Any) -> None: + from java_functional_lsp.proxy import find_module_root + + java_file = tmp_path / "src" / "Main.java" + java_file.parent.mkdir() + java_file.touch() + assert find_module_root(str(java_file)) is None + + +class TestLazyStart: + """Tests for lazy-start proxy features.""" + + def test_check_available_true(self) -> None: + from java_functional_lsp.proxy import JdtlsProxy + + proxy = JdtlsProxy() + with patch("java_functional_lsp.proxy.shutil.which", return_value="/usr/bin/jdtls"): + assert proxy.check_available() is True + assert proxy._jdtls_on_path is True + + def test_check_available_false(self) -> None: + from java_functional_lsp.proxy import JdtlsProxy + + proxy = JdtlsProxy() + with patch("java_functional_lsp.proxy.shutil.which", return_value=None): + assert proxy.check_available() is False + assert proxy._jdtls_on_path is False + + def test_queue_and_flush(self) -> None: + from java_functional_lsp.proxy import JdtlsProxy + + proxy = JdtlsProxy() + proxy.queue_notification("textDocument/didOpen", {"uri": "a"}) + proxy.queue_notification("textDocument/didChange", {"uri": "b"}) + assert len(proxy._queued_notifications) == 2 + + # flush should clear the queue + flushed: list[tuple[str, Any]] = [] + + async def mock_send(method: str, params: Any) -> None: + flushed.append((method, params)) + + proxy.send_notification = mock_send # type: ignore[assignment] + asyncio.get_event_loop().run_until_complete(proxy.flush_queued_notifications()) + assert len(flushed) == 2 + assert flushed[0] == ("textDocument/didOpen", {"uri": "a"}) + assert flushed[1] == ("textDocument/didChange", {"uri": "b"}) + assert len(proxy._queued_notifications) == 0 + + async def test_ensure_started_no_retry_after_failure(self) -> None: + from unittest.mock import AsyncMock + + from java_functional_lsp.proxy import JdtlsProxy + + proxy = JdtlsProxy() + proxy._jdtls_on_path = True + proxy.start = AsyncMock(return_value=False) # type: ignore[assignment] + result = await proxy.ensure_started({"rootUri": "file:///tmp"}, "file:///tmp/F.java") + assert result is False + assert proxy._start_failed is True + # Second call should return immediately without calling start() + proxy.start.reset_mock() # type: ignore[attr-defined] + result2 = await proxy.ensure_started({"rootUri": "file:///tmp"}, "file:///tmp/F.java") + assert result2 is False + proxy.start.assert_not_called() # type: ignore[attr-defined] + + async def test_add_module_if_new_sends_notification(self) -> None: + from unittest.mock import AsyncMock + + from java_functional_lsp.proxy import JdtlsProxy + + proxy = JdtlsProxy() + proxy._available = True + proxy.send_notification = AsyncMock() # type: ignore[assignment] + # Create a tmp dir with pom.xml + import tempfile + from pathlib import Path + + with tempfile.TemporaryDirectory() as td: + (Path(td) / "pom.xml").touch() + java_file = Path(td) / "src" / "Main.java" + java_file.parent.mkdir() + java_file.touch() + uri = java_file.as_uri() + await proxy.add_module_if_new(uri) + proxy.send_notification.assert_called_once() # type: ignore[attr-defined] + call_args = proxy.send_notification.call_args # type: ignore[attr-defined] + assert call_args[0][0] == "workspace/didChangeWorkspaceFolders" + + async def test_add_module_if_new_skips_duplicate(self) -> None: + from unittest.mock import AsyncMock + + from java_functional_lsp.proxy import JdtlsProxy + + proxy = JdtlsProxy() + proxy._available = True + proxy.send_notification = AsyncMock() # type: ignore[assignment] + import tempfile + from pathlib import Path + + with tempfile.TemporaryDirectory() as td: + (Path(td) / "pom.xml").touch() + java_file = Path(td) / "src" / "Main.java" + java_file.parent.mkdir() + java_file.touch() + uri = java_file.as_uri() + await proxy.add_module_if_new(uri) + await proxy.add_module_if_new(uri) # duplicate + assert proxy.send_notification.call_count == 1 # type: ignore[attr-defined] + + async def test_expand_full_workspace_sends_notification(self) -> None: + from unittest.mock import AsyncMock + + from java_functional_lsp.proxy import JdtlsProxy + + proxy = JdtlsProxy() + proxy._available = True + proxy._original_root_uri = "file:///workspace/monorepo" + proxy.send_notification = AsyncMock() # type: ignore[assignment] + await proxy.expand_full_workspace() + proxy.send_notification.assert_called_once() # type: ignore[attr-defined] + assert proxy._workspace_expanded is True + + async def test_expand_full_workspace_noop_when_already_added(self) -> None: + from unittest.mock import AsyncMock + + from java_functional_lsp.proxy import JdtlsProxy + + proxy = JdtlsProxy() + proxy._available = True + proxy._original_root_uri = "file:///workspace/monorepo" + proxy._added_module_uris.add("file:///workspace/monorepo") + proxy.send_notification = AsyncMock() # type: ignore[assignment] + await proxy.expand_full_workspace() + proxy.send_notification.assert_not_called() # type: ignore[attr-defined] + assert proxy._workspace_expanded is True + + async def test_ensure_started_no_build_file(self) -> None: + """ensure_started with no build file should pass module_root_uri=None.""" + from java_functional_lsp.proxy import JdtlsProxy + + proxy = JdtlsProxy() + proxy._jdtls_on_path = True + captured: dict[str, Any] = {} + + async def capturing_start(params: Any, *, module_root_uri: str | None = None) -> bool: + captured["module_root_uri"] = module_root_uri + return False + + proxy.start = capturing_start # type: ignore[assignment] + await proxy.ensure_started( + {"rootUri": "file:///monorepo", "capabilities": {}}, + "file:///nonexistent/src/Main.java", + ) + assert captured["module_root_uri"] is None + + async def test_ensure_started_with_build_file(self, tmp_path: Any) -> None: + """ensure_started should find module root and pass it to start().""" + from java_functional_lsp.proxy import JdtlsProxy + + proxy = JdtlsProxy() + proxy._jdtls_on_path = True + (tmp_path / "pom.xml").touch() + java_file = tmp_path / "src" / "Main.java" + java_file.parent.mkdir() + java_file.touch() + + captured: dict[str, Any] = {} + + async def capturing_start(params: Any, *, module_root_uri: str | None = None) -> bool: + captured["module_root_uri"] = module_root_uri + return False + + proxy.start = capturing_start # type: ignore[assignment] + await proxy.ensure_started( + {"rootUri": "file:///monorepo", "capabilities": {}}, + java_file.as_uri(), + ) + assert captured["module_root_uri"] is not None + assert str(tmp_path) in captured["module_root_uri"] + + def test_data_dir_hash_uses_original_root(self) -> None: + """Data-dir hash should be based on original rootUri, not module root.""" + import hashlib + + # The hash is computed from the original rootUri, not the module root. + # Verify these produce different hashes, confirming start() must use + # the original root for stability. + root = "file:///workspace/monorepo" + expected_hash = hashlib.sha256(root.encode()).hexdigest()[:12] + module_root = "file:///workspace/monorepo/module-a" + module_hash = hashlib.sha256(module_root.encode()).hexdigest()[:12] + assert expected_hash != module_hash diff --git a/tests/test_server.py b/tests/test_server.py index 9d2f441..1eabf4f 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -414,6 +414,46 @@ async def test_register_jdtls_capabilities_idempotent(self) -> None: srv_mod._jdtls_capabilities_registered = old_flag mock_reg.assert_not_called() + async def test_lazy_start_jdtls_success(self, caplog: Any) -> None: + """_lazy_start_jdtls logs success and registers capabilities.""" + import logging + from unittest.mock import AsyncMock, MagicMock, patch + + import java_functional_lsp.server as srv_mod + from java_functional_lsp.server import _lazy_start_jdtls + from java_functional_lsp.server import server as srv + + old_flag = srv_mod._jdtls_capabilities_registered + srv_mod._jdtls_capabilities_registered = False + try: + with ( + caplog.at_level(logging.INFO, logger="java_functional_lsp.server"), + patch.object(srv._proxy, "ensure_started", AsyncMock(return_value=True)), + patch.object(srv._proxy, "flush_queued_notifications", AsyncMock()), + patch.object(srv._proxy, "expand_full_workspace", AsyncMock()), + patch.object(srv, "feature", MagicMock(return_value=lambda fn: fn)), + patch.object(srv, "client_register_capability_async", AsyncMock()), + ): + await _lazy_start_jdtls("file:///test/F.java") + finally: + srv_mod._jdtls_capabilities_registered = old_flag + assert any("jdtls proxy active" in r.getMessage() for r in caplog.records) + + async def test_lazy_start_jdtls_failure_logged(self, caplog: Any) -> None: + """_lazy_start_jdtls logs warning on failure.""" + import logging + from unittest.mock import AsyncMock, patch + + from java_functional_lsp.server import _lazy_start_jdtls + from java_functional_lsp.server import server as srv + + with ( + caplog.at_level(logging.WARNING, logger="java_functional_lsp.server"), + patch.object(srv._proxy, "ensure_started", AsyncMock(side_effect=Exception("boom"))), + ): + await _lazy_start_jdtls("file:///test/F.java") + assert any("lazy start failed" in r.getMessage() for r in caplog.records) + def test_serialize_params_camelcase(self) -> None: from java_functional_lsp.server import _serialize_params From 866882cb202f821880f5a056a64a009647e072f3 Mon Sep 17 00:00:00 2001 From: Aviad Shiber Date: Fri, 10 Apr 2026 22:43:28 +0300 Subject: [PATCH 3/7] =?UTF-8?q?fix:=20address=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20didOpen=20queue,=20deepcopy,=20bounded=20queue,=20h?= =?UTF-8?q?elpers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/java_functional_lsp/proxy.py | 106 ++++++++++++++++++++---------- src/java_functional_lsp/server.py | 41 ++++++------ tests/test_proxy.py | 30 ++++++++- tests/test_server.py | 30 +++++++-- uv.lock | 2 +- 5 files changed, 146 insertions(+), 63 deletions(-) diff --git a/src/java_functional_lsp/proxy.py b/src/java_functional_lsp/proxy.py index 08a1032..d5967a5 100644 --- a/src/java_functional_lsp/proxy.py +++ b/src/java_functional_lsp/proxy.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +import copy import hashlib import json import logging @@ -12,6 +13,7 @@ import shutil import subprocess from collections.abc import Callable, Mapping +from functools import lru_cache from pathlib import Path from typing import Any @@ -312,16 +314,14 @@ async def read_message(reader: asyncio.StreamReader) -> dict[str, Any] | None: _BUILD_FILES = ("pom.xml", "build.gradle", "build.gradle.kts") +_WORKSPACE_DID_CHANGE_FOLDERS = "workspace/didChangeWorkspaceFolders" +_MAX_QUEUED_NOTIFICATIONS = 200 -def find_module_root(file_path: str) -> str | None: - """Walk up from *file_path* to find the nearest directory containing a build file. - - Returns the directory path, or ``None`` if no build file is found before - reaching the filesystem root. Used to scope jdtls initialization to a - single Maven/Gradle module for fast startup. - """ - current = Path(file_path).parent +@lru_cache(maxsize=256) +def _cached_module_root(dir_path: str) -> str | None: + """Cached walk up from *dir_path* to find nearest directory with a build file.""" + current = Path(dir_path) while True: if any((current / bf).is_file() for bf in _BUILD_FILES): return str(current) @@ -331,6 +331,29 @@ def find_module_root(file_path: str) -> str | None: current = parent +def find_module_root(file_path: str) -> str | None: + """Walk up from *file_path* to find the nearest directory containing a build file. + + Returns the directory path, or ``None`` if no build file is found before + reaching the filesystem root. Results are cached by parent directory. + """ + return _cached_module_root(str(Path(file_path).parent)) + + +def _resolve_module_uri(file_uri: str) -> str | None: + """Convert a file URI to the URI of its nearest module root, or None.""" + from pygls.uris import from_fs_path, to_fs_path + + file_path = to_fs_path(file_uri) + if not file_path: + return None + module_root = find_module_root(file_path) + if module_root is None: + return None + module_uri = from_fs_path(module_root) + return module_uri or None + + class JdtlsProxy: """Manages a jdtls subprocess and provides async request/notification forwarding.""" @@ -349,8 +372,10 @@ def __init__(self, on_diagnostics: Callable[[str, list[Any]], None] | None = Non self._starting = False self._start_failed = False self._jdtls_on_path = False + self._lazy_start_fired = False self._queued_notifications: list[tuple[str, Any]] = [] self._original_root_uri: str | None = None + self._initial_module_uri: str | None = None self._added_module_uris: set[str] = set() self._workspace_expanded = False @@ -387,14 +412,15 @@ async def start(self, init_params: dict[str, Any], *, module_root_uri: str | Non return False # Data-dir hash based on original workspace root (stable across module changes). - self._original_root_uri = init_params.get("rootUri") or init_params.get("rootPath") or str(Path.cwd()) - workspace_hash = hashlib.sha256(self._original_root_uri.encode()).hexdigest()[:12] + original_root: str = init_params.get("rootUri") or init_params.get("rootPath") or str(Path.cwd()) + self._original_root_uri = original_root + workspace_hash = hashlib.sha256(original_root.encode()).hexdigest()[:12] data_dir = Path.home() / ".cache" / "jdtls-data" / workspace_hash data_dir.mkdir(parents=True, exist_ok=True) - # If module_root_uri is provided, scope jdtls to that module. - effective_params = dict(init_params) - effective_root_uri = module_root_uri or self._original_root_uri + # Deep copy to avoid mutating server._init_params. + effective_params = copy.deepcopy(init_params) + effective_root_uri = module_root_uri or original_root if module_root_uri: effective_params["rootUri"] = module_root_uri from pygls.uris import to_fs_path @@ -403,7 +429,7 @@ async def start(self, init_params: dict[str, Any], *, module_root_uri: str | Non logger.info( "jdtls: scoping to module %s (full root: %s)", _redact_path(module_root_uri), - _redact_path(self._original_root_uri), + _redact_path(original_root), ) # Inject workspaceFolders capability for later expansion. @@ -412,6 +438,7 @@ async def start(self, init_params: dict[str, Any], *, module_root_uri: str | Non ws["workspaceFolders"] = True # Track the initial module as already loaded. + self._initial_module_uri = module_root_uri self._added_module_uris.add(effective_root_uri) # Build a clean environment for jdtls. @@ -475,12 +502,7 @@ async def ensure_started(self, init_params: dict[str, Any], file_uri: str) -> bo self._starting = True try: - from pygls.uris import from_fs_path, to_fs_path - - file_path = to_fs_path(file_uri) or file_uri - module_root = find_module_root(file_path) - module_uri = from_fs_path(module_root) if module_root else None - + module_uri = _resolve_module_uri(file_uri) started = await self.start(init_params, module_root_uri=module_uri) if not started: self._start_failed = True @@ -490,7 +512,13 @@ async def ensure_started(self, init_params: dict[str, Any], file_uri: str) -> bo self._starting = False def queue_notification(self, method: str, params: Any) -> None: - """Buffer a notification for replay after jdtls starts.""" + """Buffer a notification for replay after jdtls starts. + + Capped at ``_MAX_QUEUED_NOTIFICATIONS`` to prevent unbounded memory + growth during long jdtls startup. Oldest entries are dropped on overflow. + """ + if len(self._queued_notifications) >= _MAX_QUEUED_NOTIFICATIONS: + self._queued_notifications.pop(0) self._queued_notifications.append((method, params)) async def flush_queued_notifications(self) -> None: @@ -503,24 +531,25 @@ async def add_module_if_new(self, file_uri: str) -> None: """Add the module containing *file_uri* to jdtls if not already added.""" if not self._available: return - from pygls.uris import from_fs_path, to_fs_path - - file_path = to_fs_path(file_uri) or file_uri - module_root = find_module_root(file_path) - if module_root is None: - return - module_uri = from_fs_path(module_root) or file_uri - if module_uri in self._added_module_uris: + module_uri = _resolve_module_uri(file_uri) + if module_uri is None or module_uri in self._added_module_uris: return self._added_module_uris.add(module_uri) - logger.info("jdtls: adding module %s", _redact_path(module_root)) + from pygls.uris import to_fs_path + + logger.info("jdtls: adding module %s", _redact_path(to_fs_path(module_uri))) + mod_name = Path(to_fs_path(module_uri) or module_uri).name await self.send_notification( - "workspace/didChangeWorkspaceFolders", - {"event": {"added": [{"uri": module_uri, "name": Path(module_root).name}], "removed": []}}, + _WORKSPACE_DID_CHANGE_FOLDERS, + {"event": {"added": [{"uri": module_uri, "name": mod_name}], "removed": []}}, ) async def expand_full_workspace(self) -> None: - """Expand jdtls workspace to the full monorepo root (background task).""" + """Expand jdtls workspace to the full monorepo root (background task). + + Removes the initial module-scoped folder and adds the full root to + avoid double-indexing. + """ if self._workspace_expanded or not self._available or not self._original_root_uri: return from pygls.uris import from_fs_path, to_fs_path @@ -531,10 +560,17 @@ async def expand_full_workspace(self) -> None: self._workspace_expanded = True return self._added_module_uris.add(root_uri) + + # Remove initial module folder to avoid double-indexing. + removed: list[dict[str, str]] = [] + if self._initial_module_uri and self._initial_module_uri != root_uri: + ini_path = to_fs_path(self._initial_module_uri) or self._initial_module_uri + removed.append({"uri": self._initial_module_uri, "name": Path(ini_path).name}) + logger.info("jdtls: expanding to full workspace %s", _redact_path(root_path)) await self.send_notification( - "workspace/didChangeWorkspaceFolders", - {"event": {"added": [{"uri": root_uri, "name": Path(root_path).name}], "removed": []}}, + _WORKSPACE_DID_CHANGE_FOLDERS, + {"event": {"added": [{"uri": root_uri, "name": Path(root_path).name}], "removed": removed}}, ) self._workspace_expanded = True diff --git a/src/java_functional_lsp/server.py b/src/java_functional_lsp/server.py index adcf372..030d3c0 100644 --- a/src/java_functional_lsp/server.py +++ b/src/java_functional_lsp/server.py @@ -331,6 +331,16 @@ async def _deferred_validate(uri: str) -> None: _analyze_and_publish(uri) +def _forward_or_queue(method: str, serialized: Any) -> None: + """Forward a notification to jdtls if available, or queue it if starting.""" + if server._proxy.is_available: + task = asyncio.create_task(server._proxy.send_notification(method, serialized)) + _bg_tasks.add(task) + task.add_done_callback(_bg_tasks.discard) + elif server._proxy._starting: + server._proxy.queue_notification(method, serialized) + + @server.feature(lsp.TEXT_DOCUMENT_DID_OPEN) async def on_did_open(params: lsp.DidOpenTextDocumentParams) -> None: """Forward to jdtls (starting lazily if needed) and analyze immediately. @@ -346,12 +356,15 @@ async def on_did_open(params: lsp.DidOpenTextDocumentParams) -> None: # Fast path: jdtls running. Forward didOpen + add module if new. await server._proxy.send_notification("textDocument/didOpen", serialized) await server._proxy.add_module_if_new(uri) - elif server._proxy._jdtls_on_path and not server._proxy._start_failed and not server._proxy._starting: - # First file: kick off lazy start in background. + elif server._proxy._jdtls_on_path and not server._proxy._start_failed: + # Queue the didOpen (whether this is the first file or a subsequent one during startup). server._proxy.queue_notification("textDocument/didOpen", serialized) - task = asyncio.create_task(_lazy_start_jdtls(uri)) - _bg_tasks.add(task) - task.add_done_callback(_bg_tasks.discard) + if not server._proxy._lazy_start_fired: + # First file: kick off lazy start in background. + server._proxy._lazy_start_fired = True + task = asyncio.create_task(_lazy_start_jdtls(uri)) + _bg_tasks.add(task) + task.add_done_callback(_bg_tasks.discard) # Custom diagnostics always publish immediately — never blocked by jdtls. _analyze_and_publish(uri) @@ -361,11 +374,7 @@ async def on_did_open(params: lsp.DidOpenTextDocumentParams) -> None: async def on_did_change(params: lsp.DidChangeTextDocumentParams) -> None: """Forward to jdtls and schedule debounced re-analysis.""" uri = params.text_document.uri - serialized = _serialize_params(params) - if server._proxy.is_available: - await server._proxy.send_notification("textDocument/didChange", serialized) - elif server._proxy._starting: - server._proxy.queue_notification("textDocument/didChange", serialized) + _forward_or_queue("textDocument/didChange", _serialize_params(params)) # Cancel pending validation, schedule new one (150ms debounce for IDE typing) if uri in _pending: _pending[uri].cancel() @@ -375,11 +384,7 @@ async def on_did_change(params: lsp.DidChangeTextDocumentParams) -> None: @server.feature(lsp.TEXT_DOCUMENT_DID_SAVE) async def on_did_save(params: lsp.DidSaveTextDocumentParams) -> None: """Forward to jdtls and re-analyze immediately (no debounce on save).""" - serialized = _serialize_params(params) - if server._proxy.is_available: - await server._proxy.send_notification("textDocument/didSave", serialized) - elif server._proxy._starting: - server._proxy.queue_notification("textDocument/didSave", serialized) + _forward_or_queue("textDocument/didSave", _serialize_params(params)) _analyze_and_publish(params.text_document.uri) @@ -392,11 +397,7 @@ async def on_did_close(params: lsp.DidCloseTextDocumentParams) -> None: del _pending[uri] # Clear diagnostics for the closed document (LSP best practice) server.text_document_publish_diagnostics(lsp.PublishDiagnosticsParams(uri=uri, diagnostics=[])) - serialized = _serialize_params(params) - if server._proxy.is_available: - await server._proxy.send_notification("textDocument/didClose", serialized) - elif server._proxy._starting: - server._proxy.queue_notification("textDocument/didClose", serialized) + _forward_or_queue("textDocument/didClose", _serialize_params(params)) async def _lazy_start_jdtls(file_uri: str) -> None: diff --git a/tests/test_proxy.py b/tests/test_proxy.py index 64c5ea0..af75ab4 100644 --- a/tests/test_proxy.py +++ b/tests/test_proxy.py @@ -930,7 +930,7 @@ def test_check_available_false(self) -> None: assert proxy.check_available() is False assert proxy._jdtls_on_path is False - def test_queue_and_flush(self) -> None: + async def test_queue_and_flush(self) -> None: from java_functional_lsp.proxy import JdtlsProxy proxy = JdtlsProxy() @@ -938,19 +938,28 @@ def test_queue_and_flush(self) -> None: proxy.queue_notification("textDocument/didChange", {"uri": "b"}) assert len(proxy._queued_notifications) == 2 - # flush should clear the queue flushed: list[tuple[str, Any]] = [] async def mock_send(method: str, params: Any) -> None: flushed.append((method, params)) proxy.send_notification = mock_send # type: ignore[assignment] - asyncio.get_event_loop().run_until_complete(proxy.flush_queued_notifications()) + await proxy.flush_queued_notifications() assert len(flushed) == 2 assert flushed[0] == ("textDocument/didOpen", {"uri": "a"}) assert flushed[1] == ("textDocument/didChange", {"uri": "b"}) assert len(proxy._queued_notifications) == 0 + def test_queue_caps_at_max(self) -> None: + from java_functional_lsp.proxy import _MAX_QUEUED_NOTIFICATIONS, JdtlsProxy + + proxy = JdtlsProxy() + for i in range(_MAX_QUEUED_NOTIFICATIONS + 50): + proxy.queue_notification("textDocument/didChange", {"i": i}) + assert len(proxy._queued_notifications) == _MAX_QUEUED_NOTIFICATIONS + # Oldest entries dropped — last entry should be the most recent + assert proxy._queued_notifications[-1] == ("textDocument/didChange", {"i": _MAX_QUEUED_NOTIFICATIONS + 49}) + async def test_ensure_started_no_retry_after_failure(self) -> None: from unittest.mock import AsyncMock @@ -958,10 +967,13 @@ async def test_ensure_started_no_retry_after_failure(self) -> None: proxy = JdtlsProxy() proxy._jdtls_on_path = True + proxy.queue_notification("textDocument/didOpen", {"uri": "test"}) proxy.start = AsyncMock(return_value=False) # type: ignore[assignment] result = await proxy.ensure_started({"rootUri": "file:///tmp"}, "file:///tmp/F.java") assert result is False assert proxy._start_failed is True + # Queue should be cleared on failure + assert len(proxy._queued_notifications) == 0 # Second call should return immediately without calling start() proxy.start.reset_mock() # type: ignore[attr-defined] result2 = await proxy.ensure_started({"rootUri": "file:///tmp"}, "file:///tmp/F.java") @@ -1025,6 +1037,18 @@ async def test_expand_full_workspace_sends_notification(self) -> None: proxy.send_notification.assert_called_once() # type: ignore[attr-defined] assert proxy._workspace_expanded is True + async def test_expand_full_workspace_noop_when_not_available(self) -> None: + from unittest.mock import AsyncMock + + from java_functional_lsp.proxy import JdtlsProxy + + proxy = JdtlsProxy() + proxy._original_root_uri = "file:///workspace/monorepo" + proxy.send_notification = AsyncMock() # type: ignore[assignment] + await proxy.expand_full_workspace() + proxy.send_notification.assert_not_called() # type: ignore[attr-defined] + assert proxy._workspace_expanded is False + async def test_expand_full_workspace_noop_when_already_added(self) -> None: from unittest.mock import AsyncMock diff --git a/tests/test_server.py b/tests/test_server.py index 1eabf4f..0b5dc5a 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -415,7 +415,7 @@ async def test_register_jdtls_capabilities_idempotent(self) -> None: mock_reg.assert_not_called() async def test_lazy_start_jdtls_success(self, caplog: Any) -> None: - """_lazy_start_jdtls logs success and registers capabilities.""" + """_lazy_start_jdtls logs success, flushes queue, and expands workspace.""" import logging from unittest.mock import AsyncMock, MagicMock, patch @@ -423,14 +423,16 @@ async def test_lazy_start_jdtls_success(self, caplog: Any) -> None: from java_functional_lsp.server import _lazy_start_jdtls from java_functional_lsp.server import server as srv + mock_flush = AsyncMock() + mock_expand = AsyncMock() old_flag = srv_mod._jdtls_capabilities_registered srv_mod._jdtls_capabilities_registered = False try: with ( caplog.at_level(logging.INFO, logger="java_functional_lsp.server"), patch.object(srv._proxy, "ensure_started", AsyncMock(return_value=True)), - patch.object(srv._proxy, "flush_queued_notifications", AsyncMock()), - patch.object(srv._proxy, "expand_full_workspace", AsyncMock()), + patch.object(srv._proxy, "flush_queued_notifications", mock_flush), + patch.object(srv._proxy, "expand_full_workspace", mock_expand), patch.object(srv, "feature", MagicMock(return_value=lambda fn: fn)), patch.object(srv, "client_register_capability_async", AsyncMock()), ): @@ -438,9 +440,11 @@ async def test_lazy_start_jdtls_success(self, caplog: Any) -> None: finally: srv_mod._jdtls_capabilities_registered = old_flag assert any("jdtls proxy active" in r.getMessage() for r in caplog.records) + mock_flush.assert_called_once() + mock_expand.assert_called_once() async def test_lazy_start_jdtls_failure_logged(self, caplog: Any) -> None: - """_lazy_start_jdtls logs warning on failure.""" + """_lazy_start_jdtls logs warning on exception.""" import logging from unittest.mock import AsyncMock, patch @@ -454,6 +458,24 @@ async def test_lazy_start_jdtls_failure_logged(self, caplog: Any) -> None: await _lazy_start_jdtls("file:///test/F.java") assert any("lazy start failed" in r.getMessage() for r in caplog.records) + async def test_lazy_start_jdtls_silent_failure(self) -> None: + """When ensure_started returns False, flush/expand are not called.""" + from unittest.mock import AsyncMock, patch + + from java_functional_lsp.server import _lazy_start_jdtls + from java_functional_lsp.server import server as srv + + mock_flush = AsyncMock() + mock_expand = AsyncMock() + with ( + patch.object(srv._proxy, "ensure_started", AsyncMock(return_value=False)), + patch.object(srv._proxy, "flush_queued_notifications", mock_flush), + patch.object(srv._proxy, "expand_full_workspace", mock_expand), + ): + await _lazy_start_jdtls("file:///test/F.java") + mock_flush.assert_not_called() + mock_expand.assert_not_called() + def test_serialize_params_camelcase(self) -> None: from java_functional_lsp.server import _serialize_params diff --git a/uv.lock b/uv.lock index 3919fb8..650e5c7 100644 --- a/uv.lock +++ b/uv.lock @@ -184,7 +184,7 @@ wheels = [ [[package]] name = "java-functional-lsp" -version = "0.7.3" +version = "0.7.4" source = { editable = "." } dependencies = [ { name = "pygls" }, From 8af1fe6917bd71e373729c81849c232f8fdcbfbf Mon Sep 17 00:00:00 2001 From: Aviad Shiber Date: Sat, 11 Apr 2026 17:07:40 +0300 Subject: [PATCH 4/7] =?UTF-8?q?feat:=20demand-driven=20module=20prioritiza?= =?UTF-8?q?tion=20=E2=80=94=20auto-add=20and=20retry=20on=20LSP=20requests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/java_functional_lsp/proxy.py | 13 +++++--- src/java_functional_lsp/server.py | 39 +++++++++++++--------- tests/test_proxy.py | 9 ++++-- tests/test_server.py | 54 +++++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 22 deletions(-) diff --git a/src/java_functional_lsp/proxy.py b/src/java_functional_lsp/proxy.py index d5967a5..6ead56a 100644 --- a/src/java_functional_lsp/proxy.py +++ b/src/java_functional_lsp/proxy.py @@ -527,13 +527,17 @@ async def flush_queued_notifications(self) -> None: for method, params in queue: await self.send_notification(method, params) - async def add_module_if_new(self, file_uri: str) -> None: - """Add the module containing *file_uri* to jdtls if not already added.""" + async def add_module_if_new(self, file_uri: str) -> bool: + """Add the module containing *file_uri* to jdtls if not already added. + + Returns ``True`` if a new module was added (jdtls needs import time), + ``False`` if already known or unavailable. + """ if not self._available: - return + return False module_uri = _resolve_module_uri(file_uri) if module_uri is None or module_uri in self._added_module_uris: - return + return False self._added_module_uris.add(module_uri) from pygls.uris import to_fs_path @@ -543,6 +547,7 @@ async def add_module_if_new(self, file_uri: str) -> None: _WORKSPACE_DID_CHANGE_FOLDERS, {"event": {"added": [{"uri": module_uri, "name": mod_name}], "removed": []}}, ) + return True async def expand_full_workspace(self) -> None: """Expand jdtls workspace to the full monorepo root (background task). diff --git a/src/java_functional_lsp/server.py b/src/java_functional_lsp/server.py index 030d3c0..528f121 100644 --- a/src/java_functional_lsp/server.py +++ b/src/java_functional_lsp/server.py @@ -439,12 +439,29 @@ async def _expand_workspace_background() -> None: # _JDTLS_HANDLERS and registered inside _register_jdtls_capabilities() so # they only activate after jdtls starts. +_MODULE_IMPORT_WAIT_SEC = 3.0 -async def _on_completion(params: lsp.CompletionParams) -> lsp.CompletionList | None: - """Forward completion request to jdtls.""" + +async def _ensure_module_and_forward(method: str, params: Any, file_uri: str) -> Any | None: + """Forward a request to jdtls, ensuring the file's module is loaded. + + If the module was just added and jdtls returns null, retries once after + a brief wait to give jdtls time to import the newly-added module. + """ if not server._proxy.is_available: return None - result = await server._proxy.send_request("textDocument/completion", _serialize_params(params)) + module_is_new = await server._proxy.add_module_if_new(file_uri) + serialized = _serialize_params(params) + result = await server._proxy.send_request(method, serialized) + if result is None and module_is_new: + await asyncio.sleep(_MODULE_IMPORT_WAIT_SEC) + result = await server._proxy.send_request(method, serialized) + return result + + +async def _on_completion(params: lsp.CompletionParams) -> lsp.CompletionList | None: + """Forward completion request to jdtls.""" + result = await _ensure_module_and_forward("textDocument/completion", params, params.text_document.uri) if result is None: return None try: @@ -455,9 +472,7 @@ async def _on_completion(params: lsp.CompletionParams) -> lsp.CompletionList | N async def _on_hover(params: lsp.HoverParams) -> lsp.Hover | None: """Forward hover request to jdtls.""" - if not server._proxy.is_available: - return None - result = await server._proxy.send_request("textDocument/hover", _serialize_params(params)) + result = await _ensure_module_and_forward("textDocument/hover", params, params.text_document.uri) if result is None: return None try: @@ -468,9 +483,7 @@ async def _on_hover(params: lsp.HoverParams) -> lsp.Hover | None: async def _on_definition(params: lsp.DefinitionParams) -> list[lsp.Location] | None: """Forward go-to-definition request to jdtls.""" - if not server._proxy.is_available: - return None - result = await server._proxy.send_request("textDocument/definition", _serialize_params(params)) + result = await _ensure_module_and_forward("textDocument/definition", params, params.text_document.uri) if result is None: return None try: @@ -483,9 +496,7 @@ async def _on_definition(params: lsp.DefinitionParams) -> list[lsp.Location] | N async def _on_references(params: lsp.ReferenceParams) -> list[lsp.Location] | None: """Forward find-references request to jdtls.""" - if not server._proxy.is_available: - return None - result = await server._proxy.send_request("textDocument/references", _serialize_params(params)) + result = await _ensure_module_and_forward("textDocument/references", params, params.text_document.uri) if result is None: return None try: @@ -496,9 +507,7 @@ async def _on_references(params: lsp.ReferenceParams) -> list[lsp.Location] | No async def _on_document_symbol(params: lsp.DocumentSymbolParams) -> list[lsp.DocumentSymbol] | None: """Forward document symbol request to jdtls.""" - if not server._proxy.is_available: - return None - result = await server._proxy.send_request("textDocument/documentSymbol", _serialize_params(params)) + result = await _ensure_module_and_forward("textDocument/documentSymbol", params, params.text_document.uri) if result is None: return None try: diff --git a/tests/test_proxy.py b/tests/test_proxy.py index af75ab4..42ef01f 100644 --- a/tests/test_proxy.py +++ b/tests/test_proxy.py @@ -998,7 +998,8 @@ async def test_add_module_if_new_sends_notification(self) -> None: java_file.parent.mkdir() java_file.touch() uri = java_file.as_uri() - await proxy.add_module_if_new(uri) + result = await proxy.add_module_if_new(uri) + assert result is True proxy.send_notification.assert_called_once() # type: ignore[attr-defined] call_args = proxy.send_notification.call_args # type: ignore[attr-defined] assert call_args[0][0] == "workspace/didChangeWorkspaceFolders" @@ -1020,8 +1021,10 @@ async def test_add_module_if_new_skips_duplicate(self) -> None: java_file.parent.mkdir() java_file.touch() uri = java_file.as_uri() - await proxy.add_module_if_new(uri) - await proxy.add_module_if_new(uri) # duplicate + result1 = await proxy.add_module_if_new(uri) + result2 = await proxy.add_module_if_new(uri) # duplicate + assert result1 is True + assert result2 is False assert proxy.send_notification.call_count == 1 # type: ignore[attr-defined] async def test_expand_full_workspace_sends_notification(self) -> None: diff --git a/tests/test_server.py b/tests/test_server.py index 0b5dc5a..76c1199 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -476,6 +476,60 @@ async def test_lazy_start_jdtls_silent_failure(self) -> None: mock_flush.assert_not_called() mock_expand.assert_not_called() + async def test_ensure_module_and_forward_retries_on_new_module(self) -> None: + """When add_module_if_new returns True and first request returns None, retry once.""" + from unittest.mock import AsyncMock, patch + + from java_functional_lsp.server import _ensure_module_and_forward + from java_functional_lsp.server import server as srv + + mock_add = AsyncMock(return_value=True) + mock_send = AsyncMock(side_effect=[None, {"result": "ok"}]) + with ( + patch.object(srv._proxy, "add_module_if_new", mock_add), + patch.object(srv._proxy, "send_request", mock_send), + patch.object(srv._proxy, "_available", True), + ): + result = await _ensure_module_and_forward("textDocument/hover", {}, "file:///test/F.java") + assert result == {"result": "ok"} + assert mock_send.call_count == 2 + + async def test_ensure_module_and_forward_no_retry_on_known_module(self) -> None: + """When add_module_if_new returns False and request returns None, no retry.""" + from unittest.mock import AsyncMock, patch + + from java_functional_lsp.server import _ensure_module_and_forward + from java_functional_lsp.server import server as srv + + mock_add = AsyncMock(return_value=False) + mock_send = AsyncMock(return_value=None) + with ( + patch.object(srv._proxy, "add_module_if_new", mock_add), + patch.object(srv._proxy, "send_request", mock_send), + patch.object(srv._proxy, "_available", True), + ): + result = await _ensure_module_and_forward("textDocument/hover", {}, "file:///test/F.java") + assert result is None + assert mock_send.call_count == 1 + + async def test_ensure_module_and_forward_no_retry_on_success(self) -> None: + """When first request succeeds, no retry regardless of module status.""" + from unittest.mock import AsyncMock, patch + + from java_functional_lsp.server import _ensure_module_and_forward + from java_functional_lsp.server import server as srv + + mock_add = AsyncMock(return_value=True) + mock_send = AsyncMock(return_value={"result": "ok"}) + with ( + patch.object(srv._proxy, "add_module_if_new", mock_add), + patch.object(srv._proxy, "send_request", mock_send), + patch.object(srv._proxy, "_available", True), + ): + result = await _ensure_module_and_forward("textDocument/hover", {}, "file:///test/F.java") + assert result == {"result": "ok"} + assert mock_send.call_count == 1 + def test_serialize_params_camelcase(self) -> None: from java_functional_lsp.server import _serialize_params From fd79913a38499c2cb51ef3205864f4155b98a612 Mon Sep 17 00:00:00 2001 From: Aviad Shiber Date: Sat, 11 Apr 2026 17:18:12 +0300 Subject: [PATCH 5/7] =?UTF-8?q?fix:=20address=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20queue=20gate,=20deque,=20cache=20docs,=20test=20imp?= =?UTF-8?q?rovements?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/java_functional_lsp/proxy.py | 19 +++++++++++++------ src/java_functional_lsp/server.py | 2 +- tests/test_proxy.py | 29 ++++++++++++++++++++++++++++- tests/test_server.py | 5 ++++- 4 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/java_functional_lsp/proxy.py b/src/java_functional_lsp/proxy.py index 6ead56a..c24cee5 100644 --- a/src/java_functional_lsp/proxy.py +++ b/src/java_functional_lsp/proxy.py @@ -12,6 +12,7 @@ import re import shutil import subprocess +from collections import deque from collections.abc import Callable, Mapping from functools import lru_cache from pathlib import Path @@ -336,6 +337,9 @@ def find_module_root(file_path: str) -> str | None: Returns the directory path, or ``None`` if no build file is found before reaching the filesystem root. Results are cached by parent directory. + + **Note:** cache entries are never invalidated. Build files added after the + first lookup for a given directory will not be detected until process restart. """ return _cached_module_root(str(Path(file_path).parent)) @@ -373,7 +377,7 @@ def __init__(self, on_diagnostics: Callable[[str, list[Any]], None] | None = Non self._start_failed = False self._jdtls_on_path = False self._lazy_start_fired = False - self._queued_notifications: list[tuple[str, Any]] = [] + self._queued_notifications: deque[tuple[str, Any]] = deque(maxlen=_MAX_QUEUED_NOTIFICATIONS) self._original_root_uri: str | None = None self._initial_module_uri: str | None = None self._added_module_uris: set[str] = set() @@ -508,22 +512,25 @@ async def ensure_started(self, init_params: dict[str, Any], file_uri: str) -> bo self._start_failed = True self._queued_notifications.clear() return started + except Exception: + self._start_failed = True + self._queued_notifications.clear() + raise finally: self._starting = False def queue_notification(self, method: str, params: Any) -> None: """Buffer a notification for replay after jdtls starts. - Capped at ``_MAX_QUEUED_NOTIFICATIONS`` to prevent unbounded memory - growth during long jdtls startup. Oldest entries are dropped on overflow. + Uses a ``deque(maxlen=200)`` so oldest entries are dropped in O(1) + when the queue overflows during long jdtls startup. """ - if len(self._queued_notifications) >= _MAX_QUEUED_NOTIFICATIONS: - self._queued_notifications.pop(0) self._queued_notifications.append((method, params)) async def flush_queued_notifications(self) -> None: """Send all queued notifications to jdtls.""" - queue, self._queued_notifications = self._queued_notifications, [] + queue = list(self._queued_notifications) + self._queued_notifications.clear() for method, params in queue: await self.send_notification(method, params) diff --git a/src/java_functional_lsp/server.py b/src/java_functional_lsp/server.py index 528f121..639ce47 100644 --- a/src/java_functional_lsp/server.py +++ b/src/java_functional_lsp/server.py @@ -337,7 +337,7 @@ def _forward_or_queue(method: str, serialized: Any) -> None: task = asyncio.create_task(server._proxy.send_notification(method, serialized)) _bg_tasks.add(task) task.add_done_callback(_bg_tasks.discard) - elif server._proxy._starting: + elif server._proxy._lazy_start_fired and not server._proxy._start_failed: server._proxy.queue_notification(method, serialized) diff --git a/tests/test_proxy.py b/tests/test_proxy.py index 42ef01f..22ee3ec 100644 --- a/tests/test_proxy.py +++ b/tests/test_proxy.py @@ -862,6 +862,12 @@ async def fake_send_request(*_args: Any, **_kwargs: Any) -> None: class TestFindModuleRoot: """Tests for find_module_root — build-file detection for module scoping.""" + @pytest.fixture(autouse=True) + def _clear_cache(self) -> None: + from java_functional_lsp.proxy import _cached_module_root + + _cached_module_root.cache_clear() + def test_finds_pom_xml(self, tmp_path: Any) -> None: from java_functional_lsp.proxy import find_module_root @@ -957,7 +963,9 @@ def test_queue_caps_at_max(self) -> None: for i in range(_MAX_QUEUED_NOTIFICATIONS + 50): proxy.queue_notification("textDocument/didChange", {"i": i}) assert len(proxy._queued_notifications) == _MAX_QUEUED_NOTIFICATIONS - # Oldest entries dropped — last entry should be the most recent + # Oldest entries dropped — first surviving entry is i=50 + assert proxy._queued_notifications[0] == ("textDocument/didChange", {"i": 50}) + # Last entry is the most recent assert proxy._queued_notifications[-1] == ("textDocument/didChange", {"i": _MAX_QUEUED_NOTIFICATIONS + 49}) async def test_ensure_started_no_retry_after_failure(self) -> None: @@ -1040,6 +1048,25 @@ async def test_expand_full_workspace_sends_notification(self) -> None: proxy.send_notification.assert_called_once() # type: ignore[attr-defined] assert proxy._workspace_expanded is True + async def test_expand_full_workspace_removes_initial_module(self) -> None: + """When _initial_module_uri differs from root, it should be in the removed list.""" + from unittest.mock import AsyncMock + + from java_functional_lsp.proxy import JdtlsProxy + + proxy = JdtlsProxy() + proxy._available = True + proxy._original_root_uri = "file:///workspace/monorepo" + proxy._initial_module_uri = "file:///workspace/monorepo/module-a" + proxy.send_notification = AsyncMock() # type: ignore[assignment] + await proxy.expand_full_workspace() + proxy.send_notification.assert_called_once() # type: ignore[attr-defined] + call_args = proxy.send_notification.call_args[0] # type: ignore[attr-defined] + event = call_args[1]["event"] + assert len(event["removed"]) == 1 + assert event["removed"][0]["uri"] == "file:///workspace/monorepo/module-a" + assert event["added"][0]["uri"] == "file:///workspace/monorepo" + async def test_expand_full_workspace_noop_when_not_available(self) -> None: from unittest.mock import AsyncMock diff --git a/tests/test_server.py b/tests/test_server.py index 76c1199..27472b4 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -480,19 +480,22 @@ async def test_ensure_module_and_forward_retries_on_new_module(self) -> None: """When add_module_if_new returns True and first request returns None, retry once.""" from unittest.mock import AsyncMock, patch - from java_functional_lsp.server import _ensure_module_and_forward + from java_functional_lsp.server import _MODULE_IMPORT_WAIT_SEC, _ensure_module_and_forward from java_functional_lsp.server import server as srv mock_add = AsyncMock(return_value=True) mock_send = AsyncMock(side_effect=[None, {"result": "ok"}]) + mock_sleep = AsyncMock() with ( patch.object(srv._proxy, "add_module_if_new", mock_add), patch.object(srv._proxy, "send_request", mock_send), patch.object(srv._proxy, "_available", True), + patch("java_functional_lsp.server.asyncio.sleep", mock_sleep), ): result = await _ensure_module_and_forward("textDocument/hover", {}, "file:///test/F.java") assert result == {"result": "ok"} assert mock_send.call_count == 2 + mock_sleep.assert_called_once_with(_MODULE_IMPORT_WAIT_SEC) async def test_ensure_module_and_forward_no_retry_on_known_module(self) -> None: """When add_module_if_new returns False and request returns None, no retry.""" From bdbd1b62419ff53029d879e4be3a5ea40dc5f6aa Mon Sep 17 00:00:00 2001 From: Aviad Shiber Date: Sat, 11 Apr 2026 17:48:14 +0300 Subject: [PATCH 6/7] feat: ModuleRegistry with asyncio.Event for adaptive module waiting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/java_functional_lsp/proxy.py | 98 +++++++++++++++++++++++++++---- src/java_functional_lsp/server.py | 47 +++++++++++---- tests/test_proxy.py | 10 ++-- tests/test_server.py | 94 +++++++++++++++++------------ 4 files changed, 183 insertions(+), 66 deletions(-) diff --git a/src/java_functional_lsp/proxy.py b/src/java_functional_lsp/proxy.py index c24cee5..6669622 100644 --- a/src/java_functional_lsp/proxy.py +++ b/src/java_functional_lsp/proxy.py @@ -317,6 +317,73 @@ async def read_message(reader: asyncio.StreamReader) -> dict[str, Any] | None: _BUILD_FILES = ("pom.xml", "build.gradle", "build.gradle.kts") _WORKSPACE_DID_CHANGE_FOLDERS = "workspace/didChangeWorkspaceFolders" _MAX_QUEUED_NOTIFICATIONS = 200 +_MODULE_READY_TIMEOUT = 30.0 + + +class ModuleState: + """Module import states — UNKNOWN → ADDED → READY.""" + + UNKNOWN = "unknown" + ADDED = "added" + READY = "ready" + + +class ModuleRegistry: + """Thread-safe (asyncio) registry tracking jdtls module import states. + + Uses a plain dict for O(1) hot-path lookups and per-module ``asyncio.Event`` + for adaptive waiting — coroutines blocked on ``wait_until_ready()`` wake + instantly when ``mark_ready()`` is called, instead of a fixed sleep. + + Safe without locks because asyncio is single-threaded: dict mutations that + don't span an ``await`` are atomic. + """ + + def __init__(self) -> None: + self._states: dict[str, str] = {} + self._ready_events: dict[str, asyncio.Event] = {} + + def get_state(self, uri: str) -> str: + """O(1) state lookup. Returns ModuleState constant.""" + return self._states.get(uri, ModuleState.UNKNOWN) + + def is_ready(self, uri: str) -> bool: + """O(1) hot-path check — zero overhead when module is ready.""" + return self._states.get(uri) is ModuleState.READY + + def was_added(self, uri: str) -> bool: + """True if module was sent to jdtls (ADDED or READY).""" + return uri in self._states + + def mark_added(self, uri: str) -> None: + """Mark module as sent to jdtls. Pre-creates the Event for waiters. + + Must be called before any ``await`` to prevent duplicate add_module calls. + """ + self._states[uri] = ModuleState.ADDED + self._ready_events.setdefault(uri, asyncio.Event()) + + def mark_ready(self, uri: str) -> None: + """Mark module as confirmed working. Wakes all coroutines waiting on it.""" + self._states[uri] = ModuleState.READY + event = self._ready_events.get(uri) + if event is not None: + event.set() + + async def wait_until_ready(self, uri: str, timeout: float = _MODULE_READY_TIMEOUT) -> bool: + """Suspend until the module is ready or timeout expires. + + Returns True if ready, False on timeout. If already READY, returns + immediately without suspending. + """ + event = self._ready_events.setdefault(uri, asyncio.Event()) + if event.is_set(): + return True + try: + await asyncio.wait_for(event.wait(), timeout=timeout) + return True + except asyncio.TimeoutError: + return False @lru_cache(maxsize=256) @@ -380,7 +447,7 @@ def __init__(self, on_diagnostics: Callable[[str, list[Any]], None] | None = Non self._queued_notifications: deque[tuple[str, Any]] = deque(maxlen=_MAX_QUEUED_NOTIFICATIONS) self._original_root_uri: str | None = None self._initial_module_uri: str | None = None - self._added_module_uris: set[str] = set() + self.modules = ModuleRegistry() self._workspace_expanded = False @property @@ -441,9 +508,9 @@ async def start(self, init_params: dict[str, Any], *, module_root_uri: str | Non ws = caps.setdefault("workspace", {}) ws["workspaceFolders"] = True - # Track the initial module as already loaded. + # Track the initial module as already loaded (mark ADDED before await). self._initial_module_uri = module_root_uri - self._added_module_uris.add(effective_root_uri) + self.modules.mark_added(effective_root_uri) # Build a clean environment for jdtls. loop = asyncio.get_running_loop() @@ -534,18 +601,23 @@ async def flush_queued_notifications(self) -> None: for method, params in queue: await self.send_notification(method, params) - async def add_module_if_new(self, file_uri: str) -> bool: + async def add_module_if_new(self, file_uri: str) -> str | None: """Add the module containing *file_uri* to jdtls if not already added. - Returns ``True`` if a new module was added (jdtls needs import time), - ``False`` if already known or unavailable. + Returns the module URI if a new module was added (UNKNOWN → ADDED), + or ``None`` if already known or unavailable. The returned URI can be + used with ``modules.wait_until_ready()`` for adaptive waiting. + + Calls ``modules.mark_added()`` before any ``await`` to prevent + duplicate add calls from concurrent coroutines. """ if not self._available: - return False + return None module_uri = _resolve_module_uri(file_uri) - if module_uri is None or module_uri in self._added_module_uris: - return False - self._added_module_uris.add(module_uri) + if module_uri is None or self.modules.was_added(module_uri): + return None + # Mark ADDED before await — atomic in asyncio, prevents duplicate sends. + self.modules.mark_added(module_uri) from pygls.uris import to_fs_path logger.info("jdtls: adding module %s", _redact_path(to_fs_path(module_uri))) @@ -554,7 +626,7 @@ async def add_module_if_new(self, file_uri: str) -> bool: _WORKSPACE_DID_CHANGE_FOLDERS, {"event": {"added": [{"uri": module_uri, "name": mod_name}], "removed": []}}, ) - return True + return module_uri async def expand_full_workspace(self) -> None: """Expand jdtls workspace to the full monorepo root (background task). @@ -568,10 +640,10 @@ async def expand_full_workspace(self) -> None: root_path = to_fs_path(self._original_root_uri) or self._original_root_uri root_uri = from_fs_path(root_path) or self._original_root_uri - if root_uri in self._added_module_uris: + if self.modules.was_added(root_uri): self._workspace_expanded = True return - self._added_module_uris.add(root_uri) + self.modules.mark_added(root_uri) # Remove initial module folder to avoid double-indexing. removed: list[dict[str, str]] = [] diff --git a/src/java_functional_lsp/server.py b/src/java_functional_lsp/server.py index 639ce47..5f3630e 100644 --- a/src/java_functional_lsp/server.py +++ b/src/java_functional_lsp/server.py @@ -26,7 +26,7 @@ from .analyzers.null_checker import NullChecker from .analyzers.spring_checker import SpringChecker from .fixes import get_fix, get_fix_registry_keys -from .proxy import JdtlsProxy +from .proxy import JdtlsProxy, _resolve_module_uri logger = logging.getLogger(__name__) @@ -439,23 +439,48 @@ async def _expand_workspace_background() -> None: # _JDTLS_HANDLERS and registered inside _register_jdtls_capabilities() so # they only activate after jdtls starts. -_MODULE_IMPORT_WAIT_SEC = 3.0 - async def _ensure_module_and_forward(method: str, params: Any, file_uri: str) -> Any | None: """Forward a request to jdtls, ensuring the file's module is loaded. - If the module was just added and jdtls returns null, retries once after - a brief wait to give jdtls time to import the newly-added module. + Uses ``ModuleRegistry`` for adaptive waiting: + - **READY**: forward immediately (zero overhead on hot path) + - **UNKNOWN**: add module, wait until ready (adaptive, not fixed sleep) + - **ADDED**: module sent but not confirmed — wait until ready + + When a request succeeds, marks the module as READY so subsequent + requests skip the wait entirely. """ - if not server._proxy.is_available: + proxy = server._proxy + if not proxy.is_available: return None - module_is_new = await server._proxy.add_module_if_new(file_uri) + + module_uri = _resolve_module_uri(file_uri) + + # Hot path: module already confirmed working. + if module_uri and proxy.modules.is_ready(module_uri): + return await proxy.send_request(method, _serialize_params(params)) + + # Cold path: add module if unknown, then wait for ready. + new_module_uri = await proxy.add_module_if_new(file_uri) + serialized = _serialize_params(params) - result = await server._proxy.send_request(method, serialized) - if result is None and module_is_new: - await asyncio.sleep(_MODULE_IMPORT_WAIT_SEC) - result = await server._proxy.send_request(method, serialized) + result = await proxy.send_request(method, serialized) + + if result is not None: + # Success — mark module as ready so future requests are instant. + if module_uri: + proxy.modules.mark_ready(module_uri) + return result + + # Null result and module is not yet ready — wait adaptively. + wait_uri = new_module_uri or module_uri + if wait_uri and not proxy.modules.is_ready(wait_uri): + ready = await proxy.modules.wait_until_ready(wait_uri) + if ready: + result = await proxy.send_request(method, serialized) + if result is not None and module_uri: + proxy.modules.mark_ready(module_uri) return result diff --git a/tests/test_proxy.py b/tests/test_proxy.py index 22ee3ec..7bdc2e1 100644 --- a/tests/test_proxy.py +++ b/tests/test_proxy.py @@ -1007,10 +1007,12 @@ async def test_add_module_if_new_sends_notification(self) -> None: java_file.touch() uri = java_file.as_uri() result = await proxy.add_module_if_new(uri) - assert result is True + assert result is not None # Returns module URI string proxy.send_notification.assert_called_once() # type: ignore[attr-defined] call_args = proxy.send_notification.call_args # type: ignore[attr-defined] assert call_args[0][0] == "workspace/didChangeWorkspaceFolders" + # Module should be marked ADDED in registry + assert proxy.modules.was_added(result) async def test_add_module_if_new_skips_duplicate(self) -> None: from unittest.mock import AsyncMock @@ -1031,8 +1033,8 @@ async def test_add_module_if_new_skips_duplicate(self) -> None: uri = java_file.as_uri() result1 = await proxy.add_module_if_new(uri) result2 = await proxy.add_module_if_new(uri) # duplicate - assert result1 is True - assert result2 is False + assert result1 is not None # New module URI + assert result2 is None # Already known assert proxy.send_notification.call_count == 1 # type: ignore[attr-defined] async def test_expand_full_workspace_sends_notification(self) -> None: @@ -1087,7 +1089,7 @@ async def test_expand_full_workspace_noop_when_already_added(self) -> None: proxy = JdtlsProxy() proxy._available = True proxy._original_root_uri = "file:///workspace/monorepo" - proxy._added_module_uris.add("file:///workspace/monorepo") + proxy.modules.mark_added("file:///workspace/monorepo") proxy.send_notification = AsyncMock() # type: ignore[assignment] await proxy.expand_full_workspace() proxy.send_notification.assert_not_called() # type: ignore[attr-defined] diff --git a/tests/test_server.py b/tests/test_server.py index 27472b4..2a748f9 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -476,62 +476,80 @@ async def test_lazy_start_jdtls_silent_failure(self) -> None: mock_flush.assert_not_called() mock_expand.assert_not_called() - async def test_ensure_module_and_forward_retries_on_new_module(self) -> None: - """When add_module_if_new returns True and first request returns None, retry once.""" + async def test_ensure_module_and_forward_ready_module_fast_path(self) -> None: + """READY module → single send_request, no add_module call.""" from unittest.mock import AsyncMock, patch - from java_functional_lsp.server import _MODULE_IMPORT_WAIT_SEC, _ensure_module_and_forward + from java_functional_lsp.server import _ensure_module_and_forward from java_functional_lsp.server import server as srv - mock_add = AsyncMock(return_value=True) - mock_send = AsyncMock(side_effect=[None, {"result": "ok"}]) - mock_sleep = AsyncMock() - with ( - patch.object(srv._proxy, "add_module_if_new", mock_add), - patch.object(srv._proxy, "send_request", mock_send), - patch.object(srv._proxy, "_available", True), - patch("java_functional_lsp.server.asyncio.sleep", mock_sleep), - ): - result = await _ensure_module_and_forward("textDocument/hover", {}, "file:///test/F.java") + srv._proxy.modules.mark_added("file:///mod") + srv._proxy.modules.mark_ready("file:///mod") + mock_send = AsyncMock(return_value={"result": "ok"}) + try: + with ( + patch.object(srv._proxy, "send_request", mock_send), + patch.object(srv._proxy, "_available", True), + patch("java_functional_lsp.server._resolve_module_uri", return_value="file:///mod"), + ): + result = await _ensure_module_and_forward("textDocument/hover", {}, "file:///mod/F.java") + finally: + srv._proxy.modules._states.clear() + srv._proxy.modules._ready_events.clear() assert result == {"result": "ok"} - assert mock_send.call_count == 2 - mock_sleep.assert_called_once_with(_MODULE_IMPORT_WAIT_SEC) + assert mock_send.call_count == 1 - async def test_ensure_module_and_forward_no_retry_on_known_module(self) -> None: - """When add_module_if_new returns False and request returns None, no retry.""" + async def test_ensure_module_and_forward_new_module_waits_and_retries(self) -> None: + """UNKNOWN module → add, first request null, wait_until_ready, retry succeeds.""" from unittest.mock import AsyncMock, patch from java_functional_lsp.server import _ensure_module_and_forward from java_functional_lsp.server import server as srv - mock_add = AsyncMock(return_value=False) - mock_send = AsyncMock(return_value=None) - with ( - patch.object(srv._proxy, "add_module_if_new", mock_add), - patch.object(srv._proxy, "send_request", mock_send), - patch.object(srv._proxy, "_available", True), - ): - result = await _ensure_module_and_forward("textDocument/hover", {}, "file:///test/F.java") - assert result is None - assert mock_send.call_count == 1 + mock_add = AsyncMock(return_value="file:///mod") + mock_send = AsyncMock(side_effect=[None, {"result": "ok"}]) - async def test_ensure_module_and_forward_no_retry_on_success(self) -> None: - """When first request succeeds, no retry regardless of module status.""" + async def mock_wait(uri: str, timeout: float = 30.0) -> bool: + srv._proxy.modules.mark_ready(uri) + return True + + try: + with ( + patch.object(srv._proxy, "add_module_if_new", mock_add), + patch.object(srv._proxy, "send_request", mock_send), + patch.object(srv._proxy, "_available", True), + patch.object(srv._proxy.modules, "wait_until_ready", mock_wait), + patch("java_functional_lsp.server._resolve_module_uri", return_value="file:///mod"), + ): + result = await _ensure_module_and_forward("textDocument/hover", {}, "file:///mod/F.java") + finally: + srv._proxy.modules._states.clear() + srv._proxy.modules._ready_events.clear() + assert result == {"result": "ok"} + assert mock_send.call_count == 2 + + async def test_ensure_module_and_forward_success_marks_ready(self) -> None: + """First successful request marks module as READY.""" from unittest.mock import AsyncMock, patch + from java_functional_lsp.proxy import ModuleState from java_functional_lsp.server import _ensure_module_and_forward from java_functional_lsp.server import server as srv - mock_add = AsyncMock(return_value=True) + mock_add = AsyncMock(return_value="file:///mod") mock_send = AsyncMock(return_value={"result": "ok"}) - with ( - patch.object(srv._proxy, "add_module_if_new", mock_add), - patch.object(srv._proxy, "send_request", mock_send), - patch.object(srv._proxy, "_available", True), - ): - result = await _ensure_module_and_forward("textDocument/hover", {}, "file:///test/F.java") - assert result == {"result": "ok"} - assert mock_send.call_count == 1 + try: + with ( + patch.object(srv._proxy, "add_module_if_new", mock_add), + patch.object(srv._proxy, "send_request", mock_send), + patch.object(srv._proxy, "_available", True), + patch("java_functional_lsp.server._resolve_module_uri", return_value="file:///mod"), + ): + await _ensure_module_and_forward("textDocument/hover", {}, "file:///mod/F.java") + assert srv._proxy.modules.get_state("file:///mod") == ModuleState.READY + finally: + srv._proxy.modules._states.clear() + srv._proxy.modules._ready_events.clear() def test_serialize_params_camelcase(self) -> None: from java_functional_lsp.server import _serialize_params From a218adcb5ec42b44233de382c3f0f1e19db51a92 Mon Sep 17 00:00:00 2001 From: Aviad Shiber Date: Sat, 11 Apr 2026 18:01:26 +0300 Subject: [PATCH 7/7] =?UTF-8?q?fix:=20review=20findings=20=E2=80=94=20equa?= =?UTF-8?q?lity=20comparison,=20deadlock=20fix,=20Event=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- src/java_functional_lsp/proxy.py | 9 +++++++-- src/java_functional_lsp/server.py | 14 ++++++++------ tests/test_server.py | 9 +++------ 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/java_functional_lsp/proxy.py b/src/java_functional_lsp/proxy.py index 6669622..a68bc47 100644 --- a/src/java_functional_lsp/proxy.py +++ b/src/java_functional_lsp/proxy.py @@ -349,7 +349,7 @@ def get_state(self, uri: str) -> str: def is_ready(self, uri: str) -> bool: """O(1) hot-path check — zero overhead when module is ready.""" - return self._states.get(uri) is ModuleState.READY + return self._states.get(uri) == ModuleState.READY def was_added(self, uri: str) -> bool: """True if module was sent to jdtls (ADDED or READY).""" @@ -366,10 +366,15 @@ def mark_added(self, uri: str) -> None: def mark_ready(self, uri: str) -> None: """Mark module as confirmed working. Wakes all coroutines waiting on it.""" self._states[uri] = ModuleState.READY - event = self._ready_events.get(uri) + event = self._ready_events.pop(uri, None) if event is not None: event.set() + def clear(self) -> None: + """Reset all state. Used by tests.""" + self._states.clear() + self._ready_events.clear() + async def wait_until_ready(self, uri: str, timeout: float = _MODULE_READY_TIMEOUT) -> bool: """Suspend until the module is ready or timeout expires. diff --git a/src/java_functional_lsp/server.py b/src/java_functional_lsp/server.py index 5f3630e..5ede8b2 100644 --- a/src/java_functional_lsp/server.py +++ b/src/java_functional_lsp/server.py @@ -473,14 +473,16 @@ async def _ensure_module_and_forward(method: str, params: Any, file_uri: str) -> proxy.modules.mark_ready(module_uri) return result - # Null result and module is not yet ready — wait adaptively. + # Null result and module is not yet ready — wait then retry once. + # Use a short timeout (5s) so single-caller case doesn't block for 30s. + # If a concurrent request succeeds, Event.set() wakes us early. wait_uri = new_module_uri or module_uri if wait_uri and not proxy.modules.is_ready(wait_uri): - ready = await proxy.modules.wait_until_ready(wait_uri) - if ready: - result = await proxy.send_request(method, serialized) - if result is not None and module_uri: - proxy.modules.mark_ready(module_uri) + await proxy.modules.wait_until_ready(wait_uri, timeout=5.0) + # Always retry once after waiting — even on timeout the module may be ready. + result = await proxy.send_request(method, serialized) + if result is not None and module_uri: + proxy.modules.mark_ready(module_uri) return result diff --git a/tests/test_server.py b/tests/test_server.py index 2a748f9..4876612 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -494,8 +494,7 @@ async def test_ensure_module_and_forward_ready_module_fast_path(self) -> None: ): result = await _ensure_module_and_forward("textDocument/hover", {}, "file:///mod/F.java") finally: - srv._proxy.modules._states.clear() - srv._proxy.modules._ready_events.clear() + srv._proxy.modules.clear() assert result == {"result": "ok"} assert mock_send.call_count == 1 @@ -523,8 +522,7 @@ async def mock_wait(uri: str, timeout: float = 30.0) -> bool: ): result = await _ensure_module_and_forward("textDocument/hover", {}, "file:///mod/F.java") finally: - srv._proxy.modules._states.clear() - srv._proxy.modules._ready_events.clear() + srv._proxy.modules.clear() assert result == {"result": "ok"} assert mock_send.call_count == 2 @@ -548,8 +546,7 @@ async def test_ensure_module_and_forward_success_marks_ready(self) -> None: await _ensure_module_and_forward("textDocument/hover", {}, "file:///mod/F.java") assert srv._proxy.modules.get_state("file:///mod") == ModuleState.READY finally: - srv._proxy.modules._states.clear() - srv._proxy.modules._ready_events.clear() + srv._proxy.modules.clear() def test_serialize_params_camelcase(self) -> None: from java_functional_lsp.server import _serialize_params