From fd4247ced2b536c82fe76c52a6a6042eebb31ad4 Mon Sep 17 00:00:00 2001
From: Jongwook Choi <wookayin@gmail.com>
Date: Sat, 14 Oct 2023 17:26:55 -0400
Subject: [PATCH] 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.
---
 pynvim/msgpack_rpc/event_loop/asyncio.py |  8 +++++++-
 test/conftest.py                         | 20 ++++++++++++++++++--
 test/test_host.py                        | 21 ++++++++++++++-------
 3 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/pynvim/msgpack_rpc/event_loop/asyncio.py b/pynvim/msgpack_rpc/event_loop/asyncio.py
index cea7298c..a506c927 100644
--- a/pynvim/msgpack_rpc/event_loop/asyncio.py
+++ b/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,8 @@ class AsyncioEventLoop(BaseEventLoop, asyncio.Protocol,
     """`BaseEventLoop` subclass that uses `asyncio` as a backend."""
 
     _queued_data: Deque[bytes]
+    if os.name != 'nt':
+        _child_watcher: Optional['asyncio.AbstractChildWatcher']
 
     def connection_made(self, transport):
         """Used to signal `asyncio.Protocol` of a successful connection."""
@@ -78,6 +80,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 +148,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)
diff --git a/test/conftest.py b/test/conftest.py
index 85e56ab6..9bbd5295 100644
--- a/test/conftest.py
+++ b/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
diff --git a/test/test_host.py b/test/test_host.py
index 016d48b7..18cff327 100644
--- a/test/test_host.py
+++ b/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()