Skip to content

Remove redundant P/Invoke-s in S.D.Process on Apple platforms #54273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jun 21, 2021
Merged

Remove redundant P/Invoke-s in S.D.Process on Apple platforms #54273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6599d53
Remove P/Invoke to SystemNative_ConfigureTerminalForChildProcess whic…
MaximLipnin Jun 16, 2021
dc7f472
Address the feedback based on the ifdef approach
MaximLipnin Jun 17, 2021
ed331c6
Merge branch 'main' into remove_redundant_pinvokes_in_S.D.Process
MaximLipnin Jun 17, 2021
b52c7f0
Add IsiOSLike property
MaximLipnin Jun 17, 2021
e3b59d6
Use a partial method approach
MaximLipnin Jun 17, 2021
a455e85
Address the feedback
MaximLipnin Jun 17, 2021
f24f72d
Merge branch 'main' into remove_redundant_pinvokes_in_S.D.Process
MaximLipnin Jun 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
<PropertyGroup>
<GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetsAnyOS)' == 'true'">SR.Process_PlatformNotSupported</GeneratePlatformNotSupportedAssemblyMessage>
</PropertyGroup>
<PropertyGroup>
<IsiOSLike Condition="'$(TargetsMacCatalyst)' == 'true' or '$(TargetsiOS)' == 'true' or '$(TargetstvOS)' == 'true'">true</IsiOSLike>
</PropertyGroup>
<ItemGroup Condition="'$(TargetsAnyOS)' != 'true'">
<Compile Include="Microsoft\Win32\SafeHandles\SafeProcessHandle.cs" />
<Compile Include="System\Collections\Specialized\DictionaryWrapper.cs" />
Expand Down Expand Up @@ -234,8 +237,6 @@
Link="Common\Interop\Unix\Interop.GetHostName.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.SysConf.cs"
Link="Common\Interop\Unix\Interop.SysConf.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.ConfigureTerminalForChildProcess.cs"
Link="Common\Interop\Unix\Interop.ConfigureTerminalForChildProcess.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.ForkAndExecProcess.cs"
Link="Common\Interop\Unix\Interop.ForkAndExecProcess.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.GetGroupList.cs"
Expand Down Expand Up @@ -267,6 +268,11 @@
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Access.cs"
Link="Common\Interop\Unix\System.Native\Interop.Access.cs" />
</ItemGroup>
<ItemGroup Condition=" '$(TargetsUnix)' == 'true' and '$(IsiOSLike)' != 'true'">
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.ConfigureTerminalForChildProcess.cs"
Link="Common\Interop\Unix\Interop.ConfigureTerminalForChildProcess.cs" />
<Compile Include="System\Diagnostics\Process.Unix.ConfigureTerminalForChildProcesses.cs" />
</ItemGroup>
<ItemGroup Condition=" '$(TargetsLinux)' == 'true'">
<Compile Include="System\Diagnostics\Process.Linux.cs" />
<Compile Include="System\Diagnostics\ProcessManager.Linux.cs" />
Expand Down
36 changes: 36 additions & 0 deletions ...nostics.Process/src/System/Diagnostics/Process.Unix.ConfigureTerminalForChildProcesses.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Threading;

namespace System.Diagnostics
{
public partial class Process
{
private static int s_childrenUsingTerminalCount;

static partial void ConfigureTerminalForChildProcessesInner(int increment)
{
Debug.Assert(increment != 0);

int childrenUsingTerminalRemaining = Interlocked.Add(ref s_childrenUsingTerminalCount, increment);
if (increment > 0)
{
Debug.Assert(s_processStartLock.IsReadLockHeld);

// At least one child is using the terminal.
Interop.Sys.ConfigureTerminalForChildProcess(childUsesTerminal: true);
}
else
{
Debug.Assert(s_processStartLock.IsWriteLockHeld);

if (childrenUsingTerminalRemaining == 0)
{
// No more children are using the terminal.
Interop.Sys.ConfigureTerminalForChildProcess(childUsesTerminal: false);
}
}
}
}
}
24 changes: 3 additions & 21 deletions src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ public partial class Process : IDisposable
private static volatile bool s_initialized;
private static readonly object s_initializedGate = new object();
private static readonly ReaderWriterLockSlim s_processStartLock = new ReaderWriterLockSlim();
private static int s_childrenUsingTerminalCount;

/// <summary>
/// Puts a Process component in state to interact with operating system processes that run in a
Expand Down Expand Up @@ -1051,26 +1050,9 @@ private static void OnSigChild(int reapAll)
/// </summary>
internal static void ConfigureTerminalForChildProcesses(int increment)
Copy link
Member

Choose a reason for hiding this comment

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

I do not think you need the ConfigureTerminalForChildProcesses / ConfigureTerminalForChildProcessesInner split. ConfigureTerminalForChildProcesses can be the partial method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ConfigureTerminalForChildProcesses method has a modifier so there can be error CS8795: Partial method 'Process.ConfigureTerminalForChildProcesses(int)' must have an implementation part because it has accessibility modifiers.
Removing the modifier leads to src/System/Diagnostics/ProcessWaitState.Unix.cs(575,33): error CS0122: 'Process.ConfigureTerminalForChildProcesses(int)' is inaccessible due to its protection level

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, that's annoying.

{
Debug.Assert(increment != 0);

int childrenUsingTerminalRemaining = Interlocked.Add(ref s_childrenUsingTerminalCount, increment);
if (increment > 0)
{
Debug.Assert(s_processStartLock.IsReadLockHeld);

// At least one child is using the terminal.
Interop.Sys.ConfigureTerminalForChildProcess(childUsesTerminal: true);
}
else
{
Debug.Assert(s_processStartLock.IsWriteLockHeld);

if (childrenUsingTerminalRemaining == 0)
{
// No more children are using the terminal.
Interop.Sys.ConfigureTerminalForChildProcess(childUsesTerminal: false);
}
}
ConfigureTerminalForChildProcessesInner(increment);
}

static partial void ConfigureTerminalForChildProcessesInner(int increment);
}
}