Skip to content

Commit

Permalink
lsp: Use concurrent.futures.Future instead of asyncio.Future
Browse files Browse the repository at this point in the history
pygls in `v2.0a2` switched from using the low-level asyncio APIs to
using the high-level one and surprisingly, this broke `esbonio`.

The server would start ok, but wouldn't launch Sphinx processes, the
preview command did nothing, it wouldn't even produce any log
messages! That is, until the user changed a configuration setting.

After changing a setting - any setting, the server suddenly springs
into life, logs, previews, sphinx processes all of them would start
working as if nothing was wrong.

I eventually managed to figure out that the callbacks registered by
the configuration system on the server's `ready` future were never
being called.

```
self.server.ready.add_done_callback(self._notify_subscriptions)
```

By why would changing the asyncio API in use break these callbacks?!
After spending some time with the debugger I eventually spotted the culprit

```
>>> asyncio.get_running_loop()
<_UnixSelectorEventLoop running=True closed=False debug=False>

>>> self.server.ready._loop
<_UnixSelectorEventLoop running=False closed=False debug=False>
```

The server's `ready` future was using a different event loop and
because the event loop is not running, when the future is resolved,
the callbacks were never scheduled!

While I cannot explain why this was not an issue before, I can explain
why it is an issue now.

The `ready` future is created in the constructor of the
`EsbonioLanguageServer` class - before any event loop has been created
and so it uses a new one based on the current event loop policy[1][2].

Unfortunately, when pygls later starts its main loop by calling
`asyncio.run()` it creates a new event loop, orphaning the `ready`
future in its unused event loop[3]

Since we don't need to use the `ready` future asynchronously, the
simplest fix is to convert it to future from the `concurrent.futures`
module, removing the need for an event loop completely.

[1]: https://github.com/python/cpython/blob/307c63358681d669ae39e5ecd814bded4a93443a/Lib/asyncio/futures.py#L79
[2]: https://github.com/python/cpython/blob/307c63358681d669ae39e5ecd814bded4a93443a/Lib/asyncio/events.py#L791
[3]: https://github.com/python/cpython/blob/307c63358681d669ae39e5ecd814bded4a93443a/Lib/asyncio/runners.py#L146
  • Loading branch information
alcarney committed Nov 24, 2024
1 parent 73f6f2c commit ed387b4
Showing 1 changed file with 3 additions and 2 deletions.
5 changes: 3 additions & 2 deletions lib/esbonio/esbonio/server/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import platform
import traceback
import typing
from concurrent.futures import Future
from typing import TypeVar
from uuid import uuid4

Expand Down Expand Up @@ -79,7 +80,7 @@ def __init__(self, logger: logging.Logger | None = None, *args, **kwargs):
self._features: dict[type[LanguageFeature], LanguageFeature] = {}
"""The collection of language features registered with the server."""

self._ready: asyncio.Future[bool] = asyncio.Future()
self._ready: Future[bool] = Future()
"""Indicates if the server is ready."""

self._tasks: set[asyncio.Task] = set()
Expand All @@ -95,7 +96,7 @@ def __iter__(self):
return iter(self._features.items())

@property
def ready(self) -> asyncio.Future:
def ready(self) -> Future[bool]:
return self._ready

@property
Expand Down

0 comments on commit ed387b4

Please sign in to comment.