From af4a7a6ab36667419f16b69f2ffa8a3ca0026354 Mon Sep 17 00:00:00 2001 From: Roland Walker Date: Fri, 25 Sep 2026 14:09:43 -0400 Subject: [PATCH] don't wait for Boundary teardown on quit At /quit, mycli was waiting up to a couple of seconds to tear down the Boundary connection, and the logic allowed for an even longer wait. Instead we should just exit, and let the terminated Boundary process take care of itself. Other tunnel mechanisms (SSH and kubectl) are left untouched, since their teardown is faster in practice. --- changelog.md | 1 + mycli/boundary_tunnel.py | 10 +- test/pytests/test_boundary_tunnel.py | 143 ++++++++++++++------------- test/pytests/test_client.py | 18 +++- 4 files changed, 92 insertions(+), 80 deletions(-) diff --git a/changelog.md b/changelog.md index 55475ccd4..54bb2c159 100644 --- a/changelog.md +++ b/changelog.md @@ -4,6 +4,7 @@ Upcoming (TBD) Features -------- * Remove support for Python 3.10. +* Exit faster when using a Boundary tunnel. Documentation diff --git a/mycli/boundary_tunnel.py b/mycli/boundary_tunnel.py index 8075ca168..99447a9fc 100644 --- a/mycli/boundary_tunnel.py +++ b/mycli/boundary_tunnel.py @@ -246,14 +246,10 @@ def _parse_authentication_command(command: str, name: str) -> list[str]: def close(self) -> None: process = self.process if process is not None and process.poll() is None: - process.terminate() try: - process.wait(timeout=5) - except subprocess.TimeoutExpired: - process.kill() - process.wait() - if self._thread is not None and self._thread.is_alive(): - self._thread.join(timeout=5) + process.terminate() + except ProcessLookupError: + pass def _run(self) -> None: try: diff --git a/test/pytests/test_boundary_tunnel.py b/test/pytests/test_boundary_tunnel.py index deb91c5b4..3c526e01b 100644 --- a/test/pytests/test_boundary_tunnel.py +++ b/test/pytests/test_boundary_tunnel.py @@ -8,6 +8,7 @@ import threading from types import SimpleNamespace from typing import Any, cast +from unittest.mock import Mock import pytest @@ -666,38 +667,47 @@ def test_boundary_tunnel_start_reports_process_start_timeout(monkeypatch: pytest tunnel.start() -def test_boundary_tunnel_start_reports_process_output_timeout(monkeypatch: pytest.MonkeyPatch) -> None: - calls: list[str] = [] - output_released = threading.Event() +def test_boundary_tunnel_start_waits_for_process_output(monkeypatch: pytest.MonkeyPatch) -> None: + tunnel = BoundaryTunnel(target_id='ttcp_123', local_port=4406) + worker = Mock(spec=threading.Thread) + worker.start.side_effect = tunnel._started.set + monkeypatch.setattr(boundary_tunnel.threading, 'Thread', Mock(return_value=worker)) + monkeypatch.setattr(tunnel, '_is_listening', lambda: True) + sleeps: list[float] = [] - class BlockingStdout: - def readline(self) -> bytes: - calls.append('read') - output_released.wait() - return b'' + def release_output(seconds: float) -> None: + sleeps.append(seconds) + tunnel.stdout = CONNECTION_DETAILS + tunnel._output_ready.set() - class FakeProcess: - stdout = BlockingStdout() + monkeypatch.setattr(boundary_tunnel.time, 'sleep', release_output) - def poll(self) -> None: - return None + tunnel.start() - def terminate(self) -> None: - calls.append('terminate') - output_released.set() + assert sleeps == [0.05, TUNNEL_STABILIZATION_PAUSE] + assert tunnel._ready.is_set() + assert tunnel.username == '1234' + assert tunnel.password == '5678' - def wait(self, timeout: float | None = None) -> int: - calls.append(f'wait:{timeout}') - return 0 - monkeypatch.setattr(boundary_tunnel.subprocess, 'Popen', lambda *_args, **_kwargs: FakeProcess()) - tunnel = BoundaryTunnel(target_id='ttcp_123', local_port=4406, ready_timeout=0.1) +def test_boundary_tunnel_start_reports_process_output_timeout(monkeypatch: pytest.MonkeyPatch) -> None: + tunnel = BoundaryTunnel(target_id='ttcp_123', local_port=4406) + process = Mock(spec=subprocess.Popen) + process.poll.return_value = None + tunnel.process = process + worker = Mock(spec=threading.Thread) + worker.start.side_effect = tunnel._started.set + monkeypatch.setattr(boundary_tunnel.threading, 'Thread', Mock(return_value=worker)) + monkeypatch.setattr(boundary_tunnel.time, 'monotonic', Mock(side_effect=[0.0, 0.0, 0.0, 31.0])) + sleep = Mock() + monkeypatch.setattr(boundary_tunnel.time, 'sleep', sleep) with pytest.raises(BoundaryTunnelError, match='Timed out waiting for tunnel process output'): tunnel.start() - assert calls[:2] == ['read', 'terminate'] - assert sorted(calls[2:]) == ['wait:5', 'wait:None'] + sleep.assert_called_once_with(0.05) + process.terminate.assert_called_once_with() + assert not tunnel._ready.is_set() def test_boundary_tunnel_start_reports_timeout(monkeypatch: pytest.MonkeyPatch) -> None: @@ -792,76 +802,69 @@ def fake_popen(command: list[str], **kwargs: Any) -> FakeProcess: ] -def test_boundary_tunnel_close_terminates_running_process() -> None: - calls: list[str] = [] +def test_boundary_tunnel_close_terminates_without_waiting() -> None: + tunnel = BoundaryTunnel(target_id='ttcp_123', local_port=4406) + process = Mock(spec=subprocess.Popen) + process.poll.return_value = None + tunnel.process = process + worker = Mock(spec=threading.Thread) + tunnel._thread = worker - class FakeProcess: - def poll(self) -> None: - return None + tunnel.close() - def terminate(self) -> None: - calls.append('terminate') + process.terminate.assert_called_once_with() + process.wait.assert_not_called() + process.kill.assert_not_called() + worker.join.assert_not_called() - def wait(self, timeout: float | None = None) -> int: - calls.append(f'wait:{timeout}') - return 0 +def test_boundary_tunnel_close_without_process_does_not_join_worker() -> None: tunnel = BoundaryTunnel(target_id='ttcp_123', local_port=4406) - tunnel.process = cast(Any, FakeProcess()) + worker = Mock(spec=threading.Thread) + tunnel._thread = worker tunnel.close() - assert calls == ['terminate', 'wait:5'] + worker.join.assert_not_called() -def test_boundary_tunnel_close_kills_process_after_terminate_timeout() -> None: - calls: list[str] = [] - - class FakeProcess: - def __init__(self) -> None: - self.wait_calls = 0 - - def poll(self) -> None: - return None +@pytest.mark.parametrize('return_code', [0, 1]) +def test_boundary_tunnel_close_ignores_exited_process(return_code: int) -> None: + tunnel = BoundaryTunnel(target_id='ttcp_123', local_port=4406) + process = Mock(spec=subprocess.Popen) + process.poll.return_value = return_code + tunnel.process = process - def terminate(self) -> None: - calls.append('terminate') + tunnel.close() - def wait(self, timeout: float | None = None) -> int: - calls.append(f'wait:{timeout}') - self.wait_calls += 1 - if self.wait_calls == 1: - assert timeout is not None - raise subprocess.TimeoutExpired('boundary', timeout) - return 0 + process.terminate.assert_not_called() + process.wait.assert_not_called() + process.kill.assert_not_called() - def kill(self) -> None: - calls.append('kill') +def test_boundary_tunnel_close_handles_process_exiting_before_terminate() -> None: tunnel = BoundaryTunnel(target_id='ttcp_123', local_port=4406) - tunnel.process = cast(Any, FakeProcess()) + process = Mock(spec=subprocess.Popen) + process.poll.return_value = None + process.terminate.side_effect = ProcessLookupError + tunnel.process = process tunnel.close() - assert calls == ['terminate', 'wait:5', 'kill', 'wait:None'] + process.terminate.assert_called_once_with() + process.wait.assert_not_called() + process.kill.assert_not_called() -def test_boundary_tunnel_close_joins_running_thread() -> None: - calls: list[str] = [] - - class FakeThread: - def is_alive(self) -> bool: - return True - - def join(self, timeout: float | None = None) -> None: - calls.append(f'join:{timeout}') - +def test_boundary_tunnel_close_propagates_other_termination_errors() -> None: tunnel = BoundaryTunnel(target_id='ttcp_123', local_port=4406) - tunnel._thread = cast(Any, FakeThread()) - - tunnel.close() + process = Mock(spec=subprocess.Popen) + process.poll.return_value = None + process.terminate.side_effect = PermissionError('access denied') + tunnel.process = process - assert calls == ['join:5'] + with pytest.raises(PermissionError, match='access denied'): + tunnel.close() def test_boundary_tunnel_is_listening_returns_true(monkeypatch: pytest.MonkeyPatch) -> None: diff --git a/test/pytests/test_client.py b/test/pytests/test_client.py index 3a80c87c8..b407a26e4 100644 --- a/test/pytests/test_client.py +++ b/test/pytests/test_client.py @@ -493,7 +493,11 @@ def test_close_stops_refreshers_before_closing_connection_and_tunnels() -> None: cli.sqlexecute = SimpleNamespace(close=lambda: calls.append('connection')) # type: ignore[assignment] cast(Any, cli).ssh_tunnel = SimpleNamespace(close=lambda: calls.append('ssh')) cast(Any, cli).kubectl_tunnel = SimpleNamespace(close=lambda: calls.append('kubectl')) - cli.boundary_tunnel = SimpleNamespace(close=lambda: calls.append('boundary')) # type: ignore[assignment] + + def close_boundary() -> None: + calls.append('boundary') + + cli.boundary_tunnel = SimpleNamespace(close=close_boundary) # type: ignore[assignment] MyCli.close(cli) @@ -511,7 +515,7 @@ def fail() -> None: cli.sqlexecute = SimpleNamespace(close=fail) # type: ignore[assignment] cast(Any, cli).ssh_tunnel = SimpleNamespace(close=fail) cast(Any, cli).kubectl_tunnel = SimpleNamespace(close=fail) - cli.boundary_tunnel = SimpleNamespace(close=lambda: (_ for _ in ()).throw(RuntimeError('close failed'))) # type: ignore[assignment] + cli.boundary_tunnel = SimpleNamespace(close=lambda **kwargs: fail()) # type: ignore[assignment] MyCli.close(cli) @@ -523,9 +527,17 @@ def test_close_swallows_boundary_tunnel_close_error() -> None: tunnel_closed: list[bool] = [] cast(Any, cli).ssh_tunnel = SimpleNamespace(close=lambda: tunnel_closed.append(True)) cast(Any, cli).kubectl_tunnel = None - cli.boundary_tunnel = SimpleNamespace(close=lambda: (_ for _ in ()).throw(RuntimeError('close failed'))) # type: ignore[assignment] + boundary_close_calls: list[bool] = [] + + def close_boundary() -> None: + boundary_close_calls.append(True) + raise RuntimeError('close failed') + + cli.boundary_tunnel = SimpleNamespace(close=close_boundary) # type: ignore[assignment] MyCli.close(cli) + assert boundary_close_calls == [True] + def test_invalidate_prompt_session_invalidates_prompt_app() -> None: cli = MyCli.__new__(MyCli)