Skip to content

Avoid encoding unchanged process environments on Unix - #130790

Closed
adamsitnik with Copilot wants to merge 19 commits into
mainfrom
copilot/task-9141961-210716005-d2705bb8-de74-4115-b09d-ccd781cd624d
Closed

Avoid encoding unchanged process environments on Unix#130790
adamsitnik with Copilot wants to merge 19 commits into
mainfrom
copilot/task-9141961-210716005-d2705bb8-de74-4115-b09d-ccd781cd624d

Conversation

Copilot AI commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Unix process startup now passes a null environment pointer when ProcessStartInfo.Environment remains untouched, avoiding dictionary creation and UTF-8 encoding.

  • Native process creation resolves null to environ.
  • Explicit ProcessStartInfo environment changes retain existing encoding behavior.
  • Process-wide environment mutations continue using a managed snapshot to preserve semantics.
  • Added coverage for lazy environment inheritance.

Note

This description was generated with GitHub Copilot.

Copilot AI and others added 2 commits July 15, 2026 14:36
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI self-assigned this Jul 15, 2026
Copilot AI review requested due to automatic review settings July 15, 2026 15:01
Copilot AI removed the request for review from Copilot July 15, 2026 15:01
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@adamsitnik adamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment thread src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Environment.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Environment.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Environment.cs
Comment thread src/native/libs/System.Native/pal_process.c Outdated
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot July 15, 2026 16:07
Copilot AI requested a review from adamsitnik July 15, 2026 16:08

@adamsitnik adamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment thread src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs Outdated
Comment thread src/coreclr/pal/src/misc/environ.cpp Outdated
@adamsitnik

Copy link
Copy Markdown
Member

@EgorBot -osx_arm64 -linux_x64

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Diagnostics;

BenchmarkSwitcher.FromAssembly(typeof(Benchmarks).Assembly).Run(args);

[MemoryDiagnoser]
public class Benchmarks
{
    private static ProcessStartInfo s_startProcessStartInfo = new ProcessStartInfo()
    {
        FileName = "whoami", // exists on both Windows and Unix, and has very short output
        RedirectStandardOutput = true // avoid visible output
    };

    private Process? _startedProcess;

    [Benchmark]
    public void Start()
    {
        _startedProcess = Process.Start(s_startProcessStartInfo)!;
    }

    [IterationCleanup(Target = nameof(Start))]
    public void CleanupStart()
    {
        if (_startedProcess != null)
        {
            _startedProcess.WaitForExit();
            _startedProcess.Dispose();
            _startedProcess = null;
        }
    }

    [Benchmark]
    public void StartAndWaitForExit()
    {
        using (Process p = Process.Start(s_startProcessStartInfo)!)
        {
            p.WaitForExit();
        }
    }
}

Comment thread src/native/libs/System.Native/pal_environment.c Outdated
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot July 15, 2026 16:55

Copilot AI commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

@copilot address my feedback

Addressed the remaining test feedback in 73ed42b7; targeted System.Diagnostics.Process tests pass (637 total, 0 failures).

Copilot AI requested a review from adamsitnik July 15, 2026 17:01

@adamsitnik adamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment thread src/coreclr/pal/src/misc/environ.cpp
Comment thread src/libraries/Common/src/Interop/Unix/System.Native/Interop.SetEnv.cs Outdated
Comment thread src/native/libs/System.Native/entrypoints.c
Comment thread src/native/libs/System.Native/pal_environment.c
Comment thread src/native/libs/System.Native/pal_environment.h
Comment thread src/native/libs/System.Native/pal_environment.m
Comment thread src/libraries/System.Private.CoreLib/src/System/Environment.Variables.Unix.cs Outdated
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

