Skip to content

Add TCP support to pygls' LanguageClient#501

Merged
alcarney merged 9 commits into
openlawlibrary:mainfrom
alcarney:tcp-client
Oct 9, 2024
Merged

Add TCP support to pygls' LanguageClient#501
alcarney merged 9 commits into
openlawlibrary:mainfrom
alcarney:tcp-client

Conversation

@alcarney

@alcarney alcarney commented Oct 7, 2024

Copy link
Copy Markdown
Collaborator

Description (e.g. "Related to ...", etc.)

This PR adds a start_tcp method to pygls' LanguageClient. This method uses the high-level asyncio APIs I would like to transition the server side over to using (as mentioned in #334).

However, before touching the server-side code I thought it would be best to add tests around the current implementation. So this PR also extends pygls' test suite to add a --lsp-transport command line argument.

  • --lsp-transport stdio (the default), runs the end-to-end tests over stdin/stdout.
  • --lsp-transport tcp, runs the end-to-end tests over a TCP connection

The poe test task used in CI has been updated to run both style of end-to-end tests.

Finaly, this PR also includes some smaller client related fixes

  • When the server process exits (in stdio mode), any pending requests are automatically cancelled
  • In stdio mode, the client waits for the server process to exit rather than agressively terminating it (fixing coverage reporting)

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 promotes the `JsonRPCProtocol._procedure_handler` and
`JsonRPCProtocol._deserialize_message` methods to be public methods.
It also renames them to `handle_message` and `structure_message`
respectively.
Given any `asyncio.StreamReader`, it should be possible to create a
main async message handling loop over a compatible transport.
This allows pygls' LanguageClient to communicate with servers over a
TCP connection
This fixes coverage reporting, but has the chance for the process to
hang. This can be worked around by using `asyncio.wait_for()`

```python
await asyncio.wait_for(client.stop(), timeout=5.0) # seconds
```
This takes the simple cli wrapper from the example `json_server.py`
and puts it into a new `pygls.cli` module.

The wrapper makes it easier to select which transport mechanism the
server should use. It's primary intention is simply to reduce the
amount of boilerplate required to start one of the example servers
This extends pygls' test suite with a `--lsp-transport` command line
argument.

- `--lsp-transport stdio` (the default), runs the end-to-end tests
  over stdin/stdout.
- `--lsp-transport tcp`, runs the end-to-end tests over a TCP
  connection

The `poe test` task used in CI has been updated to run both style of
end-to-end tests.
@alcarney

alcarney commented Oct 8, 2024

Copy link
Copy Markdown
Collaborator Author

Running the end-to-end tests over TCP has highlighted an interesting bug (#502). Since it isn't really related to the client's new TCP capability I've skipped the test for now and we can figure out a fix for it separately - who knows maybe future server side refactorings will magically fix it? 😅 🙏

@alcarney alcarney requested a review from tombh October 8, 2024 18:32

@tombh tombh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work.

@alcarney

alcarney commented Oct 9, 2024

Copy link
Copy Markdown
Collaborator Author

Thanks!

@alcarney alcarney merged commit ff8bea2 into openlawlibrary:main Oct 9, 2024
@alcarney alcarney deleted the tcp-client branch October 9, 2024 20:17
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.

2 participants