Fix websocket based servers

alcarney commented 2 years ago

This is an attempt at fixing #268

First this commit introduces a test that runs a websocket server in a separate thread. Note: While the test appears to work as intended, due to running the server in a separate thread I'm unable to reproduce the exact error I see in the original issue. I instead get a different error about there not being an event loop.

$ pytest tests/test_server_connection.py::test_ws_server
=============================================== test session starts ================================================
platform linux -- Python 3.10.6, pytest-7.1.3, pluggy-1.0.0
rootdir: /var/home/alex/Projects/pygls, configfile: pyproject.toml
plugins: typeguard-2.13.3, timeout-2.1.0, cov-3.0.0, asyncio-0.19.0, lsp-0.1.2
asyncio: mode=auto
collected 1 item                                                                                                   

tests/test_server_connection.py ^C

================================================= warnings summary =================================================
tests/test_server_connection.py::test_ws_server
  /var/home/alex/Projects/esbonio/.env/lib64/python3.10/site-packages/websockets/legacy/server.py:1006: DeprecationWarning: There is no current event loop
    loop = asyncio.get_event_loop()

tests/test_server_connection.py::test_ws_server
  /var/home/alex/Projects/esbonio/.env/lib64/python3.10/site-packages/_pytest/threadexception.py:73: PytestUnhandledThreadExceptionWarning: Exception in thread Thread-3 (start_ws)

  Traceback (most recent call last):
    File "/usr/lib64/python3.10/threading.py", line 1016, in _bootstrap_inner
      self.run()
    File "/usr/lib64/python3.10/threading.py", line 953, in run
      self._target(*self._args, **self._kwargs)
    File "/var/home/alex/Projects/pygls/pygls/server.py", line 308, in start_ws
      start_server = websockets.serve(connection_made, host, port)
    File "/var/home/alex/Projects/esbonio/.env/lib64/python3.10/site-packages/websockets/legacy/server.py", line 1006, in __init__
      loop = asyncio.get_event_loop()
    File "/usr/lib64/python3.10/asyncio/events.py", line 656, in get_event_loop
      raise RuntimeError('There is no current event loop in thread %r.'
  RuntimeError: There is no current event loop in thread 'Thread-3 (start_ws)'.

    warnings.warn(pytest.PytestUnhandledThreadExceptionWarning(msg))

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! KeyboardInterrupt !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
/usr/lib64/python3.10/selectors.py:469: KeyboardInterrupt
(to show a full traceback on KeyboardInterrupt use --full-trace)
=============================================== 2 warnings in 4.15s ================================================
Task was destroyed but it is pending!
task: <Task pending name='Task-3' coro=<test_ws_server() done, defined at /var/home/alex/Projects/pygls/tests/test_server_connection.py:78> wait_for=<Future pending cb=[Task.task_wakeup()]>>

Next, the fix appears to work as I'm able to use websocket based servers with it, but it appears to rely on deprecated features.

================================================= warnings summary =================================================
tests/test_server_connection.py::test_ws_server
  /var/home/alex/Projects/esbonio/.env/lib64/python3.10/site-packages/websockets/legacy/server.py:1009: DeprecationWarning: remove loop argument
    warnings.warn("remove loop argument", DeprecationWarning)

tests/test_server_connection.py::test_ws_server
  /var/home/alex/Projects/esbonio/.env/lib64/python3.10/site-packages/websockets/legacy/protocol.py:203: DeprecationWarning: remove loop argument
    warnings.warn("remove loop argument", DeprecationWarning)

tests/test_server_connection.py::test_ws_server
  /var/home/alex/Projects/pygls/pygls/server.py:148: RuntimeWarning: coroutine 'WebSocketCommonProtocol.close' was never awaited
    self._ws.close()
  Enable tracemalloc to get traceback where the object was allocated.
  See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info.

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
========================================== 1 passed, 3 warnings in 0.60s ===========================================

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
[ ] CONTRIBUTORS.md was updated, as appropriate
[ ] Changelog has been updated, as needed (see CHANGELOG.md)

tombh commented 2 years ago

Oh, so does this mean that nobody in the wild is actually using Pygls with websockets?

I could recreate the bug, but I didn't get the deprecation warning. Looking at the docs, do you think the deprecation warning is for favouring async/await?

alcarney commented 2 years ago

Oh, so does this mean that nobody in the wild is actually using Pygls with websockets?

Potentially... or maybe not that many people have upgraded to pygls 0.12 yet or there's a chance this is a platform specific issue? Pygls does fiddle with the event loop based on the platform (no idea why though... :thinking:) there's always a chance that has something to do with it....

do you think the deprecation warning is for favouring async/await?

There is a note in the changelog saying the loop parameter is deprecated everywhere as of version 10.x

tombh commented 2 years ago

Ah yes 0.12 and other platforms, so there are other factors. Do you know what the advantages are of the websocket over the normal socket server?

Ok, so the loop param is clearly out of the picture. And you're still looking for another way to fix the bug? I'm curious in general why the websocket server isn't started with await, I guess it's because it would require a bit of refactoring to make all the calling parent functions async?

alcarney commented 2 years ago

Do you know what the advantages are of the websocket over the normal socket server?

The main one I'm aware of is web browsers can speak websockets natively, so you can hook up a pygls server running somewhere with some web based ui - like the Monaco Editor.

And you're still looking for another way to fix the bug?

Not currently... the loop parameter is only deprecated so should be fine until at least v11.x of websockets, but it is something that will break on us in the future. To be honest, this low level async stuff is beyond me at the moment, I'm assuming some deeper architectural changes will be required to fix this "properly" - maybe involving websocket's Sans-IO stuff??

I'm curious in general why the websocket server isn't started with await

I think it's because there's nothing in start_ws the needs to be awaited? It seems to follow the same pattern as the other start_xxx functions (excluding pyodide of course) where it gives the event loop some function to run and starts it going, so it's almost like it's "outside" any async code.

tombh commented 2 years ago

The main one I'm aware of is web browsers can speak websockets natively, so you can hook up a pygls server running somewhere with some web based ui - like the Monaco Editor.

Ohh yes of course, and that's potentially connected to your Pyodide work too?

Not currently... the loop parameter is only deprecated so should be fine until at least v11.x of websockets, but it is something that will break on us in the future. To be honest, this low level async stuff is beyond me at the moment, I'm assuming some deeper architectural changes will be required to fix this "properly" - maybe involving websocket's Sans-IO stuff??

Sure, working for the near term is 1000% better than not working at all! Oh, hadn't heard of Sans-IO.

I think it's because there's nothing in start_ws the needs to be awaited? It seems to follow the same pattern as the other start_xxx functions (excluding pyodide of course) where it gives the event loop some function to run and starts it going, so it's almost like it's "outside" any async code.

I was wondering if an async function automatically invoked some kind of internal event loop magic in the background. But anyway, let's not worry about it for now.

alcarney commented 2 years ago

and that's potentially connected to your Pyodide work too?

Not really... websockets allow you to build a more traditional client-server application with your backend either running locally on your machine or some remote server.

Pyodide lets us run the server part direct in the webpage - no separate backend required

tombh commented 2 years ago

I see, thanks for the clarification :)

Are you waiting on a review?

alcarney commented 2 years ago

Yes, that would be good thanks :)

tombh commented 2 years ago

Oh, what happened here? Did I forget to review after you asked me to? I'm really sorry if that's what happened 😞

alcarney commented 2 years ago

Not your fault :smile: the review happened, but I never followed it up with a merge

tombh commented 2 years ago

Oh 😅

openlawlibrary / pygls

Fix websocket based servers #269

Code review checklist (for code reviewer to complete)