lsp: Refactor SphinxManager

This commit builds on the changes introduced to the
`SubprocessSphinxClient` to greatly simplify and improve the
robustness of managing the set of `SphinxClients` in use by the
language server.

There were a few drawbacks to the previous approach

- While await on the `_client_creating` task during client creation
  appeared to work, it still often led to the creation of duplicated
  clients.

- By not recognizing when a given configuration was broken, the
  manager would continually attempt to spawn client instances.

- It was not uncommon to enter an infinite loop of spawning clients!

The new system introduced in this commit *shouldn't* suffer from any
of the above issues.

- Instead of indexing clients by `src_uri`, they are instead indexed
  by the scope of the configuration governing the instance. While
  still not perfect, we should be able to accurately determine the
  relevant client for a given uri in more scenarios.

- To indicate that we are unable to create a client for a given
  config we now store a `None` in the clients dictionary - preventing
  any further attempts at spawning a client.

- Taking advantage of the fact clients are indexed by scope and that
  the client itself is `await`-able, we can have multiple consumers of
  the client awaiting on the client - preventing the issue of
  duplicated clients.

Currently, there is a quirk to the system where the first batch of
callers to `get_client` in a given scope will be told there is no
client. However, they have in fact "primed" the system by creating a
config subscription which will asynchronously populate the `clients`
dict with an unstarted client.

The next batch of callers will then discover that there is a client
instance available and `await` it, starting the underlying Sphinx
process.

Based on some initial testing this "quirk" may actually be desirable,
the `get_client` method is called often enough that I wasn't able to
notice a real delay in starting the Sphinx process up. It also has the
nice property that when restarting the server with multiple projects
open, the flood of `didOpen` notifications from the client is enough
to "prime" the system for each project, but the project itself is only
activated when working on the relevant file in the editor.

One final note, there are plenty of improvements still to be made -
error handling, config changes etc, but hopefully this is a solid base
we can start building on.

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",
 )

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: