Several refactoring on asyncio eventloop implementations #545
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pynvim has been using asyncio as the only available event loop implementation, since python 3.4. Remove all the pyuv-related codes
event loops for Session and Nvim are final and can't be changed.
- Separate Protocol from AsyncioEventLoop (which were too complex). This makes it much clearer to discern which methods are asyncio- specific specializations of the abstract base event loop class (used for managing the lifecycle of event loop itself) and which serves as callback functions for IPC communication. - Document the design and the lifecycle of the BaseEventLoop class. Although asyncio is the only existing implementation, the current behavior or abstraction is documented (until further refactorings) to avoid confusions and clarify how the subclass should be implemented. - Use `typing.override` in the AsyncioEventLoop subclass (requires typing-extensions >= 4.5.0).
The proactor pipe transports for subprocess won't be automatically
closed, so "closed pipe" errors (pytest warnings) occur during garbage
collection (upon `__del__`). This results in a bunch of pytest warnings
whenever closing and freeing up fixture Nvim sessions. A solution is to
close all the internal `_ProactorBasePipeTransport` objects later when
closing the asyncio event loop.
Also, `_ProactorBasePipeTransport.close()` does not close immediately,
but rather works asynchronously; therefore the `__del__` finalizer still
can throw if called by GC after the event loop is closed. One solution
for properly closing the pipe transports is to await the graceful
shutdown of these transports.
Example CI output (the pytest warnings that are going to be fixed):
```
Exception ignored in: <function _ProactorBasePipeTransport.__del__ at 0x00000205288AD1C0>
Traceback (most recent call last):
File "C:\hostedtoolcache\windows\Python\3.11.5\x64\Lib\asyncio\proactor_events.py", line 116, in __del__
_warn(f"unclosed transport {self!r}", ResourceWarning, source=self)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\hostedtoolcache\windows\Python\3.11.5\x64\Lib\asyncio\proactor_events.py", line 80, in __repr__
info.append(f'fd={self._sock.fileno()}')
^^^^^^^^^^^^^^^^^^^
File "C:\hostedtoolcache\windows\Python\3.11.5\x64\Lib\asyncio\windows_utils.py", line 102, in fileno
raise ValueError("I/O operation on closed pipe")
ValueError: I/O operation on closed pipe
Exception ignored in: <function _ProactorBasePipeTransport.__del__ at 0x00000205288AD1C0>
Traceback (most recent call last):
File "C:\hostedtoolcache\windows\Python\3.11.5\x64\Lib\asyncio\proactor_events.py", line 116, in __del__
File "C:\hostedtoolcache\windows\Python\3.11.5\x64\Lib\asyncio\proactor_events.py", line 80, in __repr__
File "C:\hostedtoolcache\windows\Python\3.11.5\x64\Lib\asyncio\windows_utils.py", line 102, in fileno
ValueError: I/O operation on closed pipe
```
|
@justinmk I wished this were also included in 0.5.0. But since this is just a matter of refactoring (which was necessary to figure out and fix a very long-standing bug #543) we could include this in later versions (but I'm not sure if it makes much sense to merge towards "minor patch" versions such as 0.5.1 rather than 0.6.0). While I was working on some more drastic change where we completely remove the asyncio abstraction layer and greenlet (which is no longer necessary in modern python), this would be a good starting point so that I can continue refactoring and improving the codebase. |
💯 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Comments:
refactor!: completely wipe out pyuv
fix: prevent closed pipe errors on closing asyncio transport
This follows up #543 and eliminates all the pytest warnings due to Proactor eventloop.
Other minor code improvements, dependent for the above changes
If you prefer breaking this down into several independent PRs, I can also do it -- please let me know.