Skip to content

Commit

Permalink
fix: do not leak resources across tests so as to prevent side effects
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
wookayin committed Oct 15, 2023
1 parent 964e6b1 commit fd4247c
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 10 deletions.
8 changes: 7 additions & 1 deletion pynvim/msgpack_rpc/event_loop/asyncio.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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."""
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
20 changes: 18 additions & 2 deletions test/conftest.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
"""Configs for pytest."""

import gc
import json
import os
from typing import Generator

import pytest

Expand All @@ -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:
Expand All @@ -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
21 changes: 14 additions & 7 deletions test/test_host.py
Original file line number Diff line number Diff line change
Expand Up @@ -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/"),
Expand Down Expand Up @@ -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()

0 comments on commit fd4247c

Please sign in to comment.