diff --git a/lib/esbonio/esbonio/server/__init__.py b/lib/esbonio/esbonio/server/__init__.py index ae67baca5..91ccd317d 100644 --- a/lib/esbonio/esbonio/server/__init__.py +++ b/lib/esbonio/esbonio/server/__init__.py @@ -9,6 +9,7 @@ from .feature import LanguageFeature from .server import EsbonioLanguageServer from .server import EsbonioWorkspace +from .setup import create_language_server __all__ = ( "LOG_NAMESPACE", @@ -21,4 +22,5 @@ "LanguageFeature", "MemoryHandler", "Uri", + "create_language_server", ) diff --git a/lib/esbonio/esbonio/server/features/sphinx_manager/manager.py b/lib/esbonio/esbonio/server/features/sphinx_manager/manager.py index 2dbf27e0e..f1a3204bb 100644 --- a/lib/esbonio/esbonio/server/features/sphinx_manager/manager.py +++ b/lib/esbonio/esbonio/server/features/sphinx_manager/manager.py @@ -1,30 +1,30 @@ from __future__ import annotations import asyncio -import sys +import inspect import typing import uuid -from typing import Callable -from typing import Dict -from typing import Optional import lsprotocol.types as lsp import esbonio.sphinx_agent.types as types -from esbonio.server import EventSource -from esbonio.server import LanguageFeature +from esbonio import server from esbonio.server import Uri +from .client import ClientState from .config import SphinxConfig if typing.TYPE_CHECKING: - from .client import SphinxClient + from typing import Callable + from typing import Dict + from typing import Optional + from .client import SphinxClient -SphinxClientFactory = Callable[["SphinxManager"], "SphinxClient"] + SphinxClientFactory = Callable[["SphinxManager", "SphinxConfig"], "SphinxClient"] -class SphinxManager(LanguageFeature): +class SphinxManager(server.LanguageFeature): """Responsible for managing Sphinx application instances.""" def __init__(self, client_factory: SphinxClientFactory, *args, **kwargs): @@ -33,18 +33,18 @@ def __init__(self, client_factory: SphinxClientFactory, *args, **kwargs): self.client_factory = client_factory """Used to create new Sphinx client instances.""" - self.clients: Dict[Uri, SphinxClient] = {} + self.clients: Dict[str, Optional[SphinxClient]] = { + # Prevent any clients from being created in the global scope. + "": None, + } """Holds currently active Sphinx clients.""" - self._events = EventSource(self.logger) + self._events = server.EventSource(self.logger) """The SphinxManager can emit events.""" self._pending_builds: Dict[str, asyncio.Task] = {} """Holds tasks that will trigger a build after a given delay if not cancelled.""" - self._client_creating: Optional[asyncio.Task] = None - """If set, indicates we're in the process of setting up a new client.""" - self._progress_tokens: Dict[str, str] = {} """Holds work done progress tokens.""" @@ -56,7 +56,7 @@ async def document_change(self, params: lsp.DidChangeTextDocumentParams): return client = await self.get_client(uri) - if client is None or client.id is None: + if client is None: return # Cancel any existing pending builds @@ -78,7 +78,7 @@ async def document_save(self, params: lsp.DidSaveTextDocumentParams): return client = await self.get_client(uri) - if client is None or client.id is None: + if client is None: return # Cancel any existing pending builds @@ -90,20 +90,12 @@ async def document_save(self, params: lsp.DidSaveTextDocumentParams): async def shutdown(self, params: None): """Called when the server is instructed to ``shutdown``.""" - # Stop creating any new clients. - if self._client_creating and not self._client_creating.done(): - args = {} - if sys.version_info.minor > 8: - args["msg"] = "Server is shutting down" - - self.logger.debug("Aborting client creation") - self._client_creating.cancel(**args) - # Stop any existing clients. tasks = [] for client in self.clients.values(): - self.logger.debug("Stopping SphinxClient: %s", client) - tasks.append(asyncio.create_task(client.stop())) + if client: + self.logger.debug("Stopping SphinxClient: %s", client) + tasks.append(asyncio.create_task(client.stop())) await asyncio.gather(*tasks) @@ -118,7 +110,7 @@ async def trigger_build_after(self, uri: Uri, app_id: str, delay: float): async def trigger_build(self, uri: Uri): """Trigger a build for the relevant Sphinx application for the given uri.""" client = await self.get_client(uri) - if client is None or client.building: + if client is None or client.state != ClientState.Running: return # Pass through any unsaved content to the Sphinx agent. @@ -149,86 +141,50 @@ async def trigger_build(self, uri: Uri): async def get_client(self, uri: Uri) -> Optional[SphinxClient]: """Given a uri, return the relevant sphinx client instance for it.""" - # Wait until the new client is created - it might be the one we're looking for! - if self._client_creating: - await self._client_creating - - # Always check the fully resolved uri. - resolved_uri = uri.resolve() - - for src_uri, client in self.clients.items(): - if resolved_uri in (await client.get_src_uris()): - return client - - # For now assume a single client instance per srcdir. - # This *should* prevent us from spwaning endless client instances - # when given a file located near a valid Sphinx project - but not actually - # part of it. - in_src_dir = str(resolved_uri).startswith(str(src_uri)) - if in_src_dir: - # Of course, we can only tell if a uri truly is not in a project - # when the build file map is populated! - # if len(client.build_file_map) == 0: - return client - - # Create a new client instance. - self._client_creating = asyncio.create_task(self._create_client(uri)) - try: - return await self._client_creating - finally: - # Be sure to unset the task when it resolves - self._client_creating = None - - async def _create_client(self, uri: Uri) -> Optional[SphinxClient]: - """Create a new sphinx client instance.""" - # TODO: Replace with config subscription - await self.server.ready - config = self.server.configuration.get( - "esbonio.sphinx", SphinxConfig, scope=uri - ) - if config is None: + scope = self.server.configuration.scope_for(uri) + if scope not in self.clients: + self.logger.debug("No client found, creating new subscription") + self.server.configuration.subscribe( + "esbonio.sphinx", + SphinxConfig, + self._create_or_replace_client, + scope=uri, + ) + # The first few callers in a given scope will miss out, but that shouldn't matter + # too much return None - resolved = config.resolve(uri, self.server.workspace, self.logger) - if resolved is None: + if (client := self.clients[scope]) is None: + self.logger.debug("No applicable client for uri: %s", uri) return None - client = self.client_factory(self) + return await client - try: - await client.start(resolved) - except Exception as exc: - message = "Unable to start sphinx-agent" - self.logger.error(message, exc_info=True) - self.server.show_message(f"{message}: {exc}", lsp.MessageType.Error) + async def _create_or_replace_client( + self, event: server.ConfigChangeEvent[SphinxConfig] + ): + """Create or replace thesphinx client instance for the given config.""" - return None + # TODO: Handle replacement! + config = event.value - try: - sphinx_info = await client.create_application(resolved) - except Exception as exc: - message = "Unable to create sphinx application" - self.logger.error(message, exc_info=True) - self.server.show_message(f"{message}: {exc}", lsp.MessageType.Error) - - await client.stop() - return None + # Do not try and create clients in the global scope + if event.scope == "": + return - if client.src_uri is None: - self.logger.error("No src uri!") - await client.stop() - return None + resolved = config.resolve( + Uri.parse(event.scope), self.server.workspace, self.logger + ) + if resolved is None: + self.clients[event.scope] = None + return - self.server.lsp.notify("sphinx/appCreated", sphinx_info) - self.clients[client.src_uri] = client - return client + self.clients[event.scope] = self.client_factory(self, resolved) + self.logger.debug("Client created for scope %s", event.scope) async def start_progress(self, client: SphinxClient): """Start reporting work done progress for the given client.""" - if client.id is None: - return - token = str(uuid.uuid4()) self.logger.debug("Starting progress: '%s'", token) @@ -245,9 +201,6 @@ async def start_progress(self, client: SphinxClient): ) def stop_progress(self, client: SphinxClient): - if client.id is None: - return - if (token := self._progress_tokens.pop(client.id, None)) is None: return @@ -256,7 +209,7 @@ def stop_progress(self, client: SphinxClient): def report_progress(self, client: SphinxClient, progress: types.ProgressParams): """Report progress done for the given client.""" - if client.id is None: + if client.state not in {ClientState.Running, ClientState.Building}: return if (token := self._progress_tokens.get(client.id, None)) is None: diff --git a/lib/esbonio/tests/server/features/test_sphinx_manager.py b/lib/esbonio/tests/server/features/test_sphinx_manager.py index 6067c0633..36f1fdaf4 100644 --- a/lib/esbonio/tests/server/features/test_sphinx_manager.py +++ b/lib/esbonio/tests/server/features/test_sphinx_manager.py @@ -1,97 +1,266 @@ +from __future__ import annotations + import asyncio -from unittest.mock import AsyncMock -from unittest.mock import Mock +import io +import pathlib +import sys +import typing import pytest +import pytest_asyncio from lsprotocol import types as lsp from pygls.exceptions import JsonRpcInternalError +from pygls.server import StdOutTransportAdapter from esbonio.server import EsbonioLanguageServer -from esbonio.server import EsbonioWorkspace from esbonio.server import Uri -from esbonio.server.features.sphinx_manager import MockSphinxClient -from esbonio.server.features.sphinx_manager import SphinxConfig +from esbonio.server import create_language_server +from esbonio.server.features.sphinx_manager import ClientState from esbonio.server.features.sphinx_manager import SphinxManager -from esbonio.server.features.sphinx_manager import mock_sphinx_client_factory -from esbonio.sphinx_agent import types +from esbonio.server.features.sphinx_manager import make_subprocess_sphinx_client + +if typing.TYPE_CHECKING: + from typing import Any + from typing import Callable + from typing import Tuple + + ServerManager = Callable[[Any], Tuple[EsbonioLanguageServer, SphinxManager]] @pytest.fixture() -def workspace(uri_for): +def demo_workspace(uri_for): return uri_for("workspaces", "demo") @pytest.fixture() -def progress(): - p = Mock() - p.create_async = AsyncMock() - return p +def docs_workspace(uri_for): + return uri_for("..", "..", "..", "docs") -@pytest.fixture() -def server(event_loop, progress, workspace: Uri): - """A mock instance of the language""" - ready = asyncio.Future() - ready.set_result(True) - - esbonio = EsbonioLanguageServer(loop=event_loop) - esbonio._ready = ready - esbonio.lsp.progress = progress - esbonio.show_message = Mock() - esbonio.configuration.get = Mock(return_value=SphinxConfig()) - esbonio.lsp._workspace = EsbonioWorkspace( - None, - workspace_folders=[lsp.WorkspaceFolder(str(workspace), "workspace")], - ) - return esbonio +@pytest_asyncio.fixture() +async def server_manager(demo_workspace: Uri, docs_workspace): + """An instance of the language server and sphinx manager to use for the test.""" + loop = asyncio.get_running_loop() -@pytest.fixture() -def sphinx_info(workspace: Uri): - return types.SphinxInfo( - id="123", - version="6.0", - conf_dir=workspace.fs_path, - build_dir=(workspace / "_build" / "html").fs_path, - builder_name="html", - src_dir=workspace.fs_path, - dbpath=(workspace / "_build" / "html" / "esbonio.db").fs_path, - ) + esbonio = create_language_server(EsbonioLanguageServer, [], loop=loop) + esbonio.lsp.transport = StdOutTransportAdapter(io.BytesIO(), sys.stderr.buffer) + + manager = SphinxManager(make_subprocess_sphinx_client, esbonio) + esbonio.add_feature(manager) + + def initialize(init_options): + # Initialize the server. + esbonio.lsp._procedure_handler( + lsp.InitializeRequest( + id=1, + params=lsp.InitializeParams( + capabilities=lsp.ClientCapabilities(), + initialization_options=init_options, + workspace_folders=[ + lsp.WorkspaceFolder(uri=str(demo_workspace), name="demo"), + lsp.WorkspaceFolder(uri=str(docs_workspace), name="docs"), + ], + ), + ) + ) + + esbonio.lsp._procedure_handler( + lsp.InitializedNotification(params=lsp.InitializedParams()) + ) + return esbonio, manager + + yield initialize + + await manager.shutdown(None) @pytest.mark.asyncio -async def test_create_application_error(server, workspace: Uri): - """Ensure that we can handle errors during application creation correctly.""" +async def test_get_client( + server_manager: ServerManager, demo_workspace: Uri, tmp_path: pathlib.Path +): + """Ensure that we can create a SphinxClient correctly.""" - client = MockSphinxClient(JsonRpcInternalError("create sphinx application failed.")) - client_factory = mock_sphinx_client_factory(client) + server, manager = server_manager( + dict( + esbonio=dict( + sphinx=dict( + pythonCommand=[sys.executable], + buildCommand=["sphinx-build", "-M", "dirhtml", ".", str(tmp_path)], + ), + ), + ), + ) + # Ensure that the server is ready + await server.ready - manager = SphinxManager(client_factory, server) + result = await manager.get_client(demo_workspace / "index.rst") + # At least for now, the first call to get_client will not return a client + # but will instead "prime the system" ready to return a client the next + # time it is called. + assert result is None + + # Give the async tasks chance to complete. + await asyncio.sleep(0.5) + + # The system should be "primed" meaning there is a client created, but not + # running. + print(manager.clients) + client = manager.clients[str(demo_workspace)] + + assert client is not None + assert client.state is None + + # Now when we ask for the client, the client should be started and we should + # get back the same instance + result = await manager.get_client(demo_workspace / "index.rst") + + assert result is client + assert client.state == ClientState.Running - result = await manager.get_client(workspace / "index.rst") + # And that the client initialized correctly + assert client.builder == "dirhtml" + assert client.src_uri == demo_workspace + assert client.conf_uri == demo_workspace + assert client.build_uri == Uri.for_file(tmp_path / "dirhtml") + + +@pytest.mark.asyncio +async def test_get_client_with_error( + server_manager: ServerManager, demo_workspace: Uri +): + """Ensure that we correctly handle the case where there is an error with a client.""" + + server, manager = server_manager(None) + # Ensure that the server is ready + await server.ready + + result = await manager.get_client(demo_workspace / "index.rst") + # At least for now, the first call to get_client will not return a client + # but will instead "prime the system" ready to return a client the next + # time it is called. assert result is None - server.show_message.assert_called_with( - "Unable to create sphinx application: create sphinx application failed.", - lsp.MessageType.Error, - ) + # Give the async tasks chance to complete. + await asyncio.sleep(0.5) + + # The system should be "primed" meaning there is a client created, but not + # running. + print(manager.clients) + client = manager.clients[str(demo_workspace)] + + assert client is not None + assert client.state is None + + # Now when we ask for the client, the client should be started and we should + # get back the same instance + result = await manager.get_client(demo_workspace / "index.rst") + + assert result is client + assert client.state == ClientState.Errored + assert "No python environment configured" in str(client.exception) + + # Finally, if we request another uri from the same project we should get back + # the same client instance - even though it failed to start. + result = await manager.get_client(demo_workspace / "demo_myst.md") + assert result is client @pytest.mark.asyncio -async def test_trigger_build_error(sphinx_info, server, workspace): - """Ensure that we can handle build errors correctly.""" +async def test_get_client_with_many_uris( + server_manager: ServerManager, demo_workspace: Uri, tmp_path: pathlib.Path +): + """Ensure that when called in rapid succession, with many uris we only create a + single client instance.""" - client = MockSphinxClient( - sphinx_info, - build_result=JsonRpcInternalError("sphinx-build failed:"), + server, manager = server_manager( + dict( + esbonio=dict( + sphinx=dict( + pythonCommand=[sys.executable], + buildCommand=["sphinx-build", "-M", "dirhtml", ".", str(tmp_path)], + ), + ), + ), ) - client_factory = mock_sphinx_client_factory() - manager = SphinxManager(client_factory, server) - manager.clients = {workspace: client} + # Ensure that the server is ready + await server.ready - await manager.trigger_build(workspace / "index.rst") + src_uris = [Uri.for_file(f) for f in pathlib.Path(demo_workspace).glob("**/*.rst")] + coros = [manager.get_client(s) for s in src_uris] - server.show_message.assert_called_with( - "sphinx-build failed:", lsp.MessageType.Error - ) + # As with the other cases, this should only "prime" the system + result = await asyncio.gather(*coros) + assert all([r is None for r in result]) + + # There should only have been one client created (in addition to the 'dummy' global + # scoped client) + assert len(manager.clients) == 2 + + client = manager.clients[str(demo_workspace)] + assert client is not None + assert client.state == None + + # Now if we do the same again we should get the same client instance for each case. + coros = [manager.get_client(s) for s in src_uris] + result = await asyncio.gather(*coros) + + assert all([r is client for r in result]) + + client = result[0] + assert client.state == ClientState.Running + + # And that the client initialized correctly + assert client.builder == "dirhtml" + assert client.src_uri == demo_workspace + assert client.conf_uri == demo_workspace + assert client.build_uri == Uri.for_file(tmp_path / "dirhtml") + + +@pytest.mark.asyncio +async def test_get_client_with_many_uris_in_many_projects( + server_manager: ServerManager, + demo_workspace: Uri, + docs_workspace: Uri, +): + """Ensure that when called in rapid succession, with many uris we only create a + single client instance for each project.""" + + server, manager = server_manager( + dict( + esbonio=dict( + sphinx=dict(pythonCommand=[sys.executable]), + ), + ), + ) # Ensure that the server is ready + await server.ready + + src_uris = [Uri.for_file(f) for f in pathlib.Path(demo_workspace).glob("**/*.rst")] + src_uris += [Uri.for_file(f) for f in pathlib.Path(docs_workspace).glob("**/*.rst")] + coros = [manager.get_client(s) for s in src_uris] + + # As with the other cases, this should only "prime" the system + result = await asyncio.gather(*coros) + assert all([r is None for r in result]) + + # There should only have been one client created for each project (in addition to + # the 'dummy' global scoped client) + assert len(manager.clients) == 3 + + demo_client = manager.clients[str(demo_workspace)] + assert demo_client is not None + assert demo_client.state == None + + docs_client = manager.clients[str(docs_workspace)] + assert docs_client is not None + assert docs_client.state == None + + # Now if we do the same again we should get the same client instance for each case. + coros = [manager.get_client(s) for s in src_uris] + result = await asyncio.gather(*coros) + + assert all([(r is demo_client) or (r is docs_client) for r in result]) + + assert demo_client.state == ClientState.Running + assert docs_client.state == ClientState.Running