Fix Windows page/pipe encoding bug and bug in how signals are handled when running a piped shell application - #1765
tleonhardt wants to merge 21 commits into
Conversation
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1765 +/- ##
==========================================
+ Coverage 99.66% 99.75% +0.08%
==========================================
Files 23 23
Lines 5974 6431 +457
==========================================
+ Hits 5954 6415 +461
+ Misses 20 16 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
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.
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.
|
Manual testing on both I'll make sure to also do manual testing on both Linux and Windows before merging. I'm also going to spend more time looking at various potential edge cases. |
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 <command>'`. 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.
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.
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.
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.
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.
- 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.
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 <producer> | <consumer>`, 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.
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.
`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.
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.
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.
…hells 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().
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.
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.
- 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.
|
I manually tested on Mac, Linux, and Windows first on
NOTE: Steps 3 through 7 were only run on POSIX oses. This is ready for review whenever anyone has time. |
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.
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.
Summary
On POSIX, a command piped to an interactive program, such as
help -v | less, now runs that program as the terminal's foreground job, as a shell pipeline does.Prior to this on
main, every pipe process starts in its own session and never receives the terminal. Ctrl-Z andfgtherefore can't suspend and resume it together with cmd2, and it can't take the keyboard the way a shell's foreground job would.This bug was discovered and a fix for it was created on the
reserved_row_toolbarbranch (#1761). It is extracted here, without the toolbar, so it can be reviewed and released on its own since it is a bug and is unrelated to the work on that branch.This PR also fixes a Windows piping bug that was introduced when we switched to all
utf-8and discovered during manual regression testing.What changes
Terminal pipelines. When a pipe's stdout is cmd2's controlling terminal and the pipe starts on the main thread, the pipe process gets its own process group. cmd2 lends it the terminal:
/bin/shgate (read -r _ || exit 1; exec "$SHELL" -c <command>) until cmd2 has made its group the foreground one and sent a newline down its stdin pipe. So the pager never runs before it owns the terminal, however late cmd2 gets to hand it over.Otherwise the terminal stays with cmd2, so command code can still read the keyboard with
read_input(),select(),input(),getpassor raw reads.Job control.
ProcReaderwatches the pipeline's group on a thread and relays its Ctrl-Z stops to cmd2's own job, so the shell suspends and resumes both together.shellproducer, or cmd2 itself) suspends the whole job, once. A consumer that ignores Ctrl-Z therefore cannot leave a stopped producer behind.shellproducer that joins it.Ctrl-C. Ctrl-C is forwarded to the pipeline without signalling cmd2's group a second time. Once the pipeline's leader has exited and been reaped, it reaches a
shellproducer that joined the group, and otherwise nothing, since the leader's ID may have been reused. A pipe write that finds the consumer ended by Ctrl-C raisesKeyboardInterrupt, except in code undersigint_protection, which getsBrokenPipeError.Subprocess producers. A subprocess given
self.stdout, such assubprocess.run(..., stdout=self.stdout)in a custom command or a!command inrun_script s.txt | less, writes to the pipe's descriptor itself, bypassing cmd2's writes. The pipe writer'sfileno()hands such producers the write end of a relay pipe. A thread passes their output on and lends the consumer the terminal while a producer is blocked on the relay's full pipe. Otherwise, command code keeps the terminal. cmd2's own writes wait for pending relay output, which keeps output in order, and a consumer's exit still reaches producers as EPIPE or SIGPIPE.shellproducers. Ashellcommand piped to such a program (shell git log | less) joins the pipeline's job for as long as it runs. If the pipeline exits first, the command runs in cmd2's group as before.Unchanged cases. These still run in their own session:
run_script s.txt | less, where the script runsbig | cat), or one whose output cmd2 captures in a pyscriptWindows is unchanged.
Testing
tests/test_pipeline_job_control.pyruns cmd2 under an interactive bash on a pseudo-terminal and reads the screen with pyte. It covers:fgshellproducersfgand interrupt cases fail. In the Ctrl-Z case,psshows the pager in its own session while the terminal's foreground group stays cmd2's.ProcReader, the pipe writer and the descriptor relay were added totests/test_utils.py, and redirection edge cases totests/test_cmd2.py.pyteis added to thedevandtestdependency groups; it was already inuv.lock. Coverage now follows the cmd2 applications these tests start (patch = ["subprocess"]).--maxschedchunk=1spreads the slow terminal tests across workers.cattakes about as long as onmain(0.57s against 0.54s).make check,make testandmake docs-testpass locally on macOS.ProcReader's terminal methods) are private, so they are not published in the API docs.Notes
help | { sleep 0.5; less; }, can be left on a cooked terminal.mainmisbehaves the same way here, so this is not a regression./tmp/doesntmatter. With--maxschedchunk=1spreading tests across workers, those tests raced on Linux and failed on Windows, where that directory does not exist.