fix: do not leak resources across tests so as to prevent side effects

Problem:
Across unit tests, custom path_hook reigstered globally by
ScriptHost globally was not being cleared up properly. This results in
leakage of internal resources and dangling access to them (such as
asyncio event loops that was already closed and no longer valid in other
test case instances).

More specifically, the asyncio EventLoop were never closed, which can
result in "Event loop is closed" errors upon garbage collection of
internal transport objects (during cleaning up pytest sessions).

Solution:
(1) Always call ScriptHost.teardown() when done using it.
(2) Make sure all internal resources (event loops, transport channels,
    etc.) are closed by closing the embedded neovim instance.

Author: wookayin

pynvim/msgpack_rpc/event_loop/asyncio.py

@@ -14,7 +14,7 @@
 import sys
 from collections import deque
 from signal import Signals
-from typing import Any, Callable, Deque, List
+from typing import Any, Callable, Deque, List, Optional
 
 from pynvim.msgpack_rpc.event_loop.base import BaseEventLoop
 
@@ -37,6 +37,7 @@ class AsyncioEventLoop(BaseEventLoop, asyncio.Protocol,
     """`BaseEventLoop` subclass that uses `asyncio` as a backend."""
 
     _queued_data: Deque[bytes]
+    _child_watcher: Optional[asyncio.AbstractChildWatcher]
 
     def connection_made(self, transport):
         """Used to signal `asyncio.Protocol` of a successful connection."""
@@ -78,6 +79,7 @@ def _init(self) -> None:
         self._queued_data = deque()
         self._fact = lambda: self
         self._raw_transport = None
+        self._child_watcher = None
 
     def _connect_tcp(self, address: str, port: int) -> None:
         coroutine = self._loop.create_connection(self._fact, address, port)
@@ -145,6 +147,9 @@ def _close(self) -> None:
         if self._raw_transport is not None:
             self._raw_transport.close()
         self._loop.close()
+        if self._child_watcher is not None:
+            self._child_watcher.close()
+            self._child_watcher = None
 
     def _threadsafe_call(self, fn: Callable[[], Any]) -> None:
         self._loop.call_soon_threadsafe(fn)

test/conftest.py

@@ -1,5 +1,9 @@
+"""Configs for pytest."""
+
+import gc
 import json
 import os
+from typing import Generator
 
 import pytest
 
@@ -9,7 +13,10 @@
 
 
 @pytest.fixture
-def vim() -> pynvim.Nvim:
+def vim() -> Generator[pynvim.Nvim, None, None]:
+    """Create an embedded, sub-process Nvim fixture instance."""
+    editor: pynvim.Nvim
+
     child_argv = os.environ.get('NVIM_CHILD_ARGV')
     listen_address = os.environ.get('NVIM_LISTEN_ADDRESS')
     if child_argv is None and listen_address is None:
@@ -28,4 +35,13 @@ def vim() -> pynvim.Nvim:
         assert listen_address is not None and listen_address != ''
         editor = pynvim.attach('socket', path=listen_address)
 
-    return editor
+    try:
+        yield editor
+
+    finally:
+        # Ensure all internal resources (pipes, transports, etc.) are always
+        # closed properly. Otherwise, during GC finalizers (__del__) will raise
+        # "Event loop is closed" error.
+        editor.close()
+
+        gc.collect()  # force-run GC, to early-detect potential leakages

test/test_host.py

@@ -11,15 +11,19 @@
 
 def test_host_imports(vim):
     h = ScriptHost(vim)
-    assert h.module.__dict__['vim']
-    assert h.module.__dict__['vim'] == h.legacy_vim
-    assert h.module.__dict__['sys']
+    try:
+        assert h.module.__dict__['vim']
+        assert h.module.__dict__['vim'] == h.legacy_vim
+        assert h.module.__dict__['sys']
+    finally:
+        h.teardown()
 
 
 def test_host_import_rplugin_modules(vim):
     # Test whether a Host can load and import rplugins (#461).
     # See also $VIMRUNTIME/autoload/provider/pythonx.vim.
     h = Host(vim)
+
     plugins: Sequence[str] = [  # plugin paths like real rplugins
         os.path.join(__PATH__, "./fixtures/simple_plugin/rplugin/python3/simple_nvim.py"),
         os.path.join(__PATH__, "./fixtures/module_plugin/rplugin/python3/mymodule/"),
@@ -56,7 +60,10 @@ def test_host_async_error(vim):
 
 def test_legacy_vim_eval(vim):
     h = ScriptHost(vim)
-    assert h.legacy_vim.eval('1') == '1'
-    assert h.legacy_vim.eval('v:null') is None
-    assert h.legacy_vim.eval('v:true') is True
-    assert h.legacy_vim.eval('v:false') is False
+    try:
+        assert h.legacy_vim.eval('1') == '1'
+        assert h.legacy_vim.eval('v:null') is None
+        assert h.legacy_vim.eval('v:true') is True
+        assert h.legacy_vim.eval('v:false') is False
+    finally:
+        h.teardown()