Don't wait for Boundary tunnel teardown on quit

changelog.md

@@ -4,6 +4,7 @@ Upcoming (TBD)
 Features
 --------
 * Remove support for Python 3.10.
+* Exit faster when using a Boundary tunnel.
 
 
 Documentation

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:

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:

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)