Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pygls does not handle async user shutdown handler correctly #433

Open
alcarney opened this issue Jan 26, 2024 · 1 comment · May be fixed by #516
Open

pygls does not handle async user shutdown handler correctly #433

alcarney opened this issue Jan 26, 2024 · 1 comment · May be fixed by #516
Labels
bug Something isn't working

Comments

@alcarney
Copy link
Collaborator

If I register an async handler for the shutdown request, pygls does not guarantee that it will finish executing before responding to the client. This often leads to the client sending the exit notification and pygls calling sys.exit() before my code cleaned up everything it needs to.

I'm finding this is leading to a lot of orphaned esbonio processes hanging around on my machine! 😅
image

I think the issue is down to how pygls handles the user registering features that "shadow" built in features.

@functools.wraps(base_func)
def decorator(self, *args, **kwargs):
ret_val = base_func(self, *args, **kwargs)
try:
user_func = self.fm.features[method_name]
self._execute_notification(user_func, *args, **kwargs)
except KeyError:
pass
except Exception:
logger.exception(
'Failed to handle user defined notification "%s": %s', method_name, args
)
return ret_val

If we look at how _execute_notification is implemented

def _execute_notification(self, handler, *params):
"""Executes notification message handler."""
if asyncio.iscoroutinefunction(handler):
future = asyncio.ensure_future(handler(*params))
future.add_done_callback(self._execute_notification_callback)
else:
if is_thread_function(handler):
self._server.thread_pool.apply_async(handler, (*params,))
else:
handler(*params)

It uses asyncio.ensure_future to schedule the coroutine to be executed, but then will return almost immediately - meaning there's no guarantee it will complete before pygls responds to the client.

@alcarney alcarney added the bug Something isn't working label Jan 26, 2024
@alcarney
Copy link
Collaborator Author

I think I have an idea that will fix this, but it will probably have to go into #418.

If we allow pygls' built-in features to yield, there's an opportunity to run the user's code in the middle of the method, before resuming the built-in to run to completion.

Not only would this fix this issue, but it might help with #381, where pygls can do some initial setup e.g. initialize the workspace, set client capabilities on the server object etc. and then yield to the user's code - allowing them to register some extra features, before resuming to compute the server's capabilities and eventually respond to the client

alcarney added a commit to alcarney/esbonio that referenced this issue Feb 10, 2024
This is likely not enough to fully resolve the shutdown issue (as that
requires a fix in `pygls`[1]) but at least now the server will make an
attempt to shut itself down.

[1]: openlawlibrary/pygls#433
alcarney added a commit to swyddfa/esbonio that referenced this issue Feb 10, 2024
This is likely not enough to fully resolve the shutdown issue (as that
requires a fix in `pygls`[1]) but at least now the server will make an
attempt to shut itself down.

[1]: openlawlibrary/pygls#433
alcarney added a commit to alcarney/esbonio that referenced this issue Sep 17, 2024
The most recent release of pytest-lsp now surfaces the above issue in
the test suite. So that esbonio is not blocked on a fix, wrap the call
to `shutdown_session` in `asyncio.wait_for` and ignore the timeout error.
alcarney added a commit to alcarney/esbonio that referenced this issue Sep 17, 2024
The most recent release of pytest-lsp now surfaces the above issue in
the test suite. So that esbonio is not blocked on a fix, wrap the call
to `shutdown_session` in `asyncio.wait_for` and ignore the timeout error.
alcarney added a commit to swyddfa/esbonio that referenced this issue Sep 17, 2024
The most recent release of pytest-lsp now surfaces the above issue in
the test suite. So that esbonio is not blocked on a fix, wrap the call
to `shutdown_session` in `asyncio.wait_for` and ignore the timeout error.
alcarney added a commit to alcarney/pygls that referenced this issue Dec 1, 2024
The underlying cause of openlawlibrary#433 is that pygls' current implementation of
builtin feature handlers cannot guarantee that an async user handler
will finish executing before pygls responds with the answer generated
from the builtin handler.

This commit adds support for another execution model, generators.
A generator handler can yield to another sub-handler method like so

```
yield handler_func, args, kwargs
```

The `JsonRPCProtocol` class with then schedule the execution of
`handler_func(*args, **kwargs)` as if it were a normal handler
function (meaning `handler_func could be async, threaded, sync or a
generator itself!)

The result of the sub-handler is then sent back into the generator
handler allowing the top-level handler to continue and even make use
of the result!

This gives pygls' built-in handlers much greater control over exactly
when a user handler is called, allowing us to fix openlawlibrary#433 and opens up a
lot other exciting possibilities!

This also removes the need for the `LSPMeta` metaclass, so it and the
corresponding module have been deleted.
@alcarney alcarney linked a pull request Dec 4, 2024 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
1 participant