From 7f0f4fb1bec2b342fe55c127f9e0acf79a427ff0 Mon Sep 17 00:00:00 2001 From: Jongwook Choi Date: Thu, 9 Jan 2025 19:48:48 -0500 Subject: [PATCH] fix: improve EOFError exception when remote nvim crashes Problem: When the remote Nvim instance is aborted or terminates, the pynvim client will face an exception `OSError: EOF`. It is not very clear what is wrong and why EOF is received. Solution: This happens when the remote the nvim process terminates unexpectedly (or a tcp/file socket is broken or gets disconnected). We can provide a bit more detailed information about why the asyncio session is stopping, through asyncio Protocol. An `EOFError` will be raised instead of OSError. For example, during pynvim's unit tests we may see: ``` EOFError: process_exited: pid = 40000, return_code = -6 ``` which means that the Nvim subprocess (pid = 40000) exited unexpectedly after getting SIGABRT (signal 6). Other error messages (different signals such as SIGSEGV/segfault) or connection lost (when connected through socket, etc.) are also possible. --- pynvim/msgpack_rpc/event_loop/asyncio.py | 28 ++++++++++++++++++------ pynvim/msgpack_rpc/event_loop/base.py | 14 ++++++++++-- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/pynvim/msgpack_rpc/event_loop/asyncio.py b/pynvim/msgpack_rpc/event_loop/asyncio.py index d4ad1413..a0ed2080 100644 --- a/pynvim/msgpack_rpc/event_loop/asyncio.py +++ b/pynvim/msgpack_rpc/event_loop/asyncio.py @@ -47,13 +47,14 @@ def __init__(self, on_data, on_error): @override def connection_made(self, transport): """Used to signal `asyncio.Protocol` of a successful connection.""" - del transport # no-op + self._transport = transport @override def connection_lost(self, exc: Optional[Exception]) -> None: """Used to signal `asyncio.Protocol` of a lost connection.""" - debug(f"connection_lost: exc = {exc}") - self._on_error(exc if exc else EOFError()) + warn(f"connection_lost: exc = {exc}") + + self._on_error(exc if exc else EOFError("connection_lost")) @override def data_received(self, data: bytes) -> None: @@ -63,11 +64,19 @@ def data_received(self, data: bytes) -> None: @override def pipe_connection_lost(self, fd: int, exc: Optional[Exception]) -> None: """Used to signal `asyncio.SubprocessProtocol` of a lost connection.""" - debug("pipe_connection_lost: fd = %s, exc = %s", fd, exc) + + assert isinstance(self._transport, asyncio.SubprocessTransport) + debug_info = {'fd': fd, 'exc': exc, 'pid': self._transport.get_pid()} + warn(f"pipe_connection_lost {debug_info}") + if os.name == 'nt' and fd == 2: # stderr # On windows, ignore piped stderr being closed immediately (#505) return - self._on_error(exc if exc else EOFError()) + + # pipe_connection_lost() *may* be called before process_exited() is + # called, when a Nvim subprocess crashes (SIGABRT). Do not handle + # errors here, as errors will be handled somewhere else + # self._on_error(exc if exc else EOFError("pipe_connection_lost")) @override def pipe_data_received(self, fd, data): @@ -81,8 +90,13 @@ def pipe_data_received(self, fd, data): @override def process_exited(self) -> None: """Used to signal `asyncio.SubprocessProtocol` when the child exits.""" - debug("process_exited") - self._on_error(EOFError()) + assert isinstance(self._transport, asyncio.SubprocessTransport) + pid = self._transport.get_pid() + return_code = self._transport.get_returncode() + + warn("process_exited, pid = %s, return_code = %s", pid, return_code) + err = EOFError(f"process_exited: pid = {pid}, return_code = {return_code}") + self._on_error(err) class AsyncioEventLoop(BaseEventLoop): diff --git a/pynvim/msgpack_rpc/event_loop/base.py b/pynvim/msgpack_rpc/event_loop/base.py index c7def3e1..430af7a6 100644 --- a/pynvim/msgpack_rpc/event_loop/base.py +++ b/pynvim/msgpack_rpc/event_loop/base.py @@ -194,6 +194,13 @@ def run(self, data_cb: Callable[[bytes], None]) -> None: signal.signal(signal.SIGINT, default_int_handler) self._on_data = None + # eventloop was stopped due to an error, re-raise it + # (e.g. connection lost when subprocess nvim dies) + if self._error: + # Note: traceback is not preserved and attached for some reason, + # should be somewhere from msgpack_rpc.event_loop.asyncio.Protocol + raise self._error + @abstractmethod def _run(self) -> None: raise NotImplementedError() @@ -234,8 +241,11 @@ def _on_signal(self, signum: signal.Signals) -> None: self.stop() def _on_error(self, exc: Exception) -> None: - debug(str(exc)) - self._error = exc + warn('on_error: %s', repr(exc)) + if self._error is None: + # ignore subsequent exceptions, it's enough to raise only + # the first exception arrived + self._error = exc self.stop() def _on_interrupt(self) -> None: