Fix subprocess and Update subprocess from Python 3.13.11 by youknowone · Pull Request #6583 · RustPython/RustPython

youknowone

📝 Walkthrough

Walkthrough

This PR implements missing functionality in subprocess pre-exec setup (umask and signal restoration), makes POSIX spawn environment optional with automatic inheritance, adds a runtime buffering warning for edge cases, and expands Windows threading constants.

Changes

Cohort / File(s)	Summary
Pre-exec subprocess setup `crates/stdlib/src/posixsubprocess.rs`	Replaces TODOs with runtime actions: sets process umask via `libc::umask` when `child_umask >= 0`; restores SIGPIPE and SIGXFSZ to default handlers via `libc::signal` when `restore_signals` is true.
POSIX spawn environment `crates/vm/src/stdlib/posix.rs`	Changes `PosixSpawnArgs.env` from mandatory to `Option<ArgMapping>`. When env is provided, uses existing `envp_from_dict` logic. When None, derives environment from current process via `env::vars_os` with null-byte validation, falling back to automatic inheritance.
I/O buffering validation `crates/vm/src/stdlib/io.rs`	Adds runtime warning when line buffering (buffering=1) is requested in binary mode, emitted during `io_open` after `isatty` check. No control-flow impact on subsequent buffering behavior.
Windows threading constants `crates/vm/src/stdlib/winapi.rs`	Adds public constants `STARTF_FORCEOFFFEEDBACK` and `STARTF_FORCEONFEEDBACK` to Windows Threading imports, repositioning existing STARTF constants.

Sequence Diagram(s)

sequenceDiagram
    participant Child as Child Process Setup
    participant Umask as libc::umask
    participant Signal as libc::signal
    participant Exec as exec/setsid/pgid
    
    Child->>Child: Check child_umask
    alt child_umask >= 0
        Child->>Umask: Set process umask
        Umask-->>Child: umask applied
    end
    
    Child->>Child: Check restore_signals
    alt restore_signals == true
        Child->>Signal: Restore SIGPIPE to default
        Signal-->>Child: Handler set
        Child->>Signal: Restore SIGXFSZ to default
        Signal-->>Child: Handler set
    end
    
    Child->>Exec: Proceed with exec/setsid/pgid

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Fix posix tests #5811: Modifies POSIX spawn environment handling in posix.rs, including env field construction and envp conversion logic.
impl preexec_fn #6479: Implements pre-exec setup enhancements in posixsubprocess.rs, including preexec_fn handling alongside umask and signal restoration.
windows umask, win32_xstat_slow_impl, fake EXT_SUFFIX #6340: Adds umask-related runtime behavior (Windows _umask wrapper), complementing the libc umask calls in this PR.

Suggested reviewers

ShaharNaveh

Poem

🐰 Whiskers twitch with POSIX glee,
Umasks set and signals free,
Environments now born anew—
From parent's breath, the child's debut.
Windows constants dance in place,
A buffering warning, just in case! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title references subprocess fixes and updates from Python 3.13.11, which aligns with the actual changes affecting subprocess functionality across multiple files (posixsubprocess.rs, posix.rs, and updates to Windows constants).

✨ Finishing touches

📝 Generate docstrings

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2f4ca50 and 5c5cacc.

⛔ Files ignored due to path filters (3)

Lib/subprocess.py is excluded by !Lib/**
Lib/test/test_file.py is excluded by !Lib/**
Lib/test/test_subprocess.py is excluded by !Lib/**

📒 Files selected for processing (4)

crates/stdlib/src/posixsubprocess.rs
crates/vm/src/stdlib/io.rs
crates/vm/src/stdlib/posix.rs
crates/vm/src/stdlib/winapi.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

crates/vm/src/stdlib/io.rs
crates/vm/src/stdlib/posix.rs
crates/stdlib/src/posixsubprocess.rs
crates/vm/src/stdlib/winapi.rs

🧬 Code graph analysis (1)

crates/vm/src/stdlib/io.rs (1)

crates/vm/src/warn.rs (1)

warn (83-94)

🔇 Additional comments (5)

crates/stdlib/src/posixsubprocess.rs (1)

323-331: Child umask and signal restoration logic looks correct

Applying libc::umask only when child_umask >= 0 and resetting SIGPIPE/SIGXFSZ to SIG_DFL under restore_signals in the child pre-exec path matches expected POSIX subprocess semantics and is safe to do here; ignoring the return values is acceptable for these calls.

crates/vm/src/stdlib/io.rs (1)

4492-4503: RuntimeWarning on buffering=1 in binary mode is well-placed

The added warning for buffering == 1 with EncodeMode::Bytes correctly highlights that line buffering isn’t meaningful in binary mode, while leaving the existing behavior (fallback to DEFAULT_BUFFER_SIZE and unchanged line_buffering semantics) intact.

crates/vm/src/stdlib/winapi.rs (1)

62-68: Additional STARTF_ threading constants are correctly exposed*

Importing STARTF_FORCEOFFFEEDBACK and STARTF_FORCEONFEEDBACK alongside the existing Threading constants under #[pyattr] cleanly extends the _winapi surface without affecting existing behavior.

crates/vm/src/stdlib/posix.rs (2)

1518-1518: LGTM: Optional environment parameter.

Making the env parameter optional is the correct approach for allowing subprocess spawning to inherit the current environment when None is passed, which aligns with Python's subprocess module behavior.

1681-1696: Well-implemented environment inheritance.

The logic correctly handles env=None by inheriting the current process environment via env::vars_os(), which matches Python's subprocess module behavior. The null-byte validation and error handling are appropriate.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}