Stage 3 review: bound the display's teardown, not just the wait for it

The readiness timeout only bounded the waiting. Cleanup then joined the display
thread with no timeout, so a render callback blocked inside it held the command
thread anyway -- the hang moved rather than went away.

The join is bounded now. A thread that will not finish is reported and kept: it
is a daemon and still owns the application, so clearing the reference would let
a second display start over it, and one terminal cannot have two input readers.
Resuming refuses for the same reason, and a suspension that cannot restore its
display says so once and carries on without a toolbar -- the command that
suspended it is not this failure's to end.

The earlier test suppressed the readiness signal while leaving the worker
responsive, so the join it performed always returned promptly and the case
could not appear. The new tests block a real render callback.

Author: tleonhardt

cmd2/command_toolbar.py

@@ -34,6 +34,10 @@
 #: machine is not mistaken for a broken one, short enough that a command is never held forever.
 _STARTUP_TIMEOUT = 10.0
 
+#: How long to wait for the display's thread to finish. Bounded for the same reason: a render
+#: callback blocked inside it would otherwise hold whoever is tearing the display down.
+_SHUTDOWN_TIMEOUT = 10.0
+
 _F = TypeVar("_F", bound=Callable[..., Any])
 _R = TypeVar("_R")
 
@@ -87,6 +91,14 @@ def pipe_target(stream: Any) -> Any:
     return stream
 
 
+class _DisplayStillRunningError(RuntimeError):
+    """Raised when a previous display's thread has not finished.
+
+    One terminal cannot have two input readers, so a display that would not stop is a display
+    that cannot be started again.
+    """
+
+
 class _ContextStdoutProxy(StdoutProxy):
     """Keep stdout's flush worker in the toolbar's isolated application session."""
 
@@ -301,6 +313,10 @@ def _restore_stream(obj: Any, name: str, stream: ToolbarStream) -> None:
             setattr(obj, name, stream.original)
 
     def _resume(self) -> None:
+        if self._thread is not None and self._thread.is_alive():
+            # A previous display never finished. One terminal cannot have two input readers,
+            # and the old one still holds the application.
+            raise _DisplayStillRunningError("the bottom toolbar's previous display is still running")
         self._ready.clear()
         self._error = None
         stack = self._display_stack = contextlib.ExitStack()
@@ -433,8 +449,15 @@ def _pause(self) -> None:
                 if self.app.is_running and self.app.loop is not None:
                     self.app.loop.call_soon_threadsafe(self._exit)
                 if self._thread is not None:
-                    self._thread.join()
-                    self._thread = None
+                    self._thread.join(timeout=_SHUTDOWN_TIMEOUT)
+                    if self._thread.is_alive():
+                        # Bounded, so a render callback blocked inside the display cannot hold
+                        # the thread that is tearing it down. The reference is kept rather
+                        # than cleared: the thread is still running the application, and
+                        # starting a second one would put two input readers on one terminal.
+                        self.cmd.perror(f"The bottom toolbar did not stop within {_SHUTDOWN_TIMEOUT} seconds")
+                    else:
+                        self._thread = None
                 # Return the borrowed application to the main prompt, including on
                 # proxy failures. The upstream toolbar owned a separate application.
                 if self._display_stack is not None:
@@ -578,4 +601,10 @@ def suspend(self) -> Iterator[None]:
             yield
         finally:
             with self.cmd.sigint_protection:
-                self._resume()
+                try:
+                    self._resume()
+                except _DisplayStillRunningError as exc:
+                    # The display that would not stop is still holding the application, so
+                    # there is nothing to come back to. Report it once and carry on without a
+                    # toolbar: the command that suspended it is not this failure's to end.
+                    self.cmd.perror(f"Bottom toolbar not restored: {exc}")

tests/test_command_toolbar.py

@@ -826,3 +826,61 @@ def test_command_toolbar_startup_does_not_wait_forever(toolbar_app, monkeypatch,
 
     assert ran == [True]
     assert "did not start" in capsys.readouterr().err
+
+
+def test_command_toolbar_startup_timeout_does_not_block_on_cleanup(toolbar_app, monkeypatch, capsys) -> None:
+    """A blocked render callback must not hold the command thread through teardown either.
+
+    The readiness wait being bounded is only half of it: the display thread is still inside
+    the callback, so the join that follows has to be bounded too. This blocks the callback for
+    real rather than suppressing the readiness signal, which is what the earlier test did and
+    why it could not see this.
+    """
+    app, _, _ = toolbar_app
+    blocked = threading.Event()
+    monkeypatch.setattr(command_toolbar, "_STARTUP_TIMEOUT", 0.2)
+    monkeypatch.setattr(command_toolbar, "_SHUTDOWN_TIMEOUT", 0.2)
+
+    def blocking_toolbar() -> str:
+        blocked.wait(timeout=10)
+        return "STATUS"
+
+    app.main_session.bottom_toolbar = blocking_toolbar
+    ran = []
+    try:
+        started = time.monotonic()
+        with app._command_toolbar_context():
+            ran.append(True)
+        elapsed = time.monotonic() - started
+
+        assert ran == [True]
+        # Both bounded waits, and nothing unbounded between them.
+        assert elapsed < 5
+        assert "did not start" in capsys.readouterr().err
+    finally:
+        blocked.set()
+
+
+def test_command_toolbar_does_not_start_a_second_display_over_a_stuck_one(toolbar_app, monkeypatch, capsys) -> None:
+    """One terminal, one input reader: a display that would not stop cannot be restarted."""
+    app, _, _ = toolbar_app
+    blocked = threading.Event()
+    monkeypatch.setattr(command_toolbar, "_SHUTDOWN_TIMEOUT", 0.2)
+
+    try:
+        with app._command_toolbar_context():
+            display = app._command_toolbar
+            assert display is not None
+            first_thread = display._thread
+
+            # Block the display inside a render, so its thread cannot finish.
+            app.main_session.bottom_toolbar = lambda: blocked.wait(timeout=10) or "STATUS"
+            display.app.invalidate()
+
+            with app.suspend_bottom_toolbar():
+                pass
+
+            assert display._thread is first_thread
+            assert "not restored" in capsys.readouterr().err
+    finally:
+        blocked.set()