From aad3d01939cca7ffbaf93ac4094cdfb09f69ff94 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 26 Sep 2026 11:09:22 -0400 Subject: [PATCH 01/20] Run POSIX pipes to interactive programs as the terminal's foreground job A pipe process always ran in its own session, so it never received the terminal. Piped to an interactive program such as less, Ctrl-Z and fg did not suspend and resume it together with cmd2, and the program could not take the keyboard as a shell's foreground job would. When a pipe's output goes to cmd2's controlling terminal and the pipe is started on the main thread, run the pipe process in a new process group and lend it the terminal as it starts, while cmd2 writes to it, and while cmd2 waits for it. Between writes the terminal returns to cmd2, so command code can still read the keyboard. ProcReader watches the group on a thread, relays its job-control stops to cmd2's job, and forwards Ctrl-C without signalling cmd2's own group again. A shell command piped to such a program joins the pipeline's job for as long as it runs. Pipes started off the main thread, or whose output cmd2 captures, still run in their own session. The terminal tests drive cmd2 under an interactive bash on a pseudo-terminal, using pyte to read the screen, and coverage now follows the applications those tests start. Extracted from the reserved-row toolbar work (PR #1761) without the toolbar. --- CHANGELOG.md | 13 + cmd2/cmd2.py | 223 ++++++--- cmd2/utils.py | 347 +++++++++++++- docs/features/redirection.md | 4 + pyproject.toml | 6 + tests/test_cmd2.py | 119 ++++- tests/test_pipeline_job_control.py | 722 +++++++++++++++++++++++++++++ tests/test_utils.py | 408 ++++++++++++++++ 8 files changed, 1772 insertions(+), 70 deletions(-) create mode 100644 tests/test_pipeline_job_control.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 552217630..6d6af0573 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,16 @@ +## 4.2.5 (TBD) + +- Bug Fixes + - On POSIX, piping a command's output to an interactive program such as `less` + (`help -v | less`) now runs that program as the terminal's foreground job, as a shell pipeline + does. It had run in a separate session that never received the terminal, so Ctrl-Z and `fg` + did not suspend and resume it together with cmd2. The program now owns the terminal as it + starts, so a pager can set its terminal modes, and Ctrl-C and Ctrl-Z reach the whole pipeline. + Pipes started from a worker thread, or whose output cmd2 captures, still run in their own + session + - A `shell` command piped to an interactive program, such as `shell git log | less`, now joins + the pipeline's job, so both processes receive Ctrl-C and Ctrl-Z + ## 4.2.4 (September 8, 2026) - Bug Fixes diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index fcd26b2e4..84aae5185 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -3330,46 +3330,102 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: subproc_stdin = open(read_fd, encoding="utf-8") # noqa: SIM115 new_stdout: TextIO = cast(TextIO, open(write_fd, "w", encoding="utf-8")) # noqa: SIM115 - # Create pipe process in a separate group to isolate our signals from it. If a Ctrl-C event occurs, - # our sigint handler will forward it only to the most recent pipe process. This makes sure pipe - # processes close in the right order (most recent first). + # Isolate pipeline signals from cmd2. Terminal pipelines receive the + # foreground terminal; ProcReader relays their job-control stops. kwargs: dict[str, Any] = {} if sys.platform == "win32": kwargs["creationflags"] = subprocess.CREATE_NEW_PROCESS_GROUP else: - kwargs["start_new_session"] = True - # Attempt to run the pipe process in the user's preferred shell instead of the default behavior of using sh. shell = os.environ.get("SHELL") if shell: kwargs["executable"] = shell # For any stream that is a StdSim, we will use a pipe so we can capture its output - proc = subprocess.Popen( # noqa: S602 - statement.redirect_to, - stdin=subproc_stdin, - stdout=subprocess.PIPE if isinstance(self.stdout, utils.StdSim) else self.stdout, # type: ignore[unreachable] - stderr=subprocess.PIPE if isinstance(sys.stderr, utils.StdSim) else sys.stderr, - shell=True, - **kwargs, - ) + pipe_stdout = None if isinstance(self.stdout, utils.StdSim) else self.stdout # type: ignore[unreachable] + pipe_stderr = None if isinstance(sys.stderr, utils.StdSim) else sys.stderr + + terminal_fd = None + if sys.platform != "win32": + # Job control installs signal handlers, which only the main thread may do. + # Elsewhere, keep the pipeline in its own session as before. + if threading.current_thread() is threading.main_thread(): + for stream in (pipe_stdout, pipe_stderr): + if stream is not None and stream.isatty(): + with contextlib.suppress(OSError, ValueError): + if os.tcgetpgrp(stream.fileno()) == os.getpgrp(): + terminal_fd = stream.fileno() + break + if terminal_fd is None: + kwargs["start_new_session"] = True + else: + kwargs["process_group"] = 0 - # Popen was called with shell=True so the user can chain pipe commands and redirect their output - # like: !ls -l | grep user | wc -l > out.txt. But this makes it difficult to know if the pipe process - # started OK, since the shell itself always starts. Therefore, we will wait a short time and check - # if the pipe process is still running. - with contextlib.suppress(subprocess.TimeoutExpired): - proc.wait(0.2) + with contextlib.ExitStack() as terminal_stack: + with contextlib.ExitStack() as spawn_stack: + if terminal_fd is not None and os.getpgrp() == os.getsid(0): + import signal - # Check if the pipe process already exited - if proc.returncode is not None: + # A session leader's job has no outer shell to resume it. + # Its pipeline must inherit the same Ctrl-Z behavior: the + # new group would otherwise make SIGTSTP actionable again. + previous_tstp = signal.signal(signal.SIGTSTP, signal.SIG_IGN) + spawn_stack.callback(signal.signal, signal.SIGTSTP, previous_tstp) + proc = subprocess.Popen( # noqa: S602 + statement.redirect_to, + stdin=subproc_stdin, + stdout=subprocess.PIPE if pipe_stdout is None else pipe_stdout, + stderr=subprocess.PIPE if pipe_stderr is None else pipe_stderr, + shell=True, + **kwargs, + ) + # Only the child should own a read end. In particular, a consumer + # exit must unblock a producer writing to a full pipe immediately. subproc_stdin.close() - new_stdout.close() - raise RedirectionError(f"Pipe process exited with code {proc.returncode} before command could run") - redir_saved_state.redirecting = True - cmd_pipe_proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr) + if terminal_fd is not None: + cmd_pipe_proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr, terminal_fd=terminal_fd) + terminal_stack.enter_context(cmd_pipe_proc_reader.manage_terminal()) + + # Popen was called with shell=True so the user can chain pipe commands and redirect their output + # like: !ls -l | grep user | wc -l > out.txt. But this makes it difficult to know if the pipe process + # started OK, since the shell itself always starts. Therefore, we will wait a short time and check + # if the pipe process is still running. + with contextlib.suppress(subprocess.TimeoutExpired): + if cmd_pipe_proc_reader is None: + proc.wait(0.2) + else: + # A pager such as less sets its terminal modes as it starts, before it + # reads the pipe. It must own the terminal by then: a background + # tcsetattr() stops it with SIGTTOU, and on macOS that call fails with + # EINTR when the process is continued instead of being restarted. less + # ignores the failure and runs on a cooked terminal. + with cmd_pipe_proc_reader.lend_terminal(): + cmd_pipe_proc_reader.wait_for_exit(0.2) + + # Check if the pipe process already exited + if proc.returncode is not None: + if cmd_pipe_proc_reader is not None: + cmd_pipe_proc_reader.wait() + subproc_stdin.close() + new_stdout.close() + raise RedirectionError(f"Pipe process exited with code {proc.returncode} before command could run") + redir_saved_state.redirecting = True + if cmd_pipe_proc_reader is None: + cmd_pipe_proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr) - self.stdout = new_stdout + if terminal_fd is not None: + import io + + pipe_fd = os.dup(new_stdout.fileno()) + new_stdout.close() + new_stdout = io.TextIOWrapper( + io.BufferedWriter(utils.PipelineWriter(pipe_fd, cmd_pipe_proc_reader)), encoding="utf-8" + ) + + self.stdout = new_stdout + + # Keep the pipeline's job control until _restore_output() reaps the pipe process. + redir_saved_state.pipeline_job = terminal_stack.pop_all() elif statement.redirector in (constants.REDIRECTION_OVERWRITE, constants.REDIRECTION_APPEND): if statement.redirect_to: @@ -3428,29 +3484,41 @@ def _restore_output(self, statement: Statement, saved_redir_state: utils.Redirec :param statement: Statement object which contains the parsed input from the user :param saved_redir_state: contains information needed to restore state data """ - if saved_redir_state.redirecting: - # If we redirected output to the clipboard - if ( - statement.redirector in (constants.REDIRECTION_OVERWRITE, constants.REDIRECTION_APPEND) - and not statement.redirect_to - ): - self.stdout.seek(0) - write_to_paste_buffer(self.stdout.read()) + # The pipeline's job control ends once its pipe process has been reaped. + with contextlib.ExitStack() as terminal_stack: + if saved_redir_state.pipeline_job is not None: + terminal_stack.callback(saved_redir_state.pipeline_job.close) + saved_redir_state.pipeline_job = None - with contextlib.suppress(BrokenPipeError): - # Close the file or pipe that stdout was redirected to - self.stdout.close() - - # Restore self.stdout - self.stdout = cast(TextIO, saved_redir_state.saved_self_stdout) - - # Check if we need to wait for the process being piped to - if self._cur_pipe_proc_reader is not None: - self._cur_pipe_proc_reader.wait() - - # These are restored regardless of whether the command redirected - self._cur_pipe_proc_reader = saved_redir_state.saved_pipe_proc_reader - self._redirecting = saved_redir_state.saved_redirecting + try: + if saved_redir_state.redirecting: + # If we redirected output to the clipboard + if ( + statement.redirector in (constants.REDIRECTION_OVERWRITE, constants.REDIRECTION_APPEND) + and not statement.redirect_to + ): + self.stdout.seek(0) + write_to_paste_buffer(self.stdout.read()) + + with contextlib.suppress(BrokenPipeError): + # Close the file or pipe that stdout was redirected to + if self._cur_pipe_proc_reader is not None: + self._cur_pipe_proc_reader.finish_producer() + self.stdout.close() + + # Restore self.stdout + self.stdout = cast(TextIO, saved_redir_state.saved_self_stdout) + + # Check if we need to wait for the process being piped to. Handing the + # terminal back as it finishes can fail, for example after a hangup. + if self._cur_pipe_proc_reader is not None: + self._cur_pipe_proc_reader.wait() + finally: + # These are restored regardless of whether the command redirected, or whether + # restoring it failed: a pipeline left current would keep ppaged() from paging + # and send Ctrl-C to a process group that is gone. + self._cur_pipe_proc_reader = saved_redir_state.saved_pipe_proc_reader + self._redirecting = saved_redir_state.saved_redirecting def get_command_func(self, command: str) -> BoundCommandFunc[...] | None: """Get the bound command function for a command. @@ -4918,19 +4986,52 @@ def do_shell(self, args: argparse.Namespace) -> None: utils.expand_user_in_tokens(tokens) expanded_command = " ".join(tokens) - # Prevent KeyboardInterrupts while in the shell process. The shell process will - # still receive the SIGINT since it is in the same process group as us. - with self.sigint_protection: - # For any stream that is a StdSim, we will use a pipe so we can capture its output - proc = subprocess.Popen( # noqa: S602 - expanded_command, - stdout=subprocess.PIPE if isinstance(self.stdout, utils.StdSim) else self.stdout, # type: ignore[unreachable] - stderr=subprocess.PIPE if isinstance(sys.stderr, utils.StdSim) else sys.stderr, - shell=True, - **kwargs, - ) - - proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr) + # A terminal pipeline's consumer needs the terminal to drain the pipe, but a shell + # command writes into that pipe itself rather than through self.stdout, which lends + # the terminal per write. Run the command inside the pipeline's job instead, for as + # long as it runs: the consumer keeps the terminal, and Ctrl-C and Ctrl-Z reach both + # processes, as they would in a shell pipeline. + pipeline = self._cur_pipe_proc_reader + pipeline_group = None + if pipeline is not None and not isinstance(self.stdout, utils.StdSim): # type: ignore[unreachable] + pipeline_group = pipeline.terminal_group + + # Prevent KeyboardInterrupts while in the shell process. The shell process still + # receives the SIGINT: it is in our process group or in the foreground pipeline's. + with self.sigint_protection, contextlib.ExitStack() as terminal_stack: + if pipeline is not None and pipeline_group is not None: + kwargs["process_group"] = pipeline_group + terminal_stack.enter_context(pipeline.lend_terminal()) + while True: + try: + # For any stream that is a StdSim, we will use a pipe so we can capture its output. + # A command joining the pipeline is spawned inside the lend, which blocks SIGTTOU. + with utils.unblocked_sigttou() if "process_group" in kwargs else contextlib.nullcontext(): + proc = subprocess.Popen( # noqa: S602 + expanded_command, + stdout=subprocess.PIPE if isinstance(self.stdout, utils.StdSim) else self.stdout, # type: ignore[unreachable] + stderr=subprocess.PIPE if isinstance(sys.stderr, utils.StdSim) else sys.stderr, + shell=True, + **kwargs, + ) + break + except PermissionError: + # The pipeline exited before the command could join its group. + if kwargs.pop("process_group", None) is None: + raise + # The retry runs in our own group, so take the terminal back from the dead + # pipeline first. Its watcher left it lent, and the command would otherwise + # stop with SIGTTIN on its first terminal read, with nothing to resume it. + terminal_stack.close() + + # A command that joined the pipeline's job is waited for in short polls. Only the + # main thread runs Python signal handlers, and the job-control stop the pipeline's + # watcher relays may wake another thread. Once the consumer and its watcher are + # gone, the same wait relays the command's own stops, such as Ctrl-Z. + joined_pipeline = pipeline if "process_group" in kwargs else None + proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr, pipeline=joined_pipeline) + if joined_pipeline is not None: + proc_reader.wait_for_exit() proc_reader.wait() # Save the return code of the application for use in a pyscript diff --git a/cmd2/utils.py b/cmd2/utils.py index f88a46f20..23fda0c83 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -1,9 +1,11 @@ """Shared utility functions.""" import contextlib +import errno import functools import glob import inspect +import io import itertools import os import re @@ -13,6 +15,7 @@ from collections.abc import ( Callable, Iterable, + Iterator, MutableSequence, ) from difflib import SequenceMatcher @@ -533,22 +536,59 @@ def write(self, b: bytes) -> None: self.std_sim_instance.flush() +@contextlib.contextmanager +def unblocked_sigttou() -> Iterator[None]: + """Let a child started inside :meth:`ProcReader.lend_terminal` keep normal job control. + + The lend blocks SIGTTOU for its thread, and a child inherits that mask for life. Spawning + touches no terminal, so unblocking it for the spawn alone cannot stop this thread. + """ + import signal + + previous_mask = signal.pthread_sigmask(signal.SIG_UNBLOCK, {signal.SIGTTOU}) + try: + yield + finally: + signal.pthread_sigmask(signal.SIG_SETMASK, previous_mask) + + class ProcReader: """Used to capture stdout and stderr from a Popen process if any of those were set to subprocess.PIPE. If neither are pipes, then the process will run normally and no output will be captured. """ - def __init__(self, proc: PopenTextIO, stdout: StdSim | TextIO, stderr: StdSim | TextIO) -> None: + def __init__( + self, + proc: PopenTextIO, + stdout: StdSim | TextIO, + stderr: StdSim | TextIO, + *, + terminal_fd: int | None = None, + pipeline: "ProcReader | None" = None, + ) -> None: """ProcReader initializer. :param proc: the Popen process being read from :param stdout: the stream to write captured stdout :param stderr: the stream to write captured stderr. + :param terminal_fd: controlling terminal to lend to a POSIX process in its own group + :param pipeline: terminal pipeline whose process group proc joined as a producer """ self._proc = proc self._stdout = stdout self._stderr = stderr + self._terminal_fd = terminal_fd + self._pipeline = pipeline + self._process_done = threading.Event() + self._producer_finished = False + self._terminal_available = threading.Event() + # Lends in progress, guarded by _terminal_lock. The terminal is available while any is. + self._lends = 0 + self._terminal_lock = threading.RLock() + self._job_resumed = threading.Event() + if terminal_fd is not None: + self._original_group = os.tcgetpgrp(terminal_fd) self._out_thread = threading.Thread(name="out_thread", target=self._reader_thread_func, kwargs={"read_stdout": True}) @@ -573,16 +613,261 @@ def send_sigint(self) -> None: # the whole process group to make sure it propagates further than the shell try: group_id = os.getpgid(self._proc.pid) - os.killpg(group_id, signal.SIGINT) except ProcessLookupError: - return + # Pipelines lead their own group. A shell command that joined it, such + # as `shell sleep 100 | head -1`, can outlive the reaped consumer. + group_id = self._proc.pid + # Never re-signal our own group: other ProcReader callers may share it + # and already received Ctrl-C. + if group_id != os.getpgrp(): + with contextlib.suppress(ProcessLookupError): + os.killpg(group_id, signal.SIGINT) def terminate(self) -> None: """Terminate the process.""" - self._proc.terminate() + if self._terminal_fd is None: + self._proc.terminate() + else: + import signal + + # Popen.terminate() polls first, which would compete with our waitpid thread. + with contextlib.suppress(ProcessLookupError): + os.kill(self._proc.pid, signal.SIGTERM) + + @property + def terminal_group(self) -> int | None: + """Process group of a running terminal pipeline, which a producer may join, or None.""" + if self._terminal_fd is None or self._proc.returncode is not None: + return None + return self._proc.pid + + @staticmethod + def _set_foreground_group(terminal_fd: int, group_id: int) -> None: + """Transfer the terminal without stopping this background thread with SIGTTOU.""" + import signal + + previous_mask = signal.pthread_sigmask(signal.SIG_BLOCK, {signal.SIGTTOU}) + try: + os.tcsetpgrp(terminal_fd, group_id) + finally: + signal.pthread_sigmask(signal.SIG_SETMASK, previous_mask) + + @contextlib.contextmanager + def manage_terminal(self) -> Iterator[None]: + """Watch the pipeline and suspend the shell's whole job on the main thread.""" + import signal + + terminal_fd = self._terminal_fd + if terminal_fd is None: + yield + return + previous_handler = signal.getsignal(signal.SIGTSTP) + + def suspend_job(signum: int, frame: Any) -> None: + try: + if previous_handler != signal.SIG_DFL: + if callable(previous_handler): + previous_handler(signum, frame) + return + if os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) + # Ignore our group-directed copy, then stop this thread synchronously. + # Wrappers in our job must stop too. Unlike SIGSTOP, SIGTSTP is + # discarded for orphaned groups, which have no shell to resume them. + signal.signal(signal.SIGTSTP, signal.SIG_IGN) + try: + os.killpg(self._original_group, signal.SIGTSTP) + signal.signal(signal.SIGTSTP, signal.SIG_DFL) + signal.raise_signal(signal.SIGTSTP) + finally: + signal.signal(signal.SIGTSTP, suspend_job) + finally: + try: + if self._terminal_available.is_set() and os.tcgetpgrp(terminal_fd) == self._original_group: + self._set_foreground_group(terminal_fd, self._proc.pid) + finally: + self._job_resumed.set() + + signal.signal(signal.SIGTSTP, suspend_job) + try: + threading.Thread(name="pipe_job", target=self._wait_for_job, args=(terminal_fd,), daemon=True).start() + yield + finally: + signal.signal(signal.SIGTSTP, previous_handler) + + @contextlib.contextmanager + def lend_terminal(self) -> Iterator[None]: + """Lend the terminal only while writing to or waiting for the consumer. + + Command code retains foreground access between writes, including arbitrary + reads through input(), getpass(), or third-party libraries. Lending during + writes lets an interactive consumer drain a full pipe without deadlocking. + """ + import signal + + terminal_fd = self._terminal_fd + if terminal_fd is None or self._proc.returncode is not None: + yield + return + # While the consumer owns the terminal, a signal handler run on this thread may still + # write diagnostics to it. Block SIGTTOU for the lend only: a signal mask survives fork + # and exec, so blocking it for the whole pipeline would leak into every child the + # command starts. A child started during a lend must unblock it; see unblocked_sigttou(). + previous_mask = signal.pthread_sigmask(signal.SIG_BLOCK, {signal.SIGTTOU}) + try: + with self._terminal_lock: + try: + self._set_foreground_group(terminal_fd, self._proc.pid) + except OSError as error: + # The group can disappear before the watcher has reaped its leader. + if error.errno not in (errno.ESRCH, errno.EINVAL): + raise + self._lends += 1 + self._terminal_available.set() + try: + yield + finally: + with self._terminal_lock: + self._lends -= 1 + # Lends overlap: do_shell() lends for as long as a shell producer runs, + # while a pipe write from another thread lends and returns. Only the last + # to end takes the terminal back, or the producer would stop with SIGTTIN. + if not self._lends: + self._terminal_available.clear() + if os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) + finally: + signal.pthread_sigmask(signal.SIG_SETMASK, previous_mask) + + def _wait_for_job(self, terminal_fd: int) -> None: + """Reap a foreground pipeline and relay its stops to the outer shell's job. + + This is the only waitpid caller for a terminal pipeline. Watching on a separate + thread also catches Ctrl-Z while the command is blocked writing to its pipe. + """ + import signal + + try: + while True: + _, status = os.waitpid(self._proc.pid, os.WUNTRACED) + if not os.WIFSTOPPED(status): + self._proc.returncode = os.waitstatus_to_exitcode(status) + return + if os.WSTOPSIG(status) in (signal.SIGTTIN, signal.SIGTTOU): + # Command code owns the terminal between pipe writes. Defer + # consumer terminal access until the next write or final wait. + while True: + self._terminal_available.wait(0.1) + with self._terminal_lock: + # A stopped consumer can be killed before another write. + # Keep reaping even while command code owns the terminal. + pid, pending_status = os.waitpid(self._proc.pid, os.WNOHANG | os.WUNTRACED) + if pid and not os.WIFSTOPPED(pending_status): + self._proc.returncode = os.waitstatus_to_exitcode(pending_status) + return + # A short write may already have returned the terminal. + # Do not turn that ordinary handoff into a job suspension. + if not self._terminal_available.is_set(): + continue + foreground = os.tcgetpgrp(terminal_fd) + if foreground == self._proc.pid: + os.killpg(self._proc.pid, signal.SIGCONT) + break + if foreground == self._proc.pid: + continue + + if os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) + # Stop every terminal reader before returning control to the outer shell. + os.killpg(self._proc.pid, signal.SIGSTOP) + self._job_resumed.clear() + # Signal the main thread itself. Only it runs Python signal handlers, and a + # process-directed signal may be taken by another thread while the main + # thread sleeps in a system call, which then never returns to run the handler. + signal.pthread_kill(threading.main_thread().ident or 0, signal.SIGTSTP) + self._job_resumed.wait() + os.killpg(self._proc.pid, signal.SIGCONT) + finally: + try: + with self._terminal_lock: + # A shell producer in this group may outlive the consumer and still read + # the terminal. While a lend is active, its holder returns the terminal. + if not self._terminal_available.is_set() and os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) + finally: + self._process_done.set() + + def _relay_producer_stop(self) -> None: + """Suspend the shell's whole job for a stopped producer that outlived the consumer. + + Ctrl-Z reaches only the foreground group, and the producer may be all that is + left of it. The watcher ended with the consumer, so nothing else relays the stop. + """ + import signal + + terminal_fd = self._terminal_fd + if terminal_fd is None or not self._process_done.is_set(): + # A live watcher relays the consumer's stop and continues the whole group. + return + with self._terminal_lock: + if os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) + with contextlib.suppress(ProcessLookupError): + os.killpg(self._proc.pid, signal.SIGSTOP) + self._job_resumed.clear() + signal.pthread_kill(threading.main_thread().ident or 0, signal.SIGTSTP) + self._job_resumed.wait() + with contextlib.suppress(ProcessLookupError): + os.killpg(self._proc.pid, signal.SIGCONT) + + def _wait_for_producer(self, pipeline: "ProcReader", timeout: float | None) -> None: + """Wait for a producer in a terminal pipeline's job, relaying its job-control stops. + + This is the only waitpid caller for such a producer. It polls so that the main + thread keeps returning to Python code, where signal handlers run. + """ + import time + + deadline = None if timeout is None else time.monotonic() + timeout + while self._proc.returncode is None: + pid, status = os.waitpid(self._proc.pid, os.WNOHANG | os.WUNTRACED) + if not pid: + if deadline is not None and time.monotonic() >= deadline: + raise subprocess.TimeoutExpired(self._proc.args, timeout or 0) + time.sleep(0.05) + elif os.WIFSTOPPED(status): + pipeline._relay_producer_stop() + else: + self._proc.returncode = os.waitstatus_to_exitcode(status) + + def finish_producer(self) -> None: + """Disable producer cancellation before flushing and closing its pipe.""" + self._producer_finished = True + + def wait_for_exit(self, timeout: float | None = None) -> None: + """Wait for process exit without competing with the terminal job's waitpid thread. + + :param timeout: maximum seconds to wait, or None to wait indefinitely + :raises subprocess.TimeoutExpired: if the process is still running after timeout + """ + if self._pipeline is not None: + self._wait_for_producer(self._pipeline, timeout) + elif self._terminal_fd is None: + self._proc.wait(timeout) + elif timeout is None: + # A process-directed signal may reach a worker thread. Python still runs + # its handler on the main thread, so periodically return from the wait + # to dispatch it even when the main thread's system call was not interrupted. + while not self._process_done.wait(0.1): + pass + elif not self._process_done.wait(timeout): + raise subprocess.TimeoutExpired(self._proc.args, timeout) def wait(self) -> None: """Wait for the process to finish.""" + if self._terminal_fd is not None: + with self.lend_terminal(): + self.wait_for_exit() if self._out_thread.is_alive(): self._out_thread.join() if self._err_thread.is_alive(): @@ -614,7 +899,8 @@ def _reader_thread_func(self, read_stdout: bool) -> None: raise ValueError("read_stream is None") # Run until process completes - while self._proc.poll() is None: + polled = self._terminal_fd is None and self._pipeline is None + while (self._proc.poll() if polled else self._proc.returncode) is None: available = read_stream.peek() # type: ignore[attr-defined, ty:unresolved-attribute] if available: read_stream.read(len(available)) @@ -635,6 +921,54 @@ def _write_bytes(stream: StdSim | TextIO, to_write: bytes | str) -> None: stream.buffer.write(to_write) +class PipelineWriter(io.FileIO): + """A pipe whose blocking writes temporarily give the consumer terminal access.""" + + def __init__(self, fd: int, reader: ProcReader) -> None: + """Take ownership of a pipe descriptor managed by reader.""" + super().__init__(fd, "w") + self._reader = reader + + def write(self, b: Any) -> int: + """Write all of b while the consumer can interact with the terminal. + + The whole buffer goes out under one lend. Returning the terminal between two + writes, even for an instant, would stop a consumer that had just resumed a + terminal read with SIGTTIN. + + A full pipe is awaited in short polls rather than in one blocking write. Only + the main thread runs Python signal handlers, and the job-control stop ProcReader + relays may wake another thread, so the main thread has to return to Python code + on its own for the handler to run. The descriptor itself stays blocking: a shell + command inherits it, and a producer that found it non-blocking would fail with + EAGAIN once the pipe filled. + """ + import select + import signal + + view = memoryview(b).cast("B") + fd = self.fileno() + poller = select.poll() + poller.register(fd, select.POLLOUT) + try: + with self._reader.lend_terminal(): + written = 0 + while written < len(view): + # Once there is room, a write of at most PIPE_BUF bytes does not block. + if poller.poll(100): + written += os.write(fd, view[written : written + select.PIPE_BUF]) + return written + except BrokenPipeError: + # Ctrl-C during a blocking write must cancel the command, even if it + # normally catches BrokenPipeError. Raise here rather than signaling + # asynchronously: a late signal could interrupt redirection cleanup. + with contextlib.suppress(subprocess.TimeoutExpired): + self._reader.wait_for_exit(0.2) + if not self._reader._producer_finished and self._reader._proc.returncode in (-signal.SIGINT, 128 + signal.SIGINT): + raise KeyboardInterrupt from None + raise + + class ContextFlag: """A context manager which is also used as a boolean flag value within the default sigint handler. @@ -690,6 +1024,9 @@ def __init__( self.saved_pipe_proc_reader = pipe_proc_reader self.saved_redirecting = saved_redirecting + # Holds a terminal pipeline's job control until its pipe process has been reaped + self.pipeline_job: contextlib.ExitStack | None = None + def categorize(func: Callable[..., Any] | Iterable[Callable[..., Any]], category: str) -> None: """Categorize a function. diff --git a/docs/features/redirection.md b/docs/features/redirection.md index 27a238caa..27c8c79c7 100644 --- a/docs/features/redirection.md +++ b/docs/features/redirection.md @@ -30,6 +30,10 @@ Piping the output of a `cmd2` command to a shell command works just like in POSI - pipe as input to a shell command with `|`, as in `mycommand args | wc` +On POSIX systems, a pipe to an interactive program such as `less` runs as the terminal's foreground +job, as it would in a shell: the program can read the keyboard, and Ctrl-C and Ctrl-Z reach it. A +`shell` command whose output is piped this way, as in `shell git log | less`, joins the same job. + ## Multiple Pipes and Redirection Multiple pipes, optionally followed by a redirect, are supported. Thus, it is possible to do diff --git a/pyproject.toml b/pyproject.toml index ec98c9ef6..cd68a4b1f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -43,6 +43,7 @@ dev = [ "mkdocstrings[python]>=1", "mypy>=2.3.1", "prek>=0.3.5", + "pyte>=0.8.2", "pytest>=8.1.1", "pytest-cov>=5", "pytest-mock>=3.14.1", @@ -62,6 +63,7 @@ quality = ["prek>=0.3.5"] test = [ "codecov>=2.1", "coverage>=7.11.3", + "pyte>=0.8.2", "pytest>=8.1.1", "pytest-cov>=5", "pytest-mock>=3.14.1", @@ -104,6 +106,8 @@ warn_unused_ignores = false testpaths = ["tests"] addopts = [ "-n=auto", + # Spread slow terminal cases across workers instead of queuing them in one batch. + "--maxschedchunk=1", "--cov=cmd2", "--cov-config=pyproject.toml", "--cov-report=xml", @@ -112,6 +116,8 @@ addopts = [ ] [tool.coverage.run] +# Include the cmd2 applications launched by the terminal integration tests. +patch = ["subprocess"] # Use sys.monitoring on Python 3.12+; coverage falls back with a warning on 3.11. core = "sysmon" source = ["cmd2"] diff --git a/tests/test_cmd2.py b/tests/test_cmd2.py index 24009acfd..f4cb4793e 100644 --- a/tests/test_cmd2.py +++ b/tests/test_cmd2.py @@ -428,6 +428,55 @@ def test_shell_manual_call(base_app) -> None: base_app.do_shell(cmd) +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") +def test_shell_falls_back_to_own_group_when_pipeline_exited(base_app, tmp_path) -> None: + import contextlib + import subprocess + from unittest import mock + + # A group whose only member has exited cannot be joined. The consumer of a terminal + # pipeline can exit between the check and the spawn, like `shell sleep 1 | true`. + leader = subprocess.Popen([sys.executable, "-c", "pass"], process_group=0) + leader.wait() + lent = [] + + @contextlib.contextmanager + def lend_terminal(): + lent.append(True) + try: + yield + finally: + lent.pop() + + # The retry runs in our own group, so the terminal has to come back from the dead + # pipeline first. Otherwise the command stops with SIGTTIN on its first terminal read. + spawned_while_lent = [] + real_popen = subprocess.Popen + + def popen(*args, **kwargs): + spawned_while_lent.append(bool(lent)) + return real_popen(*args, **kwargs) + + base_app._cur_pipe_proc_reader = mock.Mock(terminal_group=leader.pid, lend_terminal=lend_terminal) + with (tmp_path / "output").open("w+") as output, mock.patch("subprocess.Popen", popen): + base_app.stdout = output + base_app.do_shell("echo joined") + output.seek(0) + assert output.read() == "joined\n" + assert base_app.last_result == 0 + assert spawned_while_lent == [True, False] + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX shell executable") +def test_shell_permission_error_unrelated_to_pipeline(base_app, tmp_path, monkeypatch) -> None: + unusable_shell = tmp_path / "shell" + unusable_shell.write_text("#!/bin/sh\n") + unusable_shell.chmod(0o644) + monkeypatch.setenv("SHELL", str(unusable_shell)) + with pytest.raises(PermissionError): + base_app.do_shell("echo hi") + + def test_base_error(base_app) -> None: _out, err = run_cmd(base_app, "meow") assert "is not a recognized command" in err[0] @@ -865,7 +914,11 @@ def test_pipe_to_shell_and_redirect(redirection_app, running_pipe_process) -> No os.remove(filename) -def test_pipe_to_shell_error(redirection_app, mocker, capsys) -> None: +@pytest.mark.parametrize( + "terminal", + [False, pytest.param(True, marks=pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control"))], +) +def test_pipe_to_shell_error(redirection_app, mocker, capsys, terminal) -> None: """An already-exited pipe process must be reported before the command runs. A real nonexistent command may take longer than the startup probe under load. @@ -876,15 +929,73 @@ def test_pipe_to_shell_error(redirection_app, mocker, capsys) -> None: process = popen.return_value process.returncode = 127 process.wait.return_value = 127 - - out, err = run_cmd(redirection_app, "print_output | foobarbaz.this_does_not_exist") + if terminal: + terminal_stream = mocker.Mock() + terminal_stream.isatty.return_value = True + terminal_stream.fileno.return_value = 10 + redirection_app.stdout = terminal_stream + mocker.patch("os.tcgetpgrp", return_value=os.getpgrp()) + mocker.patch("os.getsid", return_value=os.getpgrp()) + sigmask = mocker.patch("signal.pthread_sigmask", return_value=set()) + reader = mocker.patch("cmd2.utils.ProcReader").return_value + previous_tstp = signal.getsignal(signal.SIGTSTP) + + def start_pipe(*args, **kwargs): + # Session-led pipelines inherit ignored Ctrl-Z, but the caller's + # handler must be restored even when startup reports an early exit. + assert signal.getsignal(signal.SIGTSTP) == signal.SIG_IGN + return process + + popen.side_effect = start_pipe + + if terminal: + # run_cmd captures stderr in a StdSim, which deliberately disables terminal handoff. + redirection_app.onecmd_plus_hooks("print_output | foobarbaz.this_does_not_exist") + out, error_text = capsys.readouterr() + err = error_text.splitlines() + else: + out, err = run_cmd(redirection_app, "print_output | foobarbaz.this_does_not_exist") assert not out assert "Pipe process exited with code 127 before command could run" in " ".join(err) assert capsys.readouterr().out == "" - process.wait.assert_called_once() + if terminal: + assert signal.getsignal(signal.SIGTSTP) == previous_tstp + reader.wait_for_exit.assert_called_once_with(0.2) + reader.wait.assert_called_once_with() + process.wait.assert_not_called() + # SIGTTOU is blocked only inside ProcReader's lends. Blocking it for the whole + # pipeline would leak the mask into every child the command starts. + sigmask.assert_not_called() + else: + process.wait.assert_called_once() assert popen.call_args.kwargs["stdin"].closed +def test_restore_output_resets_pipe_state_when_the_wait_fails(base_app) -> None: + """A failed handback while waiting for the pipe process must not leave it current. + + Otherwise ppaged() would never page again, and Ctrl-C would keep going to a dead group. + """ + import errno + + statement = base_app.statement_parser.parse("help | less") + saved_stdout = base_app.stdout + saved = cmd2.utils.RedirectionSavedState(saved_stdout, None, False) + saved.redirecting = True + reader = mock.Mock() + reader.wait.side_effect = OSError(errno.EIO, "terminal hung up") + base_app._cur_pipe_proc_reader = reader + base_app._redirecting = True + base_app.stdout = io.StringIO() + + with pytest.raises(OSError, match="terminal hung up"): + base_app._restore_output(statement, saved) + + assert base_app.stdout is saved_stdout + assert base_app._cur_pipe_proc_reader is None + assert base_app._redirecting is False + + def test_send_to_paste_buffer(redirection_app: RedirectionApp, capsys: pytest.CaptureFixture[str], mocker) -> None: # Exercise cmd2's real clipboard redirection against a private backend, not the # shared OS clipboard (which another test run or desktop application can alter). diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py new file mode 100644 index 000000000..1ad79e343 --- /dev/null +++ b/tests/test_pipeline_job_control.py @@ -0,0 +1,722 @@ +"""Exercise pipeline job control through a real controlling terminal and outer shell.""" + +import codecs +import contextlib +import os +import re +import select +import shlex +import shutil +import signal +import subprocess +import sys +import time +from pathlib import Path + +import pyte +import pytest + +pytestmark = pytest.mark.skipif(sys.platform == "win32", reason="POSIX job control") + + +def describe_processes(root: int, master: int) -> str: + """Report the terminal's foreground group and every descendant of root, for a timeout. + + A silent transcript says only that nothing happened. Process states (T for stopped) + and wait channels say which process was waiting for whom. + """ + try: + foreground: object = os.tcgetpgrp(master) + except OSError as error: + foreground = error + try: + listing = subprocess.run( + ["ps", "-e", "-o", "pid,ppid,pgid,stat,wchan,command"], capture_output=True, text=True, check=False + ) + except OSError as error: + return f"foreground process group: {foreground}\nno process listing: {error}" + rows = listing.stdout.splitlines() + parents = {} + for row in rows[1:]: + fields = row.split(maxsplit=2) + if len(fields) >= 2 and fields[0].isdigit() and fields[1].isdigit(): + parents[int(fields[0])] = int(fields[1]) + family = {root} + while True: + grown = family | {pid for pid, parent in parents.items() if parent in family} + if grown == family: + break + family = grown + described = [row for row in rows[1:] if row.split(maxsplit=1)[0].isdigit() and int(row.split(maxsplit=1)[0]) in family] + return "\n".join([f"foreground process group: {foreground}", rows[0] if rows else "", *described]) + + +@pytest.mark.parametrize( + ("finish", "stop_job", "shell_child", "launcher", "producer", "relay"), + [ + pytest.param("interrupts", True, False, "direct", "command", "main", id="direct-signals-and-job-control"), + pytest.param("interrupts", True, True, "sh", "command", "main", id="wrapper-signals-and-job-control"), + pytest.param("exit_sigint", False, False, "direct", "command", "main", id="interrupt-busy-producer"), + pytest.param("exit_sigint", True, True, "uv", "command", "worker", id="uv-stop-and-interrupt-busy-producer"), + pytest.param("exit_sigint", False, False, "direct", "shell", "main", id="interrupt-busy-shell-producer"), + pytest.param("exit_sigint", True, True, "sh", "shell", "worker", id="wrapper-stop-and-interrupt-busy-shell-producer"), + pytest.param("read_input", False, False, "direct", "command", "main", id="nested-prompt"), + pytest.param("shell_input", False, True, "direct", "command", "main", id="shell-input"), + pytest.param("direct_input", False, False, "sh", "command", "main", id="wrapper-direct-input"), + pytest.param("direct_input", False, False, "exec", "command", "main", id="direct-input-in-orphaned-session"), + pytest.param("interrupts", False, False, "exec", "command", "main", id="orphaned-job-control"), + ], +) +def test_pipeline_stops_with_cmd2_and_returns_terminal( + tmp_path, finish, stop_job, shell_child, launcher, producer, relay +) -> None: + import fcntl + import pty + import struct + import termios + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + pager = tmp_path / "pager.py" + pager_pid = tmp_path / "pager.pid" + interrupts = tmp_path / "interrupts" + pager.write_text( + "import errno, os, pathlib, signal, sys, termios, tty\n" + f"if {launcher == 'exec'!r}: assert signal.getsignal(signal.SIGTSTP) == signal.SIG_IGN\n" + # Interactive bash leaves TTIN/TTOU ignored when exec replaces it. Give + # the simulated pager normal terminal-access stops: EOF can arrive before + # cmd2 lends it the terminal, and an ignored TTIN makes that read fail with + # EIO instead of waiting for the handoff. Preserve the inherited TSTP policy. + "signal.signal(signal.SIGTTIN, signal.SIG_DFL)\n" + "signal.signal(signal.SIGTTOU, signal.SIG_DFL)\n" + f"pathlib.Path({str(pager_pid)!r}).write_text(str(os.getpid()))\n" + f"if {finish != 'exit_sigint'!r}: sys.stdin.read()\n" + # Like less, use an inherited terminal descriptor for keyboard input when + # stdin is a pipe. This also works in the broken detached-session case. + "with os.fdopen(os.dup(sys.stderr.fileno()), 'rb', buffering=0) as terminal:\n" + " saved = termios.tcgetattr(terminal)\n" + " def setcbreak():\n" + " while True:\n" + " try:\n" + " tty.setcbreak(terminal)\n" + " return\n" + " except termios.error as error:\n" + " if error.args[0] != errno.EINTR: raise\n" + # os.write rather than print: a signal handler that uses buffered stdout raises + # "reentrant call inside <_io.BufferedWriter>" when the signal lands mid-write, + # which happens when the job is stopped while still reporting readiness. + " def resume(*args):\n" + " setcbreak()\n" + " os.write(1, b'PAGER_RESUMED\\n')\n" + " signal.signal(signal.SIGCONT, resume)\n" + " def interrupt(*args):\n" + f" fd = os.open({str(interrupts)!r}, os.O_WRONLY | os.O_CREAT | os.O_APPEND, 0o600)\n" + " os.write(fd, b'I')\n" + " os.close(fd)\n" + " os.write(1, b'PAGER_INTERRUPT\\n')\n" + f" signal.signal(signal.SIGINT, {'signal.SIG_DFL' if finish == 'exit_sigint' else 'interrupt'})\n" + " try:\n" + " setcbreak()\n" + " os.write(1, b'PAGER_READY\\n')\n" + " while True:\n" + " key = os.read(terminal.fileno(), 1)\n" + " if key == b'q': break\n" + " if key == b'p': os.write(1, b'PAGER_ALIVE\\n')\n" + " finally:\n" + " termios.tcsetattr(terminal, termios.TCSANOW, saved)\n", + encoding="utf-8", + ) + application = tmp_path / "application.py" + application_pid = tmp_path / "application.pid" + interrupt_request = tmp_path / "interrupt.request" + last_result = tmp_path / "last_result" + application.write_text( + "from cmd2 import Cmd\n" + "from cmd2.plugin import CommandFinalizationData\n" + "import getpass, os, pathlib, signal, threading, time\n" + "signal.signal(signal.SIGTSTP, signal.SIG_DFL)\n" + f"if {relay == 'worker'!r}:\n" + # The kernel may hand a signal to a thread other than the one it was aimed at. + # Deliver the job-control relay to the watcher that sends it, so the main thread, + # blocked in a pipe write or a wait, only learns of it if it returns on its own. + " _pthread_kill = signal.pthread_kill\n" + " signal.pthread_kill = lambda thread_id, signum: _pthread_kill(threading.get_ident(), signum)\n" + f"pathlib.Path({str(application_pid)!r}).write_text(str(os.getpid()))\n" + "class App(Cmd):\n" + " def do_busy(self, statement):\n" + " os.write(2, b'BUSY_READY\\n')\n" + " self.stdout.write('x' * 262144)\n" + " self.stdout.flush()\n" + " time.sleep(30)\n" + " def do_ask(self, statement):\n" + " self.poutput(self.read_input('INPUT> '))\n" + " def do_direct(self, statement):\n" + " self.stdout.buffer.write(b'x' * 262144)\n" + " self.stdout.flush()\n" + " assert self.select('first second', 'SELECT> ') == 'first'\n" + " assert input('PLAIN> ') == 'answer'\n" + " assert getpass.getpass('SECRET> ') == 'secret'\n" + " os.write(2, b'RAW> ')\n" + " assert os.read(0, 7) == b'direct\\n'\n" + " self.poutput('INPUT_COMPLETE')\n" + " def record_result(self, data: CommandFinalizationData) -> CommandFinalizationData:\n" + f" pathlib.Path({str(last_result)!r}).write_text(repr(self.last_result))\n" + " return data\n" + "app = App()\n" + "app.register_cmdfinalization_hook(app.record_result)\n" + "app.prompt = 'TEST> '\n" + "app.debug = True\n" + f"if {finish == 'interrupts'!r}:\n" + " def interrupt_from_worker():\n" + f" request = pathlib.Path({str(interrupt_request)!r})\n" + " for _ in range(2):\n" + " while not request.exists():\n" + " time.sleep(0.01)\n" + " request.unlink()\n" + # A process-directed signal can be delivered to any unblocked thread. + # Force that case so an indefinite main-thread wait cannot pass by luck. + " signal.pthread_kill(threading.get_ident(), signal.SIGINT)\n" + " threading.Thread(target=interrupt_from_worker, daemon=True).start()\n" + "app.cmdloop()\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + fcntl.ioctl(slave, termios.TIOCSWINSZ, struct.pack("HHHH", 24, 80, 0, 0)) + if finish == "exit_sigint": + settings = termios.tcgetattr(slave) + settings[3] |= termios.TOSTOP + termios.tcsetattr(slave, termios.TCSANOW, settings) + # Establish a controlling terminal in a fresh interpreter, avoiding preexec_fn + # (unsafe when pytest or its plugins have started threads). + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + # Exercise the same pipeline shell on developer machines and in CI. An + # inherited zsh can exec the pager directly, hiding bash's stop/wait behavior. + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + screen = pyte.Screen(80, 24) + screen.write_process_input = lambda data: os.write(master, data.encode()) + stream = pyte.Stream(screen) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def send(data): + os.write(master, data.encode()) + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + data = decoder.decode(os.read(master, 65536)) + transcript += data + stream.feed(data) + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") + + def stopped(*pids: int) -> bool: + """Whether every process is stopped, not merely deprived of the terminal. + + The shell takes the terminal back as soon as its child stops, but a grandchild still + blocked in a one-byte terminal read is woken by the stop signal and, if a keystroke + has arrived by then, consumes it before it stops. Typing has to wait for the whole job. + """ + pids = tuple(set(pids)) + listing = subprocess.run( + ["ps", "-o", "stat=", "-p", ",".join(map(str, pids))], capture_output=True, text=True, check=False + ) + states = listing.stdout.split() + return len(states) == len(pids) and all(state.startswith("T") for state in states) + + job_group = None + pipeline_group = None + try: + wait_until(lambda: "OUTER> " in transcript) + launch = f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}" + if launcher == "sh": + launch = f"{shlex.quote(shell)} -c {shlex.quote(launch + '; :')}" + elif launcher == "uv": + uv = shutil.which("uv") + if uv is None: + pytest.skip("requires uv") + launch = f"{shlex.quote(uv)} run --no-project -- {launch}" + elif launcher == "exec": + launch = "exec " + launch + send(launch + "\n") + wait_until(lambda: "TEST>" in "\n".join(screen.display)) + # A foreground-group query is an observation, not the child's identity. + # It can change during startup and handoffs. Never use an unverified + # foreground query as a kill()/killpg() destination. + app_pid = int(application_pid.read_text()) + job_group = os.getpgid(app_pid) + assert job_group > 1 + wait_until(lambda: os.tcgetpgrp(master) == job_group) + command = "busy" if finish == "exit_sigint" else "help -v" + if producer == "shell": + # A shell command writes into the pipe itself rather than through cmd2's + # stdout, so cmd2 cannot lend the terminal write by write. Like seq or git + # log, this producer dies from SIGINT rather than handling it. + busy_script = tmp_path / "busy.py" + busy_script.write_text( + "import os, signal, sys, time\n" + "signal.signal(signal.SIGINT, signal.SIG_DFL)\n" + "os.write(2, b'BUSY_READY\\n')\n" + "sys.stdout.write('x' * 262144)\n" + "sys.stdout.flush()\n" + "time.sleep(30)\n", + encoding="utf-8", + ) + command = f"shell {shlex.quote(sys.executable)} {shlex.quote(str(busy_script))}" + if finish == "read_input": + command = "ask" + elif finish == "direct_input": + command = "direct" + elif finish == "shell_input": + input_script = tmp_path / "input.py" + input_script.write_text("import os\nos.write(2, b'INPUT> ')\ninput()\n", encoding="utf-8") + command = f"shell {shlex.quote(sys.executable)} {shlex.quote(str(input_script))}" + pipe_command = f"{shlex.quote(sys.executable)} {shlex.quote(str(pager))}" + if shell_child: + # Keep a shell between Popen and the terminal reader, rather than allowing + # the final command to replace it with exec. + pipe_command = f"{shlex.quote(shell)} -c {shlex.quote(pipe_command + '; :')}" + send(f"{command} | {pipe_command}\n") + if finish == "direct_input": + for prompt, response in (("SELECT>", "\r"), ("PLAIN>", "answer\n"), ("SECRET>", "secret\n"), ("RAW>", "direct\n")): + wait_until(lambda prompt=prompt: prompt in "\n".join(screen.display)) + assert os.tcgetpgrp(master) == job_group + send(response) + if finish in ("read_input", "shell_input"): + wait_until(lambda: any(line.startswith("INPUT>") for line in screen.display)) + send("answer\n") + # Whole lines only: a traceback naming the marker must not satisfy the wait. + wait_until(lambda: "PAGER_READY\r\n" in transcript) + if finish == "exit_sigint": + wait_until(lambda: "BUSY_READY\r\n" in transcript) + pager_process = int(pager_pid.read_text()) + pipeline_group = os.getpgid(pager_process) + assert pipeline_group > 1 + assert pipeline_group != job_group + # Readiness output can precede the foreground handoff. Send terminal + # signals and keystrokes only once the pipeline can receive them. + wait_until(lambda: os.tcgetpgrp(master) == pipeline_group) + if launcher == "exec": + # There is no outer shell to run fg: Ctrl-Z must leave the pager + # usable. Require a fresh read acknowledgement, not a SIGCONT. + start = len(transcript) + send("\x1ap") + wait_until(lambda: "PAGER_ALIVE\r\n" in transcript[start:]) + assert os.tcgetpgrp(master) == pipeline_group + for rows in (12, 24) if stop_job else (): + send("\x1a") + wait_until(lambda: os.tcgetpgrp(master) == process.pid) + wait_until(lambda: stopped(job_group, app_pid, pager_process)) + start = len(transcript) + # A child left running can steal these keystrokes from the shell. + send("printf 'SHELL_%s\\n' OWNS_INPUT\n") + wait_until(lambda start=start: "SHELL_OWNS_INPUT" in transcript[start:]) + fcntl.ioctl(master, termios.TIOCSWINSZ, struct.pack("HHHH", rows, 80, 0, 0)) + screen.resize(lines=rows, columns=80) + start = len(transcript) + send("stty size\n") + # Bash 5.1+ turns bracketed paste off with "\x1b[?2004l\r" before running the + # command, so the reply may follow a bare "\r" rather than "\r\n". + wait_until(lambda start=start, rows=rows: re.search(rf"[\r\n]{rows} 80\r\n", transcript[start:]) is not None) + start = len(transcript) + send("fg\n") + wait_until(lambda start=start: "PAGER_RESUMED\r\n" in transcript[start:]) + assert os.tcgetpgrp(master) == pipeline_group + if finish == "exit_sigint": + send("\x03") + elif finish == "interrupts": + # Exercise every signal route on this live pipeline, avoiding a fresh + # interpreter and terminal setup for each overlapping matrix combination. + sources = ("terminal", "terminal", "process", "process", "thread", "thread", "group", "group") + for expected_count, source in enumerate(sources, start=1): + if source == "process": + # Signal cmd2 alone, as with `kill -INT `. + os.kill(app_pid, signal.SIGINT) + elif source == "thread": + interrupt_request.touch() + elif source == "group": + os.killpg(pipeline_group, signal.SIGINT) + else: + send("\x03") + wait_until(lambda count=expected_count: transcript.count("PAGER_INTERRUPT\r\n") >= count) + # Keep the handler alive long enough to observe a duplicate delivery, + # then also check that a second real interrupt is not suppressed. + deadline = time.monotonic() + 0.1 + wait_until(lambda deadline=deadline: time.monotonic() >= deadline) + assert interrupts.read_text() == "I" * expected_count + if finish != "exit_sigint": + send("q") + wait_until(lambda: os.tcgetpgrp(master) == job_group and "TEST>" in "\n".join(screen.display)) + if producer == "shell": + # Ctrl-C reached the producer directly, as in a shell pipeline. It did not + # merely die of a broken pipe once the pager was gone. + wait_until(last_result.exists) + assert last_result.read_text() == repr(-signal.SIGINT) + start = len(transcript) + send("help quit\n") + wait_until(lambda: "Exit this application" in transcript[start:]) + send("quit\n") + if launcher != "exec": + wait_until(lambda: os.tcgetpgrp(master) == process.pid) + finally: + # Kill only this test's job, including stopped descendants, on assertion failure. + if pager_pid.exists(): + with contextlib.suppress(ProcessLookupError): + os.kill(int(pager_pid.read_text()), signal.SIGKILL) + if job_group is not None and job_group != process.pid: + with contextlib.suppress(ProcessLookupError): + os.killpg(job_group, signal.SIGKILL) + if pipeline_group is not None: + with contextlib.suppress(ProcessLookupError): + os.killpg(pipeline_group, signal.SIGKILL) + # Release the PTY before reaping its session leader. On macOS, waiting + # while the master is still open can leave terminal teardown blocked. + os.close(master) + process.kill() + process.wait(timeout=5) + + +def test_pipeline_from_worker_thread_stays_isolated(tmp_path) -> None: + """A pipe started off the main thread cannot install job-control handlers. + + It must fall back to running the pipeline in its own session, as before, rather + than failing after Popen and leaving the child unreaped. + """ + import pty + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + application = tmp_path / "application.py" + application.write_text( + "from cmd2 import Cmd\n" + "import os, threading\n" + "app = Cmd()\n" + "outcome = []\n" + "worker = threading.Thread(target=lambda: outcome.append(app.onecmd_plus_hooks('help quit | cat')))\n" + "worker.start()\n" + "worker.join()\n" + "try:\n" + " reaped = os.waitpid(-1, os.WNOHANG)\n" + "except ChildProcessError:\n" + " reaped = None\n" + "os.write(1, f'WORKER_DONE {outcome} {reaped}\\n'.encode())\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + transcript += decoder.decode(os.read(master, 65536)) + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") + + try: + wait_until(lambda: "OUTER> " in transcript) + os.write(master, f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}\n".encode()) + # The whole line: a partial read must not satisfy the wait before the reap result arrives. + wait_until(lambda: re.search(r"WORKER_DONE .*\r\n", transcript) is not None) + assert "Exit this application" in transcript + assert "WORKER_DONE [False] None" in transcript + finally: + os.close(master) + process.kill() + process.wait(timeout=5) + + +def test_pipeline_pager_can_set_terminal_modes_at_startup(tmp_path) -> None: + """A pager such as less puts the terminal in raw mode as it starts, before reading its pipe. + + It has to own the terminal by then. A background tcsetattr() stops it with SIGTTOU, and + on macOS the call then fails with EINTR once it is continued rather than being restarted. + less ignores that failure, leaving a cooked terminal: q needs Enter and keys are echoed. + """ + import pty + import termios + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + pager = tmp_path / "pager.py" + outcome = tmp_path / "outcome" + pager.write_text( + "import os, pathlib, sys, termios, tty\n" + "with os.fdopen(os.dup(sys.stderr.fileno()), 'rb', buffering=0) as terminal:\n" + " saved = termios.tcgetattr(terminal)\n" + " try:\n" + # Whether cmd2 had lent the terminal yet, should the attempt fail. + " foreground = os.tcgetpgrp(terminal.fileno()) == os.getpgrp()\n" + # Like less, make a single attempt and carry on whatever comes of it. + " try:\n" + " tty.setcbreak(terminal)\n" + " result = 'ok'\n" + " except termios.error as error:\n" + " result = f'{error!r}, foreground before the attempt: {foreground}'\n" + f" pathlib.Path({str(outcome)!r}).write_text(result)\n" + " while os.read(terminal.fileno(), 1) != b'q': pass\n" + " finally:\n" + " termios.tcsetattr(terminal, termios.TCSANOW, saved)\n", + encoding="utf-8", + ) + application = tmp_path / "application.py" + application.write_text( + "import pathlib, time\n" + "from cmd2 import Cmd, utils\n" + # The pipeline's first wait is cmd2's 0.2s startup check. Hold it open until the pager + # has reported, so the test does not race that timer on a busy CI runner: what it checks + # is that the pager owns the terminal throughout the check, however slowly it starts. + "startup_wait = utils.ProcReader.wait_for_exit\n" + "def held_startup_wait(reader, timeout=None):\n" + " utils.ProcReader.wait_for_exit = startup_wait\n" + f" outcome = pathlib.Path({str(outcome)!r})\n" + " deadline = time.monotonic() + 5\n" + " while time.monotonic() < deadline and not (outcome.exists() and outcome.read_text()):\n" + " time.sleep(0.01)\n" + " return startup_wait(reader, timeout)\n" + "utils.ProcReader.wait_for_exit = held_startup_wait\n" + "app = Cmd()\n" + "app.prompt = 'TEST> '\n" + "app.cmdloop()\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + data = decoder.decode(os.read(master, 65536)) + transcript += data + if "\x1b[6n" in data: + # Answer prompt-toolkit's cursor-position request as a terminal would. + os.write(master, b"\x1b[1;1R") + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") + + try: + wait_until(lambda: "OUTER> " in transcript) + os.write(master, f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}\n".encode()) + wait_until(lambda: "TEST>" in transcript) + os.write(master, f"help -v | {shlex.quote(sys.executable)} {shlex.quote(str(pager))}\n".encode()) + wait_until(outcome.exists) + wait_until(lambda: outcome.read_text() != "") + assert outcome.read_text() == "ok" + assert not termios.tcgetattr(master)[3] & termios.ICANON + # A cooked terminal would hold the key back until Enter. + start = len(transcript) + os.write(master, b"q") + wait_until(lambda: "TEST>" in transcript[start:]) + os.write(master, b"quit\n") + wait_until(lambda: os.tcgetpgrp(master) == process.pid) + finally: + os.close(master) + process.kill() + process.wait(timeout=5) + + +@pytest.mark.parametrize("producer", ["command", "shell"]) +def test_pipeline_children_inherit_an_ordinary_signal_mask(tmp_path, producer) -> None: + """Processes started during a terminal pipeline must not inherit a blocked SIGTTOU. + + cmd2 blocks SIGTTOU for itself while it lends the terminal. A signal mask survives fork + and exec, so a child spawned with it blocked -- a shell producer, or a subprocess run by + command code -- would keep it for life, and change terminal modes from the background + where it should be stopped. + """ + import pty + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + probe = tmp_path / "probe.py" + outcome = tmp_path / "outcome" + probe.write_text( + "import pathlib, signal\n" + "blocked = signal.SIGTTOU in signal.pthread_sigmask(signal.SIG_BLOCK, [])\n" + f"pathlib.Path({str(outcome)!r}).write_text(repr(blocked))\n", + encoding="utf-8", + ) + application = tmp_path / "application.py" + application.write_text( + "import subprocess, sys\n" + "from cmd2 import Cmd\n" + "class App(Cmd):\n" + " def do_probe(self, _):\n" + " self.poutput('probing')\n" + f" subprocess.run([sys.executable, {str(probe)!r}], check=True)\n" + "app = App()\n" + "app.prompt = 'TEST> '\n" + "app.cmdloop()\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + data = decoder.decode(os.read(master, 65536)) + transcript += data + if "\x1b[6n" in data: + # Answer prompt-toolkit's cursor-position request as a terminal would. + os.write(master, b"\x1b[1;1R") + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") + + command = "probe" if producer == "command" else f"shell {shlex.quote(sys.executable)} {shlex.quote(str(probe))}" + try: + wait_until(lambda: "OUTER> " in transcript) + os.write(master, f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}\n".encode()) + wait_until(lambda: "TEST>" in transcript) + start = len(transcript) + os.write(master, f"{command} | cat\n".encode()) + wait_until(lambda: outcome.exists() and outcome.read_text() != "" and "TEST>" in transcript[start:]) + assert outcome.read_text() == "False" + os.write(master, b"quit\n") + wait_until(lambda: os.tcgetpgrp(master) == process.pid) + finally: + os.close(master) + process.kill() + process.wait(timeout=5) + + +@pytest.mark.parametrize("suspend", [False, True]) +def test_shell_producer_keeps_the_terminal_after_its_consumer_exits(tmp_path, suspend) -> None: + """A shell producer that outlives its consumer still reads the terminal. + + do_shell() lends the terminal to the pipeline's group for as long as the producer runs. + The consumer's exit must not take it back early: the producer would stop with SIGTTIN on + its next terminal read, and nothing watches an ordinary shell command for stops. + + Ctrl-Z then reaches the producer alone, since it is all that is left of the foreground + group. With the consumer's watcher gone, do_shell() has to relay that stop to the + whole job itself, or it waits forever on a stopped child. + """ + import pty + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + consumer = tmp_path / "consumer.py" + consumer.write_text("import os, time\ntime.sleep(0.5)\nos.write(2, b'CONSUMER_DONE\\n')\n", encoding="utf-8") + producer = tmp_path / "producer.py" + producer.write_text( + "import os, signal, time\n" + # Interactive bash leaves TTIN ignored in what it execs, which turns a background read into EIO. + "signal.signal(signal.SIGTTIN, signal.SIG_DFL)\n" + "time.sleep(1.5)\n" + "os.write(2, b'PRODUCER> ')\n" + "os.write(2, b'GOT ' + os.read(0, 7))\n", + encoding="utf-8", + ) + application = tmp_path / "application.py" + application.write_text( + "from cmd2 import Cmd\napp = Cmd()\napp.prompt = 'TEST> '\napp.cmdloop()\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + data = decoder.decode(os.read(master, 65536)) + transcript += data + if "\x1b[6n" in data: + # Answer prompt-toolkit's cursor-position request as a terminal would. + os.write(master, b"\x1b[1;1R") + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") + + python = shlex.quote(sys.executable) + try: + wait_until(lambda: "OUTER> " in transcript) + os.write(master, f"{python} {shlex.quote(str(application))}\n".encode()) + wait_until(lambda: "TEST>" in transcript) + os.write(master, f"shell {python} {shlex.quote(str(producer))} | {python} {shlex.quote(str(consumer))}\n".encode()) + wait_until(lambda: "CONSUMER_DONE\r\n" in transcript) + wait_until(lambda: "PRODUCER> " in transcript) + if suspend: + start = len(transcript) + os.write(master, b"\x1a") + wait_until(lambda: os.tcgetpgrp(master) == process.pid and "OUTER> " in transcript[start:]) + os.write(master, b"fg\n") + wait_until(lambda: os.tcgetpgrp(master) not in (process.pid, os.getpgid(process.pid))) + os.write(master, b"answer\n") + wait_until(lambda: "GOT answer" in transcript) + # cmd2 owns the terminal again once the producer is done. + start = len(transcript) + wait_until(lambda: "TEST>" in transcript[start:]) + os.write(master, b"help quit\n") + wait_until(lambda: "Exit this application" in transcript[start:]) + os.write(master, b"quit\n") + wait_until(lambda: os.tcgetpgrp(master) == process.pid) + finally: + os.close(master) + process.kill() + process.wait(timeout=5) diff --git a/tests/test_utils.py b/tests/test_utils.py index 9c737d800..d5aa5a806 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,9 +1,12 @@ """Unit testing for cmd2/utils.py module.""" +import contextlib +import errno import math import os import signal import sys +import threading import time from unittest import ( mock, @@ -219,6 +222,108 @@ def test_proc_reader_send_sigint(pr_none) -> None: assert ret_code == -signal.SIGINT +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") +def test_proc_reader_does_not_resignal_its_own_group(pr_none) -> None: + try: + with mock.patch("os.getpgrp", return_value=pr_none._proc.pid), mock.patch("os.killpg") as killpg: + pr_none.send_sigint() + killpg.assert_not_called() + finally: + pr_none.terminate() + pr_none.wait() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") +def test_proc_reader_sigint_after_pipeline_exit() -> None: + reader = cu.ProcReader(mock.Mock(pid=os.getpid() + 1, stdout=None, stderr=None), sys.stdout, sys.stderr) + with ( + mock.patch("os.getpgid", side_effect=ProcessLookupError), + mock.patch("os.killpg", side_effect=ProcessLookupError) as killpg, + ): + reader.send_sigint() + killpg.assert_called_once_with(reader._proc.pid, signal.SIGINT) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") +def test_proc_reader_sigint_reaches_group_after_leader_exit() -> None: + """A shell producer joins the pipeline's group and can outlive the consumer that led it.""" + import subprocess + + # A terminal pipeline leads its own group within our session, so a producer may join it. + leader = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(30)"], process_group=0) + reader = cu.ProcReader(leader, sys.stdout, sys.stderr) + member_code = ( + "import signal, time; signal.signal(signal.SIGINT, signal.SIG_DFL); print('ready', flush=True); time.sleep(30)" + ) + member = subprocess.Popen([sys.executable, "-c", member_code], stdout=subprocess.PIPE, process_group=leader.pid) + try: + assert member.stdout is not None + assert member.stdout.readline().strip() == b"ready" + reader.terminate() + reader.wait() + assert leader.returncode == -signal.SIGTERM + + reader.send_sigint() + assert member.wait(timeout=5) == -signal.SIGINT + finally: + member.kill() + member.wait() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") +def test_proc_reader_terminal_group() -> None: + proc = mock.Mock(pid=4242, returncode=None, stdout=None, stderr=None) + assert cu.ProcReader(proc, sys.stdout, sys.stderr).terminal_group is None + + with mock.patch("os.tcgetpgrp", return_value=os.getpgrp()): + reader = cu.ProcReader(proc, sys.stdout, sys.stderr, terminal_fd=0) + assert reader.terminal_group == proc.pid + proc.returncode = 0 + assert reader.terminal_group is None + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipes") +@pytest.mark.parametrize("producer", ["writer", "child"]) +def test_pipeline_writer_delivers_more_than_the_pipe_holds(producer) -> None: + """Both cmd2's writes and a child inheriting the descriptor must wait for a slow consumer. + + A shell producer gets the descriptor itself, so it must stay blocking: a child that + inherits O_NONBLOCK fails with EAGAIN once the pipe is full. + """ + import subprocess + import threading + + payload = b"x" * 4 * 1024 * 1024 + read_fd, write_fd = os.pipe() + received = bytearray() + + def drain() -> None: + while chunk := os.read(read_fd, 65536): + received.extend(chunk) + time.sleep(0.001) + + reader = mock.Mock(lend_terminal=contextlib.nullcontext) + writer = cu.PipelineWriter(write_fd, reader) + consumer = threading.Thread(target=drain) + consumer.start() + try: + if producer == "writer": + assert writer.write(payload) == len(payload) + else: + child = subprocess.run( + [sys.executable, "-c", f"import sys; sys.stdout.buffer.write(b'x' * {len(payload)})"], + stdout=writer.fileno(), + stderr=subprocess.PIPE, + check=False, + ) + assert child.returncode == 0, child.stderr.decode() + finally: + writer.close() + consumer.join() + os.close(read_fd) + assert bytes(received) == payload + + def test_proc_reader_terminate(pr_none) -> None: assert pr_none._proc.poll() is None pr_none.terminate() @@ -238,6 +343,254 @@ def test_proc_reader_terminate(pr_none) -> None: assert ret_code == -signal.SIGTERM +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +@pytest.mark.parametrize("already_exited", [False, True]) +def test_proc_reader_terminate_terminal_job(already_exited) -> None: + proc = mock.Mock(stdout=None, stderr=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr) + reader._terminal_fd = 10 + with mock.patch("os.kill", side_effect=ProcessLookupError if already_exited else None) as kill: + reader.terminate() + kill.assert_called_once_with(proc.pid, signal.SIGTERM) + # Only the job watcher may reap this process; Popen.terminate() would poll it. + proc.terminate.assert_not_called() + proc.poll.assert_not_called() + proc.wait.assert_not_called() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +@pytest.mark.parametrize("stop_signal", ["SIGTTIN", "SIGTTOU"]) +@pytest.mark.parametrize("expired_handoff", [False, True]) +def test_proc_reader_resumes_terminal_access_after_handoff(stop_signal, expired_handoff) -> None: + proc = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + reader._terminal_available.set() + stopped_status = (getattr(signal, stop_signal) << 8) | 0x7F + handoffs = iter([False, True] if expired_handoff else [True]) + + def handoff(timeout): + assert timeout == 0.1 + if next(handoffs): + reader._terminal_available.set() + else: + reader._terminal_available.clear() + return True + + with ( + mock.patch( + "os.waitpid", + side_effect=[(proc.pid, stopped_status), *([(0, 0)] * (2 if expired_handoff else 1)), (proc.pid, 0)], + ), + mock.patch("os.tcgetpgrp", return_value=proc.pid), + mock.patch.object(reader, "_set_foreground_group") as foreground, + mock.patch.object(reader._terminal_available, "wait", side_effect=handoff) as available, + mock.patch("os.killpg") as killpg, + mock.patch("signal.raise_signal") as stop, + ): + reader._wait_for_job(10) + assert available.call_count == (2 if expired_handoff else 1) + killpg.assert_called_once_with(proc.pid, signal.SIGCONT) + stop.assert_not_called() + # The lend is still active: its holder returns the terminal, not the watcher. + foreground.assert_not_called() + assert proc.returncode == 0 + assert reader._process_done.is_set() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +@pytest.mark.parametrize("lent", [False, True]) +def test_proc_reader_exit_returns_terminal_unless_lent(lent) -> None: + """A shell producer in a lent pipeline group may outlive the consumer and still need the terminal.""" + proc = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + if lent: + reader._terminal_available.set() + with ( + mock.patch("os.waitpid", return_value=(proc.pid, 0)), + mock.patch("os.tcgetpgrp", return_value=proc.pid), + mock.patch.object(reader, "_set_foreground_group") as foreground, + ): + reader._wait_for_job(10) + if lent: + foreground.assert_not_called() + else: + foreground.assert_called_once_with(10, reader._original_group) + assert reader._process_done.is_set() + + +def test_proc_reader_wait_for_exit_without_terminal() -> None: + proc = mock.Mock(stdout=None, stderr=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr) + reader.wait_for_exit(timeout=0.2) + proc.wait.assert_called_once_with(0.2) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +@pytest.mark.parametrize("handler_kind", ["default", "ignored", "custom"]) +def test_proc_reader_suspend_restores_signal_handler(handler_kind) -> None: + proc = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + reader._terminal_available.set() + previous = {"default": signal.SIG_DFL, "ignored": signal.SIG_IGN, "custom": mock.Mock()}[handler_kind] + groups = [proc.pid, reader._original_group] if handler_kind == "default" else [reader._original_group] + with ( + mock.patch("signal.getsignal", return_value=previous), + mock.patch("signal.signal") as set_handler, + mock.patch("signal.raise_signal") as stop, + mock.patch("os.killpg") as killpg, + mock.patch("os.tcgetpgrp", side_effect=groups), + mock.patch("threading.Thread"), + mock.patch.object(reader, "_set_foreground_group") as foreground, + reader.manage_terminal(), + ): + handler = set_handler.call_args.args[1] + handler(signal.SIGTSTP, None) + assert reader._job_resumed.is_set() + set_handler.assert_called_with(signal.SIGTSTP, previous) + foreground.assert_called_with(10, proc.pid) + if handler_kind == "default": + killpg.assert_called_once_with(reader._original_group, signal.SIGTSTP) + stop.assert_called_once_with(signal.SIGTSTP) + assert set_handler.call_args_list == [ + mock.call(signal.SIGTSTP, handler), + mock.call(signal.SIGTSTP, signal.SIG_IGN), + mock.call(signal.SIGTSTP, signal.SIG_DFL), + mock.call(signal.SIGTSTP, handler), + mock.call(signal.SIGTSTP, previous), + ] + else: + killpg.assert_not_called() + stop.assert_not_called() + if handler_kind == "custom": + previous.assert_called_once_with(signal.SIGTSTP, None) + + +def test_proc_reader_captured_pipeline_needs_no_terminal() -> None: + reader = cu.ProcReader(mock.Mock(stdout=None, stderr=None), sys.stdout, sys.stderr) + with reader.manage_terminal(), reader.lend_terminal(): + assert not reader._terminal_available.is_set() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_lending_restores_terminal_on_write_error() -> None: + reader = cu.ProcReader(mock.Mock(pid=123, stdout=None, stderr=None, returncode=None), sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + + def failing_write(): + with reader.lend_terminal(): + assert reader._terminal_available.is_set() + raise BrokenPipeError + + with ( + mock.patch("os.tcgetpgrp", return_value=123), + mock.patch.object(reader, "_set_foreground_group") as foreground, + pytest.raises(BrokenPipeError), + ): + failing_write() + assert not reader._terminal_available.is_set() + assert foreground.call_args_list == [mock.call(10, 123), mock.call(10, 456)] + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +@pytest.mark.parametrize("inner", ["nested", "thread"]) +def test_proc_reader_keeps_the_terminal_lent_until_the_last_lend_ends(inner) -> None: + """A shorter lend inside a longer one must not take the terminal back early. + + do_shell() lends for as long as a shell producer runs. A pipe write from another thread + meanwhile lends and returns. If its return took the terminal back, the producer would stop + with SIGTTIN on its next terminal read, and nothing would resume it. + """ + import threading + + reader = cu.ProcReader(mock.Mock(pid=123, stdout=None, stderr=None, returncode=None), sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + + def short_lend() -> None: + with reader.lend_terminal(): + pass + + with ( + mock.patch("os.tcgetpgrp", return_value=123), + mock.patch.object(reader, "_set_foreground_group") as foreground, + ): + with reader.lend_terminal(): + if inner == "nested": + short_lend() + else: + worker = threading.Thread(target=short_lend) + worker.start() + worker.join() + assert reader._terminal_available.is_set() + assert mock.call(10, 456) not in foreground.call_args_list + assert not reader._terminal_available.is_set() + assert foreground.call_args_list[-1] == mock.call(10, 456) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +@pytest.mark.parametrize("error_number", [errno.ESRCH, errno.EINVAL, errno.EBADF]) +def test_proc_reader_handoff_to_disappearing_group(error_number) -> None: + reader = cu.ProcReader(mock.Mock(pid=123, stdout=None, stderr=None, returncode=None), sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + + def write(): + with reader.lend_terminal(): + assert reader._terminal_available.is_set() + + with ( + mock.patch("os.tcgetpgrp", return_value=456), + mock.patch.object(reader, "_set_foreground_group", side_effect=OSError(error_number, "handoff failed")), + ): + if error_number == errno.EBADF: + with pytest.raises(OSError, match="handoff failed"): + write() + else: + write() + assert not reader._terminal_available.is_set() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_reaps_killed_consumer_without_another_handoff() -> None: + proc = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + stopped_status = (signal.SIGTTIN << 8) | 0x7F + with ( + mock.patch("os.waitpid", side_effect=[(123, stopped_status), (123, signal.SIGKILL)]), + mock.patch("os.tcgetpgrp", return_value=456), + mock.patch.object(reader._terminal_available, "wait", return_value=False), + mock.patch("os.killpg") as killpg, + ): + reader._wait_for_job(10) + assert proc.returncode == -signal.SIGKILL + assert reader._process_done.is_set() + killpg.assert_not_called() + proc.wait.assert_not_called() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal pipeline writer") +@pytest.mark.parametrize("returncode", [-signal.SIGINT, 128 + signal.SIGINT, 0]) +@pytest.mark.parametrize("finished", [False, True]) +def test_pipeline_writer_cancels_interrupted_producer_but_not_cleanup(returncode, finished) -> None: + reader = cu.ProcReader(mock.Mock(stdout=None, stderr=None, returncode=returncode), sys.stdout, sys.stderr) + if finished: + reader.finish_producer() + read_fd, write_fd = os.pipe() + os.close(read_fd) + expected = KeyboardInterrupt if returncode != 0 and not finished else BrokenPipeError + with cu.PipelineWriter(write_fd, reader) as writer, pytest.raises(expected): + writer.write(b"output") + + @pytest.fixture def context_flag(): return cu.ContextFlag() @@ -438,3 +791,58 @@ def bar_method(self) -> None: cu.categorize([func2, b.bar_method], category) assert getattr(func2, attr_name) == category assert getattr(Bar.bar_method, attr_name) == category + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_producer_wait_times_out() -> None: + import subprocess + + pipeline = mock.Mock() + proc = mock.Mock(pid=321, stdout=None, stderr=None, returncode=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr, pipeline=pipeline) + with mock.patch("os.waitpid", return_value=(0, 0)), pytest.raises(subprocess.TimeoutExpired): + reader.wait_for_exit(0) + pipeline._relay_producer_stop.assert_not_called() + proc.wait.assert_not_called() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +@pytest.mark.parametrize("watcher_done", [False, True]) +@pytest.mark.parametrize("foreground_group", [123, 456]) +def test_proc_reader_relays_producer_stop_once_the_watcher_is_gone(watcher_done, foreground_group) -> None: + consumer = mock.Mock(pid=123, stdout=None, stderr=None, returncode=0) + pipeline = cu.ProcReader(consumer, sys.stdout, sys.stderr) + pipeline._terminal_fd = 10 + pipeline._original_group = 456 + if watcher_done: + pipeline._process_done.set() + proc = mock.Mock(pid=321, stdout=None, stderr=None, returncode=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr, pipeline=pipeline) + stopped_status = (signal.SIGTSTP << 8) | 0x7F + + def resume(thread_id, signum): + assert thread_id == threading.main_thread().ident + assert signum == signal.SIGTSTP + pipeline._job_resumed.set() + + with ( + mock.patch("os.waitpid", side_effect=[(proc.pid, stopped_status), (proc.pid, 0)]), + mock.patch("os.tcgetpgrp", return_value=foreground_group), + mock.patch.object(pipeline, "_set_foreground_group") as foreground, + mock.patch("os.killpg") as killpg, + mock.patch("signal.pthread_kill", side_effect=resume) as relay, + ): + reader.wait_for_exit() + assert proc.returncode == 0 + if not watcher_done: + # The consumer's watcher sees the same Ctrl-Z and suspends the job itself. + relay.assert_not_called() + killpg.assert_not_called() + foreground.assert_not_called() + return + relay.assert_called_once() + assert killpg.call_args_list == [mock.call(consumer.pid, signal.SIGSTOP), mock.call(consumer.pid, signal.SIGCONT)] + if foreground_group == consumer.pid: + foreground.assert_called_once_with(10, pipeline._original_group) + else: + foreground.assert_not_called() From 0380c1c0a65522aba60e89f687a6854d27c89ea1 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Fri, 11 Sep 2026 20:45:18 -0400 Subject: [PATCH 02/20] Give the corrupt-history tests their own temp files test_history_file_bad_compression and test_history_file_bad_json wrote to a fixed /tmp/doesntmatter, which on the Windows runner is D:\tmp and does not exist on a fresh machine. They passed only when the preceding permission-error test had already run: mocking builtins.open there does not stop the history setup from creating the file's parent directory, so that test created D:\tmp as a side effect. Under xdist the order is not fixed, and adding tests elsewhere shifted the schedule so both writers reached a worker first and failed on every Windows job. All three tests now use tmp_path, so none depends on a directory existing or on another test having run. Each passes alone on a single worker. Validation: 2587 passed, 6 skipped with coverage; make check, make test, make docs-test and git diff --check passed. --- tests/test_history.py | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/tests/test_history.py b/tests/test_history.py index 8fff6b7e5..a503a95e2 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -910,38 +910,39 @@ def test_history_cannot_create_directory(mocker, capsys) -> None: assert "Error creating persistent history file directory" in err -def test_history_file_permission_error(mocker, capsys) -> None: +def test_history_file_permission_error(mocker, capsys, tmp_path) -> None: mock_open = mocker.patch("builtins.open") mock_open.side_effect = PermissionError - cmd2.Cmd(persistent_history_file="/tmp/doesntmatter") + # A path under tmp_path rather than a fixed one: mocking open() does not stop the + # history setup from creating the file's parent directory, and a fixed path would leave + # that directory behind as a side effect other tests could come to depend on. + cmd2.Cmd(persistent_history_file=str(tmp_path / "doesntmatter")) out, err = capsys.readouterr() assert not out assert "Cannot read persistent history file" in err -def test_history_file_bad_compression(mocker, capsys) -> None: - history_file = "/tmp/doesntmatter" - with open(history_file, "wb") as f: - f.write(b"THIS IS NOT COMPRESSED DATA") +def test_history_file_bad_compression(capsys, tmp_path) -> None: + history_file = tmp_path / "doesntmatter" + history_file.write_bytes(b"THIS IS NOT COMPRESSED DATA") - cmd2.Cmd(persistent_history_file=history_file) + cmd2.Cmd(persistent_history_file=str(history_file)) out, err = capsys.readouterr() assert not out assert "Error decompressing persistent history data" in err -def test_history_file_bad_json(mocker, capsys) -> None: +def test_history_file_bad_json(capsys, tmp_path) -> None: import lzma data = b"THIS IS NOT JSON" compressed_data = lzma.compress(data) - history_file = "/tmp/doesntmatter" - with open(history_file, "wb") as f: - f.write(compressed_data) + history_file = tmp_path / "doesntmatter" + history_file.write_bytes(compressed_data) - cmd2.Cmd(persistent_history_file=history_file) + cmd2.Cmd(persistent_history_file=str(history_file)) out, err = capsys.readouterr() assert not out assert "Error processing persistent history data" in err From f4c1d676cd5aed53a97f6b8e0c34678b66f3a33c Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 26 Sep 2026 11:50:45 -0400 Subject: [PATCH 03/20] Report terminal sizes when the job-control test's resize step times out On Ubuntu 3.11 CI the outer shell once printed the old size after the test resized the pseudo-terminal while the job was stopped. Record each resize with the size read back straight after it, and include the size at the timeout, to tell a resize that never took effect from one undone later. --- tests/test_pipeline_job_control.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index 1ad79e343..bdb5154d9 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -205,6 +205,13 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal( stream = pyte.Stream(screen) decoder = codecs.getincrementaldecoder("utf-8")("replace") transcript = "" + # Each resize with the size read back straight after it, to tell a resize that never + # took effect from one undone later. + resizes: list[str] = [] + + def terminal_size() -> tuple[int, int]: + rows, columns, _, _ = struct.unpack("HHHH", fcntl.ioctl(master, termios.TIOCGWINSZ, b"\0" * 8)) + return rows, columns def send(data): os.write(master, data.encode()) @@ -219,7 +226,10 @@ def wait_until(predicate): stream.feed(data) if predicate(): return - pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") + pytest.fail( + f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}\n" + f"resizes: {resizes}\nterminal size at timeout: {terminal_size()}" + ) def stopped(*pids: int) -> bool: """Whether every process is stopped, not merely deprived of the terminal. @@ -323,6 +333,7 @@ def stopped(*pids: int) -> bool: send("printf 'SHELL_%s\\n' OWNS_INPUT\n") wait_until(lambda start=start: "SHELL_OWNS_INPUT" in transcript[start:]) fcntl.ioctl(master, termios.TIOCSWINSZ, struct.pack("HHHH", rows, 80, 0, 0)) + resizes.append(f"requested {rows}x80, read back {terminal_size()}") screen.resize(lines=rows, columns=80) start = len(transcript) send("stty size\n") From 46a1c1a40cbe21ac665acf3b8996c176f5433072 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 10:22:44 -0400 Subject: [PATCH 04/20] Fix pager hangs and a startup race in terminal pipelines Three bugs in the terminal-pipeline job control, found in review and reproduced on a pseudo-terminal with real less: 1. Output written straight to the pipe's descriptor hung the pager. cmd2 lent the pager the terminal only during its own writes through PipelineWriter. A subprocess given self.stdout (for example subprocess.run(..., stdout=self.stdout) in a custom command, or a `!` command run by `run_script s.txt | less`) writes to the descriptor itself, so no lend happened. Once the pipe filled, less stopped with SIGTTIN reading the keyboard, the watcher waited for a lend that never came, and the producer blocked on the full pipe forever. On main these cases worked, because the pager ran in its own session. PipelineWriter.fileno() now hands out the write end of a relay pipe. A thread passes that output on to the consumer and lends it the terminal while a producer is blocked on the relay's full pipe, which is the only time the consumer needs the terminal to make progress. Once no producer is waiting, command code gets the terminal back, as between cmd2's own writes, so a later input() still works. cmd2's own writes first wait for pending relay output, which keeps output in order. When the consumer exits, the relay closes its pipe, so producers still get EPIPE or SIGPIPE. 2. A pipe nested in a piped command took the terminal from the outer pager. A pipeline counted as the terminal's job when either its stdout or its stderr was the foreground terminal. In `run_script s.txt | less` with `big | cat` in the script, the inner pipeline's stdout is the outer pipe, but its stderr is the terminal. It became a terminal job and its lends went to cat's group, so less was never lent the terminal, and the command hung as in (1). Only a pipeline whose stdout is the foreground terminal is now the terminal's job. Others, including nested pipelines and pipelines whose stdout cmd2 captures, run in their own session, as on main. 3. The pager could start before it owned the terminal. The consumer could run between Popen() and cmd2's first terminal lend. A pager such as less sets its terminal modes as it starts, and a background tcsetattr() stops it with SIGTTOU. On macOS the call then fails with EINTR once the process continues, and less carries on with a cooked terminal, so q needs Enter and keys are echoed. This was reproduced by delaying cmd2 after it starts the pipeline. A terminal pipeline now starts as `/bin/sh -c 'read -r _ || exit 1; exec "$SHELL" -c '`. cmd2 sends a single newline down the consumer's stdin pipe only after making the pipeline's group the foreground one. From a pipe, the read builtin takes no more than that line, so no extra descriptor is needed. That matters because dash, /bin/sh on Debian and Ubuntu, cannot redirect descriptors above 9. Checked with sh, bash, dash and zsh. If cmd2 fails before sending the newline, it closes the pipe, and the held pipeline reads EOF and exits rather than waiting forever. Tests: - A PTY test covering the three hang cases in (1) and (2): a subprocess writer, a script's shell command, and a nested pipe. It fails without this change. - The pager-startup PTY test gains a variant that delays cmd2 after it starts the pipeline, which fails without this change. - Unit tests for the relay: lending only while a producer waits, giving the terminal back whether or not output is pending, ordering with cmd2's own writes, and passing the consumer's exit on to producers. --- CHANGELOG.md | 4 +- cmd2/cmd2.py | 49 ++++++---- cmd2/utils.py | 149 ++++++++++++++++++++++++++++- tests/test_pipeline_job_control.py | 108 ++++++++++++++++++++- tests/test_utils.py | 131 +++++++++++++++++++++++++ 5 files changed, 421 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d6af0573..190fa047d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,8 @@ does. It had run in a separate session that never received the terminal, so Ctrl-Z and `fg` did not suspend and resume it together with cmd2. The program now owns the terminal as it starts, so a pager can set its terminal modes, and Ctrl-C and Ctrl-Z reach the whole pipeline. - Pipes started from a worker thread, or whose output cmd2 captures, still run in their own - session + Pipes started from a worker thread, or whose output does not go to the terminal, such as one + nested in a command whose own output is piped, still run in their own session - A `shell` command piped to an interactive program, such as `shell git log | less`, now joins the pipeline's job, so both processes receive Ctrl-C and Ctrl-Z diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 84aae5185..d89896a18 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -3346,22 +3346,40 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: pipe_stderr = None if isinstance(sys.stderr, utils.StdSim) else sys.stderr terminal_fd = None + popen_command = statement.redirect_to if sys.platform != "win32": - # Job control installs signal handlers, which only the main thread may do. - # Elsewhere, keep the pipeline in its own session as before. - if threading.current_thread() is threading.main_thread(): - for stream in (pipe_stdout, pipe_stderr): - if stream is not None and stream.isatty(): - with contextlib.suppress(OSError, ValueError): - if os.tcgetpgrp(stream.fileno()) == os.getpgrp(): - terminal_fd = stream.fileno() - break + # Only a pipeline whose output goes to the terminal is the terminal's job. One + # nested in a command whose output is piped, for instance, feeds that outer + # pipeline, whose consumer needs the terminal instead. Job control installs + # signal handlers, which only the main thread may do. Otherwise, keep the + # pipeline in its own session as before. + if threading.current_thread() is threading.main_thread() and pipe_stdout is not None and pipe_stdout.isatty(): + with contextlib.suppress(OSError, ValueError): + if os.tcgetpgrp(pipe_stdout.fileno()) == os.getpgrp(): + terminal_fd = pipe_stdout.fileno() if terminal_fd is None: kwargs["start_new_session"] = True else: kwargs["process_group"] = 0 - with contextlib.ExitStack() as terminal_stack: + # A pager such as less sets its terminal modes as it starts, before it reads + # the pipe. It must own the terminal by then: a background tcsetattr() stops + # it with SIGTTOU, and on macOS that call fails with EINTR when the process + # is continued instead of being restarted. less ignores the failure and runs + # on a cooked terminal. So hold the pipeline in a POSIX sh until cmd2 has + # made its group the foreground one and sent a newline down the pipe. The + # read builtin takes no more than that line from a pipe. Then exec the + # user's shell as before. + import shlex + + user_shell = shlex.quote(kwargs.get("executable", "/bin/sh")) + popen_command = f"read -r _ || exit 1; exec {user_shell} -c {shlex.quote(statement.redirect_to)}" + kwargs["executable"] = "/bin/sh" + + with contextlib.ExitStack() as terminal_stack, contextlib.ExitStack() as gate_stack: + if terminal_fd is not None: + # Should cmd2 fail before opening the gate, the held pipeline reads EOF and exits. + gate_stack.callback(new_stdout.close) with contextlib.ExitStack() as spawn_stack: if terminal_fd is not None and os.getpgrp() == os.getsid(0): import signal @@ -3372,7 +3390,7 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: previous_tstp = signal.signal(signal.SIGTSTP, signal.SIG_IGN) spawn_stack.callback(signal.signal, signal.SIGTSTP, previous_tstp) proc = subprocess.Popen( # noqa: S602 - statement.redirect_to, + popen_command, stdin=subproc_stdin, stdout=subprocess.PIPE if pipe_stdout is None else pipe_stdout, stderr=subprocess.PIPE if pipe_stderr is None else pipe_stderr, @@ -3394,12 +3412,11 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: if cmd_pipe_proc_reader is None: proc.wait(0.2) else: - # A pager such as less sets its terminal modes as it starts, before it - # reads the pipe. It must own the terminal by then: a background - # tcsetattr() stops it with SIGTTOU, and on macOS that call fails with - # EINTR when the process is continued instead of being restarted. less - # ignores the failure and runs on a cooked terminal. + # Open the start gate only once the pipeline owns the terminal. with cmd_pipe_proc_reader.lend_terminal(): + with contextlib.suppress(OSError): + os.write(new_stdout.fileno(), b"\n") + gate_stack.pop_all() cmd_pipe_proc_reader.wait_for_exit(0.2) # Check if the pipe process already exited diff --git a/cmd2/utils.py b/cmd2/utils.py index 23fda0c83..d801c6cc4 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -921,6 +921,129 @@ def _write_bytes(stream: StdSim | TextIO, to_write: bytes | str) -> None: stream.buffer.write(to_write) +class _DescriptorRelay: + """Carry output that subprocesses write to a terminal pipeline's descriptor. + + A subprocess given :meth:`PipelineWriter.fileno` writes on its own, so cmd2 cannot lend + the terminal write by write. It writes into this relay's pipe instead, and a thread passes + that output on to the consumer. The consumer is lent the terminal while a producer is + blocked on the relay's full pipe, since only then does it need the terminal to make + progress. Otherwise command code keeps it, as it does between cmd2's own writes. + """ + + def __init__(self, out_fd: int, reader: ProcReader) -> None: + """Start relaying to out_fd, a descriptor the relay takes ownership of. + + :param out_fd: the consumer's pipe + :param reader: the terminal pipeline that reads out_fd + """ + self._out_fd = out_fd + self._reader = reader + # write_fd is the descriptor handed to producers. PipelineWriter closes it; once every + # producer has closed its copy too, the relay reads EOF and closes the consumer's pipe. + self._in_fd, self.write_fd = os.pipe() + self._write_fd_open = True + self._lock = threading.Condition() + # Bytes read from the relay's pipe, and bytes passed on to the consumer's pipe + self._received = 0 + self._sent = 0 + self._done = False + threading.Thread(name="pipe_relay", target=self._relay, daemon=True).start() + + def close_write_fd(self) -> None: + """Close cmd2's copy of the producers' descriptor. The relay finishes once theirs close.""" + with self._lock: + if self._write_fd_open: + os.close(self.write_fd) + self._write_fd_open = False + + def _unread(self) -> int: + """Bytes producers have written that the relay has not read yet. Requires _lock.""" + import fcntl + import struct + import termios + + if self._done: + return 0 + return int(struct.unpack("i", fcntl.ioctl(self._in_fd, termios.FIONREAD, b"\0" * 4))[0]) + + def _producer_blocked(self) -> bool: + """Whether the relay's pipe is full, which means a producer is waiting on the consumer.""" + import select + + with self._lock: + if not self._write_fd_open: + # The command is done. ProcReader.wait() lends the terminal from here on. + return False + poller = select.poll() + poller.register(self.write_fd, select.POLLOUT) + return not poller.poll(0) + + def flush(self) -> None: + """Wait until output already written to the relay has reached the consumer's pipe. + + cmd2's own writes go straight to the consumer's pipe, so they wait for this first to + keep their order with the output of a producer that has finished. The wait is in short + polls so that the main thread still runs Python signal handlers. + """ + with self._lock: + target = self._received + self._unread() + while not self._done and self._sent < target: + self._lock.wait(0.1) + + def _relay(self) -> None: + """Pass producer output on to the consumer until producers close or the consumer exits.""" + import select + + poller = select.poll() + poller.register(self._out_fd, select.POLLOUT) + lend = contextlib.ExitStack() + lending = False + try: + while True: + with self._lock: + idle = not self._unread() + if idle and lending: + # No producer is waiting. Let command code have the terminal back. + lend.close() + lending = False + data = os.read(self._in_fd, 65536) + if not data: + return + with self._lock: + self._received += len(data) + view = memoryview(data) + written = 0 + while written < len(view): + # Once there is room, a write of at most PIPE_BUF bytes does not block. + if poller.poll(100): + count = os.write(self._out_fd, view[written : written + select.PIPE_BUF]) + written += count + with self._lock: + self._sent += count + self._lock.notify_all() + elif self._producer_blocked(): + if not lending: + lend.enter_context(self._reader.lend_terminal()) + lending = True + elif lending: + # A producer that stopped writing does not need the consumer to go on. + lend.close() + lending = False + except OSError: + # The consumer exited. Closing the relay's pipe below passes that on to producers, + # which get EPIPE or SIGPIPE just as they would writing to the consumer directly. + return + finally: + with contextlib.suppress(OSError): + lend.close() + with self._lock: + self._done = True + os.close(self._in_fd) + os.close(self._out_fd) + self._lock.notify_all() + + class PipelineWriter(io.FileIO): """A pipe whose blocking writes temporarily give the consumer terminal access.""" @@ -928,6 +1051,28 @@ def __init__(self, fd: int, reader: ProcReader) -> None: """Take ownership of a pipe descriptor managed by reader.""" super().__init__(fd, "w") self._reader = reader + self._relay: _DescriptorRelay | None = None + + def fileno(self) -> int: + """Return a descriptor for subprocesses, such as a shell command's stdout. + + A subprocess writes to it directly, bypassing :meth:`write`. It is the write end of a + relay (see :class:`_DescriptorRelay`), which lends the consumer the terminal whenever + such a producer is waiting for the consumer to drain the pipe. + """ + if self.closed: + raise ValueError("I/O operation on closed file") + if self._relay is None: + self._relay = _DescriptorRelay(os.dup(super().fileno()), self._reader) + return self._relay.write_fd + + def close(self) -> None: + """Close the pipe. The consumer sees EOF once every producer has closed its descriptor too.""" + try: + super().close() + finally: + if self._relay is not None: + self._relay.close_write_fd() def write(self, b: Any) -> int: """Write all of b while the consumer can interact with the terminal. @@ -947,11 +1092,13 @@ def write(self, b: Any) -> int: import signal view = memoryview(b).cast("B") - fd = self.fileno() + fd = super().fileno() poller = select.poll() poller.register(fd, select.POLLOUT) try: with self._reader.lend_terminal(): + if self._relay is not None: + self._relay.flush() written = 0 while written < len(view): # Once there is room, a write of at most PIPE_BUF bytes does not block. diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index bdb5154d9..4d4c63f13 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -461,12 +461,16 @@ def wait_until(predicate): process.wait(timeout=5) -def test_pipeline_pager_can_set_terminal_modes_at_startup(tmp_path) -> None: +@pytest.mark.parametrize("parent_delay", [False, True]) +def test_pipeline_pager_can_set_terminal_modes_at_startup(tmp_path, parent_delay) -> None: """A pager such as less puts the terminal in raw mode as it starts, before reading its pipe. It has to own the terminal by then. A background tcsetattr() stops it with SIGTTOU, and on macOS the call then fails with EINTR once it is continued rather than being restarted. less ignores that failure, leaving a cooked terminal: q needs Enter and keys are echoed. + + However late cmd2 gets to hand the terminal over after starting the pipeline, the pager + must not start before it has. """ import pty import termios @@ -511,6 +515,13 @@ def test_pipeline_pager_can_set_terminal_modes_at_startup(tmp_path) -> None: " time.sleep(0.01)\n" " return startup_wait(reader, timeout)\n" "utils.ProcReader.wait_for_exit = held_startup_wait\n" + f"if {parent_delay!r}:\n" + # Stall between starting the pipeline and handing it the terminal. + " start_job_control = utils.ProcReader.manage_terminal\n" + " def late_job_control(reader):\n" + " time.sleep(0.5)\n" + " return start_job_control(reader)\n" + " utils.ProcReader.manage_terminal = late_job_control\n" "app = Cmd()\n" "app.prompt = 'TEST> '\n" "app.cmdloop()\n", @@ -731,3 +742,98 @@ def wait_until(predicate): os.close(master) process.kill() process.wait(timeout=5) + + +@pytest.mark.parametrize("producer", ["subprocess", "script_shell", "nested_pipe"]) +def test_pager_gets_the_terminal_for_output_written_to_the_descriptor(tmp_path, producer) -> None: + """A producer that writes to the pipe's descriptor itself, bypassing cmd2's writes, cannot trigger a lend per write. + + Such as a subprocess given self.stdout, a shell command run by a script, or a pipeline nested + in a piped command. Once the pipe is full, the pager has to be lent the terminal to read the + keys that let it go on. Otherwise it stops with SIGTTIN while the producer waits on it forever. + """ + import pty + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + big = tmp_path / "big.py" + big.write_text("import sys\nsys.stdout.write('x' * 1048576)\n", encoding="utf-8") + pager = tmp_path / "pager.py" + pager.write_text( + "import os, signal, sys\n" + # Interactive bash leaves TTIN ignored in what it execs, which turns a background read into EIO. + "signal.signal(signal.SIGTTIN, signal.SIG_DFL)\n" + "sys.stdin.buffer.read(4096)\n" + "os.write(2, b'PAGER_READY\\n')\n" + # Like less, read the keyboard from an inherited terminal descriptor. Quit without + # draining the pipe, which the producer must then learn of. + "os.write(2, b'PAGER_GOT ' + os.read(2, 1) + b'\\n')\n", + encoding="utf-8", + ) + python = shlex.quote(sys.executable) + script = tmp_path / "script.txt" + script.write_text( + f"!{python} {shlex.quote(str(big))}\n" if producer == "script_shell" else "big | cat\n", encoding="utf-8" + ) + application = tmp_path / "application.py" + application.write_text( + "import subprocess, sys\n" + "from cmd2 import Cmd\n" + "class App(Cmd):\n" + " def do_sub(self, _):\n" + f" subprocess.run([sys.executable, {str(big)!r}], stdout=self.stdout, check=False)\n" + " def do_big(self, _):\n" + " self.poutput('y' * 1048576)\n" + "app = App()\n" + "app.prompt = 'TEST> '\n" + "app.cmdloop()\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + data = decoder.decode(os.read(master, 65536)) + transcript += data + if "\x1b[6n" in data: + # Answer prompt-toolkit's cursor-position request as a terminal would. + os.write(master, b"\x1b[1;1R") + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") + + command = "sub" if producer == "subprocess" else f"run_script {shlex.quote(str(script))}" + try: + wait_until(lambda: "OUTER> " in transcript) + os.write(master, f"{python} {shlex.quote(str(application))}\n".encode()) + wait_until(lambda: "TEST>" in transcript) + os.write(master, f"{command} | {python} {shlex.quote(str(pager))}\n".encode()) + wait_until(lambda: "PAGER_READY\r\n" in transcript) + # The pipe fills once the pager has read its first chunk. + time.sleep(0.5) + start = len(transcript) + # The terminal is still canonical: this pager, unlike less, sets no modes. + os.write(master, b"q\n") + wait_until(lambda: "PAGER_GOT q" in transcript[start:]) + wait_until(lambda: "TEST>" in transcript[start:]) + os.write(master, b"quit\n") + wait_until(lambda: os.tcgetpgrp(master) == process.pid) + finally: + os.close(master) + process.kill() + process.wait(timeout=5) diff --git a/tests/test_utils.py b/tests/test_utils.py index d5aa5a806..bf8ab5e30 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -324,6 +324,137 @@ def drain() -> None: assert bytes(received) == payload +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipes") +@pytest.mark.parametrize("pauses", [False, True]) +def test_pipeline_writer_relay_lends_only_while_a_producer_waits(pauses) -> None: + """Output a child writes to fileno() is relayed. The consumer gets the terminal only while the child is blocked. + + A consumer such as less stops reading the pipe until it can read the keyboard. A child that + has filled the relay's pipe waits on it, so the relay lends the terminal. Once no child is + waiting, command code gets the terminal back, whether or not the consumer has read all + its output yet. That is as between cmd2's own writes. + """ + import subprocess + import threading + + # More than the relay and both pipes hold, so the child blocks until the consumer reads. + # What remains after the consumer's first read fits, so the child can then finish. + payload = b"x" * 280000 + first_read = 200000 if pauses else len(payload) + read_fd, write_fd = os.pipe() + received = bytearray() + lent = threading.Event() + resume = threading.Event() + lends = [] + + @contextlib.contextmanager + def lend_terminal(): + lends.append(1) + lent.set() + try: + yield + finally: + lends.pop() + + def drain() -> None: + # Like a pager waiting for a key, drain nothing until lent the terminal. Then read + # a page, or all of it, and wait for the next key. + lent.wait(10) + while len(received) < first_read: + received.extend(os.read(read_fd, min(65536, first_read - len(received)))) + resume.wait(10) + while chunk := os.read(read_fd, 65536): + received.extend(chunk) + + writer = cu.PipelineWriter(write_fd, mock.Mock(lend_terminal=lend_terminal)) + consumer = threading.Thread(target=drain) + consumer.start() + try: + child = subprocess.run( + [sys.executable, "-c", f"import sys; sys.stdout.buffer.write(b'x' * {len(payload)})"], + stdout=writer.fileno(), + stderr=subprocess.PIPE, + check=False, + timeout=10, + ) + assert child.returncode == 0, child.stderr.decode() + assert lent.is_set() + deadline = time.monotonic() + 5 + while lends and time.monotonic() < deadline: + time.sleep(0.01) + assert not lends + if pauses: + assert len(received) == first_read + # cmd2's own writes follow what the child wrote, which is still pending. + threading.Timer(0.3, resume.set).start() + assert writer.write(b"end") == 3 + finally: + resume.set() + writer.close() + consumer.join(10) + os.close(read_fd) + assert bytes(received) == payload + b"end" + assert not lends + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipes") +def test_pipeline_writer_relay_leaves_the_terminal_after_a_producer_finishes() -> None: + import subprocess + + payload = b"x" * 70000 + read_fd, write_fd = os.pipe() + reader = mock.Mock(lend_terminal=mock.Mock(side_effect=contextlib.nullcontext)) + writer = cu.PipelineWriter(write_fd, reader) + try: + # More than the consumer's pipe holds, but it fits in the relay: the child exits + # without waiting on the consumer, so the consumer is not lent the terminal. + subprocess.run( + [sys.executable, "-c", f"import sys; sys.stdout.buffer.write(b'x' * {len(payload)})"], + stdout=writer.fileno(), + check=True, + ) + time.sleep(0.3) + # Nor once the command is done: waiting for the consumer lends it the terminal then. + writer.close() + time.sleep(0.3) + reader.lend_terminal.assert_not_called() + # A closed writer hands out no descriptor. + with pytest.raises(ValueError, match="closed file"): + writer.fileno() + received = bytearray() + while chunk := os.read(read_fd, 65536): + received.extend(chunk) + assert received == payload + finally: + writer.close() + os.close(read_fd) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipes") +def test_pipeline_writer_relay_passes_consumer_exit_to_the_producer() -> None: + import subprocess + + read_fd, write_fd = os.pipe() + writer = cu.PipelineWriter(write_fd, mock.Mock(lend_terminal=contextlib.nullcontext)) + try: + os.close(read_fd) + # The child would block forever on a full pipe if the relay kept reading nothing out. + child = subprocess.run( + [sys.executable, "-c", "import sys\nwhile True: sys.stdout.buffer.write(b'x' * 65536)"], + stdout=writer.fileno(), + stderr=subprocess.DEVNULL, + check=False, + timeout=10, + ) + assert child.returncode != 0 + # With the relay gone, cmd2's own writes see the consumer's exit too. + with pytest.raises(BrokenPipeError): + writer.write(b"more") + finally: + with contextlib.suppress(BrokenPipeError): + writer.close() + + def test_proc_reader_terminate(pr_none) -> None: assert pr_none._proc.poll() is None pr_none.terminate() From f31659f42901adfabcb3780f76a105ac4b38284b Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 11:15:16 -0400 Subject: [PATCH 05/20] Harden terminal pipeline job control against failures found in review Fixes for the remaining review findings on terminal pipelines. Each bug was reproduced on a pseudo-terminal before being fixed, except where noted. - The pipeline's watcher thread could die, leaving the pipeline without a return code, so cmd2 treated it as still running. - With SIGCHLD ignored, the system reaps the pipeline itself and waitpid() fails with ECHILD. The watcher died with a traceback. - On macOS, signaling a group whose only member is a zombie fails with EPERM, not ESRCH. That happens when a suspended pager is killed before fg (2 of 6 runs failed). A setuid consumer such as sudo gives the same error. Every signal to the pipeline's group now goes through one helper that tolerates both errors. The watcher records a return code in all cases: 0 when the system reaped the pipeline, as Popen.wait() reports, and a plain reap if job control fails, for instance after a hangup. Handing the terminal to a group that has just vanished also tolerates Linux's EPERM. - Ctrl-Z while cmd2 owned the terminal, between writes to its pipe, stopped only cmd2. The consumer ran on in the background, writing into the shell session. cmd2 now stops the pipeline along with itself and continues it on fg, as a shell stops its whole job. The watcher counts these stops, so it does not relay them back to cmd2 as a second suspension. - A pipe write inside sigint_protection raised KeyboardInterrupt once Ctrl-C had ended the consumer. The writer now asks whether cmd2's SIGINT handler would interrupt at that point. Protected code, including redirection cleanup, gets BrokenPipeError. This replaces the separate finish_producer() flag. - Every pipe write handed the terminal to the consumer and back. Piping 20,000 lines to cat took about 1.0s, against 0.54s on main. Writes the pipe accepts at once now go straight through. The terminal is lent only once a write would block, since only then does the consumer need it. Output a producer left in the descriptor relay is still passed on first, under a lend. The same test now takes 0.57s. - Once the pipeline's leader had been reaped, Ctrl-C fell back to signaling its process ID as a group. After the group is gone, the system may reuse that ID for an unrelated process. The pipeline now tracks the shell producers that joined its group, and finds the group through one that cmd2 has not reaped yet. Otherwise it signals nothing. (Confirmed from the code, not reproduced: pid reuse cannot be forced.) Cleanups: - The watcher and the producer-stop relay shared a copy of the suspend sequence; it is now one helper, _suspend_with_cmd2(). - New internals are private, so the API docs do not publish them: _PipelineWriter, _unblocked_sigttou, and ProcReader's _terminal_group, _manage_terminal, _lend_terminal and _wait_for_exit. Tests: - PTY tests: Ctrl-Z between pipe writes stops and resumes the whole pipeline exactly once; an application that ignores SIGCHLD gets no watcher traceback; Ctrl-C ending the pager gives protected code BrokenPipeError. Each fails without its fix. - Unit tests: the watcher records an exit after ECHILD or a hangup, with ESRCH or EPERM from killpg; it skips the stops cmd2 sent; a direct Ctrl-Z signals the pipeline and a relayed one does not; send_sigint signals nothing once the pipeline and its producers are gone, and tolerates EPERM; the writer does not interrupt protected code. --- cmd2/cmd2.py | 23 ++- cmd2/utils.py | 287 +++++++++++++++++++---------- tests/test_cmd2.py | 6 +- tests/test_pipeline_job_control.py | 257 +++++++++++++++++++++++++- tests/test_utils.py | 163 ++++++++++++---- 5 files changed, 587 insertions(+), 149 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index d89896a18..2e29548ca 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -3402,7 +3402,7 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: subproc_stdin.close() if terminal_fd is not None: cmd_pipe_proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr, terminal_fd=terminal_fd) - terminal_stack.enter_context(cmd_pipe_proc_reader.manage_terminal()) + terminal_stack.enter_context(cmd_pipe_proc_reader._manage_terminal()) # Popen was called with shell=True so the user can chain pipe commands and redirect their output # like: !ls -l | grep user | wc -l > out.txt. But this makes it difficult to know if the pipe process @@ -3413,11 +3413,11 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: proc.wait(0.2) else: # Open the start gate only once the pipeline owns the terminal. - with cmd_pipe_proc_reader.lend_terminal(): + with cmd_pipe_proc_reader._lend_terminal(): with contextlib.suppress(OSError): os.write(new_stdout.fileno(), b"\n") gate_stack.pop_all() - cmd_pipe_proc_reader.wait_for_exit(0.2) + cmd_pipe_proc_reader._wait_for_exit(0.2) # Check if the pipe process already exited if proc.returncode is not None: @@ -3436,7 +3436,12 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: pipe_fd = os.dup(new_stdout.fileno()) new_stdout.close() new_stdout = io.TextIOWrapper( - io.BufferedWriter(utils.PipelineWriter(pipe_fd, cmd_pipe_proc_reader)), encoding="utf-8" + io.BufferedWriter( + utils._PipelineWriter( + pipe_fd, cmd_pipe_proc_reader, interruptible=lambda: not self.sigint_protection + ) + ), + encoding="utf-8", ) self.stdout = new_stdout @@ -3519,8 +3524,6 @@ def _restore_output(self, statement: Statement, saved_redir_state: utils.Redirec with contextlib.suppress(BrokenPipeError): # Close the file or pipe that stdout was redirected to - if self._cur_pipe_proc_reader is not None: - self._cur_pipe_proc_reader.finish_producer() self.stdout.close() # Restore self.stdout @@ -5011,19 +5014,19 @@ def do_shell(self, args: argparse.Namespace) -> None: pipeline = self._cur_pipe_proc_reader pipeline_group = None if pipeline is not None and not isinstance(self.stdout, utils.StdSim): # type: ignore[unreachable] - pipeline_group = pipeline.terminal_group + pipeline_group = pipeline._terminal_group # Prevent KeyboardInterrupts while in the shell process. The shell process still # receives the SIGINT: it is in our process group or in the foreground pipeline's. with self.sigint_protection, contextlib.ExitStack() as terminal_stack: if pipeline is not None and pipeline_group is not None: kwargs["process_group"] = pipeline_group - terminal_stack.enter_context(pipeline.lend_terminal()) + terminal_stack.enter_context(pipeline._lend_terminal()) while True: try: # For any stream that is a StdSim, we will use a pipe so we can capture its output. # A command joining the pipeline is spawned inside the lend, which blocks SIGTTOU. - with utils.unblocked_sigttou() if "process_group" in kwargs else contextlib.nullcontext(): + with utils._unblocked_sigttou() if "process_group" in kwargs else contextlib.nullcontext(): proc = subprocess.Popen( # noqa: S602 expanded_command, stdout=subprocess.PIPE if isinstance(self.stdout, utils.StdSim) else self.stdout, # type: ignore[unreachable] @@ -5048,7 +5051,7 @@ def do_shell(self, args: argparse.Namespace) -> None: joined_pipeline = pipeline if "process_group" in kwargs else None proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr, pipeline=joined_pipeline) if joined_pipeline is not None: - proc_reader.wait_for_exit() + proc_reader._wait_for_exit() proc_reader.wait() # Save the return code of the application for use in a pyscript diff --git a/cmd2/utils.py b/cmd2/utils.py index d801c6cc4..303e21e00 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -537,8 +537,8 @@ def write(self, b: bytes) -> None: @contextlib.contextmanager -def unblocked_sigttou() -> Iterator[None]: - """Let a child started inside :meth:`ProcReader.lend_terminal` keep normal job control. +def _unblocked_sigttou() -> Iterator[None]: + """Let a child started inside :meth:`ProcReader._lend_terminal` keep normal job control. The lend blocks SIGTTOU for its thread, and a child inherits that mask for life. Spawning touches no terminal, so unblocking it for the spawn alone cannot stop this thread. @@ -580,13 +580,21 @@ def __init__( self._stderr = stderr self._terminal_fd = terminal_fd self._pipeline = pipeline + # Shell producers that joined this pipeline's process group + self._joined: list[PopenTextIO] = [] + if pipeline is not None: + pipeline._joined.append(proc) self._process_done = threading.Event() - self._producer_finished = False self._terminal_available = threading.Event() # Lends in progress, guarded by _terminal_lock. The terminal is available while any is. self._lends = 0 self._terminal_lock = threading.RLock() self._job_resumed = threading.Event() + # Set by a thread that relays a pipeline's stop to cmd2's own job. Otherwise Ctrl-Z + # reached cmd2 directly, and cmd2 stops the pipeline itself. The watcher skips the + # SIGSTOPs cmd2 sends that way, counted under _terminal_lock. + self._relaying_stop = False + self._own_stops = 0 if terminal_fd is not None: self._original_group = os.tcgetpgrp(terminal_fd) @@ -614,13 +622,21 @@ def send_sigint(self) -> None: try: group_id = os.getpgid(self._proc.pid) except ProcessLookupError: - # Pipelines lead their own group. A shell command that joined it, such - # as `shell sleep 100 | head -1`, can outlive the reaped consumer. - group_id = self._proc.pid + # Pipelines lead their own group. A shell command that joined it, such as + # `shell sleep 100 | head -1`, can outlive the reaped consumer. Find the group + # through that command, which cmd2 has not reaped yet. Never signal the + # consumer's own ID: once its group is gone, the system may reuse it. + for producer in self._joined: + if producer.returncode is None: + with contextlib.suppress(ProcessLookupError): + group_id = os.getpgid(producer.pid) + break + else: + return # Never re-signal our own group: other ProcReader callers may share it # and already received Ctrl-C. if group_id != os.getpgrp(): - with contextlib.suppress(ProcessLookupError): + with contextlib.suppress(ProcessLookupError, PermissionError): os.killpg(group_id, signal.SIGINT) def terminate(self) -> None: @@ -635,12 +651,24 @@ def terminate(self) -> None: os.kill(self._proc.pid, signal.SIGTERM) @property - def terminal_group(self) -> int | None: + def _terminal_group(self) -> int | None: """Process group of a running terminal pipeline, which a producer may join, or None.""" if self._terminal_fd is None or self._proc.returncode is not None: return None return self._proc.pid + def _signal_pipeline(self, signum: int) -> bool: + """Signal the pipeline's process group. Return whether any process could be signaled. + + The group may be gone, or hold only processes cmd2 may not signal: a zombie on + macOS, or a program running as another user, such as sudo. + """ + try: + os.killpg(self._proc.pid, signum) + except (ProcessLookupError, PermissionError): + return False + return True + @staticmethod def _set_foreground_group(terminal_fd: int, group_id: int) -> None: """Transfer the terminal without stopping this background thread with SIGTTOU.""" @@ -653,7 +681,7 @@ def _set_foreground_group(terminal_fd: int, group_id: int) -> None: signal.pthread_sigmask(signal.SIG_SETMASK, previous_mask) @contextlib.contextmanager - def manage_terminal(self) -> Iterator[None]: + def _manage_terminal(self) -> Iterator[None]: """Watch the pipeline and suspend the shell's whole job on the main thread.""" import signal @@ -664,6 +692,9 @@ def manage_terminal(self) -> Iterator[None]: previous_handler = signal.getsignal(signal.SIGTSTP) def suspend_job(signum: int, frame: Any) -> None: + relayed = self._relaying_stop + self._relaying_stop = False + stopped_pipeline = False try: if previous_handler != signal.SIG_DFL: if callable(previous_handler): @@ -671,6 +702,15 @@ def suspend_job(signum: int, frame: Any) -> None: return if os.tcgetpgrp(terminal_fd) == self._proc.pid: self._set_foreground_group(terminal_fd, self._original_group) + if not relayed and self._proc.returncode is None: + # Ctrl-Z reached only cmd2's group, which owns the terminal between pipe + # writes. Stop the pipeline too, as a shell stops its whole job. + with self._terminal_lock: + self._own_stops += 1 + stopped_pipeline = self._signal_pipeline(signal.SIGSTOP) + if not stopped_pipeline: + with self._terminal_lock: + self._own_stops -= 1 # Ignore our group-directed copy, then stop this thread synchronously. # Wrappers in our job must stop too. Unlike SIGSTOP, SIGTSTP is # discarded for orphaned groups, which have no shell to resume them. @@ -686,6 +726,8 @@ def suspend_job(signum: int, frame: Any) -> None: if self._terminal_available.is_set() and os.tcgetpgrp(terminal_fd) == self._original_group: self._set_foreground_group(terminal_fd, self._proc.pid) finally: + if stopped_pipeline: + self._signal_pipeline(signal.SIGCONT) self._job_resumed.set() signal.signal(signal.SIGTSTP, suspend_job) @@ -696,7 +738,7 @@ def suspend_job(signum: int, frame: Any) -> None: signal.signal(signal.SIGTSTP, previous_handler) @contextlib.contextmanager - def lend_terminal(self) -> Iterator[None]: + def _lend_terminal(self) -> Iterator[None]: """Lend the terminal only while writing to or waiting for the consumer. Command code retains foreground access between writes, including arbitrary @@ -712,15 +754,16 @@ def lend_terminal(self) -> Iterator[None]: # While the consumer owns the terminal, a signal handler run on this thread may still # write diagnostics to it. Block SIGTTOU for the lend only: a signal mask survives fork # and exec, so blocking it for the whole pipeline would leak into every child the - # command starts. A child started during a lend must unblock it; see unblocked_sigttou(). + # command starts. A child started during a lend must unblock it; see _unblocked_sigttou(). previous_mask = signal.pthread_sigmask(signal.SIG_BLOCK, {signal.SIGTTOU}) try: with self._terminal_lock: try: self._set_foreground_group(terminal_fd, self._proc.pid) except OSError as error: - # The group can disappear before the watcher has reaped its leader. - if error.errno not in (errno.ESRCH, errno.EINVAL): + # The group can disappear before the watcher has recorded its exit. + # Linux reports a group that no longer exists as EPERM. + if error.errno not in (errno.ESRCH, errno.EINVAL, errno.EPERM): raise self._lends += 1 self._terminal_available.set() @@ -748,48 +791,23 @@ def _wait_for_job(self, terminal_fd: int) -> None: import signal try: - while True: - _, status = os.waitpid(self._proc.pid, os.WUNTRACED) - if not os.WIFSTOPPED(status): - self._proc.returncode = os.waitstatus_to_exitcode(status) - return - if os.WSTOPSIG(status) in (signal.SIGTTIN, signal.SIGTTOU): - # Command code owns the terminal between pipe writes. Defer - # consumer terminal access until the next write or final wait. - while True: - self._terminal_available.wait(0.1) - with self._terminal_lock: - # A stopped consumer can be killed before another write. - # Keep reaping even while command code owns the terminal. - pid, pending_status = os.waitpid(self._proc.pid, os.WNOHANG | os.WUNTRACED) - if pid and not os.WIFSTOPPED(pending_status): - self._proc.returncode = os.waitstatus_to_exitcode(pending_status) - return - # A short write may already have returned the terminal. - # Do not turn that ordinary handoff into a job suspension. - if not self._terminal_available.is_set(): - continue - foreground = os.tcgetpgrp(terminal_fd) - if foreground == self._proc.pid: - os.killpg(self._proc.pid, signal.SIGCONT) - break - if foreground == self._proc.pid: - continue - - if os.tcgetpgrp(terminal_fd) == self._proc.pid: - self._set_foreground_group(terminal_fd, self._original_group) - # Stop every terminal reader before returning control to the outer shell. - os.killpg(self._proc.pid, signal.SIGSTOP) - self._job_resumed.clear() - # Signal the main thread itself. Only it runs Python signal handlers, and a - # process-directed signal may be taken by another thread while the main - # thread sleeps in a system call, which then never returns to run the handler. - signal.pthread_kill(threading.main_thread().ident or 0, signal.SIGTSTP) - self._job_resumed.wait() - os.killpg(self._proc.pid, signal.SIGCONT) + self._watch_job(terminal_fd) + except ChildProcessError: + # The application ignores SIGCHLD, so the system reaped the pipeline and its exit + # status is lost. Popen.wait() reports success in this case, too. + self._proc.returncode = 0 + except OSError: + # Job control failed, for instance because the terminal hung up. Still reap the + # pipeline: until it has a return code, cmd2 treats it as running. + self._signal_pipeline(signal.SIGCONT) + try: + _, status = os.waitpid(self._proc.pid, 0) + self._proc.returncode = os.waitstatus_to_exitcode(status) + except ChildProcessError: + self._proc.returncode = 0 finally: try: - with self._terminal_lock: + with self._terminal_lock, contextlib.suppress(OSError): # A shell producer in this group may outlive the consumer and still read # the terminal. While a lend is active, its holder returns the terminal. if not self._terminal_available.is_set() and os.tcgetpgrp(terminal_fd) == self._proc.pid: @@ -797,28 +815,76 @@ def _wait_for_job(self, terminal_fd: int) -> None: finally: self._process_done.set() + def _watch_job(self, terminal_fd: int) -> None: + """Wait for a foreground pipeline to exit, relaying its stops. See _wait_for_job().""" + import signal + + while True: + _, status = os.waitpid(self._proc.pid, os.WUNTRACED) + if not os.WIFSTOPPED(status): + self._proc.returncode = os.waitstatus_to_exitcode(status) + return + if os.WSTOPSIG(status) == signal.SIGSTOP: + with self._terminal_lock: + if self._own_stops: + # cmd2 stopped the pipeline along with itself, and continues it too. + self._own_stops -= 1 + continue + if os.WSTOPSIG(status) in (signal.SIGTTIN, signal.SIGTTOU): + # Command code owns the terminal between pipe writes. Defer + # consumer terminal access until the next write or final wait. + while True: + self._terminal_available.wait(0.1) + with self._terminal_lock: + # A stopped consumer can be killed before another write. + # Keep reaping even while command code owns the terminal. + pid, pending_status = os.waitpid(self._proc.pid, os.WNOHANG | os.WUNTRACED) + if pid and not os.WIFSTOPPED(pending_status): + self._proc.returncode = os.waitstatus_to_exitcode(pending_status) + return + # A short write may already have returned the terminal. + # Do not turn that ordinary handoff into a job suspension. + if not self._terminal_available.is_set(): + continue + foreground = os.tcgetpgrp(terminal_fd) + if foreground == self._proc.pid: + # A group that died since is reaped by the next waitpid. + self._signal_pipeline(signal.SIGCONT) + break + if foreground == self._proc.pid: + continue + + self._suspend_with_cmd2(terminal_fd) + + def _suspend_with_cmd2(self, terminal_fd: int) -> None: + """Relay a pipeline's stop to cmd2's own job, and continue the pipeline once cmd2 resumes.""" + import signal + + with self._terminal_lock: + if os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) + # Stop every terminal reader before returning control to the outer shell. + self._signal_pipeline(signal.SIGSTOP) + self._job_resumed.clear() + self._relaying_stop = True + # Signal the main thread itself. Only it runs Python signal handlers, and a + # process-directed signal may be taken by another thread while the main + # thread sleeps in a system call, which then never returns to run the handler. + signal.pthread_kill(threading.main_thread().ident or 0, signal.SIGTSTP) + self._job_resumed.wait() + self._signal_pipeline(signal.SIGCONT) + def _relay_producer_stop(self) -> None: """Suspend the shell's whole job for a stopped producer that outlived the consumer. Ctrl-Z reaches only the foreground group, and the producer may be all that is left of it. The watcher ended with the consumer, so nothing else relays the stop. """ - import signal - terminal_fd = self._terminal_fd if terminal_fd is None or not self._process_done.is_set(): # A live watcher relays the consumer's stop and continues the whole group. return - with self._terminal_lock: - if os.tcgetpgrp(terminal_fd) == self._proc.pid: - self._set_foreground_group(terminal_fd, self._original_group) - with contextlib.suppress(ProcessLookupError): - os.killpg(self._proc.pid, signal.SIGSTOP) - self._job_resumed.clear() - signal.pthread_kill(threading.main_thread().ident or 0, signal.SIGTSTP) - self._job_resumed.wait() - with contextlib.suppress(ProcessLookupError): - os.killpg(self._proc.pid, signal.SIGCONT) + self._suspend_with_cmd2(terminal_fd) def _wait_for_producer(self, pipeline: "ProcReader", timeout: float | None) -> None: """Wait for a producer in a terminal pipeline's job, relaying its job-control stops. @@ -830,7 +896,12 @@ def _wait_for_producer(self, pipeline: "ProcReader", timeout: float | None) -> N deadline = None if timeout is None else time.monotonic() + timeout while self._proc.returncode is None: - pid, status = os.waitpid(self._proc.pid, os.WNOHANG | os.WUNTRACED) + try: + pid, status = os.waitpid(self._proc.pid, os.WNOHANG | os.WUNTRACED) + except ChildProcessError: + # The application ignores SIGCHLD. As Popen.wait() does, report success. + self._proc.returncode = 0 + return if not pid: if deadline is not None and time.monotonic() >= deadline: raise subprocess.TimeoutExpired(self._proc.args, timeout or 0) @@ -840,11 +911,7 @@ def _wait_for_producer(self, pipeline: "ProcReader", timeout: float | None) -> N else: self._proc.returncode = os.waitstatus_to_exitcode(status) - def finish_producer(self) -> None: - """Disable producer cancellation before flushing and closing its pipe.""" - self._producer_finished = True - - def wait_for_exit(self, timeout: float | None = None) -> None: + def _wait_for_exit(self, timeout: float | None = None) -> None: """Wait for process exit without competing with the terminal job's waitpid thread. :param timeout: maximum seconds to wait, or None to wait indefinitely @@ -866,8 +933,8 @@ def wait_for_exit(self, timeout: float | None = None) -> None: def wait(self) -> None: """Wait for the process to finish.""" if self._terminal_fd is not None: - with self.lend_terminal(): - self.wait_for_exit() + with self._lend_terminal(): + self._wait_for_exit() if self._out_thread.is_alive(): self._out_thread.join() if self._err_thread.is_alive(): @@ -924,7 +991,7 @@ def _write_bytes(stream: StdSim | TextIO, to_write: bytes | str) -> None: class _DescriptorRelay: """Carry output that subprocesses write to a terminal pipeline's descriptor. - A subprocess given :meth:`PipelineWriter.fileno` writes on its own, so cmd2 cannot lend + A subprocess given :meth:`_PipelineWriter.fileno` writes on its own, so cmd2 cannot lend the terminal write by write. It writes into this relay's pipe instead, and a thread passes that output on to the consumer. The consumer is lent the terminal while a producer is blocked on the relay's full pipe, since only then does it need the terminal to make @@ -939,7 +1006,7 @@ def __init__(self, out_fd: int, reader: ProcReader) -> None: """ self._out_fd = out_fd self._reader = reader - # write_fd is the descriptor handed to producers. PipelineWriter closes it; once every + # write_fd is the descriptor handed to producers. _PipelineWriter closes it; once every # producer has closed its copy too, the relay reads EOF and closes the consumer's pipe. self._in_fd, self.write_fd = os.pipe() self._write_fd_open = True @@ -979,6 +1046,11 @@ def _producer_blocked(self) -> bool: poller.register(self.write_fd, select.POLLOUT) return not poller.poll(0) + def idle(self) -> bool: + """Whether all output written to the relay has reached the consumer's pipe.""" + with self._lock: + return self._done or self._sent >= self._received + self._unread() + def flush(self) -> None: """Wait until output already written to the relay has reached the consumer's pipe. @@ -1024,7 +1096,7 @@ def _relay(self) -> None: self._lock.notify_all() elif self._producer_blocked(): if not lending: - lend.enter_context(self._reader.lend_terminal()) + lend.enter_context(self._reader._lend_terminal()) lending = True elif lending: # A producer that stopped writing does not need the consumer to go on. @@ -1044,14 +1116,25 @@ def _relay(self) -> None: self._lock.notify_all() -class PipelineWriter(io.FileIO): +class _PipelineWriter(io.FileIO): """A pipe whose blocking writes temporarily give the consumer terminal access.""" - def __init__(self, fd: int, reader: ProcReader) -> None: - """Take ownership of a pipe descriptor managed by reader.""" + def __init__(self, fd: int, reader: ProcReader, interruptible: Callable[[], bool] = lambda: True) -> None: + """Take ownership of a pipe descriptor managed by reader. + + :param fd: the write end of the consumer's pipe + :param reader: the terminal pipeline that reads fd + :param interruptible: whether a write may raise KeyboardInterrupt now, rather than + BrokenPipeError, when Ctrl-C has ended the consumer + """ + import select + super().__init__(fd, "w") self._reader = reader + self._interruptible = interruptible self._relay: _DescriptorRelay | None = None + self._poller = select.poll() + self._poller.register(fd, select.POLLOUT) def fileno(self) -> int: """Return a descriptor for subprocesses, such as a shell command's stdout. @@ -1075,11 +1158,13 @@ def close(self) -> None: self._relay.close_write_fd() def write(self, b: Any) -> int: - """Write all of b while the consumer can interact with the terminal. + """Write all of b, lending the consumer the terminal whenever the pipe is full. - The whole buffer goes out under one lend. Returning the terminal between two - writes, even for an instant, would stop a consumer that had just resumed a - terminal read with SIGTTIN. + A consumer such as less stops reading its pipe to read the keyboard, so a write + that would block finishes under a lend. What the pipe takes at once needs none, + which spares each flush a terminal handoff. Once lent, the terminal stays lent for + the rest of the buffer: returning it between two writes, even for an instant, + would stop a consumer that had just resumed a terminal read with SIGTTIN. A full pipe is awaited in short polls rather than in one blocking write. Only the main thread runs Python signal handlers, and the job-control stop ProcReader @@ -1093,27 +1178,33 @@ def write(self, b: Any) -> int: view = memoryview(b).cast("B") fd = super().fileno() - poller = select.poll() - poller.register(fd, select.POLLOUT) + written = 0 try: - with self._reader.lend_terminal(): - if self._relay is not None: - self._relay.flush() - written = 0 - while written < len(view): - # Once there is room, a write of at most PIPE_BUF bytes does not block. - if poller.poll(100): - written += os.write(fd, view[written : written + select.PIPE_BUF]) - return written + # Output a producer wrote to the relay comes first, which may need a lend. + if self._relay is None or self._relay.idle(): + # Once there is room, a write of at most PIPE_BUF bytes does not block. + while written < len(view) and self._poller.poll(0): + written += os.write(fd, view[written : written + select.PIPE_BUF]) + if written < len(view): + with self._reader._lend_terminal(): + if self._relay is not None: + self._relay.flush() + while written < len(view): + if self._poller.poll(100): + written += os.write(fd, view[written : written + select.PIPE_BUF]) except BrokenPipeError: # Ctrl-C during a blocking write must cancel the command, even if it # normally catches BrokenPipeError. Raise here rather than signaling # asynchronously: a late signal could interrupt redirection cleanup. - with contextlib.suppress(subprocess.TimeoutExpired): - self._reader.wait_for_exit(0.2) - if not self._reader._producer_finished and self._reader._proc.returncode in (-signal.SIGINT, 128 + signal.SIGINT): - raise KeyboardInterrupt from None + # Code that cmd2's SIGINT handler would not interrupt, such as that + # cleanup, gets the BrokenPipeError instead. + if self._interruptible(): + with contextlib.suppress(subprocess.TimeoutExpired): + self._reader._wait_for_exit(0.2) + if self._reader._proc.returncode in (-signal.SIGINT, 128 + signal.SIGINT): + raise KeyboardInterrupt from None raise + return written class ContextFlag: diff --git a/tests/test_cmd2.py b/tests/test_cmd2.py index f4cb4793e..afa07960e 100644 --- a/tests/test_cmd2.py +++ b/tests/test_cmd2.py @@ -441,7 +441,7 @@ def test_shell_falls_back_to_own_group_when_pipeline_exited(base_app, tmp_path) lent = [] @contextlib.contextmanager - def lend_terminal(): + def _lend_terminal(): lent.append(True) try: yield @@ -457,7 +457,7 @@ def popen(*args, **kwargs): spawned_while_lent.append(bool(lent)) return real_popen(*args, **kwargs) - base_app._cur_pipe_proc_reader = mock.Mock(terminal_group=leader.pid, lend_terminal=lend_terminal) + base_app._cur_pipe_proc_reader = mock.Mock(_terminal_group=leader.pid, _lend_terminal=_lend_terminal) with (tmp_path / "output").open("w+") as output, mock.patch("subprocess.Popen", popen): base_app.stdout = output base_app.do_shell("echo joined") @@ -960,7 +960,7 @@ def start_pipe(*args, **kwargs): assert capsys.readouterr().out == "" if terminal: assert signal.getsignal(signal.SIGTSTP) == previous_tstp - reader.wait_for_exit.assert_called_once_with(0.2) + reader._wait_for_exit.assert_called_once_with(0.2) reader.wait.assert_called_once_with() process.wait.assert_not_called() # SIGTTOU is blocked only inside ProcReader's lends. Blocking it for the whole diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index 4d4c63f13..d96e4884b 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -506,22 +506,22 @@ def test_pipeline_pager_can_set_terminal_modes_at_startup(tmp_path, parent_delay # The pipeline's first wait is cmd2's 0.2s startup check. Hold it open until the pager # has reported, so the test does not race that timer on a busy CI runner: what it checks # is that the pager owns the terminal throughout the check, however slowly it starts. - "startup_wait = utils.ProcReader.wait_for_exit\n" + "startup_wait = utils.ProcReader._wait_for_exit\n" "def held_startup_wait(reader, timeout=None):\n" - " utils.ProcReader.wait_for_exit = startup_wait\n" + " utils.ProcReader._wait_for_exit = startup_wait\n" f" outcome = pathlib.Path({str(outcome)!r})\n" " deadline = time.monotonic() + 5\n" " while time.monotonic() < deadline and not (outcome.exists() and outcome.read_text()):\n" " time.sleep(0.01)\n" " return startup_wait(reader, timeout)\n" - "utils.ProcReader.wait_for_exit = held_startup_wait\n" + "utils.ProcReader._wait_for_exit = held_startup_wait\n" f"if {parent_delay!r}:\n" # Stall between starting the pipeline and handing it the terminal. - " start_job_control = utils.ProcReader.manage_terminal\n" + " start_job_control = utils.ProcReader._manage_terminal\n" " def late_job_control(reader):\n" " time.sleep(0.5)\n" " return start_job_control(reader)\n" - " utils.ProcReader.manage_terminal = late_job_control\n" + " utils.ProcReader._manage_terminal = late_job_control\n" "app = Cmd()\n" "app.prompt = 'TEST> '\n" "app.cmdloop()\n", @@ -837,3 +837,250 @@ def wait_until(predicate): os.close(master) process.kill() process.wait(timeout=5) + + +def test_ctrl_z_between_pipe_writes_stops_the_whole_pipeline(tmp_path) -> None: + """Ctrl-Z while cmd2 owns the terminal, between writes to its pipe, reaches only cmd2's group. + + A shell would stop the whole job. cmd2 must stop the pipeline along with itself, or a + consumer that keeps working goes on writing into the shell session, and continue it on fg. + """ + import pty + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + ticks = tmp_path / "ticks" + ticker = tmp_path / "ticker.py" + ticker.write_text( + "import os, select, sys\n" + # Tick until cmd2 closes the pipe. + "while not select.select([sys.stdin], [], [], 0.05)[0] or sys.stdin.read(1):\n" + f" with open({str(ticks)!r}, 'a') as out: out.write('t')\n", + encoding="utf-8", + ) + application = tmp_path / "application.py" + application.write_text( + "import time\n" + "from cmd2 import Cmd\n" + "class App(Cmd):\n" + " def do_slow(self, _):\n" + " time.sleep(3)\n" + "app = App()\n" + "app.prompt = 'TEST> '\n" + "app.cmdloop()\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + data = decoder.decode(os.read(master, 65536)) + transcript += data + if "\x1b[6n" in data: + # Answer prompt-toolkit's cursor-position request as a terminal would. + os.write(master, b"\x1b[1;1R") + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") + + def tick_count() -> int: + return len(ticks.read_text()) if ticks.exists() else 0 + + python = shlex.quote(sys.executable) + try: + wait_until(lambda: "OUTER> " in transcript) + os.write(master, f"{python} {shlex.quote(str(application))}\n".encode()) + wait_until(lambda: "TEST>" in transcript) + os.write(master, f"slow | {python} {shlex.quote(str(ticker))}\n".encode()) + wait_until(lambda: tick_count() >= 3) + start = len(transcript) + os.write(master, b"\x1a") + wait_until(lambda: os.tcgetpgrp(master) == process.pid and "OUTER> " in transcript[start:]) + # A tick in flight as the job stops may still land. + deadline = time.monotonic() + 0.2 + wait_until(lambda: time.monotonic() >= deadline) + stopped_at = tick_count() + deadline = time.monotonic() + 0.5 + wait_until(lambda: time.monotonic() >= deadline) + assert tick_count() == stopped_at + start = len(transcript) + os.write(master, b"fg\n") + wait_until(lambda: tick_count() > stopped_at) + # Only once: cmd2 must not relay the stop it sent the pipeline as a second suspension. + wait_until(lambda: "TEST>" in transcript[start:]) + assert "Stopped" not in transcript[start:] + os.write(master, b"quit\n") + wait_until(lambda: os.tcgetpgrp(master) == process.pid) + finally: + os.close(master) + process.kill() + process.wait(timeout=5) + + +def test_pipeline_when_the_application_ignores_sigchld(tmp_path) -> None: + """With SIGCHLD ignored, the system reaps the pipeline and waitpid() fails with ECHILD. + + The pipeline's watcher must record an exit rather than die with a traceback. + """ + import pty + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + application = tmp_path / "application.py" + application.write_text( + "import signal\n" + "from cmd2 import Cmd\n" + "class App(Cmd):\n" + " def do_ignoring(self, statement):\n" + " signal.signal(signal.SIGCHLD, signal.SIG_IGN)\n" + " self.onecmd_plus_hooks(statement.args)\n" + " self.poutput('IGNORING_DONE')\n" + "app = App()\n" + "app.prompt = 'TEST> '\n" + "app.cmdloop()\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + data = decoder.decode(os.read(master, 65536)) + transcript += data + if "\x1b[6n" in data: + # Answer prompt-toolkit's cursor-position request as a terminal would. + os.write(master, b"\x1b[1;1R") + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") + + try: + wait_until(lambda: "OUTER> " in transcript) + os.write(master, f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}\n".encode()) + wait_until(lambda: "TEST>" in transcript) + start = len(transcript) + os.write(master, b"ignoring help quit | cat\n") + wait_until(lambda: "IGNORING_DONE" in transcript[start:] and "TEST>" in transcript[start:]) + assert "Exit this application" in transcript[start:] + # A dying watcher thread reports its traceback on its own time, which nothing here + # can wait for. Allow another command and a moment more. + os.write(master, b"help quit\n") + wait_until(lambda: transcript[start:].count("Exit this application") == 2) + deadline = time.monotonic() + 0.5 + wait_until(lambda: time.monotonic() >= deadline) + assert "Exception in thread" not in transcript[start:] + assert "Traceback" not in transcript[start:] + os.write(master, b"quit\n") + wait_until(lambda: os.tcgetpgrp(master) == process.pid) + finally: + os.close(master) + process.kill() + process.wait(timeout=5) + + +def test_ctrl_c_ending_the_pager_does_not_interrupt_protected_code(tmp_path) -> None: + """Code under sigint_protection must not get a KeyboardInterrupt, even from a pipe write. + + A pipe write that finds the consumer ended by Ctrl-C raises KeyboardInterrupt, so a command + that catches BrokenPipeError is still cancelled. Protected code gets the BrokenPipeError. + """ + import pty + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + application = tmp_path / "application.py" + application.write_text( + "import os\n" + "from cmd2 import Cmd\n" + "class App(Cmd):\n" + " def do_protected(self, _):\n" + " try:\n" + " with self.sigint_protection:\n" + " os.write(2, b'WRITING\\n')\n" + " self.stdout.write('x' * 1048576)\n" + " self.stdout.flush()\n" + " except BrokenPipeError:\n" + " os.write(2, b'GOT_BROKEN_PIPE\\n')\n" + " except KeyboardInterrupt:\n" + " os.write(2, b'GOT_KEYBOARD_INTERRUPT\\n')\n" + "app = App()\n" + "app.prompt = 'TEST> '\n" + "app.cmdloop()\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + data = decoder.decode(os.read(master, 65536)) + transcript += data + if "\x1b[6n" in data: + # Answer prompt-toolkit's cursor-position request as a terminal would. + os.write(master, b"\x1b[1;1R") + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") + + try: + wait_until(lambda: "OUTER> " in transcript) + os.write(master, f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}\n".encode()) + wait_until(lambda: "TEST>" in transcript) + # sleep reads nothing, so the write blocks with the terminal lent to it. + os.write(master, b"protected | sleep 30\n") + wait_until(lambda: "WRITING\r\n" in transcript) + wait_until(lambda: os.tcgetpgrp(master) not in (process.pid, os.getpgid(process.pid))) + start = len(transcript) + os.write(master, b"\x03") + wait_until(lambda: "GOT_" in transcript[start:]) + assert "GOT_BROKEN_PIPE" in transcript[start:] + wait_until(lambda: "TEST>" in transcript[start:]) + os.write(master, b"quit\n") + wait_until(lambda: os.tcgetpgrp(master) == process.pid) + finally: + os.close(master) + process.kill() + process.wait(timeout=5) diff --git a/tests/test_utils.py b/tests/test_utils.py index bf8ab5e30..ddd8d98e1 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -234,16 +234,32 @@ def test_proc_reader_does_not_resignal_its_own_group(pr_none) -> None: @pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") -def test_proc_reader_sigint_after_pipeline_exit() -> None: +def test_proc_reader_sigint_to_a_group_cmd2_may_not_signal() -> None: reader = cu.ProcReader(mock.Mock(pid=os.getpid() + 1, stdout=None, stderr=None), sys.stdout, sys.stderr) with ( - mock.patch("os.getpgid", side_effect=ProcessLookupError), - mock.patch("os.killpg", side_effect=ProcessLookupError) as killpg, + mock.patch("os.getpgid", return_value=reader._proc.pid), + mock.patch("os.killpg", side_effect=PermissionError) as killpg, ): reader.send_sigint() killpg.assert_called_once_with(reader._proc.pid, signal.SIGINT) +@pytest.mark.parametrize("producer", ["none", "reaped"]) +def test_proc_reader_sigint_after_pipeline_exit(producer) -> None: + """Once the pipeline is gone, its ID may belong to another process group. Signal nothing.""" + reader = cu.ProcReader(mock.Mock(pid=os.getpid() + 1, stdout=None, stderr=None), sys.stdout, sys.stderr) + if producer == "reaped": + cu.ProcReader( + mock.Mock(pid=os.getpid() + 2, stdout=None, stderr=None, returncode=0), sys.stdout, sys.stderr, pipeline=reader + ) + with ( + mock.patch("os.getpgid", side_effect=ProcessLookupError), + mock.patch("os.killpg") as killpg, + ): + reader.send_sigint() + killpg.assert_not_called() + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") def test_proc_reader_sigint_reaches_group_after_leader_exit() -> None: """A shell producer joins the pipeline's group and can outlive the consumer that led it.""" @@ -259,6 +275,10 @@ def test_proc_reader_sigint_reaches_group_after_leader_exit() -> None: try: assert member.stdout is not None assert member.stdout.readline().strip() == b"ready" + # The member joins as a shell producer does. + cu.ProcReader( + mock.Mock(pid=member.pid, stdout=None, stderr=None, returncode=None), sys.stdout, sys.stderr, pipeline=reader + ) reader.terminate() reader.wait() assert leader.returncode == -signal.SIGTERM @@ -273,13 +293,13 @@ def test_proc_reader_sigint_reaches_group_after_leader_exit() -> None: @pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") def test_proc_reader_terminal_group() -> None: proc = mock.Mock(pid=4242, returncode=None, stdout=None, stderr=None) - assert cu.ProcReader(proc, sys.stdout, sys.stderr).terminal_group is None + assert cu.ProcReader(proc, sys.stdout, sys.stderr)._terminal_group is None with mock.patch("os.tcgetpgrp", return_value=os.getpgrp()): reader = cu.ProcReader(proc, sys.stdout, sys.stderr, terminal_fd=0) - assert reader.terminal_group == proc.pid + assert reader._terminal_group == proc.pid proc.returncode = 0 - assert reader.terminal_group is None + assert reader._terminal_group is None @pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipes") @@ -302,8 +322,8 @@ def drain() -> None: received.extend(chunk) time.sleep(0.001) - reader = mock.Mock(lend_terminal=contextlib.nullcontext) - writer = cu.PipelineWriter(write_fd, reader) + reader = mock.Mock(_lend_terminal=contextlib.nullcontext) + writer = cu._PipelineWriter(write_fd, reader) consumer = threading.Thread(target=drain) consumer.start() try: @@ -348,7 +368,7 @@ def test_pipeline_writer_relay_lends_only_while_a_producer_waits(pauses) -> None lends = [] @contextlib.contextmanager - def lend_terminal(): + def _lend_terminal(): lends.append(1) lent.set() try: @@ -366,7 +386,7 @@ def drain() -> None: while chunk := os.read(read_fd, 65536): received.extend(chunk) - writer = cu.PipelineWriter(write_fd, mock.Mock(lend_terminal=lend_terminal)) + writer = cu._PipelineWriter(write_fd, mock.Mock(_lend_terminal=_lend_terminal)) consumer = threading.Thread(target=drain) consumer.start() try: @@ -403,8 +423,8 @@ def test_pipeline_writer_relay_leaves_the_terminal_after_a_producer_finishes() - payload = b"x" * 70000 read_fd, write_fd = os.pipe() - reader = mock.Mock(lend_terminal=mock.Mock(side_effect=contextlib.nullcontext)) - writer = cu.PipelineWriter(write_fd, reader) + reader = mock.Mock(_lend_terminal=mock.Mock(side_effect=contextlib.nullcontext)) + writer = cu._PipelineWriter(write_fd, reader) try: # More than the consumer's pipe holds, but it fits in the relay: the child exits # without waiting on the consumer, so the consumer is not lent the terminal. @@ -417,7 +437,7 @@ def test_pipeline_writer_relay_leaves_the_terminal_after_a_producer_finishes() - # Nor once the command is done: waiting for the consumer lends it the terminal then. writer.close() time.sleep(0.3) - reader.lend_terminal.assert_not_called() + reader._lend_terminal.assert_not_called() # A closed writer hands out no descriptor. with pytest.raises(ValueError, match="closed file"): writer.fileno() @@ -435,7 +455,7 @@ def test_pipeline_writer_relay_passes_consumer_exit_to_the_producer() -> None: import subprocess read_fd, write_fd = os.pipe() - writer = cu.PipelineWriter(write_fd, mock.Mock(lend_terminal=contextlib.nullcontext)) + writer = cu._PipelineWriter(write_fd, mock.Mock(_lend_terminal=contextlib.nullcontext)) try: os.close(read_fd) # The child would block forever on a full pipe if the relay kept reading nothing out. @@ -530,6 +550,69 @@ def handoff(timeout): assert reader._process_done.is_set() +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +@pytest.mark.parametrize( + ("waits", "hung_up", "returncode"), + [ + # The application ignores SIGCHLD, so the system reaps the pipeline itself. + pytest.param([ChildProcessError(errno.ECHILD, "no child")], False, 0, id="sigchld-ignored"), + # The terminal hangs up as the watcher relays a stop. + pytest.param([(123, (signal.SIGTSTP << 8) | 0x7F), (123, 2 << 8)], True, 2, id="hangup"), + pytest.param([(123, (signal.SIGTSTP << 8) | 0x7F), ChildProcessError()], True, 0, id="hangup-sigchld-ignored"), + ], +) +@pytest.mark.parametrize("signal_error", [ProcessLookupError, PermissionError]) +def test_proc_reader_watcher_always_records_an_exit(waits, hung_up, returncode, signal_error) -> None: + """If the watcher cannot do its job, it must still set a return code, or cmd2 treats the pipeline as running. + + On macOS, signaling a group whose only process is a zombie fails with EPERM. + """ + proc = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + with ( + mock.patch("os.waitpid", side_effect=waits), + mock.patch("os.tcgetpgrp", side_effect=OSError(errno.EIO, "hung up") if hung_up else None, return_value=456), + mock.patch("os.killpg", side_effect=signal_error) as killpg, + ): + reader._wait_for_job(10) + assert proc.returncode == returncode + assert reader._process_done.is_set() + if hung_up: + # The watcher continues the pipeline, which may be stopped, before waiting for it plainly. + killpg.assert_called_once_with(proc.pid, signal.SIGCONT) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_watcher_skips_the_stop_cmd2_sent() -> None: + proc = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + reader._own_stops = 1 + stopped = (signal.SIGSTOP << 8) | 0x7F + with ( + mock.patch("os.waitpid", side_effect=[(proc.pid, stopped), (proc.pid, 0)]), + mock.patch("os.tcgetpgrp", return_value=456), + mock.patch("signal.pthread_kill") as relay, + ): + reader._wait_for_job(10) + relay.assert_not_called() + assert reader._own_stops == 0 + assert proc.returncode == 0 + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_producer_wait_when_sigchld_is_ignored() -> None: + pipeline = cu.ProcReader(mock.Mock(pid=123, stdout=None, stderr=None, returncode=None), sys.stdout, sys.stderr) + proc = mock.Mock(pid=789, stdout=None, stderr=None, returncode=None) + producer = cu.ProcReader(proc, sys.stdout, sys.stderr, pipeline=pipeline) + with mock.patch("os.waitpid", side_effect=ChildProcessError): + producer._wait_for_exit() + assert proc.returncode == 0 + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") @pytest.mark.parametrize("lent", [False, True]) def test_proc_reader_exit_returns_terminal_unless_lent(lent) -> None: @@ -556,13 +639,16 @@ def test_proc_reader_exit_returns_terminal_unless_lent(lent) -> None: def test_proc_reader_wait_for_exit_without_terminal() -> None: proc = mock.Mock(stdout=None, stderr=None) reader = cu.ProcReader(proc, sys.stdout, sys.stderr) - reader.wait_for_exit(timeout=0.2) + reader._wait_for_exit(timeout=0.2) proc.wait.assert_called_once_with(0.2) @pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") -@pytest.mark.parametrize("handler_kind", ["default", "ignored", "custom"]) -def test_proc_reader_suspend_restores_signal_handler(handler_kind) -> None: +@pytest.mark.parametrize( + ("handler_kind", "relayed"), [("default", False), ("default", True), ("ignored", False), ("custom", False)] +) +def test_proc_reader_suspend_restores_signal_handler(handler_kind, relayed) -> None: + """Ctrl-Z that reached cmd2 directly stops and continues the pipeline too. One the watcher relayed does not.""" proc = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) reader = cu.ProcReader(proc, sys.stdout, sys.stderr) reader._terminal_fd = 10 @@ -578,15 +664,27 @@ def test_proc_reader_suspend_restores_signal_handler(handler_kind) -> None: mock.patch("os.tcgetpgrp", side_effect=groups), mock.patch("threading.Thread"), mock.patch.object(reader, "_set_foreground_group") as foreground, - reader.manage_terminal(), + reader._manage_terminal(), ): handler = set_handler.call_args.args[1] + reader._relaying_stop = relayed handler(signal.SIGTSTP, None) assert reader._job_resumed.is_set() + assert not reader._relaying_stop set_handler.assert_called_with(signal.SIGTSTP, previous) foreground.assert_called_with(10, proc.pid) if handler_kind == "default": - killpg.assert_called_once_with(reader._original_group, signal.SIGTSTP) + own_stop = [mock.call(reader._original_group, signal.SIGTSTP)] + if relayed: + assert killpg.call_args_list == own_stop + else: + assert killpg.call_args_list == [ + mock.call(proc.pid, signal.SIGSTOP), + *own_stop, + mock.call(proc.pid, signal.SIGCONT), + ] + # The watcher skips the stop cmd2 sent. + assert reader._own_stops == 1 stop.assert_called_once_with(signal.SIGTSTP) assert set_handler.call_args_list == [ mock.call(signal.SIGTSTP, handler), @@ -604,7 +702,7 @@ def test_proc_reader_suspend_restores_signal_handler(handler_kind) -> None: def test_proc_reader_captured_pipeline_needs_no_terminal() -> None: reader = cu.ProcReader(mock.Mock(stdout=None, stderr=None), sys.stdout, sys.stderr) - with reader.manage_terminal(), reader.lend_terminal(): + with reader._manage_terminal(), reader._lend_terminal(): assert not reader._terminal_available.is_set() @@ -615,7 +713,7 @@ def test_proc_reader_lending_restores_terminal_on_write_error() -> None: reader._original_group = 456 def failing_write(): - with reader.lend_terminal(): + with reader._lend_terminal(): assert reader._terminal_available.is_set() raise BrokenPipeError @@ -645,14 +743,14 @@ def test_proc_reader_keeps_the_terminal_lent_until_the_last_lend_ends(inner) -> reader._original_group = 456 def short_lend() -> None: - with reader.lend_terminal(): + with reader._lend_terminal(): pass with ( mock.patch("os.tcgetpgrp", return_value=123), mock.patch.object(reader, "_set_foreground_group") as foreground, ): - with reader.lend_terminal(): + with reader._lend_terminal(): if inner == "nested": short_lend() else: @@ -666,14 +764,14 @@ def short_lend() -> None: @pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") -@pytest.mark.parametrize("error_number", [errno.ESRCH, errno.EINVAL, errno.EBADF]) +@pytest.mark.parametrize("error_number", [errno.ESRCH, errno.EINVAL, errno.EPERM, errno.EBADF]) def test_proc_reader_handoff_to_disappearing_group(error_number) -> None: reader = cu.ProcReader(mock.Mock(pid=123, stdout=None, stderr=None, returncode=None), sys.stdout, sys.stderr) reader._terminal_fd = 10 reader._original_group = 456 def write(): - with reader.lend_terminal(): + with reader._lend_terminal(): assert reader._terminal_available.is_set() with ( @@ -710,15 +808,14 @@ def test_proc_reader_reaps_killed_consumer_without_another_handoff() -> None: @pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal pipeline writer") @pytest.mark.parametrize("returncode", [-signal.SIGINT, 128 + signal.SIGINT, 0]) -@pytest.mark.parametrize("finished", [False, True]) -def test_pipeline_writer_cancels_interrupted_producer_but_not_cleanup(returncode, finished) -> None: +@pytest.mark.parametrize("protected", [False, True]) +def test_pipeline_writer_cancels_interrupted_producer_but_not_protected_code(returncode, protected) -> None: + """Protected code, such as redirection cleanup, gets BrokenPipeError rather than KeyboardInterrupt.""" reader = cu.ProcReader(mock.Mock(stdout=None, stderr=None, returncode=returncode), sys.stdout, sys.stderr) - if finished: - reader.finish_producer() read_fd, write_fd = os.pipe() os.close(read_fd) - expected = KeyboardInterrupt if returncode != 0 and not finished else BrokenPipeError - with cu.PipelineWriter(write_fd, reader) as writer, pytest.raises(expected): + expected = KeyboardInterrupt if returncode != 0 and not protected else BrokenPipeError + with cu._PipelineWriter(write_fd, reader, interruptible=lambda: not protected) as writer, pytest.raises(expected): writer.write(b"output") @@ -932,7 +1029,7 @@ def test_proc_reader_producer_wait_times_out() -> None: proc = mock.Mock(pid=321, stdout=None, stderr=None, returncode=None) reader = cu.ProcReader(proc, sys.stdout, sys.stderr, pipeline=pipeline) with mock.patch("os.waitpid", return_value=(0, 0)), pytest.raises(subprocess.TimeoutExpired): - reader.wait_for_exit(0) + reader._wait_for_exit(0) pipeline._relay_producer_stop.assert_not_called() proc.wait.assert_not_called() @@ -963,7 +1060,7 @@ def resume(thread_id, signum): mock.patch("os.killpg") as killpg, mock.patch("signal.pthread_kill", side_effect=resume) as relay, ): - reader.wait_for_exit() + reader._wait_for_exit() assert proc.returncode == 0 if not watcher_done: # The consumer's watcher sees the same Ctrl-Z and suspends the job itself. From e48ef2713fdaa438b31beb1c59463bf0c0b8899e Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 11:20:32 -0400 Subject: [PATCH 06/20] Build the watcher test's stop status at run time, not collection The parameters of test_proc_reader_watcher_always_records_an_exit used signal.SIGTSTP, which Windows lacks. Parameters are evaluated when pytest collects the module, before the test's skipif applies, so the whole of tests/test_utils.py failed to collect on Windows. The parameters now name the stop, and the test builds its status. --- tests/test_utils.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/test_utils.py b/tests/test_utils.py index ddd8d98e1..90a0ffc19 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -556,9 +556,10 @@ def handoff(timeout): [ # The application ignores SIGCHLD, so the system reaps the pipeline itself. pytest.param([ChildProcessError(errno.ECHILD, "no child")], False, 0, id="sigchld-ignored"), - # The terminal hangs up as the watcher relays a stop. - pytest.param([(123, (signal.SIGTSTP << 8) | 0x7F), (123, 2 << 8)], True, 2, id="hangup"), - pytest.param([(123, (signal.SIGTSTP << 8) | 0x7F), ChildProcessError()], True, 0, id="hangup-sigchld-ignored"), + # The terminal hangs up as the watcher relays a stop. "stopped" stands for a Ctrl-Z + # stop status: parameters are built at collection, where Windows has no SIGTSTP. + pytest.param(["stopped", (123, 2 << 8)], True, 2, id="hangup"), + pytest.param(["stopped", ChildProcessError()], True, 0, id="hangup-sigchld-ignored"), ], ) @pytest.mark.parametrize("signal_error", [ProcessLookupError, PermissionError]) @@ -571,6 +572,7 @@ def test_proc_reader_watcher_always_records_an_exit(waits, hung_up, returncode, reader = cu.ProcReader(proc, sys.stdout, sys.stderr) reader._terminal_fd = 10 reader._original_group = 456 + waits = [(proc.pid, (signal.SIGTSTP << 8) | 0x7F) if wait == "stopped" else wait for wait in waits] with ( mock.patch("os.waitpid", side_effect=waits), mock.patch("os.tcgetpgrp", side_effect=OSError(errno.EIO, "hung up") if hung_up else None, return_value=456), From 50fab1fcd15d2695f9c9c28baf28c2fab42b9433 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 11:28:44 -0400 Subject: [PATCH 07/20] Restore the POSIX skip on the send_sigint pipeline-exit test Adding test_proc_reader_sigint_to_a_group_cmd2_may_not_signal above it took its skipif, so the test patched os.getpgid on Windows, which has no such function. Both tests now carry the skip. --- tests/test_utils.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_utils.py b/tests/test_utils.py index 90a0ffc19..697b57cac 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -244,6 +244,7 @@ def test_proc_reader_sigint_to_a_group_cmd2_may_not_signal() -> None: killpg.assert_called_once_with(reader._proc.pid, signal.SIGINT) +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") @pytest.mark.parametrize("producer", ["none", "reaped"]) def test_proc_reader_sigint_after_pipeline_exit(producer) -> None: """Once the pipeline is gone, its ID may belong to another process group. Signal nothing.""" From 3fe7a97a0930c2fa4ef458a5a27460aef06b5b11 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 12:00:02 -0400 Subject: [PATCH 08/20] Keep the job-control test's terminal setup out of bash's way Two intermittent failures of test_pipeline_stops_with_cmd2_and_returns_terminal under parallel load, neither caused by cmd2: - Interactive bash intermittently stops a job it has just started on a TOSTOP terminal, before the job runs any code of its own. The test set TOSTOP before starting bash in its exit_sigint cases, so the launch of the application, or of printf or stty during a stop, was sometimes reported Stopped. Reproduced without cmd2: across 14 parallel bash sessions launching Python, 10 of 2,240 launches stopped with TOSTOP set and none of 2,240 without it. The test now sets TOSTOP only once the pager owns the terminal, and again after fg: while the job is stopped, bash restores its own terminal modes, so its commands run without it. TOSTOP is still in effect when Ctrl-C is sent, which is what the test needs it for. - The pseudo-terminal occasionally reported its old size after the test resized it while the job was stopped, though every process of the job was stopped and nothing else sets a size. The test now confirms the new size through the shell and resizes again if the shell saw the old one, reporting every attempt if it never takes. --- tests/test_pipeline_job_control.py | 46 +++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index d96e4884b..5f5305a51 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -183,10 +183,6 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal( ) master, slave = pty.openpty() fcntl.ioctl(slave, termios.TIOCSWINSZ, struct.pack("HHHH", 24, 80, 0, 0)) - if finish == "exit_sigint": - settings = termios.tcgetattr(slave) - settings[3] |= termios.TOSTOP - termios.tcsetattr(slave, termios.TCSANOW, settings) # Establish a controlling terminal in a fresh interpreter, avoiding preexec_fn # (unsafe when pytest or its plugins have started threads). bootstrap = ( @@ -216,6 +212,18 @@ def terminal_size() -> tuple[int, int]: def send(data): os.write(master, data.encode()) + def stop_background_output(): + """Set TOSTOP, so that a write from the background stops the writer. + + Ctrl-C's handler may write while cmd2 has lent the terminal to the pager. Set it only + while cmd2's command runs: bash intermittently stops a job it has just started on a + TOSTOP terminal, before the job runs a line of its own. When the job stops, bash + restores its own terminal modes, so its commands during the stop run without it. + """ + settings = termios.tcgetattr(master) + settings[3] |= termios.TOSTOP + termios.tcsetattr(master, termios.TCSANOW, settings) + def wait_until(predicate): nonlocal transcript deadline = time.monotonic() + 10 @@ -317,6 +325,8 @@ def stopped(*pids: int) -> bool: # Readiness output can precede the foreground handoff. Send terminal # signals and keystrokes only once the pipeline can receive them. wait_until(lambda: os.tcgetpgrp(master) == pipeline_group) + if finish == "exit_sigint": + stop_background_output() if launcher == "exec": # There is no outer shell to run fg: Ctrl-Z must leave the pager # usable. Require a fresh read acknowledgement, not a SIGCONT. @@ -332,18 +342,32 @@ def stopped(*pids: int) -> bool: # A child left running can steal these keystrokes from the shell. send("printf 'SHELL_%s\\n' OWNS_INPUT\n") wait_until(lambda start=start: "SHELL_OWNS_INPUT" in transcript[start:]) - fcntl.ioctl(master, termios.TIOCSWINSZ, struct.pack("HHHH", rows, 80, 0, 0)) - resizes.append(f"requested {rows}x80, read back {terminal_size()}") screen.resize(lines=rows, columns=80) - start = len(transcript) - send("stty size\n") - # Bash 5.1+ turns bracketed paste off with "\x1b[?2004l\r" before running the - # command, so the reply may follow a bare "\r" rather than "\r\n". - wait_until(lambda start=start, rows=rows: re.search(rf"[\r\n]{rows} 80\r\n", transcript[start:]) is not None) + # The shell sees the resize while the job is stopped. Under a loaded parallel run the + # pseudo-terminal occasionally reports its old size again, though every process of + # the job is stopped and nothing here sets a size. So confirm the size through the + # shell, and resize again if it saw the old one. Every attempt is reported on failure. + for _ in range(5): + fcntl.ioctl(master, termios.TIOCSWINSZ, struct.pack("HHHH", rows, 80, 0, 0)) + resizes.append(f"requested {rows}x80, read back {terminal_size()}") + start = len(transcript) + send("stty size\n") + # Bash 5.1+ turns bracketed paste off with "\x1b[?2004l\r" before running the + # command, so the reply may follow a bare "\r" rather than "\r\n". + wait_until(lambda start=start: re.search(r"[\r\n]\d+ \d+\r\n", transcript[start:]) is not None) + reply = re.search(r"[\r\n](\d+) (\d+)\r\n", transcript[start:]) + assert reply is not None + resizes.append(f"the shell saw {reply.group(1)}x{reply.group(2)}") + if reply.groups() == (str(rows), "80"): + break + else: + pytest.fail(f"the shell never saw a {rows}x80 terminal: {resizes}") start = len(transcript) send("fg\n") wait_until(lambda start=start: "PAGER_RESUMED\r\n" in transcript[start:]) assert os.tcgetpgrp(master) == pipeline_group + if finish == "exit_sigint": + stop_background_output() if finish == "exit_sigint": send("\x03") elif finish == "interrupts": From 1345818de5cbfb01c0079fcf20906d1d786ae553 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 12:01:59 -0400 Subject: [PATCH 09/20] Cover the last untested job-control lines in cmd2.utils - A Ctrl-Z that reaches cmd2 directly while the pipeline's group is already gone: cmd2 neither counts the stop nor continues the group later, and still suspends itself. - A descriptor relay whose producers have all closed its pipe finishes and closes the consumer's pipe, and then has nothing pending: flush() returns at once and idle() is true. --- tests/test_utils.py | 48 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/test_utils.py b/tests/test_utils.py index 697b57cac..3475e57e7 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -451,6 +451,25 @@ def test_pipeline_writer_relay_leaves_the_terminal_after_a_producer_finishes() - os.close(read_fd) +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipes") +def test_descriptor_relay_that_finished_has_nothing_pending() -> None: + """Once every producer has closed the relay's pipe, the relay finishes and closes it, and nothing is left to wait for.""" + read_fd, write_fd = os.pipe() + relay = cu._DescriptorRelay(write_fd, mock.Mock(_lend_terminal=contextlib.nullcontext)) + try: + relay.close_write_fd() + # The relay closes the consumer's pipe as it finishes. + assert os.read(read_fd, 1) == b"" + deadline = time.monotonic() + 5 + while not relay._done and time.monotonic() < deadline: + time.sleep(0.01) + assert relay._done + relay.flush() + assert relay.idle() + finally: + os.close(read_fd) + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipes") def test_pipeline_writer_relay_passes_consumer_exit_to_the_producer() -> None: import subprocess @@ -703,6 +722,35 @@ def test_proc_reader_suspend_restores_signal_handler(handler_kind, relayed) -> N previous.assert_called_once_with(signal.SIGTSTP, None) +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_direct_suspend_when_the_pipeline_is_gone() -> None: + """If the pipeline cannot be stopped along with cmd2, cmd2 does not count the stop or continue it.""" + proc = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + + def killpg(group, signum): + if group == proc.pid: + raise ProcessLookupError + + with ( + mock.patch("signal.getsignal", return_value=signal.SIG_DFL), + mock.patch("signal.signal") as set_handler, + mock.patch("signal.raise_signal") as stop, + mock.patch("os.killpg", side_effect=killpg) as sent, + mock.patch("os.tcgetpgrp", return_value=reader._original_group), + mock.patch("threading.Thread"), + reader._manage_terminal(), + ): + handler = set_handler.call_args.args[1] + handler(signal.SIGTSTP, None) + assert sent.call_args_list == [mock.call(proc.pid, signal.SIGSTOP), mock.call(reader._original_group, signal.SIGTSTP)] + assert reader._own_stops == 0 + stop.assert_called_once_with(signal.SIGTSTP) + assert reader._job_resumed.is_set() + + def test_proc_reader_captured_pipeline_needs_no_terminal() -> None: reader = cu.ProcReader(mock.Mock(stdout=None, stderr=None), sys.stdout, sys.stderr) with reader._manage_terminal(), reader._lend_terminal(): From e1cb8bcc236466d1b77b9a23b11604d348ae9322 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 12:37:04 -0400 Subject: [PATCH 10/20] Coordinate Ctrl-Z across a pipeline's job and fix review findings Fixes from further review of the terminal pipeline job control. Each was reproduced before being fixed, except where noted. - A `shell` producer could hang on Ctrl-Z when cmd2 is a session leader (started with exec, so there is no outer shell). The pipeline's consumer inherited an ignored SIGTSTP, but the producer that joins its job did not. Ctrl-Z during `shell sleep 3 | cat` stopped the producer for good while cat ran on, and cmd2 waited on the producer forever. Both spawns now share one helper, _session_leader_job_stops(), that applies the session leader's Ctrl-Z behavior. - A producer's stop was dropped when the consumer ignores Ctrl-Z. The producer's wait relayed its stop only once the consumer's watcher had exited, assuming the consumer would stop too. With a consumer that ignores SIGTSTP, the producer stayed stopped and cmd2 hung. - A direct Ctrl-Z to cmd2 counted the SIGSTOP it sent the pipeline, so that the watcher could skip the resulting stop report. A consumer that was already stopped, for instance waiting for the terminal, produces no report, and the count never came back down. A later genuine stop of the consumer would then have been ignored. The last two are fixed together. Suspensions of the job now take one lock, and count as they finish. Whichever stop is reported first, the consumer's, a producer's, or cmd2's own, suspends the whole job. A suspension ends by continuing the whole pipeline, so a stop reported before one finished needs nothing more and is skipped. This replaces the count of cmd2's own stops. - send_sigint() and terminate() used the pipeline's process ID even after its watcher had reaped it, when the system may already have given that ID to an unrelated process. Both now check the return code first. (Confirmed from the code; pid reuse cannot be forced in a test.) - The descriptor relay could report output as passed on while it was still on its way: bytes read from the relay's pipe were neither in the pipe nor counted until the relay thread took its lock again. cmd2's next write could then overtake the end of a subprocess's output. The relay now waits for output outside the lock, then reads and counts it under the lock. The next release is 4.3.0 rather than 4.2.5: the changelog heading is updated, as these changes have grown beyond a patch release. Tests: - PTY test: Ctrl-Z during `shell | `, run from a shell or as a session leader, with a consumer that stops or one that ignores Ctrl-Z. Three of the four cases hung before this change. - Unit tests: a producer's stop suspends the job while the watcher runs, unless a finished suspension already resolved it; the watcher skips such stops and follows newer ones while it waits for the terminal; a suspension waits for one in progress; a direct Ctrl-Z while another thread relays a stop leaves the pipeline to that thread; a reaped pipeline is never signaled; the relay counts output it is reading. --- CHANGELOG.md | 2 +- cmd2/cmd2.py | 19 ++- cmd2/utils.py | 191 ++++++++++++++++++----------- tests/test_pipeline_job_control.py | 87 +++++++++++++ tests/test_utils.py | 175 ++++++++++++++++++++++---- 5 files changed, 365 insertions(+), 109 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 190fa047d..25264b0a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,4 @@ -## 4.2.5 (TBD) +## 4.3.0 (TBD) - Bug Fixes - On POSIX, piping a command's output to an interactive program such as `less` diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 2e29548ca..68dbb7dab 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -3380,15 +3380,7 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: if terminal_fd is not None: # Should cmd2 fail before opening the gate, the held pipeline reads EOF and exits. gate_stack.callback(new_stdout.close) - with contextlib.ExitStack() as spawn_stack: - if terminal_fd is not None and os.getpgrp() == os.getsid(0): - import signal - - # A session leader's job has no outer shell to resume it. - # Its pipeline must inherit the same Ctrl-Z behavior: the - # new group would otherwise make SIGTSTP actionable again. - previous_tstp = signal.signal(signal.SIGTSTP, signal.SIG_IGN) - spawn_stack.callback(signal.signal, signal.SIGTSTP, previous_tstp) + with utils._session_leader_job_stops() if terminal_fd is not None else contextlib.nullcontext(): proc = subprocess.Popen( # noqa: S602 popen_command, stdin=subproc_stdin, @@ -5025,8 +5017,13 @@ def do_shell(self, args: argparse.Namespace) -> None: while True: try: # For any stream that is a StdSim, we will use a pipe so we can capture its output. - # A command joining the pipeline is spawned inside the lend, which blocks SIGTTOU. - with utils._unblocked_sigttou() if "process_group" in kwargs else contextlib.nullcontext(): + # A command joining the pipeline is spawned inside the lend, which blocks SIGTTOU, + # and with the job's Ctrl-Z behavior. + joining = "process_group" in kwargs + with ( + utils._unblocked_sigttou() if joining else contextlib.nullcontext(), + utils._session_leader_job_stops() if joining else contextlib.nullcontext(), + ): proc = subprocess.Popen( # noqa: S602 expanded_command, stdout=subprocess.PIPE if isinstance(self.stdout, utils.StdSim) else self.stdout, # type: ignore[unreachable] diff --git a/cmd2/utils.py b/cmd2/utils.py index 303e21e00..9a226999d 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -552,6 +552,26 @@ def _unblocked_sigttou() -> Iterator[None]: signal.pthread_sigmask(signal.SIG_SETMASK, previous_mask) +@contextlib.contextmanager +def _session_leader_job_stops() -> Iterator[None]: + """Spawn a process for a terminal pipeline's job with the same Ctrl-Z behavior as cmd2. + + A session leader's job has no outer shell to resume it, so Ctrl-Z must not stop any part + of it. A new process group would otherwise make SIGTSTP actionable again. An ignored + signal stays ignored across exec, so ignore SIGTSTP while spawning. + """ + import signal + + if os.getpgrp() != os.getsid(0): + yield + return + previous = signal.signal(signal.SIGTSTP, signal.SIG_IGN) + try: + yield + finally: + signal.signal(signal.SIGTSTP, previous) + + class ProcReader: """Used to capture stdout and stderr from a Popen process if any of those were set to subprocess.PIPE. @@ -591,10 +611,13 @@ def __init__( self._terminal_lock = threading.RLock() self._job_resumed = threading.Event() # Set by a thread that relays a pipeline's stop to cmd2's own job. Otherwise Ctrl-Z - # reached cmd2 directly, and cmd2 stops the pipeline itself. The watcher skips the - # SIGSTOPs cmd2 sends that way, counted under _terminal_lock. + # reached cmd2 directly, and cmd2 stops the pipeline itself. self._relaying_stop = False - self._own_stops = 0 + # One suspension of the whole job at a time, and how many have finished. A suspension + # continues the whole pipeline as it ends, so a stop reported before it finished is + # already dealt with: see _suspend_with_cmd2(). + self._suspension_lock = threading.Lock() + self._suspensions = 0 if terminal_fd is not None: self._original_group = os.tcgetpgrp(terminal_fd) @@ -618,21 +641,22 @@ def send_sigint(self) -> None: self._proc.send_signal(signal.CTRL_BREAK_EVENT) else: # Since cmd2 uses shell=True in its Popen calls, we need to send the SIGINT to - # the whole process group to make sure it propagates further than the shell - try: - group_id = os.getpgid(self._proc.pid) - except ProcessLookupError: - # Pipelines lead their own group. A shell command that joined it, such as - # `shell sleep 100 | head -1`, can outlive the reaped consumer. Find the group - # through that command, which cmd2 has not reaped yet. Never signal the - # consumer's own ID: once its group is gone, the system may reuse it. - for producer in self._joined: - if producer.returncode is None: - with contextlib.suppress(ProcessLookupError): - group_id = os.getpgid(producer.pid) - break - else: - return + # the whole process group to make sure it propagates further than the shell. + # Once reaped, the process's ID may already belong to another process. + group_id = None + if self._proc.returncode is None: + with contextlib.suppress(ProcessLookupError): + group_id = os.getpgid(self._proc.pid) + # Pipelines lead their own group. A shell command that joined it, such as + # `shell sleep 100 | head -1`, can outlive the reaped consumer. Find the group + # through that command, which cmd2 has not reaped yet. Never signal the + # consumer's own ID: once its group is gone, the system may reuse it. + for producer in self._joined: + if group_id is None and producer.returncode is None: + with contextlib.suppress(ProcessLookupError): + group_id = os.getpgid(producer.pid) + if group_id is None: + return # Never re-signal our own group: other ProcReader callers may share it # and already received Ctrl-C. if group_id != os.getpgrp(): @@ -647,8 +671,10 @@ def terminate(self) -> None: import signal # Popen.terminate() polls first, which would compete with our waitpid thread. - with contextlib.suppress(ProcessLookupError): - os.kill(self._proc.pid, signal.SIGTERM) + # Once the watcher has reaped the process, its ID may belong to another one. + if self._proc.returncode is None: + with contextlib.suppress(ProcessLookupError): + os.kill(self._proc.pid, signal.SIGTERM) @property def _terminal_group(self) -> int | None: @@ -694,7 +720,7 @@ def _manage_terminal(self) -> Iterator[None]: def suspend_job(signum: int, frame: Any) -> None: relayed = self._relaying_stop self._relaying_stop = False - stopped_pipeline = False + own_suspension = False try: if previous_handler != signal.SIG_DFL: if callable(previous_handler): @@ -702,15 +728,12 @@ def suspend_job(signum: int, frame: Any) -> None: return if os.tcgetpgrp(terminal_fd) == self._proc.pid: self._set_foreground_group(terminal_fd, self._original_group) - if not relayed and self._proc.returncode is None: - # Ctrl-Z reached only cmd2's group, which owns the terminal between pipe - # writes. Stop the pipeline too, as a shell stops its whole job. - with self._terminal_lock: - self._own_stops += 1 - stopped_pipeline = self._signal_pipeline(signal.SIGSTOP) - if not stopped_pipeline: - with self._terminal_lock: - self._own_stops -= 1 + # Ctrl-Z reached only cmd2's group, which owns the terminal between pipe writes. + # Stop the pipeline too, as a shell stops its whole job. Should another thread + # be relaying a stop already, it has stopped the pipeline. + if not relayed and self._suspension_lock.acquire(blocking=False): + own_suspension = True + self._signal_pipeline(signal.SIGSTOP) # Ignore our group-directed copy, then stop this thread synchronously. # Wrappers in our job must stop too. Unlike SIGSTOP, SIGTSTP is # discarded for orphaned groups, which have no shell to resume them. @@ -726,8 +749,10 @@ def suspend_job(signum: int, frame: Any) -> None: if self._terminal_available.is_set() and os.tcgetpgrp(terminal_fd) == self._original_group: self._set_foreground_group(terminal_fd, self._proc.pid) finally: - if stopped_pipeline: + if own_suspension: self._signal_pipeline(signal.SIGCONT) + self._suspensions += 1 + self._suspension_lock.release() self._job_resumed.set() signal.signal(signal.SIGTSTP, suspend_job) @@ -820,16 +845,11 @@ def _watch_job(self, terminal_fd: int) -> None: import signal while True: + seen = self._suspensions _, status = os.waitpid(self._proc.pid, os.WUNTRACED) if not os.WIFSTOPPED(status): self._proc.returncode = os.waitstatus_to_exitcode(status) return - if os.WSTOPSIG(status) == signal.SIGSTOP: - with self._terminal_lock: - if self._own_stops: - # cmd2 stopped the pipeline along with itself, and continues it too. - self._own_stops -= 1 - continue if os.WSTOPSIG(status) in (signal.SIGTTIN, signal.SIGTTOU): # Command code owns the terminal between pipe writes. Defer # consumer terminal access until the next write or final wait. @@ -838,10 +858,14 @@ def _watch_job(self, terminal_fd: int) -> None: with self._terminal_lock: # A stopped consumer can be killed before another write. # Keep reaping even while command code owns the terminal. + recent = self._suspensions pid, pending_status = os.waitpid(self._proc.pid, os.WNOHANG | os.WUNTRACED) - if pid and not os.WIFSTOPPED(pending_status): - self._proc.returncode = os.waitstatus_to_exitcode(pending_status) - return + if pid: + if not os.WIFSTOPPED(pending_status): + self._proc.returncode = os.waitstatus_to_exitcode(pending_status) + return + # A newer stop: the one a suspension would deal with. + seen = recent # A short write may already have returned the terminal. # Do not turn that ordinary handoff into a job suspension. if not self._terminal_available.is_set(): @@ -854,37 +878,55 @@ def _watch_job(self, terminal_fd: int) -> None: if foreground == self._proc.pid: continue - self._suspend_with_cmd2(terminal_fd) + self._suspend_with_cmd2(terminal_fd, seen) + + def _suspend_with_cmd2(self, terminal_fd: int, seen: int) -> None: + """Relay a stop in the pipeline's job to cmd2's own, and continue the pipeline once cmd2 resumes. + + Ctrl-Z stops every process of the job that does not ignore it, and each stop may be + reported: the consumer's to its watcher, a producer's to the thread that waits for it. + Only the first to arrive suspends the job. A suspension ends by continuing the whole + pipeline, so a stop reported before it finished needs nothing more. - def _suspend_with_cmd2(self, terminal_fd: int) -> None: - """Relay a pipeline's stop to cmd2's own job, and continue the pipeline once cmd2 resumes.""" + :param terminal_fd: the controlling terminal + :param seen: the number of finished suspensions when the stop was reported + """ import signal - with self._terminal_lock: - if os.tcgetpgrp(terminal_fd) == self._proc.pid: - self._set_foreground_group(terminal_fd, self._original_group) - # Stop every terminal reader before returning control to the outer shell. - self._signal_pipeline(signal.SIGSTOP) - self._job_resumed.clear() - self._relaying_stop = True - # Signal the main thread itself. Only it runs Python signal handlers, and a - # process-directed signal may be taken by another thread while the main - # thread sleeps in a system call, which then never returns to run the handler. - signal.pthread_kill(threading.main_thread().ident or 0, signal.SIGTSTP) - self._job_resumed.wait() - self._signal_pipeline(signal.SIGCONT) - - def _relay_producer_stop(self) -> None: - """Suspend the shell's whole job for a stopped producer that outlived the consumer. - - Ctrl-Z reaches only the foreground group, and the producer may be all that is - left of it. The watcher ended with the consumer, so nothing else relays the stop. + # Wait in short polls: on the main thread, the suspension being waited for may need + # this thread to run the SIGTSTP handler. + while not self._suspension_lock.acquire(timeout=0.1): + pass + try: + if self._suspensions != seen: + return + with self._terminal_lock: + if os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) + # Stop every terminal reader before returning control to the outer shell. + self._signal_pipeline(signal.SIGSTOP) + self._job_resumed.clear() + self._relaying_stop = True + # Signal the main thread itself. Only it runs Python signal handlers, and a + # process-directed signal may be taken by another thread while the main + # thread sleeps in a system call, which then never returns to run the handler. + signal.pthread_kill(threading.main_thread().ident or 0, signal.SIGTSTP) + self._job_resumed.wait() + self._signal_pipeline(signal.SIGCONT) + self._suspensions += 1 + finally: + self._suspension_lock.release() + + def _relay_producer_stop(self, seen: int) -> None: + """Suspend the shell's whole job for a stopped producer that joined this pipeline. + + The consumer need not stop with it: it may ignore Ctrl-Z, or be gone already, and + then nothing else would relay the stop. + + :param seen: the number of finished suspensions when the stop was reported """ - terminal_fd = self._terminal_fd - if terminal_fd is None or not self._process_done.is_set(): - # A live watcher relays the consumer's stop and continues the whole group. - return - self._suspend_with_cmd2(terminal_fd) + if self._terminal_fd is not None: + self._suspend_with_cmd2(self._terminal_fd, seen) def _wait_for_producer(self, pipeline: "ProcReader", timeout: float | None) -> None: """Wait for a producer in a terminal pipeline's job, relaying its job-control stops. @@ -896,6 +938,7 @@ def _wait_for_producer(self, pipeline: "ProcReader", timeout: float | None) -> N deadline = None if timeout is None else time.monotonic() + timeout while self._proc.returncode is None: + seen = pipeline._suspensions try: pid, status = os.waitpid(self._proc.pid, os.WNOHANG | os.WUNTRACED) except ChildProcessError: @@ -907,7 +950,7 @@ def _wait_for_producer(self, pipeline: "ProcReader", timeout: float | None) -> N raise subprocess.TimeoutExpired(self._proc.args, timeout or 0) time.sleep(0.05) elif os.WIFSTOPPED(status): - pipeline._relay_producer_stop() + pipeline._relay_producer_stop(seen) else: self._proc.returncode = os.waitstatus_to_exitcode(status) @@ -1069,6 +1112,8 @@ def _relay(self) -> None: poller = select.poll() poller.register(self._out_fd, select.POLLOUT) + incoming = select.poll() + incoming.register(self._in_fd, select.POLLIN) lend = contextlib.ExitStack() lending = False try: @@ -1079,11 +1124,15 @@ def _relay(self) -> None: # No producer is waiting. Let command code have the terminal back. lend.close() lending = False - data = os.read(self._in_fd, 65536) - if not data: - return + # Wait for output outside the lock, then take and count it under the lock. Output + # taken out of the pipe but not yet counted would look passed on to idle() and + # flush(), and cmd2's next write could overtake it. + incoming.poll() with self._lock: + data = os.read(self._in_fd, 65536) self._received += len(data) + if not data: + return view = memoryview(data) written = 0 while written < len(view): diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index 5f5305a51..054404b6c 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -1108,3 +1108,90 @@ def wait_until(predicate): os.close(master) process.kill() process.wait(timeout=5) + + +@pytest.mark.parametrize("launcher", ["plain", "exec"]) +@pytest.mark.parametrize("consumer", ["stops", "ignores_ctrl_z"]) +def test_ctrl_z_with_a_shell_producer_in_the_pipeline(tmp_path, launcher, consumer) -> None: + """Ctrl-Z in `shell | ` suspends the whole job, or none of it. + + The producer joins the pipeline's job. Run from a shell, whichever process stops first + suspends the job, including when the consumer ignores Ctrl-Z and never stops itself. As a + session leader, cmd2's job has no shell to resume it, so Ctrl-Z must stop none of it: the + producer ignores SIGTSTP just as the consumer does. Otherwise the producer stops for good + while cmd2 waits for it. + """ + import pty + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + python = shlex.quote(sys.executable) + producer = tmp_path / "producer.py" + producer.write_text( + "import os, time\nos.write(2, b'PRODUCER_READY\\n')\ntime.sleep(1.5)\nprint('PRODUCED')\n", + encoding="utf-8", + ) + ignoring = tmp_path / "ignoring.py" + ignoring.write_text( + "import signal, sys\nsignal.signal(signal.SIGTSTP, signal.SIG_IGN)\nsys.stdout.write(sys.stdin.read())\n", + encoding="utf-8", + ) + application = tmp_path / "application.py" + application.write_text( + "from cmd2 import Cmd\napp = Cmd()\napp.prompt = 'TEST> '\napp.cmdloop()\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + data = decoder.decode(os.read(master, 65536)) + transcript += data + if "\x1b[6n" in data: + # Answer prompt-toolkit's cursor-position request as a terminal would. + os.write(master, b"\x1b[1;1R") + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") + + consumer_command = "cat" if consumer == "stops" else f"{python} {shlex.quote(str(ignoring))}" + try: + wait_until(lambda: "OUTER> " in transcript) + prefix = "exec " if launcher == "exec" else "" + os.write(master, f"{prefix}{python} {shlex.quote(str(application))}\n".encode()) + wait_until(lambda: "TEST>" in transcript) + start = len(transcript) + os.write(master, f"shell {python} {shlex.quote(str(producer))} | {consumer_command}\n".encode()) + wait_until(lambda: "PRODUCER_READY\r\n" in transcript[start:]) + # The producer and consumer own the terminal, so Ctrl-Z reaches both. + wait_until(lambda: os.tcgetpgrp(master) not in (process.pid, os.getpgid(process.pid))) + os.write(master, b"\x1a") + if launcher == "plain": + wait_until(lambda: os.tcgetpgrp(master) == process.pid and "OUTER> " in transcript[start:]) + os.write(master, b"fg\n") + wait_until(lambda: "PRODUCED\r\n" in transcript[start:]) + # cmd2 gets its prompt back. The command, sent early, runs once it has. + os.write(master, b"help quit\n") + wait_until(lambda: "Exit this application" in transcript[start:]) + if launcher == "exec": + assert "Stopped" not in transcript[start:] + os.write(master, b"quit\n") + finally: + os.close(master) + process.kill() + process.wait(timeout=5) diff --git a/tests/test_utils.py b/tests/test_utils.py index 3475e57e7..531b36d83 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -235,7 +235,7 @@ def test_proc_reader_does_not_resignal_its_own_group(pr_none) -> None: @pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") def test_proc_reader_sigint_to_a_group_cmd2_may_not_signal() -> None: - reader = cu.ProcReader(mock.Mock(pid=os.getpid() + 1, stdout=None, stderr=None), sys.stdout, sys.stderr) + reader = cu.ProcReader(mock.Mock(pid=os.getpid() + 1, stdout=None, stderr=None, returncode=None), sys.stdout, sys.stderr) with ( mock.patch("os.getpgid", return_value=reader._proc.pid), mock.patch("os.killpg", side_effect=PermissionError) as killpg, @@ -248,7 +248,7 @@ def test_proc_reader_sigint_to_a_group_cmd2_may_not_signal() -> None: @pytest.mark.parametrize("producer", ["none", "reaped"]) def test_proc_reader_sigint_after_pipeline_exit(producer) -> None: """Once the pipeline is gone, its ID may belong to another process group. Signal nothing.""" - reader = cu.ProcReader(mock.Mock(pid=os.getpid() + 1, stdout=None, stderr=None), sys.stdout, sys.stderr) + reader = cu.ProcReader(mock.Mock(pid=os.getpid() + 1, stdout=None, stderr=None, returncode=None), sys.stdout, sys.stderr) if producer == "reaped": cu.ProcReader( mock.Mock(pid=os.getpid() + 2, stdout=None, stderr=None, returncode=0), sys.stdout, sys.stderr, pipeline=reader @@ -451,6 +451,48 @@ def test_pipeline_writer_relay_leaves_the_terminal_after_a_producer_finishes() - os.close(read_fd) +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipes") +def test_descriptor_relay_counts_output_it_is_reading() -> None: + """Output the relay has taken out of its pipe, but not yet passed on, is still pending. + + Otherwise cmd2's next write could overtake the end of a subprocess's output. + """ + import threading + + read_fd, write_fd = os.pipe() + real_read = os.read + taken = threading.Event() + release = threading.Event() + + def slow_read(fd, size): + data = real_read(fd, size) + if threading.current_thread().name == "pipe_relay" and data: + # The output has left the relay's pipe but has not reached the consumer's. + taken.set() + release.wait(5) + return data + + answers = [] + # The relay thread must start inside the patch, or it is already in the real read. + with mock.patch("os.read", side_effect=slow_read): + relay = cu._DescriptorRelay(write_fd, mock.Mock(_lend_terminal=contextlib.nullcontext)) + try: + with mock.patch("os.read", side_effect=slow_read): + os.write(relay.write_fd, b"tail of a subprocess's output") + assert taken.wait(5) + asker = threading.Thread(target=lambda: answers.append(relay.idle())) + asker.start() + asker.join(0.3) + release.set() + asker.join(5) + assert answers == [False] + finally: + release.set() + relay.close_write_fd() + assert real_read(read_fd, 100) == b"tail of a subprocess's output" + os.close(read_fd) + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipes") def test_descriptor_relay_that_finished_has_nothing_pending() -> None: """Once every producer has closed the relay's pipe, the relay finishes and closes it, and nothing is left to wait for.""" @@ -517,7 +559,7 @@ def test_proc_reader_terminate(pr_none) -> None: @pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") @pytest.mark.parametrize("already_exited", [False, True]) def test_proc_reader_terminate_terminal_job(already_exited) -> None: - proc = mock.Mock(stdout=None, stderr=None) + proc = mock.Mock(stdout=None, stderr=None, returncode=None) reader = cu.ProcReader(proc, sys.stdout, sys.stderr) reader._terminal_fd = 10 with mock.patch("os.kill", side_effect=ProcessLookupError if already_exited else None) as kill: @@ -529,6 +571,19 @@ def test_proc_reader_terminate_terminal_job(already_exited) -> None: proc.wait.assert_not_called() +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_does_not_signal_a_reaped_pipeline() -> None: + """Once the watcher has reaped the pipeline, its process ID may already belong to an unrelated process.""" + reader = cu.ProcReader(mock.Mock(pid=os.getpid() + 1, stdout=None, stderr=None, returncode=0), sys.stdout, sys.stderr) + reader._terminal_fd = 10 + with mock.patch("os.getpgid") as getpgid, mock.patch("os.killpg") as killpg, mock.patch("os.kill") as kill: + reader.send_sigint() + reader.terminate() + getpgid.assert_not_called() + killpg.assert_not_called() + kill.assert_not_called() + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") @pytest.mark.parametrize("stop_signal", ["SIGTTIN", "SIGTTOU"]) @pytest.mark.parametrize("expired_handoff", [False, True]) @@ -607,21 +662,33 @@ def test_proc_reader_watcher_always_records_an_exit(waits, hung_up, returncode, @pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") -def test_proc_reader_watcher_skips_the_stop_cmd2_sent() -> None: +def test_proc_reader_watcher_skips_a_stop_a_suspension_already_resolved() -> None: + """Ctrl-Z reaches every process of the job, but only the first stop reported suspends it. + + Another suspension -- cmd2's own, or one a producer's stop set off -- finished after this + stop was reported, and it continued the whole pipeline as it ended. + """ proc = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) reader = cu.ProcReader(proc, sys.stdout, sys.stderr) reader._terminal_fd = 10 reader._original_group = 456 - reader._own_stops = 1 - stopped = (signal.SIGSTOP << 8) | 0x7F + stopped = (signal.SIGTSTP << 8) | 0x7F + reports = iter([(proc.pid, stopped), (proc.pid, 0)]) + + def waitpid(pid, options): + report = next(reports) + if report[1] == stopped: + reader._suspensions += 1 + return report + with ( - mock.patch("os.waitpid", side_effect=[(proc.pid, stopped), (proc.pid, 0)]), + mock.patch("os.waitpid", side_effect=waitpid), mock.patch("os.tcgetpgrp", return_value=456), mock.patch("signal.pthread_kill") as relay, ): reader._wait_for_job(10) relay.assert_not_called() - assert reader._own_stops == 0 + assert reader._suspensions == 1 assert proc.returncode == 0 @@ -705,8 +772,9 @@ def test_proc_reader_suspend_restores_signal_handler(handler_kind, relayed) -> N *own_stop, mock.call(proc.pid, signal.SIGCONT), ] - # The watcher skips the stop cmd2 sent. - assert reader._own_stops == 1 + # A stop reported before this suspension ended is left alone. + assert reader._suspensions == 1 + assert not reader._suspension_lock.locked() stop.assert_called_once_with(signal.SIGTSTP) assert set_handler.call_args_list == [ mock.call(signal.SIGTSTP, handler), @@ -723,31 +791,76 @@ def test_proc_reader_suspend_restores_signal_handler(handler_kind, relayed) -> N @pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") -def test_proc_reader_direct_suspend_when_the_pipeline_is_gone() -> None: - """If the pipeline cannot be stopped along with cmd2, cmd2 does not count the stop or continue it.""" +def test_proc_reader_suspension_waits_for_one_in_progress() -> None: + """A stop reported during another suspension waits for it, then needs nothing more: that one continued the pipeline.""" + import threading + + reader = cu.ProcReader(mock.Mock(pid=123, stdout=None, stderr=None, returncode=None), sys.stdout, sys.stderr) + reader._original_group = 456 + assert reader._suspension_lock.acquire(blocking=False) + + def finish_suspension(): + time.sleep(0.25) + reader._suspensions += 1 + reader._suspension_lock.release() + + other = threading.Thread(target=finish_suspension) + other.start() + with mock.patch("os.killpg") as killpg, mock.patch("signal.pthread_kill") as relay: + reader._suspend_with_cmd2(10, seen=0) + other.join() + killpg.assert_not_called() + relay.assert_not_called() + assert reader._suspensions == 1 + assert not reader._suspension_lock.locked() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_watcher_follows_newer_stops_while_waiting_for_the_terminal() -> None: + """A consumer continued by a suspension stops again on its next terminal read. That newer stop is the one to handle.""" proc = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) reader = cu.ProcReader(proc, sys.stdout, sys.stderr) reader._terminal_fd = 10 reader._original_group = 456 + reader._terminal_available.set() + ttin = (signal.SIGTTIN << 8) | 0x7F + with ( + mock.patch("os.waitpid", side_effect=[(proc.pid, ttin), (proc.pid, ttin), (proc.pid, 0)]), + mock.patch("os.tcgetpgrp", return_value=proc.pid), + mock.patch("os.killpg") as killpg, + mock.patch("signal.pthread_kill") as relay, + ): + reader._wait_for_job(10) + # The lend is active, so the consumer is continued to read the terminal. + killpg.assert_called_once_with(proc.pid, signal.SIGCONT) + relay.assert_not_called() + assert proc.returncode == 0 - def killpg(group, signum): - if group == proc.pid: - raise ProcessLookupError +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_direct_suspend_while_another_thread_relays_one() -> None: + """A Ctrl-Z that reaches cmd2 while another thread relays a stop leaves the pipeline to that thread.""" + proc = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + assert reader._suspension_lock.acquire(blocking=False) with ( mock.patch("signal.getsignal", return_value=signal.SIG_DFL), mock.patch("signal.signal") as set_handler, mock.patch("signal.raise_signal") as stop, - mock.patch("os.killpg", side_effect=killpg) as sent, + mock.patch("os.killpg") as sent, mock.patch("os.tcgetpgrp", return_value=reader._original_group), mock.patch("threading.Thread"), reader._manage_terminal(), ): handler = set_handler.call_args.args[1] handler(signal.SIGTSTP, None) - assert sent.call_args_list == [mock.call(proc.pid, signal.SIGSTOP), mock.call(reader._original_group, signal.SIGTSTP)] - assert reader._own_stops == 0 + assert sent.call_args_list == [mock.call(reader._original_group, signal.SIGTSTP)] stop.assert_called_once_with(signal.SIGTSTP) + # The relaying thread counts its suspension and releases the lock. + assert reader._suspensions == 0 + assert reader._suspension_lock.locked() assert reader._job_resumed.is_set() @@ -1086,18 +1199,28 @@ def test_proc_reader_producer_wait_times_out() -> None: @pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") -@pytest.mark.parametrize("watcher_done", [False, True]) +@pytest.mark.parametrize("resolved", [False, True]) @pytest.mark.parametrize("foreground_group", [123, 456]) -def test_proc_reader_relays_producer_stop_once_the_watcher_is_gone(watcher_done, foreground_group) -> None: - consumer = mock.Mock(pid=123, stdout=None, stderr=None, returncode=0) +def test_proc_reader_relays_producer_stop(resolved, foreground_group) -> None: + """A producer's stop suspends the job even while the consumer's watcher runs: the consumer may ignore Ctrl-Z. + + Unless a suspension that finished after the stop was reported, such as the one the + consumer's own stop set off, has continued the pipeline already. + """ + consumer = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) pipeline = cu.ProcReader(consumer, sys.stdout, sys.stderr) pipeline._terminal_fd = 10 pipeline._original_group = 456 - if watcher_done: - pipeline._process_done.set() proc = mock.Mock(pid=321, stdout=None, stderr=None, returncode=None) reader = cu.ProcReader(proc, sys.stdout, sys.stderr, pipeline=pipeline) stopped_status = (signal.SIGTSTP << 8) | 0x7F + reports = iter([(proc.pid, stopped_status), (proc.pid, 0)]) + + def waitpid(pid, options): + report = next(reports) + if resolved and report[1] == stopped_status: + pipeline._suspensions += 1 + return report def resume(thread_id, signum): assert thread_id == threading.main_thread().ident @@ -1105,7 +1228,7 @@ def resume(thread_id, signum): pipeline._job_resumed.set() with ( - mock.patch("os.waitpid", side_effect=[(proc.pid, stopped_status), (proc.pid, 0)]), + mock.patch("os.waitpid", side_effect=waitpid), mock.patch("os.tcgetpgrp", return_value=foreground_group), mock.patch.object(pipeline, "_set_foreground_group") as foreground, mock.patch("os.killpg") as killpg, @@ -1113,8 +1236,8 @@ def resume(thread_id, signum): ): reader._wait_for_exit() assert proc.returncode == 0 - if not watcher_done: - # The consumer's watcher sees the same Ctrl-Z and suspends the job itself. + assert pipeline._suspensions == 1 + if resolved: relay.assert_not_called() killpg.assert_not_called() foreground.assert_not_called() From 330f14961c7ebd41030803535a500c024dad163e Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 13:21:09 -0400 Subject: [PATCH 11/20] Type-check as Linux on every OS, as CI does mypy checks the platform it runs on. On Windows, it rejected the POSIX-only terminal job control in cmd2.utils with 69 attr-defined errors (os.tcsetpgrp, signal.SIGTSTP, select.poll and the like), and narrowed terminal_fd in _redirect_output() to None, reporting the pipeline code as unreachable. CI runs mypy on Ubuntu only, so it never saw either. Guarding each POSIX-only function does not help: mypy reports the code after an `if sys.platform == "win32": raise` guard as unreachable, and ruff forbids the assert it does accept. Pin mypy's platform to Linux instead, so a Windows run matches CI. Windows-only branches go unchecked by mypy, as they already did in CI; ty still checks them. Also annotate terminal_fd, whose type should not depend on narrowing. --- cmd2/cmd2.py | 2 +- pyproject.toml | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 68dbb7dab..3fd2ecfb5 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -3345,7 +3345,7 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: pipe_stdout = None if isinstance(self.stdout, utils.StdSim) else self.stdout # type: ignore[unreachable] pipe_stderr = None if isinstance(sys.stderr, utils.StdSim) else sys.stderr - terminal_fd = None + terminal_fd: int | None = None popen_command = statement.redirect_to if sys.platform != "win32": # Only a pipeline whose output goes to the terminal is the terminal's job. One diff --git a/pyproject.toml b/pyproject.toml index cd68a4b1f..c0da63572 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -93,6 +93,9 @@ exclude = [ "^tests/", # tests directory ] files = ['.'] +# Check as CI does, on Linux, so results don't depend on the developer's OS. Windows-only +# branches are skipped, and POSIX-only job control would otherwise fail to check on Windows. +platform = "linux" show_column_numbers = true show_error_codes = true show_error_context = true From ebe7a39ee56d9f403421231a97662fd9bd96cacc Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 13:21:18 -0400 Subject: [PATCH 12/20] Pipe output in the console's code page on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `help -v | more` showed every box-drawing character as mojibake, such as ΓöÇ for ─. Since 4.2.4, pipes are written as UTF-8, but Windows console programs such as more, sort and findstr decode piped input with the console's output code page, which is 437 or 850 on most systems. On Windows, pipes now use the console's output code page. It cannot represent everything, an emoji for instance, and 4.2.4 moved to UTF-8 because encoding failures aborted the command. So replace what the code page lacks instead of failing. Code page 65001 (chcp 65001) is reported as utf-8. Without a console, and on other platforms, pipes still use UTF-8. The POSIX terminal pipeline's writer uses the same encoding and error handling. --- CHANGELOG.md | 5 +++++ cmd2/cmd2.py | 16 ++++++++++------ cmd2/utils.py | 19 +++++++++++++++++++ tests/test_suite_environment.py | 28 +++++++++++++++++++++++++--- tests/test_utils.py | 26 ++++++++++++++++++++++++++ 5 files changed, 85 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 25264b0a3..dd49e36cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,11 @@ nested in a command whose own output is piped, still run in their own session - A `shell` command piped to an interactive program, such as `shell git log | less`, now joins the pipeline's job, so both processes receive Ctrl-C and Ctrl-Z + - On Windows, fixed piped output appearing garbled in console programs such as `more` + (`help -v | more`), a regression in 4.2.4. Pipes were written as UTF-8, but console programs + decode their input with the console's code page. Pipes now use that code page, and characters + it cannot represent are replaced rather than failing the command. Without a console, and on + other platforms, pipes still use UTF-8 ## 4.2.4 (September 8, 2026) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 3fd2ecfb5..b520cca74 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -3324,11 +3324,14 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: # Create a pipe with read and write sides read_fd, write_fd = os.pipe() - # Open each side of the pipe. Both ends are given an explicit encoding: - # command output is rendered by Rich and routinely contains non-ASCII, which - # the locale encoding cannot always represent. - subproc_stdin = open(read_fd, encoding="utf-8") # noqa: SIM115 - new_stdout: TextIO = cast(TextIO, open(write_fd, "w", encoding="utf-8")) # noqa: SIM115 + # Open each side of the pipe. Both ends are given an explicit encoding: command + # output is rendered by Rich and routinely contains non-ASCII, which the locale + # encoding cannot always represent. On Windows, that is the console's code page, + # which console programs such as more decode with. It cannot represent everything + # either, so replace what it lacks rather than fail the command. + pipe_encoding = utils._pipe_encoding() + subproc_stdin = open(read_fd, encoding=pipe_encoding) # noqa: SIM115 + new_stdout: TextIO = cast(TextIO, open(write_fd, "w", encoding=pipe_encoding, errors="replace")) # noqa: SIM115 # Isolate pipeline signals from cmd2. Terminal pipelines receive the # foreground terminal; ProcReader relays their job-control stops. @@ -3433,7 +3436,8 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: pipe_fd, cmd_pipe_proc_reader, interruptible=lambda: not self.sigint_protection ) ), - encoding="utf-8", + encoding=pipe_encoding, + errors="replace", ) self.stdout = new_stdout diff --git a/cmd2/utils.py b/cmd2/utils.py index 9a226999d..144f37fcc 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -536,6 +536,25 @@ def write(self, b: bytes) -> None: self.std_sim_instance.flush() +def _pipe_encoding() -> str: + """Return the encoding for output cmd2 pipes to a shell command. + + Windows console programs such as more, sort, and findstr decode piped input with the + console's output code page, and would show UTF-8 as mojibake. Elsewhere, and on Windows + without a console, use UTF-8. + """ + if sys.platform == "win32": + import codecs + import ctypes + + code_page = ctypes.windll.kernel32.GetConsoleOutputCP() + if code_page: + with contextlib.suppress(LookupError): + # Normalized, so that code page 65001 is reported as utf-8 + return codecs.lookup(f"cp{code_page}").name + return "utf-8" + + @contextlib.contextmanager def _unblocked_sigttou() -> Iterator[None]: """Let a child started inside :meth:`ProcReader._lend_terminal` keep normal job control. diff --git a/tests/test_suite_environment.py b/tests/test_suite_environment.py index a49d3b8ac..fb2e1f71c 100644 --- a/tests/test_suite_environment.py +++ b/tests/test_suite_environment.py @@ -44,6 +44,10 @@ def do_show_encoding(self, _: str) -> None: """Print the current output stream's encoding.""" self.poutput(f"ENCODING={getattr(self.stdout, 'encoding', None)}") + def do_say(self, text: str) -> None: + """Print the argument unchanged.""" + self.poutput(text) + def test_redirection_to_a_file_uses_utf8(tmp_path) -> None: """cmd2 renders non-ASCII, so a redirect target must not use the locale encoding. @@ -63,9 +67,27 @@ def test_redirection_to_a_file_uses_utf8(tmp_path) -> None: PASS_THROUGH = "import sys; sys.stdin.reconfigure(encoding='utf-8'); sys.stdout.write(sys.stdin.read())" -def test_piping_uses_utf8(tmp_path, running_pipe_process) -> None: - """The same applies to the pipe the subprocess reads from.""" +def test_piping_uses_pipe_encoding(tmp_path, running_pipe_process) -> None: + """The pipe the subprocess reads from uses the encoding its consumer expects, not the locale's. + + That is UTF-8, except on Windows, where console programs such as more decode with the + console's code page. + """ app = EncodingProbe(allow_cli_args=False) target = tmp_path / "piped.txt" app.onecmd_plus_hooks(f'show_encoding | "{sys.executable}" -c "{PASS_THROUGH}" > "{target}"') - assert "ENCODING=utf-8" in target.read_text(encoding="utf-8") + assert f"ENCODING={cmd2.utils._pipe_encoding()}" in target.read_text(encoding="utf-8") + + +#: A pass-through filter that copies bytes, so the test sees exactly what cmd2 wrote to the pipe. +BYTES_PASS_THROUGH = "import sys; sys.stdout.buffer.write(sys.stdin.buffer.read())" + + +def test_piping_replaces_what_the_pipe_encoding_lacks(tmp_path, monkeypatch, running_pipe_process) -> None: + """A console code page cannot represent all output, which must not fail the command.""" + monkeypatch.setattr(cmd2.utils, "_pipe_encoding", lambda: "cp437") + app = EncodingProbe(allow_cli_args=False) + target = tmp_path / "piped.bin" + app.onecmd_plus_hooks(f'say ─\U0001f607 | "{sys.executable}" -c "{BYTES_PASS_THROUGH}" > "{target}"') + # Box drawing exists in cp437, the emoji does not + assert target.read_bytes().startswith(b"\xc4?") diff --git a/tests/test_utils.py b/tests/test_utils.py index 531b36d83..602f91c2e 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -186,6 +186,32 @@ def test_stdsim_line_buffering(base_app) -> None: assert os.path.getsize(file.name) == saved_size + len(bytes_to_write) +@pytest.mark.skipif(sys.platform == "win32", reason="Windows pipes use the console code page") +def test_pipe_encoding_is_utf8() -> None: + assert cu._pipe_encoding() == "utf-8" + + +@pytest.mark.skipif(sys.platform != "win32", reason="Windows console code pages") +@pytest.mark.parametrize( + ("code_page", "encoding"), + [ + (437, "cp437"), + (850, "cp850"), + # chcp 65001 + (65001, "utf-8"), + # No console attached + (0, "utf-8"), + # A code page Python has no codec for + (50220, "utf-8"), + ], +) +def test_pipe_encoding_follows_the_console_code_page(monkeypatch, code_page, encoding) -> None: + import ctypes + + monkeypatch.setattr(ctypes.windll.kernel32, "GetConsoleOutputCP", lambda: code_page) + assert cu._pipe_encoding() == encoding + + @pytest.fixture def pr_none(): import subprocess From f4fdddb6d07090ae279c0cfe28598659c27f4fd9 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 13:37:08 -0400 Subject: [PATCH 13/20] Judge the relay race test only by answers given during the paused read test_descriptor_relay_counts_output_it_is_reading paused the relay after it had taken output out of its pipe, asked idle() from another thread, and released the relay after 0.3s whether or not that thread had asked yet. On the slower macOS runners (Python 3.11 to 3.13), the thread sometimes asked only after the release. By then the relay had passed the output on, so idle() rightly answered True, and the test failed. The asking thread now reports that it is about to ask, and the test checks only the answer given while the read is paused: none yet, because idle() waits for the relay, or False. It still fails every time against the relay that counted output only after reading it. --- tests/test_utils.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tests/test_utils.py b/tests/test_utils.py index 602f91c2e..39d6d3899 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -499,6 +499,12 @@ def slow_read(fd, size): return data answers = [] + asking = threading.Event() + + def ask() -> None: + asking.set() + answers.append(relay.idle()) + # The relay thread must start inside the patch, or it is already in the real read. with mock.patch("os.read", side_effect=slow_read): relay = cu._DescriptorRelay(write_fd, mock.Mock(_lend_terminal=contextlib.nullcontext)) @@ -506,12 +512,17 @@ def slow_read(fd, size): with mock.patch("os.read", side_effect=slow_read): os.write(relay.write_fd, b"tail of a subprocess's output") assert taken.wait(5) - asker = threading.Thread(target=lambda: answers.append(relay.idle())) + asker = threading.Thread(target=ask) asker.start() + assert asking.wait(5) asker.join(0.3) + # While the output is in the relay's hands, idle() waits for the relay or says the + # output is still pending. Once released, the relay passes it on, so a later answer + # may rightly be that nothing is left. + answered_during_read = list(answers) release.set() asker.join(5) - assert answers == [False] + assert answered_during_read in ([], [False]) finally: release.set() relay.close_write_fd() From f5fe7cb0aecc2521db73eb69251d8b394c0f5249 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 14:19:56 -0400 Subject: [PATCH 14/20] Correct the branch policy in CLAUDE.md main is the branch for every release, whether patch, minor or major. Feature branches merge into it, and releases are tagged and published from it. CLAUDE.md said main held only the next patch release. --- CLAUDE.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index d29899dab..3cba0d64b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -111,8 +111,8 @@ embedded Python/IPython shells and `run_pyscript` while keeping isolation. - Anything not documented under `docs/api/` is not public API (`cmd2/constants.py` says so explicitly). - Add user-visible changes to `CHANGELOG.md` under the current in-progress version heading. -- `main` is the branch for the next PATCH release; MAJOR/MINOR work happens on a branch named for - the target version. Releases are tagged and published from `main`. +- `main` is the branch for the next release, whether PATCH, MINOR, or MAJOR. Feature branches merge + into `main`, and all releases are tagged and published from it. - Do not commit spec, plan, or markdown documents without asking first. Save plans to `~/.superpowers/plans/` rather than the project directory. From 6b63ca9c1bb02336aa5816760180ea9adedea28f Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 14:19:56 -0400 Subject: [PATCH 15/20] Fix a writer race, the Android shell, code pages, and worker-thread shells Fixes from another review of the pipeline changes: - A cmd2 write could hang when a subprocess wrote through the descriptor relay at the same time. The write fast path checked the consumer's pipe for room, then wrote without a terminal lend. The relay thread writes to the same pipe, and could fill it in between. The write then blocked with no lend, while the pager, waiting for a key, stayed stopped. Once a relay exists, every write is now made under a lend. Without one, writes the pipe has room for still need none. DescriptorRelay.idle() had no other caller, and is gone. - The start gate ran /bin/sh, which Android does not have, so every terminal pipe there failed to start. It now uses the POSIX shell that Popen(shell=True) itself uses: /system/bin/sh on Android. - On Windows before Python 3.14, a console code page Python has no cpNNNNN codec for fell back to UTF-8, although Python knows many of them by other names. Pipes now try those names too: the ISO-8859 family, KOI8-R and KOI8-U, US-ASCII, GB18030, EUC-JP and EUC-KR. Python 3.14 has a codec for every code page Windows supports. - A shell command run from a worker thread joined the main thread's terminal pipeline. Joining relays job-control stops to the main thread, and may change signal handlers, which only the main thread may do: as a session leader, it raised ValueError. Only the main thread's shell commands join now. Tests cover each, and each fails without its fix: every write is lent once a relay exists; the gate uses Android's shell; code pages map to their codecs on any platform; a worker thread's shell command stays out of the pipeline. The relay tests that used idle() now use flush(). --- CHANGELOG.md | 7 +++-- cmd2/cmd2.py | 14 +++++++-- cmd2/utils.py | 56 ++++++++++++++++++++++++++++-------- tests/test_cmd2.py | 58 +++++++++++++++++++++++++++++++++++++ tests/test_utils.py | 69 +++++++++++++++++++++++++++++++++++++++------ 5 files changed, 177 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dd49e36cd..030d1214b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,9 +12,10 @@ the pipeline's job, so both processes receive Ctrl-C and Ctrl-Z - On Windows, fixed piped output appearing garbled in console programs such as `more` (`help -v | more`), a regression in 4.2.4. Pipes were written as UTF-8, but console programs - decode their input with the console's code page. Pipes now use that code page, and characters - it cannot represent are replaced rather than failing the command. Without a console, and on - other platforms, pipes still use UTF-8 + decode their input with the console's code page. Pipes now use that code page, including ones + Python names otherwise, such as 20866 (KOI8-R) or 28591 (ISO-8859-1), and characters it cannot + represent are replaced rather than failing the command. Without a console, with a code page + Python has no codec for, and on other platforms, pipes still use UTF-8 ## 4.2.4 (September 8, 2026) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index b520cca74..153ade6cf 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -3375,9 +3375,11 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: # user's shell as before. import shlex - user_shell = shlex.quote(kwargs.get("executable", "/bin/sh")) + # The POSIX shell Popen() itself runs with shell=True + posix_shell = "/system/bin/sh" if hasattr(sys, "getandroidapilevel") else "/bin/sh" + user_shell = shlex.quote(kwargs.get("executable", posix_shell)) popen_command = f"read -r _ || exit 1; exec {user_shell} -c {shlex.quote(statement.redirect_to)}" - kwargs["executable"] = "/bin/sh" + kwargs["executable"] = posix_shell with contextlib.ExitStack() as terminal_stack, contextlib.ExitStack() as gate_stack: if terminal_fd is not None: @@ -5007,9 +5009,15 @@ def do_shell(self, args: argparse.Namespace) -> None: # the terminal per write. Run the command inside the pipeline's job instead, for as # long as it runs: the consumer keeps the terminal, and Ctrl-C and Ctrl-Z reach both # processes, as they would in a shell pipeline. + # A worker thread's command stays out of it: job control relays stops to the main + # thread, and only the main thread may change signal handlers. pipeline = self._cur_pipe_proc_reader pipeline_group = None - if pipeline is not None and not isinstance(self.stdout, utils.StdSim): # type: ignore[unreachable] + if ( + pipeline is not None + and threading.current_thread() is threading.main_thread() + and not isinstance(self.stdout, utils.StdSim) # type: ignore[unreachable] + ): pipeline_group = pipeline._terminal_group # Prevent KeyboardInterrupts while in the shell process. The shell process still diff --git a/cmd2/utils.py b/cmd2/utils.py index 144f37fcc..54a4d9b02 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -536,22 +536,57 @@ def write(self, b: bytes) -> None: self.std_sim_instance.flush() +# Windows code pages Python names other than cpNNNNN. Before Python 3.14, which covers every +# code page Windows supports, a console set to one of these would otherwise get UTF-8. +_CODE_PAGE_CODECS = { + 20127: "ascii", + 20866: "koi8_r", + 21866: "koi8_u", + 28591: "latin_1", + 28592: "iso8859_2", + 28593: "iso8859_3", + 28594: "iso8859_4", + 28595: "iso8859_5", + 28596: "iso8859_6", + 28597: "iso8859_7", + 28598: "iso8859_8", + 28599: "iso8859_9", + 28603: "iso8859_13", + 28605: "iso8859_15", + 51932: "euc_jp", + 51949: "euc_kr", + 54936: "gb18030", +} + + +def _code_page_encoding(code_page: int) -> str | None: + """Return the name of the Python codec for a Windows code page, or None if there is none. + + :param code_page: a Windows code page identifier, such as 437 + """ + import codecs + + for name in (f"cp{code_page}", _CODE_PAGE_CODECS.get(code_page)): + if name is not None: + with contextlib.suppress(LookupError): + # Normalized, so that code page 65001 is reported as utf-8 + return codecs.lookup(name).name + return None + + def _pipe_encoding() -> str: """Return the encoding for output cmd2 pipes to a shell command. Windows console programs such as more, sort, and findstr decode piped input with the console's output code page, and would show UTF-8 as mojibake. Elsewhere, and on Windows - without a console, use UTF-8. + without a console or with a code page Python cannot encode, use UTF-8. """ if sys.platform == "win32": - import codecs import ctypes code_page = ctypes.windll.kernel32.GetConsoleOutputCP() if code_page: - with contextlib.suppress(LookupError): - # Normalized, so that code page 65001 is reported as utf-8 - return codecs.lookup(f"cp{code_page}").name + return _code_page_encoding(code_page) or "utf-8" return "utf-8" @@ -1108,11 +1143,6 @@ def _producer_blocked(self) -> bool: poller.register(self.write_fd, select.POLLOUT) return not poller.poll(0) - def idle(self) -> bool: - """Whether all output written to the relay has reached the consumer's pipe.""" - with self._lock: - return self._done or self._sent >= self._received + self._unread() - def flush(self) -> None: """Wait until output already written to the relay has reached the consumer's pipe. @@ -1248,8 +1278,10 @@ def write(self, b: Any) -> int: fd = super().fileno() written = 0 try: - # Output a producer wrote to the relay comes first, which may need a lend. - if self._relay is None or self._relay.idle(): + # Once a relay exists, its thread writes to the consumer's pipe too, and can fill it + # between a check for room and the write. So only without one is room checked + # without a lend. Output a producer wrote to the relay comes first in any case. + if self._relay is None: # Once there is room, a write of at most PIPE_BUF bytes does not block. while written < len(view) and self._poller.poll(0): written += os.write(fd, view[written : written + select.PIPE_BUF]) diff --git a/tests/test_cmd2.py b/tests/test_cmd2.py index afa07960e..c0295acf6 100644 --- a/tests/test_cmd2.py +++ b/tests/test_cmd2.py @@ -1,8 +1,10 @@ """Cmd2 unit/functional testing""" +import contextlib import io import os import signal +import subprocess import sys import tempfile import threading @@ -971,6 +973,62 @@ def start_pipe(*args, **kwargs): assert popen.call_args.kwargs["stdin"].closed +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +@pytest.mark.parametrize("android", [False, True]) +def test_terminal_pipe_start_gate_uses_the_platform_shell(redirection_app, mocker, monkeypatch, android) -> None: + """The start gate runs in the POSIX shell Popen() itself would use: Android has no /bin/sh.""" + if android: + monkeypatch.setattr(sys, "getandroidapilevel", lambda: 30, raising=False) + else: + monkeypatch.delattr(sys, "getandroidapilevel", raising=False) + monkeypatch.delenv("SHELL", raising=False) + popen = mocker.patch("subprocess.Popen", autospec=True) + popen.return_value.returncode = 127 + terminal_stream = mocker.Mock() + terminal_stream.isatty.return_value = True + terminal_stream.fileno.return_value = 10 + redirection_app.stdout = terminal_stream + mocker.patch("os.tcgetpgrp", return_value=os.getpgrp()) + mocker.patch("cmd2.utils.ProcReader") + redirection_app.onecmd_plus_hooks("print_output | less") + shell = "/system/bin/sh" if android else "/bin/sh" + assert popen.call_args.kwargs["executable"] == shell + # With SHELL unset, the gate hands the command to that shell, too. + assert f"exec {shell} -c less" in popen.call_args.args[0] + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_shell_from_a_worker_thread_stays_out_of_the_pipeline(base_app, tmp_path) -> None: + """Joining a pipeline's job means relaying stops to the main thread, and may change signal handlers. + + Only the main thread may do that, so a shell command run from another thread does not join. + """ + lent = [] + + @contextlib.contextmanager + def _lend_terminal(): + lent.append(True) + yield + + spawned = [] + real_popen = subprocess.Popen + + def popen(*args, **kwargs): + spawned.append(kwargs) + return real_popen(*args, **kwargs) + + base_app._cur_pipe_proc_reader = mock.Mock(_terminal_group=os.getpgrp(), _lend_terminal=_lend_terminal) + with (tmp_path / "output").open("w+") as output, mock.patch("subprocess.Popen", popen): + base_app.stdout = output + worker = threading.Thread(target=base_app.do_shell, args=("echo worker",)) + worker.start() + worker.join(10) + output.seek(0) + assert output.read() == "worker\n" + assert "process_group" not in spawned[0] + assert not lent + + def test_restore_output_resets_pipe_state_when_the_wait_fails(base_app) -> None: """A failed handback while waiting for the pipe process must not leave it current. diff --git a/tests/test_utils.py b/tests/test_utils.py index 39d6d3899..4ea200368 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -212,6 +212,31 @@ def test_pipe_encoding_follows_the_console_code_page(monkeypatch, code_page, enc assert cu._pipe_encoding() == encoding +@pytest.mark.parametrize( + ("code_page", "sample", "codec"), + [ + (437, "─", "cp437"), + # chcp 65001 + (65001, "─", "utf-8"), + # Code pages Python names otherwise. Python 3.14 on Windows has a cpNNNNN codec for them too. + (20866, "Ж", "koi8_r"), + (28591, "é", "latin_1"), + (20127, "a", "ascii"), + ], +) +def test_code_page_encoding(code_page, sample, codec) -> None: + encoding = cu._code_page_encoding(code_page) + assert encoding is not None + assert sample.encode(encoding) == sample.encode(codec) + + +@pytest.mark.parametrize("code_page", [50220, 1]) +def test_code_page_encoding_without_a_codec(code_page) -> None: + if sys.platform == "win32" and sys.version_info >= (3, 14) and code_page == 50220: + pytest.skip("Python 3.14 on Windows may support every code page Windows does") + assert cu._code_page_encoding(code_page) is None + + @pytest.fixture def pr_none(): import subprocess @@ -498,12 +523,13 @@ def slow_read(fd, size): release.wait(5) return data - answers = [] + flushed = threading.Event() asking = threading.Event() def ask() -> None: asking.set() - answers.append(relay.idle()) + relay.flush() + flushed.set() # The relay thread must start inside the patch, or it is already in the real read. with mock.patch("os.read", side_effect=slow_read): @@ -515,14 +541,12 @@ def ask() -> None: asker = threading.Thread(target=ask) asker.start() assert asking.wait(5) - asker.join(0.3) - # While the output is in the relay's hands, idle() waits for the relay or says the - # output is still pending. Once released, the relay passes it on, so a later answer - # may rightly be that nothing is left. - answered_during_read = list(answers) + # While the output is in the relay's hands, flush() must wait for it. + assert not flushed.wait(0.3) release.set() + # Once released, the relay passes the output on, and flush() returns. + assert flushed.wait(5) asker.join(5) - assert answered_during_read in ([], [False]) finally: release.set() relay.close_write_fd() @@ -544,11 +568,38 @@ def test_descriptor_relay_that_finished_has_nothing_pending() -> None: time.sleep(0.01) assert relay._done relay.flush() - assert relay.idle() finally: os.close(read_fd) +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipes") +@pytest.mark.parametrize("relaying", [False, True]) +def test_pipeline_writer_lends_every_write_once_a_relay_exists(relaying) -> None: + """The relay's thread writes to the consumer's pipe too. It could fill the pipe between a check for room and a write. + + So once a relay exists, even a write the pipe has room for is made with the terminal lent, + in case it blocks. Without one, such a write needs no lend. + """ + read_fd, write_fd = os.pipe() + lends = [] + + @contextlib.contextmanager + def lend_terminal(): + lends.append(1) + yield + + writer = cu._PipelineWriter(write_fd, mock.Mock(_lend_terminal=lend_terminal)) + try: + if relaying: + writer.fileno() + assert writer.write(b"fits") == 4 + assert lends == ([1] if relaying else []) + finally: + writer.close() + assert os.read(read_fd, 100) == b"fits" + os.close(read_fd) + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipes") def test_pipeline_writer_relay_passes_consumer_exit_to_the_producer() -> None: import subprocess From 9fb1da2a81bd0fd54daa6f8f0199330d5dccf7b7 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 14:24:35 -0400 Subject: [PATCH 16/20] Skip the no-codec code page test on Windows with Python 3.14+ From Python 3.14, Windows has a codec for every code page Windows accepts. That includes pseudo code pages such as 1 (CP_OEMCP), which the test used as a code page without a codec, so it failed there. The test already skipped code page 50220 on those versions for the same reason. --- tests/test_utils.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_utils.py b/tests/test_utils.py index 4ea200368..4f5693f90 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -230,10 +230,12 @@ def test_code_page_encoding(code_page, sample, codec) -> None: assert sample.encode(encoding) == sample.encode(codec) +@pytest.mark.skipif( + sys.platform == "win32" and sys.version_info >= (3, 14), + reason="Python 3.14 on Windows has a codec for every code page Windows accepts, including pseudo code pages such as 1", +) @pytest.mark.parametrize("code_page", [50220, 1]) def test_code_page_encoding_without_a_codec(code_page) -> None: - if sys.platform == "win32" and sys.version_info >= (3, 14) and code_page == 50220: - pytest.skip("Python 3.14 on Windows may support every code page Windows does") assert cu._code_page_encoding(code_page) is None From ab7ce16835589088d16116e9db460f130455fe7f Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 14:49:32 -0400 Subject: [PATCH 17/20] Harden pipeline signal handling and page in the console's code page Fixes from another review of the pipeline changes: - A pipe write that found the consumer ended by Ctrl-C raised KeyboardInterrupt on whatever thread was writing. Ctrl-C interrupts only the main thread, so a worker thread printing to a piped command died with a traceback, where it had quietly got BrokenPipeError before. Only the main thread is cancelled now. - A Ctrl-Z that reached cmd2 directly sent SIGSTOP and SIGCONT to the consumer's process ID even after its watcher had reaped it, when the system may already have given that ID to an unrelated process group. The pipeline's group is now signaled through the consumer only until it is reaped, and then through a shell producer that joined the group and is still running, or not at all. send_sigint() shares that lookup, _joined_group(). - Taking the terminal back at the end of a lend raised OSError once the terminal had hung up, replacing the SystemExit that SIGHUP raises. cmd2 then carried on over a dead terminal instead of exiting. The handoff now ignores the failure, as it does not matter once the terminal is gone. - The Ctrl-Z handler's terminal handoffs could raise from a signal handler into whatever the main thread was doing, once the terminal or the pipeline's group was gone. They now ignore that failure too. - ppaged() still encoded its output as UTF-8, which Windows' default pager, more, shows as mojibake, just as it did for pipes. It now uses the console's code page and replaces what the code page lacks. Each has a test that fails without its fix. --- cmd2/cmd2.py | 3 +- cmd2/utils.py | 53 +++++++++++++++++--------- tests/test_cmd2.py | 18 +++++++++ tests/test_utils.py | 90 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 146 insertions(+), 18 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 153ade6cf..54edb3ce1 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -1965,7 +1965,8 @@ def ppaged( soft_wrap=soft_wrap, **(rich_print_kwargs if rich_print_kwargs is not None else {}), ) - output_bytes = capture.get().encode("utf-8", "replace") + # As for a pipe: on Windows, the pager decodes with the console's code page. + output_bytes = capture.get().encode(utils._pipe_encoding(), "replace") # Prevent KeyboardInterrupts while in the pager. The pager application will # still receive the SIGINT since it is in the same process group as us. diff --git a/cmd2/utils.py b/cmd2/utils.py index 54a4d9b02..4e677cc42 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -701,14 +701,8 @@ def send_sigint(self) -> None: if self._proc.returncode is None: with contextlib.suppress(ProcessLookupError): group_id = os.getpgid(self._proc.pid) - # Pipelines lead their own group. A shell command that joined it, such as - # `shell sleep 100 | head -1`, can outlive the reaped consumer. Find the group - # through that command, which cmd2 has not reaped yet. Never signal the - # consumer's own ID: once its group is gone, the system may reuse it. - for producer in self._joined: - if group_id is None and producer.returncode is None: - with contextlib.suppress(ProcessLookupError): - group_id = os.getpgid(producer.pid) + if group_id is None: + group_id = self._joined_group() if group_id is None: return # Never re-signal our own group: other ProcReader callers may share it @@ -737,14 +731,31 @@ def _terminal_group(self) -> int | None: return None return self._proc.pid + def _joined_group(self) -> int | None: + """Process group of the pipeline's job, found through a shell producer that joined it. + + Pipelines lead their own group. A shell command that joined it, such as + `shell sleep 100 | head -1`, can outlive the reaped consumer. cmd2 has not reaped that + command yet, so its group is certain. Never use the consumer's own ID once it is + reaped: the system may give it to an unrelated process. + """ + for producer in self._joined: + if producer.returncode is None: + with contextlib.suppress(ProcessLookupError): + return os.getpgid(producer.pid) + return None + def _signal_pipeline(self, signum: int) -> bool: """Signal the pipeline's process group. Return whether any process could be signaled. The group may be gone, or hold only processes cmd2 may not signal: a zombie on macOS, or a program running as another user, such as sudo. """ + group_id = self._proc.pid if self._proc.returncode is None else self._joined_group() + if group_id is None: + return False try: - os.killpg(self._proc.pid, signum) + os.killpg(group_id, signum) except (ProcessLookupError, PermissionError): return False return True @@ -780,8 +791,11 @@ def suspend_job(signum: int, frame: Any) -> None: if callable(previous_handler): previous_handler(signum, frame) return - if os.tcgetpgrp(terminal_fd) == self._proc.pid: - self._set_foreground_group(terminal_fd, self._original_group) + # A signal handler must not raise into whatever the main thread was doing. A + # handoff fails only once the terminal or the pipeline's group is gone. + with contextlib.suppress(OSError): + if os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) # Ctrl-Z reached only cmd2's group, which owns the terminal between pipe writes. # Stop the pipeline too, as a shell stops its whole job. Should another thread # be relaying a stop already, it has stopped the pipeline. @@ -800,8 +814,9 @@ def suspend_job(signum: int, frame: Any) -> None: signal.signal(signal.SIGTSTP, suspend_job) finally: try: - if self._terminal_available.is_set() and os.tcgetpgrp(terminal_fd) == self._original_group: - self._set_foreground_group(terminal_fd, self._proc.pid) + with contextlib.suppress(OSError): + if self._terminal_available.is_set() and os.tcgetpgrp(terminal_fd) == self._original_group: + self._set_foreground_group(terminal_fd, self._proc.pid) finally: if own_suspension: self._signal_pipeline(signal.SIGCONT) @@ -856,8 +871,11 @@ def _lend_terminal(self) -> Iterator[None]: # to end takes the terminal back, or the producer would stop with SIGTTIN. if not self._lends: self._terminal_available.clear() - if os.tcgetpgrp(terminal_fd) == self._proc.pid: - self._set_foreground_group(terminal_fd, self._original_group) + # This fails only once the terminal is gone, as after a hangup. It must + # not replace an exception in flight, such as the SystemExit of SIGHUP. + with contextlib.suppress(OSError): + if os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) finally: signal.pthread_sigmask(signal.SIG_SETMASK, previous_mask) @@ -1297,8 +1315,9 @@ def write(self, b: Any) -> int: # normally catches BrokenPipeError. Raise here rather than signaling # asynchronously: a late signal could interrupt redirection cleanup. # Code that cmd2's SIGINT handler would not interrupt, such as that - # cleanup, gets the BrokenPipeError instead. - if self._interruptible(): + # cleanup, gets the BrokenPipeError instead. So does any thread but the + # main one, which Ctrl-C does not interrupt either. + if threading.current_thread() is threading.main_thread() and self._interruptible(): with contextlib.suppress(subprocess.TimeoutExpired): self._reader._wait_for_exit(0.2) if self._reader._proc.returncode in (-signal.SIGINT, 128 + signal.SIGINT): diff --git a/tests/test_cmd2.py b/tests/test_cmd2.py index c0295acf6..95052f337 100644 --- a/tests/test_cmd2.py +++ b/tests/test_cmd2.py @@ -3620,6 +3620,24 @@ def test_ppaged_with_pager(outsim_app, monkeypatch, chop) -> None: assert expected_cmd == popen_mock.call_args_list[0].args[0] +def test_ppaged_encodes_for_the_console(outsim_app, monkeypatch) -> None: + """As for a pipe: on Windows, the pager decodes with the console's code page, and what it lacks is replaced.""" + stdin_mock = mock.MagicMock() + stdin_mock.isatty.return_value = True + monkeypatch.setattr(outsim_app, "stdin", stdin_mock) + stdout_mock = mock.MagicMock() + stdout_mock.isatty.return_value = True + monkeypatch.setattr(outsim_app, "stdout", stdout_mock) + if not sys.platform.startswith("win") and os.environ.get("TERM") is None: + monkeypatch.setenv("TERM", "simulated") + monkeypatch.setattr("cmd2.utils._pipe_encoding", lambda: "cp437") + popen_mock = mock.MagicMock(name="Popen") + monkeypatch.setattr("subprocess.Popen", popen_mock) + outsim_app.ppaged("box ─ smile \U0001f642") + paged = popen_mock.return_value.communicate.call_args.args[0] + assert "box ─ smile ?".encode("cp437") in paged + + def test_ppaged_no_pager(outsim_app) -> None: """Since we're not in a fully-functional terminal, ppaged() will just call poutput().""" msg = "testing..." diff --git a/tests/test_utils.py b/tests/test_utils.py index 4f5693f90..ae862392a 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1073,6 +1073,96 @@ def test_pipeline_writer_cancels_interrupted_producer_but_not_protected_code(ret writer.write(b"output") +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal pipeline writer") +def test_pipeline_writer_does_not_cancel_a_worker_thread() -> None: + """Ctrl-C interrupts only the main thread. A worker writing to the pipe gets the BrokenPipeError.""" + import threading + + reader = cu.ProcReader(mock.Mock(stdout=None, stderr=None, returncode=-signal.SIGINT), sys.stdout, sys.stderr) + read_fd, write_fd = os.pipe() + os.close(read_fd) + raised = [] + + def write() -> None: + try: + writer.write(b"output") + except (BrokenPipeError, KeyboardInterrupt) as error: + raised.append(type(error)) + + with cu._PipelineWriter(write_fd, reader) as writer: + worker = threading.Thread(target=write) + worker.start() + worker.join(5) + assert raised == [BrokenPipeError] + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") +@pytest.mark.parametrize("producer", [None, "running", "reaped"]) +def test_proc_reader_signals_a_reaped_pipeline_only_through_its_producer(producer) -> None: + """Once the watcher has reaped the consumer, its ID may belong to an unrelated process group. + + Ctrl-Z stops and continues the pipeline's group only through a shell producer that joined + it and is still running. + """ + reader = cu.ProcReader(mock.Mock(pid=123, stdout=None, stderr=None, returncode=0), sys.stdout, sys.stderr) + if producer is not None: + cu.ProcReader( + mock.Mock(pid=789, stdout=None, stderr=None, returncode=None if producer == "running" else 0), + sys.stdout, + sys.stderr, + pipeline=reader, + ) + with mock.patch("os.getpgid", return_value=123) as getpgid, mock.patch("os.killpg") as killpg: + signaled = reader._signal_pipeline(signal.SIGSTOP) + if producer == "running": + assert signaled + getpgid.assert_called_once_with(789) + killpg.assert_called_once_with(123, signal.SIGSTOP) + else: + assert not signaled + killpg.assert_not_called() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_lend_keeps_the_exception_in_flight_after_a_hangup() -> None: + """After a hangup, taking the terminal back fails. That must not replace SIGHUP's SystemExit.""" + reader = cu.ProcReader(mock.Mock(pid=123, stdout=None, stderr=None, returncode=None), sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + with ( + mock.patch.object(reader, "_set_foreground_group"), + mock.patch("os.tcgetpgrp", side_effect=OSError(errno.EIO, "hung up")), + pytest.raises(SystemExit), + reader._lend_terminal(), + ): + raise SystemExit(129) + assert not reader._terminal_available.is_set() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_suspend_survives_failed_terminal_handoffs() -> None: + """A handoff fails once the terminal or the pipeline's group is gone. The Ctrl-Z handler must not raise.""" + reader = cu.ProcReader(mock.Mock(pid=123, stdout=None, stderr=None, returncode=None), sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + reader._terminal_available.set() + with ( + mock.patch("signal.getsignal", return_value=signal.SIG_DFL), + mock.patch("signal.signal") as set_handler, + mock.patch("signal.raise_signal") as stop, + mock.patch("os.killpg"), + mock.patch("os.tcgetpgrp", side_effect=[reader._proc.pid, reader._original_group]), + mock.patch("threading.Thread"), + mock.patch.object(reader, "_set_foreground_group", side_effect=OSError(errno.EPERM, "gone")), + reader._manage_terminal(), + ): + handler = set_handler.call_args.args[1] + handler(signal.SIGTSTP, None) + stop.assert_called_once_with(signal.SIGTSTP) + assert reader._job_resumed.is_set() + assert not reader._suspension_lock.locked() + + @pytest.fixture def context_flag(): return cu.ContextFlag() From 66a9713154609442d235183e32b383d955ee3c7d Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 15:13:46 -0400 Subject: [PATCH 18/20] Give joined shell producers the pipe, and detach the watcher at teardown - A shell command that joins a terminal pipeline now writes to the consumer's pipe itself instead of through the descriptor relay. It holds the terminal lent for as long as it runs, so it never needed the relay's copying thread. Creating the relay also sent every later cmd2 write down the slower lend path. Output already written to the relay, and cmd2's own buffered output, reach the consumer first. A command now joins only when self.stdout is that pipeline's writer. One whose output a command redirected elsewhere, such as to a file, has no reason to share the consumer's terminal and job. - A pipeline's watcher can outlive job control, when waiting for the pipeline fails. Job control then restored the previous SIGTSTP handler, and a stop the watcher relayed later would stop cmd2 with nothing to resume it or the pipeline. Ending job control now marks the pipeline detached under the suspension lock, after any suspension in progress, and a detached watcher relays nothing. - Cleanup: one helper, _sigttou_mask(), now blocks or unblocks SIGTTOU for the calling thread wherever that was done by hand. ppaged() takes the terminal back with the same thread-local block rather than ignoring SIGTTOU for the whole process, which was not thread-safe. _redirect_output() no longer closes the pipe's read end twice, and do_shell() tracks whether its command joined in one variable. --- cmd2/cmd2.py | 54 +++++++++++++++-------------- cmd2/utils.py | 66 ++++++++++++++++++++++++----------- tests/test_cmd2.py | 84 +++++++++++++++++++++++++++++++++++++++------ tests/test_utils.py | 66 +++++++++++++++++++++++++++++++++-- 4 files changed, 211 insertions(+), 59 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 54edb3ce1..d75f12c1e 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -1985,17 +1985,12 @@ def ppaged( # Attempt to restore terminal settings and foreground process group. if self._initial_termios_settings is not None and self.stdin.isatty(): # type: ignore[unreachable] try: # type: ignore[unreachable] - import signal import termios - # Ensure we are in the foreground process group + # Ensure we are in the foreground process group, without being stopped + # with SIGTTOU for asking from the background if hasattr(os, "tcsetpgrp") and hasattr(os, "getpgrp"): - # Ignore SIGTTOU to avoid getting stopped when calling tcsetpgrp from background - old_handler = signal.signal(signal.SIGTTOU, signal.SIG_IGN) - try: - os.tcsetpgrp(self.stdin.fileno(), os.getpgrp()) - finally: - signal.signal(signal.SIGTTOU, old_handler) + utils.ProcReader._set_foreground_group(self.stdin.fileno(), os.getpgrp()) # Restore terminal attributes if self._initial_termios_settings is not None: @@ -3421,7 +3416,6 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: if proc.returncode is not None: if cmd_pipe_proc_reader is not None: cmd_pipe_proc_reader.wait() - subproc_stdin.close() new_stdout.close() raise RedirectionError(f"Pipe process exited with code {proc.returncode} before command could run") redir_saved_state.redirecting = True @@ -5009,37 +5003,45 @@ def do_shell(self, args: argparse.Namespace) -> None: # command writes into that pipe itself rather than through self.stdout, which lends # the terminal per write. Run the command inside the pipeline's job instead, for as # long as it runs: the consumer keeps the terminal, and Ctrl-C and Ctrl-Z reach both - # processes, as they would in a shell pipeline. - # A worker thread's command stays out of it: job control relays stops to the main - # thread, and only the main thread may change signal handlers. + # processes, as they would in a shell pipeline. Only a command whose output goes to + # that pipeline joins it. A worker thread's command stays out of it too: job control + # relays stops to the main thread, and only the main thread may change signal handlers. pipeline = self._cur_pipe_proc_reader - pipeline_group = None + writer = utils._pipeline_writer_of(self.stdout) + joined_writer = None if ( pipeline is not None + and writer is not None + and writer.feeds(pipeline) and threading.current_thread() is threading.main_thread() - and not isinstance(self.stdout, utils.StdSim) # type: ignore[unreachable] + and pipeline._terminal_group is not None ): - pipeline_group = pipeline._terminal_group + joined_writer = writer # Prevent KeyboardInterrupts while in the shell process. The shell process still # receives the SIGINT: it is in our process group or in the foreground pipeline's. with self.sigint_protection, contextlib.ExitStack() as terminal_stack: - if pipeline is not None and pipeline_group is not None: - kwargs["process_group"] = pipeline_group + if pipeline is not None and joined_writer is not None: + kwargs["process_group"] = pipeline._terminal_group terminal_stack.enter_context(pipeline._lend_terminal()) while True: try: # For any stream that is a StdSim, we will use a pipe so we can capture its output. - # A command joining the pipeline is spawned inside the lend, which blocks SIGTTOU, - # and with the job's Ctrl-Z behavior. - joining = "process_group" in kwargs + # A command joining the pipeline writes to its consumer's pipe directly, after what + # cmd2 wrote before it. It is spawned inside the lend, which blocks SIGTTOU, and + # with the job's Ctrl-Z behavior. + if joined_writer is not None: + self.stdout.flush() + stdout: Any = joined_writer.producer_fileno() + else: + stdout = subprocess.PIPE if isinstance(self.stdout, utils.StdSim) else self.stdout # type: ignore[unreachable] with ( - utils._unblocked_sigttou() if joining else contextlib.nullcontext(), - utils._session_leader_job_stops() if joining else contextlib.nullcontext(), + utils._sigttou_mask(block=False) if joined_writer is not None else contextlib.nullcontext(), + utils._session_leader_job_stops() if joined_writer is not None else contextlib.nullcontext(), ): proc = subprocess.Popen( # noqa: S602 expanded_command, - stdout=subprocess.PIPE if isinstance(self.stdout, utils.StdSim) else self.stdout, # type: ignore[unreachable] + stdout=stdout, stderr=subprocess.PIPE if isinstance(sys.stderr, utils.StdSim) else sys.stderr, shell=True, **kwargs, @@ -5047,8 +5049,10 @@ def do_shell(self, args: argparse.Namespace) -> None: break except PermissionError: # The pipeline exited before the command could join its group. - if kwargs.pop("process_group", None) is None: + if joined_writer is None: raise + joined_writer = None + del kwargs["process_group"] # The retry runs in our own group, so take the terminal back from the dead # pipeline first. Its watcher left it lent, and the command would otherwise # stop with SIGTTIN on its first terminal read, with nothing to resume it. @@ -5058,7 +5062,7 @@ def do_shell(self, args: argparse.Namespace) -> None: # main thread runs Python signal handlers, and the job-control stop the pipeline's # watcher relays may wake another thread. Once the consumer and its watcher are # gone, the same wait relays the command's own stops, such as Ctrl-Z. - joined_pipeline = pipeline if "process_group" in kwargs else None + joined_pipeline = pipeline if joined_writer is not None else None proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr, pipeline=joined_pipeline) if joined_pipeline is not None: proc_reader._wait_for_exit() diff --git a/cmd2/utils.py b/cmd2/utils.py index 4e677cc42..74c58aa9d 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -591,15 +591,19 @@ def _pipe_encoding() -> str: @contextlib.contextmanager -def _unblocked_sigttou() -> Iterator[None]: - """Let a child started inside :meth:`ProcReader._lend_terminal` keep normal job control. +def _sigttou_mask(*, block: bool) -> Iterator[None]: + """Block or unblock SIGTTOU for the calling thread, then restore its signal mask. - The lend blocks SIGTTOU for its thread, and a child inherits that mask for life. Spawning - touches no terminal, so unblocking it for the spawn alone cannot stop this thread. + Blocked, SIGTTOU cannot stop a thread that changes the terminal from the background, as a + handoff does, and the mask is the thread's own, so other threads keep normal job control. + A child inherits the mask for life, though, so a child started during a lend is spawned + with SIGTTOU unblocked. Spawning touches no terminal, so that cannot stop this thread. + + :param block: True to block SIGTTOU, False to unblock it """ import signal - previous_mask = signal.pthread_sigmask(signal.SIG_UNBLOCK, {signal.SIGTTOU}) + previous_mask = signal.pthread_sigmask(signal.SIG_BLOCK if block else signal.SIG_UNBLOCK, {signal.SIGTTOU}) try: yield finally: @@ -672,6 +676,8 @@ def __init__( # already dealt with: see _suspend_with_cmd2(). self._suspension_lock = threading.Lock() self._suspensions = 0 + # Set once job control has ended, which the watcher may outlive: see _manage_terminal() + self._detached = False if terminal_fd is not None: self._original_group = os.tcgetpgrp(terminal_fd) @@ -763,13 +769,8 @@ def _signal_pipeline(self, signum: int) -> bool: @staticmethod def _set_foreground_group(terminal_fd: int, group_id: int) -> None: """Transfer the terminal without stopping this background thread with SIGTTOU.""" - import signal - - previous_mask = signal.pthread_sigmask(signal.SIG_BLOCK, {signal.SIGTTOU}) - try: + with _sigttou_mask(block=True): os.tcsetpgrp(terminal_fd, group_id) - finally: - signal.pthread_sigmask(signal.SIG_SETMASK, previous_mask) @contextlib.contextmanager def _manage_terminal(self) -> Iterator[None]: @@ -829,7 +830,17 @@ def suspend_job(signum: int, frame: Any) -> None: threading.Thread(name="pipe_job", target=self._wait_for_job, args=(terminal_fd,), daemon=True).start() yield finally: - signal.signal(signal.SIGTSTP, previous_handler) + # The watcher may outlive job control, if waiting for the pipeline failed. Without + # suspend_job(), a stop it relayed would stop cmd2 with nothing to resume either. + # So detach it first, under the lock a suspension holds, and it relays no more. Wait + # in short polls: a suspension in progress needs this thread to run suspend_job(). + while not self._suspension_lock.acquire(timeout=0.1): + pass + try: + self._detached = True + signal.signal(signal.SIGTSTP, previous_handler) + finally: + self._suspension_lock.release() @contextlib.contextmanager def _lend_terminal(self) -> Iterator[None]: @@ -839,8 +850,6 @@ def _lend_terminal(self) -> Iterator[None]: reads through input(), getpass(), or third-party libraries. Lending during writes lets an interactive consumer drain a full pipe without deadlocking. """ - import signal - terminal_fd = self._terminal_fd if terminal_fd is None or self._proc.returncode is not None: yield @@ -848,9 +857,8 @@ def _lend_terminal(self) -> Iterator[None]: # While the consumer owns the terminal, a signal handler run on this thread may still # write diagnostics to it. Block SIGTTOU for the lend only: a signal mask survives fork # and exec, so blocking it for the whole pipeline would leak into every child the - # command starts. A child started during a lend must unblock it; see _unblocked_sigttou(). - previous_mask = signal.pthread_sigmask(signal.SIG_BLOCK, {signal.SIGTTOU}) - try: + # command starts. A child started during a lend must unblock it; see _sigttou_mask(). + with _sigttou_mask(block=True): with self._terminal_lock: try: self._set_foreground_group(terminal_fd, self._proc.pid) @@ -876,8 +884,6 @@ def _lend_terminal(self) -> Iterator[None]: with contextlib.suppress(OSError): if os.tcgetpgrp(terminal_fd) == self._proc.pid: self._set_foreground_group(terminal_fd, self._original_group) - finally: - signal.pthread_sigmask(signal.SIG_SETMASK, previous_mask) def _wait_for_job(self, terminal_fd: int) -> None: """Reap a foreground pipeline and relay its stops to the outer shell's job. @@ -970,7 +976,7 @@ def _suspend_with_cmd2(self, terminal_fd: int, seen: int) -> None: while not self._suspension_lock.acquire(timeout=0.1): pass try: - if self._suspensions != seen: + if self._suspensions != seen or self._detached: return with self._terminal_lock: if os.tcgetpgrp(terminal_fd) == self._proc.pid: @@ -1265,6 +1271,20 @@ def fileno(self) -> int: self._relay = _DescriptorRelay(os.dup(super().fileno()), self._reader) return self._relay.write_fd + def feeds(self, pipeline: ProcReader) -> bool: + """Whether this is the pipe to pipeline's consumer.""" + return self._reader is pipeline + + def producer_fileno(self) -> int: + """Return the consumer's pipe itself, for a shell producer that joins the pipeline's job. + + Such a producer runs with the terminal lent for as long as it runs, so it needs no relay. + Output producers already wrote to the relay goes first, which needs that lend too. + """ + if self._relay is not None: + self._relay.flush() + return super().fileno() + def close(self) -> None: """Close the pipe. The consumer sees EOF once every producer has closed its descriptor too.""" try: @@ -1326,6 +1346,12 @@ def write(self, b: Any) -> int: return written +def _pipeline_writer_of(stream: object) -> _PipelineWriter | None: + """Return the terminal pipeline's pipe a text stream writes to, or None if it writes to anything else.""" + raw = getattr(getattr(stream, "buffer", None), "raw", None) + return raw if isinstance(raw, _PipelineWriter) else None + + class ContextFlag: """A context manager which is also used as a boolean flag value within the default sigint handler. diff --git a/tests/test_cmd2.py b/tests/test_cmd2.py index 95052f337..025c47460 100644 --- a/tests/test_cmd2.py +++ b/tests/test_cmd2.py @@ -430,6 +430,21 @@ def test_shell_manual_call(base_app) -> None: base_app.do_shell(cmd) +def pipeline_stdout(pipeline) -> tuple[io.TextIOWrapper, int]: + """Return a stdout that writes to pipeline's consumer, as cmd2 builds one, and the pipe's read end.""" + read_fd, write_fd = os.pipe() + writer = cmd2.utils._PipelineWriter(write_fd, pipeline) + return io.TextIOWrapper(io.BufferedWriter(writer), encoding="utf-8"), read_fd + + +def read_all(fd: int) -> bytes: + data = b"" + while chunk := os.read(fd, 65536): + data += chunk + os.close(fd) + return data + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") def test_shell_falls_back_to_own_group_when_pipeline_exited(base_app, tmp_path) -> None: import contextlib @@ -459,12 +474,13 @@ def popen(*args, **kwargs): spawned_while_lent.append(bool(lent)) return real_popen(*args, **kwargs) - base_app._cur_pipe_proc_reader = mock.Mock(_terminal_group=leader.pid, _lend_terminal=_lend_terminal) - with (tmp_path / "output").open("w+") as output, mock.patch("subprocess.Popen", popen): - base_app.stdout = output + pipeline = mock.Mock(_terminal_group=leader.pid, _lend_terminal=_lend_terminal) + base_app._cur_pipe_proc_reader = pipeline + base_app.stdout, read_fd = pipeline_stdout(pipeline) + with mock.patch("subprocess.Popen", popen): base_app.do_shell("echo joined") - output.seek(0) - assert output.read() == "joined\n" + base_app.stdout.close() + assert read_all(read_fd) == b"joined\n" assert base_app.last_result == 0 assert spawned_while_lent == [True, False] @@ -997,6 +1013,49 @@ def test_terminal_pipe_start_gate_uses_the_platform_shell(redirection_app, mocke assert f"exec {shell} -c less" in popen.call_args.args[0] +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") +@pytest.mark.parametrize("stdout", ["pipeline", "file"]) +def test_shell_joins_only_the_pipeline_it_writes_to(base_app, tmp_path, stdout) -> None: + """A shell command joins the pipeline's job only when its output goes to that pipeline. + + Then it writes to the consumer's pipe itself: it holds the terminal lent for as long as it + runs, so it needs no relay. A command whose output goes elsewhere, such as a file a + command redirected self.stdout to, has no reason to share the consumer's terminal. + """ + leader = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(30)"], process_group=0) + spawned = [] + real_popen = subprocess.Popen + + def popen(*args, **kwargs): + spawned.append(kwargs) + return real_popen(*args, **kwargs) + + pipeline = mock.Mock(_terminal_group=leader.pid, _lend_terminal=contextlib.nullcontext) + base_app._cur_pipe_proc_reader = pipeline + try: + if stdout == "pipeline": + base_app.stdout, read_fd = pipeline_stdout(pipeline) + writer = base_app.stdout.buffer.raw + with mock.patch("subprocess.Popen", popen): + base_app.do_shell("echo joined") + assert spawned[0]["process_group"] == leader.pid + assert isinstance(spawned[0]["stdout"], int) + assert writer._relay is None + base_app.stdout.close() + assert read_all(read_fd) == b"joined\n" + else: + with (tmp_path / "output").open("w+") as output, mock.patch("subprocess.Popen", popen): + base_app.stdout = output + base_app.do_shell("echo elsewhere") + output.seek(0) + assert output.read() == "elsewhere\n" + assert "process_group" not in spawned[0] + assert base_app.last_result == 0 + finally: + leader.kill() + leader.wait() + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") def test_shell_from_a_worker_thread_stays_out_of_the_pipeline(base_app, tmp_path) -> None: """Joining a pipeline's job means relaying stops to the main thread, and may change signal handlers. @@ -1017,14 +1076,15 @@ def popen(*args, **kwargs): spawned.append(kwargs) return real_popen(*args, **kwargs) - base_app._cur_pipe_proc_reader = mock.Mock(_terminal_group=os.getpgrp(), _lend_terminal=_lend_terminal) - with (tmp_path / "output").open("w+") as output, mock.patch("subprocess.Popen", popen): - base_app.stdout = output + pipeline = mock.Mock(_terminal_group=os.getpgrp(), _lend_terminal=_lend_terminal) + base_app._cur_pipe_proc_reader = pipeline + base_app.stdout, read_fd = pipeline_stdout(pipeline) + with mock.patch("subprocess.Popen", popen): worker = threading.Thread(target=base_app.do_shell, args=("echo worker",)) worker.start() worker.join(10) - output.seek(0) - assert output.read() == "worker\n" + base_app.stdout.close() + assert read_all(read_fd) == b"worker\n" assert "process_group" not in spawned[0] assert not lent @@ -3696,7 +3756,9 @@ def test_ppaged_terminal_restoration(outsim_app, monkeypatch, has_tcsetpgrp) -> # Verify restoration logic if has_tcsetpgrp: os.tcsetpgrp.assert_called_once_with(0, 123) - signal_mock.signal.assert_any_call(signal_mock.SIGTTOU, signal_mock.SIG_IGN) + # SIGTTOU is blocked for this thread alone, not ignored for the whole process. + signal_mock.pthread_sigmask.assert_any_call(signal_mock.SIG_BLOCK, {signal_mock.SIGTTOU}) + signal_mock.signal.assert_not_called() termios_mock.tcsetattr.assert_called_once_with(0, termios_mock.TCSANOW, dummy_settings) diff --git a/tests/test_utils.py b/tests/test_utils.py index ae862392a..893190f10 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -602,6 +602,26 @@ def lend_terminal(): os.close(read_fd) +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipes") +def test_pipeline_writer_gives_a_joining_producer_the_consumers_pipe_after_relayed_output() -> None: + """A shell producer that joins the pipeline writes to the consumer's pipe itself, after what went through the relay.""" + read_fd, write_fd = os.pipe() + writer = cu._PipelineWriter(write_fd, mock.Mock(_lend_terminal=contextlib.nullcontext)) + try: + # Something already created the relay and wrote through it. + os.write(writer.fileno(), b"relayed ") + producer_fd = writer.producer_fileno() + assert producer_fd != writer.fileno() + os.write(producer_fd, b"direct") + finally: + writer.close() + received = b"" + while chunk := os.read(read_fd, 65536): + received += chunk + os.close(read_fd) + assert received == b"relayed direct" + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipes") def test_pipeline_writer_relay_passes_consumer_exit_to_the_producer() -> None: import subprocess @@ -946,14 +966,54 @@ def test_proc_reader_direct_suspend_while_another_thread_relays_one() -> None: ): handler = set_handler.call_args.args[1] handler(signal.SIGTSTP, None) + # The relaying thread counts its suspension and releases the lock. + assert reader._suspensions == 0 + assert reader._suspension_lock.locked() + reader._suspension_lock.release() assert sent.call_args_list == [mock.call(reader._original_group, signal.SIGTSTP)] stop.assert_called_once_with(signal.SIGTSTP) - # The relaying thread counts its suspension and releases the lock. - assert reader._suspensions == 0 - assert reader._suspension_lock.locked() assert reader._job_resumed.is_set() +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_watcher_relays_nothing_once_job_control_ends() -> None: + """If waiting for the pipeline failed, its watcher may outlive job control and see a stop later. + + Relaying it would stop cmd2, whose SIGTSTP handler is back to the default, with nothing to + resume either. Job control ends only once a suspension in progress has finished. + """ + import threading + + reader = cu.ProcReader(mock.Mock(pid=123, stdout=None, stderr=None, returncode=None), sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + previous = mock.Mock() + ended = threading.Event() + # Patched below, so that _manage_terminal() starts no real watcher + real_thread = threading.Thread + with ( + mock.patch("signal.getsignal", return_value=previous), + mock.patch("signal.signal") as set_handler, + mock.patch("threading.Thread"), + ): + job_control = reader._manage_terminal() + job_control.__enter__() + # A suspension is in progress. + assert reader._suspension_lock.acquire(blocking=False) + ending = real_thread(target=lambda: (job_control.__exit__(None, None, None), ended.set())) + ending.start() + assert not ended.wait(0.3) + reader._suspension_lock.release() + assert ended.wait(5) + ending.join() + assert set_handler.call_args_list[-1] == mock.call(signal.SIGTSTP, previous) + assert reader._detached + with mock.patch("os.killpg") as killpg, mock.patch("signal.pthread_kill") as relay: + reader._suspend_with_cmd2(10, seen=reader._suspensions) + killpg.assert_not_called() + relay.assert_not_called() + + def test_proc_reader_captured_pipeline_needs_no_terminal() -> None: reader = cu.ProcReader(mock.Mock(stdout=None, stderr=None), sys.stdout, sys.stderr) with reader._manage_terminal(), reader._lend_terminal(): From e6c31f2ce905454d0401f9f532965b11cf61d565 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 15:32:49 -0400 Subject: [PATCH 19/20] Lend the relay's terminal only to a consumer that waits for it The descriptor relay lent the terminal to the consumer whenever its own pipe was full, taken as a sign that a producer was blocked on the consumer. A producer that has just finished can leave that pipe full, though. A command that ran subprocess.run(..., stdout=self.stdout) into a slow consumer then resumed while the relay still held the lend, passing the rest of the output on. The command's next terminal read came from the background: it stopped cmd2 with SIGTTIN, or failed in an orphaned session. Reproduced with a 256 KiB producer and a consumer reading 16 KiB every 150 ms, whose input() failed on every run. The relay now lends only to a consumer that is stopped, waiting for the terminal to read the keyboard or set its modes, which the pipeline's watcher now records. A consumer that never touches the terminal, however slowly it reads, is never lent it. The relay also hands the terminal back as soon as no producer is waiting, checking on every pass rather than only when a write stalls. That check replaces the two narrower release paths it had. Tests: the reviewer's scenario through a real terminal, launched from a shell and as a session leader; a relay that does not lend to a slow consumer that never reads the keyboard; and the watcher marking the consumer waiting only while it is stopped for the terminal. Each fails without this change. --- cmd2/utils.py | 33 +++++++----- tests/test_pipeline_job_control.py | 84 ++++++++++++++++++++++++++++++ tests/test_utils.py | 51 ++++++++++++++++++ 3 files changed, 155 insertions(+), 13 deletions(-) diff --git a/cmd2/utils.py b/cmd2/utils.py index 74c58aa9d..87a9c5831 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -668,6 +668,9 @@ def __init__( self._lends = 0 self._terminal_lock = threading.RLock() self._job_resumed = threading.Event() + # Set while the consumer is stopped, waiting for the terminal to read the keyboard or + # set its modes. Only then does it need to be lent the terminal to go on. + self._consumer_waiting = threading.Event() # Set by a thread that relays a pipeline's stop to cmd2's own job. Otherwise Ctrl-Z # reached cmd2 directly, and cmd2 stops the pipeline itself. self._relaying_stop = False @@ -931,6 +934,7 @@ def _watch_job(self, terminal_fd: int) -> None: if os.WSTOPSIG(status) in (signal.SIGTTIN, signal.SIGTTOU): # Command code owns the terminal between pipe writes. Defer # consumer terminal access until the next write or final wait. + self._consumer_waiting.set() while True: self._terminal_available.wait(0.1) with self._terminal_lock: @@ -940,6 +944,7 @@ def _watch_job(self, terminal_fd: int) -> None: pid, pending_status = os.waitpid(self._proc.pid, os.WNOHANG | os.WUNTRACED) if pid: if not os.WIFSTOPPED(pending_status): + self._consumer_waiting.clear() self._proc.returncode = os.waitstatus_to_exitcode(pending_status) return # A newer stop: the one a suspension would deal with. @@ -948,6 +953,7 @@ def _watch_job(self, terminal_fd: int) -> None: # Do not turn that ordinary handoff into a job suspension. if not self._terminal_available.is_set(): continue + self._consumer_waiting.clear() foreground = os.tcgetpgrp(terminal_fd) if foreground == self._proc.pid: # A group that died since is reaped by the next waitpid. @@ -1156,7 +1162,10 @@ def _unread(self) -> int: return int(struct.unpack("i", fcntl.ioctl(self._in_fd, termios.FIONREAD, b"\0" * 4))[0]) def _producer_blocked(self) -> bool: - """Whether the relay's pipe is full, which means a producer is waiting on the consumer.""" + """Whether the relay's pipe is full, which suggests a producer is waiting on the consumer. + + Only suggests: a producer that has just exited can leave the pipe full. + """ import select with self._lock: @@ -1191,12 +1200,6 @@ def _relay(self) -> None: lending = False try: while True: - with self._lock: - idle = not self._unread() - if idle and lending: - # No producer is waiting. Let command code have the terminal back. - lend.close() - lending = False # Wait for output outside the lock, then take and count it under the lock. Output # taken out of the pipe but not yet counted would look passed on to idle() and # flush(), and cmd2's next write could overtake it. @@ -1216,12 +1219,16 @@ def _relay(self) -> None: with self._lock: self._sent += count self._lock.notify_all() - elif self._producer_blocked(): - if not lending: - lend.enter_context(self._reader._lend_terminal()) - lending = True - elif lending: - # A producer that stopped writing does not need the consumer to go on. + elif not lending and self._reader._consumer_waiting.is_set() and self._producer_blocked(): + # Lend only to a consumer that is stopped waiting for the terminal. One that + # never touches it, however slowly it reads, drains the pipe without a lend. + # So a lend never outlives a producer that finishes as the consumer reads. + lend.enter_context(self._reader._lend_terminal()) + lending = True + if lending and not self._producer_blocked(): + # A producer that is not waiting does not need the consumer to go on. It may + # have finished, and command code that resumes must own the terminal: a read + # from the background would stop cmd2. lend.close() lending = False except OSError: diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index 054404b6c..fa90cd176 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -1195,3 +1195,87 @@ def wait_until(predicate): os.close(master) process.kill() process.wait(timeout=5) + + +@pytest.mark.parametrize("launcher", ["plain", "exec"]) +def test_command_reads_the_terminal_after_a_subprocess_wrote_to_a_slow_pipe(tmp_path, launcher) -> None: + """Command code resumes in the foreground once a subprocess writing to the pipe has finished. + + Its output can still be on its way to a slow consumer. The consumer never reads the keyboard, + so it is never lent the terminal: were it lent, the command's next terminal read would stop + cmd2 with SIGTTIN, or fail with EIO in an orphaned session. + """ + import pty + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + python = shlex.quote(sys.executable) + producer = tmp_path / "producer.py" + producer.write_text("import sys\nsys.stdout.write('x' * 262144)\n", encoding="utf-8") + slow = tmp_path / "slow.py" + slow.write_text("import sys, time\nwhile sys.stdin.buffer.read(16384):\n time.sleep(0.15)\n", encoding="utf-8") + application = tmp_path / "application.py" + application.write_text( + "import os, subprocess, sys\n" + "from cmd2 import Cmd\n" + "class App(Cmd):\n" + " def do_produce(self, _):\n" + f" subprocess.run([sys.executable, {str(producer)!r}], stdout=self.stdout, check=True)\n" + " os.write(2, b'ASKING\\n')\n" + " try:\n" + " os.write(2, f'ANSWER={input()}\\n'.encode())\n" + " except (OSError, EOFError) as error:\n" + " os.write(2, f'READ_FAILED {error!r}\\n'.encode())\n" + "app = App()\n" + "app.prompt = 'TEST> '\n" + "app.cmdloop()\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 15 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + data = decoder.decode(os.read(master, 65536)) + transcript += data + if "\x1b[6n" in data: + # Answer prompt-toolkit's cursor-position request as a terminal would. + os.write(master, b"\x1b[1;1R") + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") + + try: + wait_until(lambda: "OUTER> " in transcript) + prefix = "exec " if launcher == "exec" else "" + os.write(master, f"{prefix}{python} {shlex.quote(str(application))}\n".encode()) + wait_until(lambda: "TEST>" in transcript) + start = len(transcript) + os.write(master, f"produce | {python} {shlex.quote(str(slow))}\n".encode()) + # The subprocess finishes long before the consumer has read its output. + wait_until(lambda: "ASKING\r\n" in transcript[start:]) + os.write(master, b"answer\n") + wait_until( + lambda: ( + "ANSWER=answer" in transcript[start:] or "READ_FAILED" in transcript[start:] or "Stopped" in transcript[start:] + ) + ) + assert "ANSWER=answer" in transcript[start:] + finally: + os.close(master) + process.kill() + process.wait(timeout=5) diff --git a/tests/test_utils.py b/tests/test_utils.py index 893190f10..d08a2e7c4 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -471,6 +471,53 @@ def drain() -> None: assert not lends +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipes") +def test_pipeline_writer_relay_lends_only_to_a_consumer_waiting_for_the_terminal() -> None: + """A slow consumer that never reads the keyboard drains the pipe without a lend. + + A full relay pipe only suggests a waiting producer: one that has just finished can leave it + full. A lend it does not need would then outlive the producer, and command code that reads the + terminal next would do so from the background. + """ + import subprocess + import threading + + payload = b"x" * 262144 + read_fd, write_fd = os.pipe() + received = bytearray() + lends = [] + + @contextlib.contextmanager + def lend_terminal(): + lends.append(1) + yield + + def drain() -> None: + # So much slower than the producer that the relay's pipe fills up and its writes stall + while chunk := os.read(read_fd, 16384): + received.extend(chunk) + time.sleep(0.15) + + reader = mock.Mock(_lend_terminal=lend_terminal, _consumer_waiting=threading.Event()) + writer = cu._PipelineWriter(write_fd, reader) + consumer = threading.Thread(target=drain) + consumer.start() + try: + child = subprocess.run( + [sys.executable, "-c", f"import sys; sys.stdout.buffer.write(b'x' * {len(payload)})"], + stdout=writer.fileno(), + check=False, + timeout=30, + ) + assert child.returncode == 0 + finally: + writer.close() + consumer.join(30) + os.close(read_fd) + assert bytes(received) == payload + assert not lends + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipes") def test_pipeline_writer_relay_leaves_the_terminal_after_a_producer_finishes() -> None: import subprocess @@ -708,6 +755,8 @@ def test_proc_reader_resumes_terminal_access_after_handoff(stop_signal, expired_ def handoff(timeout): assert timeout == 0.1 + # The consumer is stopped, waiting for the terminal. + assert reader._consumer_waiting.is_set() if next(handoffs): reader._terminal_available.set() else: @@ -733,6 +782,8 @@ def handoff(timeout): foreground.assert_not_called() assert proc.returncode == 0 assert reader._process_done.is_set() + # Continued, it no longer waits. + assert not reader._consumer_waiting.is_set() @pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") From e39777f4f3c02d830913b3e100ba1a2dac21df9f Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 27 Sep 2026 15:41:49 -0400 Subject: [PATCH 20/20] Keep the watcher's suspension count current across its wait The pipeline's watcher read the count of finished suspensions before its blocking waitpid, to recognize a stop that a suspension had already dealt with. A whole suspension could finish during that wait, such as a direct Ctrl-Z to cmd2 followed by fg. Its SIGCONT discards a stop the wait has not collected yet, so the wait went on and kept the old count. The next genuine Ctrl-Z then looked already dealt with and was skipped, leaving the consumer stopped and cmd2 waiting for it indefinitely. Reproduced with a real child by delaying the watcher's first wait until after a stop and continue, then stopping the child again. The watcher's wait now also reports continues (WCONTINUED). A suspension always ends by continuing the pipeline, and the continue is reported even when it discarded a stop, so the watcher reads the count afresh after every change in the process's state. The wait for a shell producer that joined the pipeline does the same, and takes a continue for neither a stop nor an exit. Tests: the reproduction above, and the producer's wait handling a continue. Both fail without this change. --- cmd2/utils.py | 12 ++++++-- tests/test_utils.py | 75 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 3 deletions(-) diff --git a/cmd2/utils.py b/cmd2/utils.py index 87a9c5831..7cbe3ff12 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -926,8 +926,13 @@ def _watch_job(self, terminal_fd: int) -> None: import signal while True: + # The suspensions finished before this wait. One that finishes during it continues the + # consumer, which is reported too, even when the continue discards a stop the wait has + # not collected. So the count is read afresh after each change of state. seen = self._suspensions - _, status = os.waitpid(self._proc.pid, os.WUNTRACED) + _, status = os.waitpid(self._proc.pid, os.WUNTRACED | os.WCONTINUED) + if os.WIFCONTINUED(status): + continue if not os.WIFSTOPPED(status): self._proc.returncode = os.waitstatus_to_exitcode(status) return @@ -1022,14 +1027,15 @@ def _wait_for_producer(self, pipeline: "ProcReader", timeout: float | None) -> N deadline = None if timeout is None else time.monotonic() + timeout while self._proc.returncode is None: + # As in _watch_job(), a suspension's continue is reported, so the count stays current. seen = pipeline._suspensions try: - pid, status = os.waitpid(self._proc.pid, os.WNOHANG | os.WUNTRACED) + pid, status = os.waitpid(self._proc.pid, os.WNOHANG | os.WUNTRACED | os.WCONTINUED) except ChildProcessError: # The application ignores SIGCHLD. As Popen.wait() does, report success. self._proc.returncode = 0 return - if not pid: + if not pid or os.WIFCONTINUED(status): if deadline is not None and time.monotonic() >= deadline: raise subprocess.TimeoutExpired(self._proc.args, timeout or 0) time.sleep(0.05) diff --git a/tests/test_utils.py b/tests/test_utils.py index d08a2e7c4..bc47925ca 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -951,6 +951,81 @@ def test_proc_reader_suspend_restores_signal_handler(handler_kind, relayed) -> N previous.assert_called_once_with(signal.SIGTSTP, None) +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_watcher_relays_a_stop_after_a_suspension_it_never_saw() -> None: + """A suspension can stop and continue the consumer before the watcher collects the stop. + + The continue then discards the stop report, so the watcher never sees it. It must still + count that suspension, or it would take the next genuine stop for one the suspension + already dealt with, and leave the consumer stopped for good. + """ + import subprocess + import threading + + child = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(30)"], process_group=0) + reader = cu.ProcReader(child, sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + real_waitpid = os.waitpid + first = [] + relayed = threading.Event() + + def waitpid(pid, options): + if not first: + first.append(True) + # A whole suspension happens before the watcher's first wait, as a direct Ctrl-Z + # to cmd2 and fg would: the consumer is stopped, then continued. + os.killpg(child.pid, signal.SIGSTOP) + time.sleep(0.2) + os.killpg(child.pid, signal.SIGCONT) + reader._suspensions += 1 + return real_waitpid(pid, options) + + def relay(thread_id, signum): + relayed.set() + reader._job_resumed.set() + + try: + with ( + mock.patch("os.waitpid", side_effect=waitpid), + mock.patch("os.tcgetpgrp", return_value=456), + mock.patch.object(reader, "_set_foreground_group"), + mock.patch("signal.pthread_kill", side_effect=relay), + ): + watcher = threading.Thread(target=reader._wait_for_job, args=(10,), daemon=True) + watcher.start() + deadline = time.monotonic() + 5 + while not first and time.monotonic() < deadline: + time.sleep(0.01) + time.sleep(0.3) + # A genuine Ctrl-Z + os.killpg(child.pid, signal.SIGTSTP) + assert relayed.wait(5) + finally: + child.kill() + watcher.join(5) + assert reader._process_done.is_set() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_producer_wait_reads_a_continue_as_neither_stop_nor_exit() -> None: + """The producer's wait asks to hear of continues, to keep its suspension count current.""" + pipeline = cu.ProcReader(mock.Mock(pid=123, stdout=None, stderr=None, returncode=None), sys.stdout, sys.stderr) + proc = mock.Mock(pid=321, stdout=None, stderr=None, returncode=None) + producer = cu.ProcReader(proc, sys.stdout, sys.stderr, pipeline=pipeline) + # Linux reports a continue as 0xffff, BSDs and macOS as a stop by SIGCONT + continued = next(status for status in (0xFFFF, (signal.SIGCONT << 8) | 0x7F) if os.WIFCONTINUED(status)) + with ( + mock.patch("os.waitpid", side_effect=[(proc.pid, continued), (proc.pid, 3 << 8)]) as waitpid, + mock.patch("time.sleep"), + mock.patch.object(pipeline, "_relay_producer_stop") as relay, + ): + producer._wait_for_exit() + assert waitpid.call_args.args[1] & os.WCONTINUED + relay.assert_not_called() + assert proc.returncode == 3 + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") def test_proc_reader_suspension_waits_for_one_in_progress() -> None: """A stop reported during another suspension waits for it, then needs nothing more: that one continued the pipeline."""