diff --git a/changelog.md b/changelog.md index 55475ccd..54bb2c15 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 8075ca16..99447a9f 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 deb91c5b..3c526e01 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 3a80c87c..b407a26e 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)