openlawlibrary/pygls

Improve `asyncio` event loop handling

Opened this issue · 5 comments

alcarney commented

I've been struggling to track down the source of this DeprecationWarning when using pytest-lsp with the latest pytest-asyncio

DeprecationWarning: pytest-asyncio detected an unclosed event loop when tearing down the event_loop
  fixture: <_UnixSelectorEventLoop running=False closed=False debug=False>
  pytest-asyncio will close the event loop for you, but future versions of the
  library will no longer do so. In order to ensure compatibility with future
  versions, please make sure that:
      1. Any custom "event_loop" fixture properly closes the loop after yielding it
      2. Your code does not modify the event loop in async fixtures or tests
  
    warnings.warn(

Long story short, I think I've finally tracked it down to the following block of code.

pygls/pygls/server.py

Lines 187 to 192 in fad812a

if IS_WIN:
asyncio.set_event_loop(asyncio.ProactorEventLoop())
elif not IS_PYODIDE:
asyncio.set_event_loop(asyncio.SelectorEventLoop())
self.loop = loop or asyncio.new_event_loop()

When giving pygls a specific loop to use, it goes ahead and creates a new one anyway, resulting in a second, unused event loop that pytest-asyncio is correctly warning about.
While tweaking the code above to something like

        if loop is None and IS_WIN:
            asyncio.set_event_loop(asyncio.ProactorEventLoop())
        elif loop is None and not IS_PYODIDE:
            asyncio.set_event_loop(asyncio.SelectorEventLoop())

        self.loop = loop or asyncio.new_event_loop()

appears to resolve the issue. I do wonder whether it's worth revisiting how pygls handles event loops more generally.

As far as I can tell, most of pygls' event loop code was written pre-Python 3.7 where the current high level asyncio APIs didn't exist.

I guess is this is a long winded way of asking - can anyone think of any use cases that require pygls to stick with the current low-level APIs? I admit, despite pytest-lsp forcing me to care more about event loops, I still don't fully understand them 😅

alcarney commented

I might just try migrating pygls over anyway to see if it would even work, but it's also probably worth checking to see what people's thoughts were on this 🙂

tombh commented

That's a lot of debugging! Really awesome that you're getting to the bottom of this. I think it'd be great to modernise Pygls' async code. Though of course I'm not deeply familiar with the current design decisions. The fact that you've built up so much momentum in this area now, I'm sure you'd do a great job of migrating to the high level APIs.

alcarney commented

Ok, as promised in #375, some thoughts on where I'd like to try and take this.

JsonRPCProtocol

  • I don't think we need JsonRPCProtocol to derive from asyncio.Protocol anymore
  • Instead, by relying on the high level Reader and Writer objects, we should be able to reuse the same main loop for everything and stop parsing messages twice
  • As well as cleaning up some deprecation warnings it should make it much easier to make type checkers like mypy happy as we won't be "misusing" interfaces like BaseTransport
  • I'd also like to experiment with having JsonRPCProtocol be based on asyncio.Tasks instead of futures, but I'm fully prepared to back off from this if looks like we'd lose something significant

Servers

  • Have an auto-generated BaseLanguageServer (just like BaseLanguageClient), that way we should have consistent methods etc and resolve #306, by removing the need for methods to be implemented on LanguageServerProtocol itself.
  • Have a new JsonRPCServer class (mirroring JsonRPCClient) that replaces Server and uses the high-level asyncio APIs
  • We could also use it as an opportunity to experiment with some alternative approaches to registering features and hopefully start to resolve some of the issues that have been building up

I'm sure I've forgotten a few things but those should be the headlines.

I appreciate it will take some time to implement all of that! I was thinking we could develop most, if not all of this side-by-side with the existing stuff. That way we can incrementally ship the new approach and get feedback, then, (assuming it all works out!) deprecate and eventually remove the existing approach in a v2 release.

I'm not saying I want to rewrite the world! But in cleaning up some of the inconsistencies we will wind up introducing some breaking changes - but we can try to make them as minimal as possible.

Let me know your thoughts! 😄

tombh commented

I don't have any particular technical feedback here. The main thing is that I totally support a v2. All your suggestions sound really positive and that they have wider benefits to the codebase. Pygls doesn't actually have a particularly large codebase, so maybe rewriting the world isn't such a bad perspective to have!

My initial reason for separating protocol.py into smaller, class-based files (apart from just making it more digestible) was to make it easier to piecemeal add strict typing. I was wondering how much of your suggestions here I could contribute to as I was doing that, but I'm not sure I have enough of an insight. But I'll definitely be open to it. And even if a lot of the code is just going to be replaced anyway, I'll add the types anyway, I don't want to put any expectations on you.

Have an auto-generated BaseLanguageServer (just like BaseLanguageClient)

This sounds awesome 🤓

BTW, did we ever talk about putting the client code in the test folder? Not saying we should, just wondering about your thoughts on it. My thinking is that it's only use is for testing, so it would make more sense amongst the other test code. But it probably has other uses right?

alcarney commented

I was wondering how much of your suggestions here I could contribute to as I was doing that, but I'm not sure I have enough of an insight. But I'll definitely be open to it.

Great thanks! Let me know if you want me to expand on anything

BTW, did we ever talk about putting the client code in the test folder?

Possibly? 🤔

I think it's fine where it is, it's a solid base for the client side of the LSP protocol and while pytest-lsp might be the primary consumer of it for now. There's nothing stopping someone from attempting to create a "proper" language client based off of it 😄

  • ❤️1