diff --git a/CHANGES b/CHANGES index 7a691b0cc9..5a3f8c49a7 100644 --- a/CHANGES +++ b/CHANGES @@ -45,6 +45,45 @@ $ uvx --from 'libtmux' --prerelease allow python _Notes on the upcoming release will go here._ +### What's new + +#### A tmux call can be given a deadline (#757) + +{class}`~libtmux.common.tmux_cmd` takes a `timeout`, and +{class}`~libtmux.Server` takes one as the default for every command it +issues, including the listings behind +{attr}`Server.sessions `, +{attr}`Session.windows ` and +{attr}`Window.panes `. Those listings do not go +through {meth}`Server.cmd() `, so until now a +consumer could not bound them at all. + +Without a timeout nothing changes: the default is to wait, as before. + +A tmux server can accept a connection and then never reply, and a call +waiting on one never returns. Giving up on it does not stop it either, +so the waiting calls only accumulate, and enough of them will exhaust a +thread pool and stall work aimed at healthy sockets. + +On expiry the tmux process is killed and reaped before +{exc}`~libtmux.exc.TmuxTimeout` is raised, so a call that timed out +leaves nothing running behind it. The command may still have taken +effect -- the process was stopped mid-command, and nothing can be said +either way. + +{exc}`~libtmux.exc.TmuxTimeout` is deliberately not a +{exc}`~libtmux.exc.LibTmuxException`. The listing accessors read one of +those as "nothing to list", which is correct for a daemon that has not +started and wrong for a server that has stopped answering: a caller +told there are no sessions goes on to create one on a server that +already has them. + +```python +server = Server(timeout=5) +server.sessions # raises TmuxTimeout rather than reporting [] +server.cmd("kill-server", timeout=30) # per-call override +``` + ### Documentation #### Cleaner `from_env` examples (#719) diff --git a/src/libtmux/common.py b/src/libtmux/common.py index 2871547700..9b8d05b9a2 100644 --- a/src/libtmux/common.py +++ b/src/libtmux/common.py @@ -303,13 +303,33 @@ class tmux_cmd: $ tmux new-session -s my session + Parameters + ---------- + tmux_bin : str, optional + Path to the tmux binary. Defaults to the first ``tmux`` on ``PATH``. + timeout : float, optional + Seconds to wait for the command. On expiry the tmux process is + killed and reaped, then :exc:`~libtmux.exc.TmuxTimeout` is + raised. ``None`` waits indefinitely. + + Raises + ------ + :exc:`~libtmux.exc.TmuxTimeout` + ``timeout`` elapsed. The command may or may not have taken + effect -- the process was killed mid-command. + Notes ----- .. versionchanged:: 0.8 Renamed from ``tmux`` to ``tmux_cmd``. """ - def __init__(self, *args: t.Any, tmux_bin: str | None = None) -> None: + def __init__( + self, + *args: t.Any, + tmux_bin: str | None = None, + timeout: float | None = None, + ) -> None: resolved = tmux_bin or shutil.which("tmux") if not resolved: raise exc.TmuxCommandNotFound @@ -336,8 +356,15 @@ def __init__(self, *args: t.Any, tmux_bin: str | None = None) -> None: encoding="utf-8", errors="backslashreplace", ) - stdout, stderr = self.process.communicate() + stdout, stderr = self.process.communicate(timeout=timeout) returncode = self.process.returncode + except subprocess.TimeoutExpired: + # Kill and reap before raising. A caller that gives up on an + # unbounded call leaves the child running, so repeated + # timeouts accumulate tmux clients that nothing is waiting on. + self.process.kill() + self.process.communicate() + raise exc.TmuxTimeout(cmd, t.cast("float", timeout)) from None except FileNotFoundError: raise exc.TmuxCommandNotFound from None except Exception: diff --git a/src/libtmux/exc.py b/src/libtmux/exc.py index 57bb06102f..f87c9be0de 100644 --- a/src/libtmux/exc.py +++ b/src/libtmux/exc.py @@ -350,6 +350,26 @@ class WaitTimeout(LibTmuxException): """Function timed out without meeting condition.""" +class TmuxTimeout(Exception): + """A tmux command did not return within its timeout. + + Deliberately NOT a :exc:`LibTmuxException`. The listing accessors + absorb one of those as "nothing to list", which is right for a + daemon that has not started and wrong for a server that stopped + answering -- a caller told there are no sessions goes on to create + one on a server that already has them. + + Distinct from :exc:`WaitTimeout`, which is a helper giving up on a + condition. This one means the tmux process was killed mid-command, + so nothing can be said about whether it took effect. + """ + + def __init__(self, cmd: list[str], timeout: float, *args: object) -> None: + self.cmd = cmd + self.timeout = timeout + super().__init__(f"tmux did not return within {timeout}s: {' '.join(cmd)}") + + class VariableUnpackingError(LibTmuxException): """Error unpacking variable.""" diff --git a/src/libtmux/neo.py b/src/libtmux/neo.py index 98ece86fa5..bb645e191d 100644 --- a/src/libtmux/neo.py +++ b/src/libtmux/neo.py @@ -1133,6 +1133,7 @@ def fetch_objs( proc = tmux_cmd( *tmux_cmds, tmux_bin=server.tmux_bin, + timeout=server.timeout, ) raise_if_stderr(proc, list_cmd) diff --git a/src/libtmux/server.py b/src/libtmux/server.py index e650557c34..9d39e05d92 100644 --- a/src/libtmux/server.py +++ b/src/libtmux/server.py @@ -176,10 +176,12 @@ def __init__( on_init: t.Callable[[Server], None] | None = None, socket_name_factory: t.Callable[[], str] | None = None, tmux_bin: str | pathlib.Path | None = None, + timeout: float | None = None, **kwargs: t.Any, ) -> None: EnvironmentMixin.__init__(self, "-g") self.tmux_bin = str(tmux_bin) if tmux_bin is not None else None + self.timeout = timeout self._windows: list[WindowDict] = [] self._panes: list[PaneDict] = [] @@ -342,6 +344,7 @@ def cmd( cmd: str, *args: t.Any, target: str | int | None = None, + timeout: float | None = None, ) -> tmux_cmd: """Execute tmux command respective of socket name and file, return output. @@ -408,7 +411,12 @@ def cmd( cmd_args = ["-t", str(target), *args] if target is not None else [*args] - return tmux_cmd(*svr_args, *cmd_args, tmux_bin=self.tmux_bin) + return tmux_cmd( + *svr_args, + *cmd_args, + tmux_bin=self.tmux_bin, + timeout=self.timeout if timeout is None else timeout, + ) @property def attached_sessions(self) -> list[Session]: diff --git a/tests/conftest.py b/tests/conftest.py index c0015e4e65..e745ced251 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,10 +2,15 @@ from __future__ import annotations +import typing as t + import pytest from libtmux.common import get_version, get_version_str +if t.TYPE_CHECKING: + import pathlib + @pytest.fixture(autouse=True) def _clear_get_version_cache() -> None: @@ -20,3 +25,26 @@ def _clear_get_version_cache() -> None: """ get_version.cache_clear() get_version_str.cache_clear() + + +@pytest.fixture +def hanging_tmux(tmp_path: pathlib.Path) -> tuple[str, pathlib.Path]: + """Return a stand-in tmux that never answers, and its pid file. + + Models the failure a real fixture cannot produce cheaply: a server + that accepts a connection and never replies. ``-V`` is answered, + because a wedged SERVER does not stop the local binary reporting its + own version, and `Server.sessions` reads it on the way past. + ``exec`` preserves the recorded pid, so a test can check the process + afterwards instead of trusting that it was killed. + """ + pid_file = tmp_path / "pid" + binary = tmp_path / "tmux" + binary.write_text( + "#!/bin/sh\n" + 'if [ "$1" = "-V" ]; then echo "tmux 3.7"; exit 0; fi\n' + f"echo $$ > {pid_file}\n" + "exec sleep 30\n" + ) + binary.chmod(0o755) + return str(binary), pid_file diff --git a/tests/test_common.py b/tests/test_common.py index 426b72d573..b6e41bd2c5 100644 --- a/tests/test_common.py +++ b/tests/test_common.py @@ -4,8 +4,10 @@ import locale import logging +import os import re import sys +import time import typing as t import pytest @@ -29,6 +31,8 @@ ) if t.TYPE_CHECKING: + import pathlib + from libtmux.server import Server from libtmux.session import Session @@ -762,3 +766,39 @@ def test_tmux_cmd_format_separator_survives_non_utf8_locale( result = parse_output(line, "list-sessions", tmux_version) assert isinstance(result, dict) assert "session_id" in result + + +def test_tmux_cmd_timeout_kills_and_reaps( + hanging_tmux: tuple[str, pathlib.Path], +) -> None: + """An expired command leaves no tmux process behind. + + The kill is the load-bearing half. Without it a caller that gives up + only stops waiting, so repeated timeouts accumulate tmux processes + nobody is listening to. + """ + binary, pid_file = hanging_tmux + + with pytest.raises(exc.TmuxTimeout) as excinfo: + tmux_cmd("list-sessions", tmux_bin=binary, timeout=0.3) + + assert excinfo.value.timeout == 0.3 + assert "list-sessions" in str(excinfo.value) + + pid = int(pid_file.read_text()) + with pytest.raises(ProcessLookupError): + os.kill(pid, 0) + + +def test_tmux_cmd_without_timeout_still_waits( + hanging_tmux: tuple[str, pathlib.Path], +) -> None: + """The bound is opt-in; omitting it keeps the historical behaviour.""" + binary, _pid_file = hanging_tmux + started = time.monotonic() + + with pytest.raises(exc.TmuxTimeout): + tmux_cmd("list-sessions", tmux_bin=binary, timeout=0.3) + + # A test that never waits would pass whether or not `timeout` is read. + assert time.monotonic() - started >= 0.3 diff --git a/tests/test_server.py b/tests/test_server.py index 6175a7f9ad..cfe00e9c03 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -1765,3 +1765,34 @@ def test_server_display_message_warns_on_tmux_error( """ with pytest.warns(UserWarning, match="only one of -F or argument"): server.display_message("x", get_text=True, format_string="#{version}") + + +def test_server_timeout_bounds_every_command( + hanging_tmux: tuple[str, pathlib.Path], +) -> None: + """A server-level timeout is the policy for commands through it.""" + binary, _pid_file = hanging_tmux + server = Server(tmux_bin=binary, timeout=0.3) + + with pytest.raises(exc.TmuxTimeout): + server.cmd("list-sessions") + + +@pytest.mark.parametrize("accessor", ["sessions", "windows", "panes", "clients"]) +def test_a_wedged_server_is_not_reported_as_empty( + hanging_tmux: tuple[str, pathlib.Path], + accessor: str, +) -> None: + """A listing must raise rather than answer empty. + + A tmux server that stopped answering has not said it has nothing. + Returning ``[]`` sends a caller on to create a session on a server + that already has them. Every accessor is covered because three of + them reimplemented the "empty means not ready" rule inline instead + of sharing it. + """ + binary, _pid_file = hanging_tmux + server = Server(tmux_bin=binary, timeout=0.3) + + with pytest.raises(exc.TmuxTimeout): + _ = getattr(server, accessor)