Implement PTY (Pseudo-Terminal) support for System.Diagnostics.Process#129425
Implement PTY (Pseudo-Terminal) support for System.Diagnostics.Process#129425Copilot wants to merge 25 commits into
Conversation
… assembly - Add PseudoTerminal/PseudoTerminalOptions public classes with platform-specific impls - Add SafePseudoConsoleHandle for Windows - Wire up pal_process.c with pseudoTerminalSecondaryFd parameter (setsid + TIOCSCTTY) - Add CreatePseudoConsole/ResizePseudoConsole/ClosePseudoConsole interop (Windows) - Add OpenPseudoTerminal/ResizePseudoTerminal interop (Unix via pal_console.c) - Update ProcessStartInfo with PseudoTerminal property and validation - Update Process.cs start flow for PTY child handles and stream opening - Update SafeProcessHandle.Windows.cs with PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE - Update SafeProcessHandle.Unix.cs to pass PTY secondary fd to ForkAndExecProcess - Update ref assembly with new public API surface Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
…s PTY, add tests Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot please address my feedback
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process |
…solidation, magic number removal, posix_spawn PTY fix Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
All feedback addressed in commit 2839dc9. Key changes:
|
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address all of my unresolved feedback
… usePty, FileStream on Windows, simplify Dispose Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
All unresolved feedback addressed in the latest commit:
|
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs:187
- When PseudoTerminal is used, STARTF_USESTDHANDLES is still set, but the current code skips initializing StartupInfo.hStdInput/Output/Error. If these remain zero/invalid, CreateProcess can fail with invalid parameter. Initialize the std handles unconditionally (using the handles provided by Process.Start).
if (startInfo.PseudoTerminal is null)
{
ProcessUtils.DuplicateAsInheritableIfNeeded(stdinHandle!, ref startupInfoEx.StartupInfo.hStdInput, ref stdinRefAdded);
ProcessUtils.DuplicateAsInheritableIfNeeded(stdoutHandle!, ref startupInfoEx.StartupInfo.hStdOutput, ref stdoutRefAdded);
ProcessUtils.DuplicateAsInheritableIfNeeded(stderrHandle!, ref startupInfoEx.StartupInfo.hStdError, ref stderrRefAdded);
}
// If STARTF_USESTDHANDLES is not set, the new process will inherit the standard handles.
startupInfoEx.StartupInfo.dwFlags = Interop.Advapi32.StartupInfoOptions.STARTF_USESTDHANDLES;
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| // When PTY is used, the child handles are owned by the PseudoTerminal object. | ||
| if (startInfo.PseudoTerminal is null) | ||
| { | ||
| childErrorHandle?.Dispose(); | ||
| if (startInfo.StandardInputHandle is null) | ||
| { |
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Addressed in I updated the native CMake logic to handle
Validation:
|
This reverts commit 91231b8. Not needed for supported glibc versions. Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address my feedback
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
| private static bool? s_isCreatePseudoConsoleSupported; | ||
|
|
||
| private static bool IsCreatePseudoConsoleSupported => | ||
| s_isCreatePseudoConsoleSupported ??= !PlatformDetection.IsWindows || CanCreatePseudoConsole(); |
There was a problem hiding this comment.
This is not threadsafe (bool? updates are not atomic)
There was a problem hiding this comment.
@jkotas this PR is not ready for review yet, it's just a proof of concept (I am trying to get a better understanding of how PTYs work across various OSes and what shape of the API we could support).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs:1291
- When starting with a PTY on Windows,
child*Handlevalues can remain null (the WindowsSetChildHandlesForPseudoTerminalis a no-op), butValidateInheritedHandlesassumes all three are non-null wheninheritedHandlesis non-empty. This can turnProcessStartInfo.InheritedHandles+PseudoTerminalinto a runtimeNullReferenceExceptionin release builds. Consider guarding the validation so it only runs when all three std handles are available (or otherwise explicitly define/validate std-handle behavior for PTY starts on Windows).
if (startInfo.PseudoTerminal is not null)
{
SetChildHandlesForPseudoTerminal(startInfo.PseudoTerminal, ref childInputHandle, ref childOutputHandle, ref childErrorHandle);
}
else if (childInputHandle is null || childOutputHandle is null || childErrorHandle is null)
{
if (startInfo.StartDetached)
{
SafeFileHandle nullDeviceHandle = File.OpenNullHandle();
childInputHandle ??= nullDeviceHandle;
childOutputHandle ??= nullDeviceHandle;
childErrorHandle ??= nullDeviceHandle;
}
else if (ProcessUtils.PlatformSupportsConsole)
{
childInputHandle ??= Console.OpenStandardInputHandle();
childOutputHandle ??= Console.OpenStandardOutputHandle();
childErrorHandle ??= Console.OpenStandardErrorHandle();
}
}
ProcessStartInfo.ValidateInheritedHandles(childInputHandle, childOutputHandle, childErrorHandle, inheritedHandles);
}
| #if HAVE_POSIX_ADVISE | ||
| #include <stdlib.h> | ||
| #endif |
main PR
Description
Implements PTY support for
System.Diagnostics.Processon supported Unix and Windows platforms, including new public APIs (PseudoTerminal,PseudoTerminalOptions, andProcessStartInfo.PseudoTerminal) and native integration for process startup.Follow-up feedback updates are included:
ProcessStartInfo.PseudoTerminaldocs to state that stderr is not opened as a separate stream and is observed viaStandardOutput.ProcessStartInfo.ThrowIfInvalidto allowStandardInputEncodingandStandardOutputEncodingwhen PTY is used.Customer Impact
Enables terminal-emulator/interactive CLI scenarios (including proper PTY-backed stdin/stdout/stderr behavior) directly via
System.Diagnostics.Process, with clearer stream semantics and configurable encodings for PTY use.Regression
No known product regression is being fixed. This is primarily new functionality with follow-up correctness/behavior refinements from review feedback.
Testing
dotnet build /home/runner/work/runtime/runtime/src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -p:Configuration=Releasedotnet build /t:test /home/runner/work/runtime/runtime/src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj -p:Configuration=Release./build.sh clr+libs -rc release./build.sh clr+libs+host -rc release -lc releaseSystem.Diagnostics.Process.Tests: 652 passed, 0 failed, 6 skipped on validated Unix target.Risk
Moderate. The change introduces new API surface and platform-specific PTY plumbing, but includes validation, platform guards, and targeted test coverage (including follow-up tests for encoding behavior) to reduce risk.
Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.