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

[WIP] Introduce support for generator feature handlers #516

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

alcarney
Copy link
Collaborator

@alcarney alcarney commented Dec 4, 2024

This isn't quite ready to be merged just yet, but I may as well open it now for some initial feedback. It started out as an attempt to fix #433, but this approach should also allow us to also implement #381 (just needs some more testing).

This PR unifies all handler execution under a common pattern and adds support for a new feature handler type - generator functions.

These generators may yield a new handler function plus the args and kwargs to call it with. The yielded handler function is executed via pygls' (now standardised) execution framework meaning it can be sync, async, threaded or even another generator! Once complete the result of the yielded handler is sent back into the generator so it may continue. (JsonRPCProtocol._run_generator)

This PR also re-implements all of pygls' built-in feature handlers as generator functions -
removing the need for the LSPMeta metaclass and associated call_user_feature wrapper.

While this does mean every built-in handler must explicitly yield to the user's handler function, it does mean the built-in handlers get fine grained control over exactly when the user's handler is called - depending on what makes the most sense.

Before

    @lsp_method(types.SHUTDOWN)
    def lsp_shutdown(self: JsonRPCProtocol, *args) -> None:
        """Request from client which asks server to shutdown."""

        if (user_handler := self.fm.features.get(types.SHUTDOWN)) is not None:
            yield user_handler, args, None

        # Don't cancel the future for this request!
        current_id = self.msg_id

        for msg_id, future in self._request_futures.items():
            if msg_id != current_id and not future.done():
                future.cancel()

        self._shutdown = True
        return None

After

    @lsp_method(types.TEXT_DOCUMENT_DID_CHANGE)
    def lsp_text_document__did_change(self, params: types.DidChangeTextDocumentParams):
        for change in params.content_changes:
            self.workspace.update_text_document(params.text_document, change)

        if (user_handler := self.fm.features.get(types.TEXT_DOCUMENT_DID_CHANGE)) is not None:
            yield user_handler, (params,), None

During

    @lsp_method(types.INITIALIZE)
    def lsp_initialize(
        self, params: types.InitializeParams
    ) -> Generator[Any, Any, types.InitializeResult]:

        # Initialize the workspace before yielding to the user's initialize handler
        workspace_folders = params.workspace_folders or []
        self._workspace = Workspace(
            root_uri,
            text_document_sync_kind,
            workspace_folders,
            position_encoding,
        )

        if (user_handler := self.fm.features.get(types.INITIALIZE)) is not None:
            yield user_handler, (params,), None

        # Now that the user has had the opportunity to setup additional features, calculate
        # the server's capabilities
        self.server_capabilities = ServerCapabilitiesBuilder(
            self.client_capabilities,
            set({**self.fm.features, **self.fm.builtin_features}.keys()),
            self.fm.feature_options,
            list(self.fm.commands.keys()),
            text_document_sync_kind,
            notebook_document_sync,
            position_encoding,
        ).build()

        return types.InitializeResult(
            capabilities=self.server_capabilities,
            server_info=self.server_info,
        )

Other details

  • For consistency, all handlers (including synchronous handlers!) are wrapped in a future
  • Notifications and request handlers now use the same code path, (JsonRPCProtocol._execute_handler) their differences are handled by the callback passed to the future.
  • If available, the current id of the request being handled is now available via a ContextVar (JsonRPCProtocol.msg_id)

Code review checklist (for code reviewer to complete)

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Commit messages are meaningful (see this for details)
  • Tests have been included and/or updated, as appropriate
  • Docstrings have been included and/or updated, as appropriate
  • Standalone docs have been updated accordingly

Automated linters

You can run the lints that are run on CI locally with:

poetry install --all-extras --with dev
poetry run poe lint

This commit removes the separate handling of requests and
notifications, opting instead for a generic `_execute_handler` method
that can handle both types.

The difference between the two can be captured in how the results of
the handlers are processed. Request handlers are given a callback that
send the result onto the client, while notification handlers are
simply checked for errors.

This commit also uses a future to report the result of a synchronous
handler function. While not necessary, it allows synchronous handlers
to treated the same as the other async execution types, simplifying
the code overall.
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.
This commit makes use of a `ContextVar` to keep track of the current
request's id, allowing handlers to reference it. Most importantly so
that the shutdown request handler does not cancel its own future!
This should enable the dynamic registration of features during
initialization, as discussed in openlawlibrary#381
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pygls does not handle async user shutdown handler correctly
1 participant