{
public static partial class Environment
{
internal static bool HasEnvironmentVariablesBeenModified => throw new PlatformNotSupportedException();
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

{
public static partial class Environment
{
private static volatile bool s_hasEnvironmentVariablesBeenModified;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to other reviewers: this logic is part of Environment.Unix.cs rather than Environment.Variables.Unix.cs because, the latter is used only for Mono (or at least not for CLR):

<Compile Include="$(MSBuildThisFileDirectory)System\Environment.Variables.Unix.cs" Condition="'$(FeatureCoreCLR)' != 'true'" />

As it seems that for CLR we compile Environment.Variables.Windows.cs on UNIX!!!

<ItemGroup Condition="'$(TargetsWindows)' == 'true' or '$(FeatureCoreCLR)'=='true'">
<Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.GetEnvironmentVariable.cs">
<Link>Common\Interop\Windows\Kernel32\Interop.GetEnvironmentVariable.cs</Link>
</Compile>
<Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.GetEnvironmentStrings.cs">
<Link>Common\Interop\Windows\Kernel32\Interop.GetEnvironmentStrings.cs</Link>
</Compile>
<Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.FreeEnvironmentStrings.cs">
<Link>Common\Interop\Windows\Kernel32\Interop.FreeEnvironmentStrings.cs</Link>
</Compile>
<Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.SetEnvironmentVariable.cs">
<Link>Common\Interop\Windows\Kernel32\Interop.SetEnvironmentVariable.cs</Link>
</Compile>
<Compile Include="$(MSBuildThisFileDirectory)System\Environment.Variables.Windows.cs" />
</ItemGroup>

@adamsitnik

Copy link
Copy Markdown
Member

@EgorBot -osx_arm64 -linux_x64

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Diagnostics;

BenchmarkSwitcher.FromAssembly(typeof(Benchmarks).Assembly).Run(args);

[MemoryDiagnoser]
public class Benchmarks
{
    private static ProcessStartInfo s_startProcessStartInfo = new ProcessStartInfo()
    {
        FileName = "whoami", // exists on both Windows and Unix, and has very short output
        RedirectStandardOutput = true // avoid visible output
    };

    private Process? _startedProcess;

    [Benchmark]
    public void Start()
    {
        _startedProcess = Process.Start(s_startProcessStartInfo)!;
    }

    [IterationCleanup(Target = nameof(Start))]
    public void CleanupStart()
    {
        if (_startedProcess != null)
        {
            _startedProcess.WaitForExit();
            _startedProcess.Dispose();
            _startedProcess = null;
        }
    }

    [Benchmark]
    public void StartAndWaitForExit()
    {
        using (Process p = Process.Start(s_startProcessStartInfo)!)
        {
            p.WaitForExit();
        }
    }
}

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

adamsitnik and others added 2 commits July 17, 2026 16:58
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@adamsitnik

Copy link
Copy Markdown
Member

@EgorBot -osx_arm64 -linux_x64

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Diagnostics;

BenchmarkSwitcher.FromAssembly(typeof(Benchmarks).Assembly).Run(args);

[MemoryDiagnoser]
public class Benchmarks
{
    private static ProcessStartInfo s_startProcessStartInfo = new ProcessStartInfo()
    {
        FileName = "whoami", // exists on both Windows and Unix, and has very short output
        RedirectStandardOutput = true // avoid visible output
    };

    private Process? _startedProcess;

    [Benchmark]
    public void Start()
    {
        _startedProcess = Process.Start(s_startProcessStartInfo)!;
    }

    [IterationCleanup(Target = nameof(Start))]
    public void CleanupStart()
    {
        if (_startedProcess != null)
        {
            _startedProcess.WaitForExit();
            _startedProcess.Dispose();
            _startedProcess = null;
        }
    }

    [Benchmark]
    public void StartAndWaitForExit()
    {
        using (Process p = Process.Start(s_startProcessStartInfo)!)
        {
            p.WaitForExit();
        }
    }
}

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comment on lines +283 to +288
[ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
[InlineData("not_null", true)]
[InlineData("not_null", false)]
[InlineData("null", false)]
[InlineData("null", true)]
public void SeesEnvironmentVariableSetBeforeStart(string value, bool modifyParent)
@adamsitnik

Copy link
Copy Markdown
Member

@EgorBot -osx_arm64 -linux_x64

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Engines;
using BenchmarkDotNet.Running;
using Microsoft.Win32.SafeHandles;
using System;
using System.Diagnostics;
using System.IO;
using System.Text;
using System.Threading.Tasks;

BenchmarkSwitcher.FromAssembly(typeof(DefaultEnvVars).Assembly).Run(args);

[MemoryDiagnoser]
[SimpleJob(launchCount:5, warmupCount:1, iterationCount:100, invocationCount: 1), JitTiering(JitTieringMode.Skip)]
public class DefaultEnvVars
{
    private static readonly SafeFileHandle s_nullHandle = File.OpenNullHandle();
    private static ProcessStartInfo s_startProcessStartInfo = new ProcessStartInfo()
    {
        FileName = "whoami", // exists on both Windows and Unix, and has very short output
        StandardInputHandle = s_nullHandle,
        StandardOutputHandle = s_nullHandle,
        StandardErrorHandle = s_nullHandle,
    };

    private Process? _startedProcess;

    [Benchmark]
    public void Start() => _startedProcess = Process.Start(s_startProcessStartInfo)!;

    [IterationCleanup(Target = nameof(Start))]
    public void CleanupStart()
    {
        if (_startedProcess != null)
        {
            _startedProcess.WaitForExit();
            _startedProcess.Dispose();
            _startedProcess = null;
        }
    }

    [Benchmark]
    public int Run() => Process.Run(s_startProcessStartInfo).ExitCode;

    [GlobalCleanup]
    public void Cleanup() => s_nullHandle.Dispose();
}

@adamsitnik

Copy link
Copy Markdown
Member

@jkotas I've converted it to a draft again, as the benchmarks show regressions in some cases and I simply need to run them locally and profile if needed. Please ignore this PR for now.

@adamsitnik

adamsitnik commented Aug 25, 2026

Copy link
Copy Markdown
Member

@EgorBot -osx_arm64 -linux_x64

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Engines;
using BenchmarkDotNet.Running;
using Microsoft.Win32.SafeHandles;
using System;
using System.Diagnostics;
using System.IO;
using System.Text;
using System.Threading.Tasks;

BenchmarkSwitcher.FromAssembly(typeof(DefaultEnvVars).Assembly).Run(args);

[MemoryDiagnoser]
public class DefaultEnvVars
{
    private static readonly SafeFileHandle s_nullHandle = File.OpenNullHandle();
    private static ProcessStartInfo s_startProcessStartInfo = new ProcessStartInfo()
    {
        FileName = "whoami", // exists on both Windows and Unix, and has very short output
        StandardInputHandle = s_nullHandle,
        StandardOutputHandle = s_nullHandle,
        StandardErrorHandle = s_nullHandle,
    };

    private Process? _startedProcess;

    [Benchmark]
    public void Start() => _startedProcess = Process.Start(s_startProcessStartInfo)!;

    [IterationCleanup(Target = nameof(Start))]
    public void CleanupStart()
    {
        if (_startedProcess != null)
        {
            _startedProcess.WaitForExit();
            _startedProcess.Dispose();
            _startedProcess = null;
        }
    }

    [Benchmark]
    public int Run() => Process.Run(s_startProcessStartInfo).ExitCode;

    [GlobalCleanup]
    public void Cleanup() => s_nullHandle.Dispose();
}

@adamsitnik

Copy link
Copy Markdown
Member

@EgorBot -osx_arm64 -linux_x64

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Engines;
using BenchmarkDotNet.Running;
using Microsoft.Win32.SafeHandles;
using System;
using System.Diagnostics;
using System.IO;
using System.Text;
using System.Threading.Tasks;

BenchmarkSwitcher.FromAssembly(typeof(DefaultEnvVars).Assembly).Run(args);

[MemoryDiagnoser]
public class DefaultEnvVars
{
    private static readonly SafeFileHandle s_nullHandle = File.OpenNullHandle();
    private static ProcessStartInfo s_startProcessStartInfo = new ProcessStartInfo()
    {
        FileName = "whoami", // exists on both Windows and Unix, and has very short output
        StandardInputHandle = s_nullHandle,
        StandardOutputHandle = s_nullHandle,
        StandardErrorHandle = s_nullHandle,
    };

    private Process? _startedProcess;

    [Benchmark]
    public void Start() => _startedProcess = Process.Start(s_startProcessStartInfo)!;

    [IterationCleanup(Target = nameof(Start))]
    public void CleanupStart()
    {
        if (_startedProcess != null)
        {
            _startedProcess.WaitForExit();
            _startedProcess.Dispose();
            _startedProcess = null;
        }
    }

    [Benchmark]
    public int Run() => Process.Run(s_startProcessStartInfo).ExitCode;

    [GlobalCleanup]
    public void Cleanup() => s_nullHandle.Dispose();
}

@adamsitnik

Copy link
Copy Markdown
Member

The benchmarks don't show an obvious win. There seems to be always a regression on macOS and sometimes a win on Linux. So the benchmark can be simply bimodal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Diagnostics.Process linkable-framework Issues associated with delivering a linker friendly framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants