Bad performance: launching processes in parallel on Win32 / Windows

[bernd5]

I would really like to launch multiple worker-processes in parallel with minimal blocking.
My code is heavily using async / await. Unfortunately the overall perfomance is not as good as I would expect.

For me the perfomance killer seems to be line 479 lock (s_createProcessLock) which disallows effectively parallel process launching.

Why is the lock static?

[danmoseley]

I do not know why that lock is static. It is so in the internal code back to at least 2010 after which I do not currently know how to find history.

It's only used for Windows.

@stephentoub do you know?

[bernd5]

any news?

[stephentoub]

do you know?

I know there was a reason for it, as this question has come up before and there was discussion about it, but I just spent a while searching and can't find remnants of those discussions.

[bernd5]

I can't see any reason, too.

I have checked the final assembly code (including all other parts). The variable s_createProcessLock is only used in StartWithCreateProcess. But there seems to be no static data to protect.
The most simple solution would be just to remove static from that lock object.

Via blame I can see it was moved 5 years ago to winodows only in a very huge commit, which refactored the Process library to have a PAL. Before this it was in the "initial commit" - so in this repository there is no further history...

For my project I could access win api directly via PInvoke, but that is something I would really like to avoid because it breaks portability...

[danmoseley]

@jkotas do you recall any context?

[jkotas]

This lock is trying fix the problem with the inheritable stdin/stdout/stderr handles. We need to make sure that only one process inherits them. If the redirected handle gets inherited by multiple processes starting in parallel by accident, bad things happen.

For the record, here is the .NET Framework change that introduced the lock:

change 2624613 edit on 2007/06/21 11:19:03

	DevDiv Bugs #111431: Port for DTS - SRX070508601750 - Process.WaitForExit
	Method is broken
	Title: DevDiv Bugs #111431: Port for DTS - SRX070508601750 - Process.WaitForExit
	Method is broken
	Bugs fixed:
	111431
	Issue description:
	DevDiv Bugs #111431: Port for DTS - SRX070508601750 - Process.WaitForExit
	Method is broken
	Change description:
	Take a lock during process creation to ensure only one process is
	created while handles which can be duplicated into spawned processes are
	open

[jkotas]

The fix to remove this lock is to use UpdateProcThreadAttribute and PROC_THREAD_ATTRIBUTE_HANDLE_LIST to explicitly specify the list of handles to be inherited by the process.

[jkotas]

While you are on it, you can also replace DuplicateHandle here:

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs

Line 862 in 4f9ae42

if (!Interop.Kernel32.DuplicateHandle(currentProcHandle,

with just SetHandleInformation. Patching the existing handle should have slightly better performance than creating new ones.

[stephentoub]

he fix to remove this lock is to use UpdateProcThreadAttribute and PROC_THREAD_ATTRIBUTE_HANDLE_LIST to explicitly specify the list of handles to be inherited by the process.

Won't that exclude from being inherited other handles in the process that should have been, e.g. any created by AnonymousPipeServerStream?

[jkotas]

Ok, I have not realized that other components may be depending on the CreateProcess always being called with bInheritHandles: TRUE. If it is the case, this fix requires to either create a new API to opt into CreateProcess that scales well (but limits handles that get inherited) and/or accept some breaking changes.

[jkotas]

Related: https://github.com/dotnet/corefx/issues/306

[bernd5]

In my case I have explicitly redirected all handles via RedirectStandardOutput / RedirectStandardError / RedirectStandardInput. So the API should create some Pipes.

Is it still required to do some locking?

[stephentoub]

Is it still required to do some locking?

You don't need to do any, but the implementation still does, yes. Let's say you launch two processes concurrently. If one of them redirects standard input/output/error, that means it's opening new handles to be inherited into the child process to use for communication between the parent and child. But those handles could also end up getting inherited by the other concurrently started child process.

[bernd5]

Up to now I dont got the point.
I understand that some synchronization would be required without redirection. Otherwise you would get strange results (mixed output / or buffer corruption...).
But if all inputs/outputs are redirected by inheriting these private redirect pipe handles than it should just work in parallel (or why not??). They can't be accessed by a second CSharp-Process.

=> The lock should be aquired only if not all standard inputs / outputs are redirected.

If it is not possible, the locked region should be smaller (e.g. getting the working directory ...).

[stephentoub]

But if all inputs/outputs are redirected by inheriting these private redirect pipe handles than it should just work in parallel (or why not??). They can't be accessed by a second CSharp-Process.

When it goes to start a child process, it creates an anonymous pipe with an inheritable handle for the child: the child will inherit that handle, which is how it communicates with the parent. If any other process is started at the same time, it would also inherit that same handle that wasn't intended for it, such that both child processes inherited the same handle. That's the problem the lock is avoiding. Jan's suggestion was to explicitly tell the child process which handle to inherit (i.e. "inherit these and only these"), which would address this, because then an unrelated child wouldn't inherit the other's handle, but that's problematic for other reasons, because other parts of the system were designed with the expectation that their handles would be inherited by children.

If it is not possible, the locked region should be smaller (e.g. getting the working directory ...).

It's not about the working directly. It's about making the whole region around handle creation / process starting / handle destruction atomic.

[bernd5]

Thanks for your fast response.

Thats what I don't understand:

If any other process is started at the same time, it would also inherit that same handle that wasn't intended for it, such that both child processes inherited the same handle.

As far as I can see these handles are submitted explicitly in the startup info:

startupInfo.hStdInput = childInputPipeHandle.DangerousGetHandle();
startupInfo.hStdOutput = childOutputPipeHandle.DangerousGetHandle();
startupInfo.hStdError = childErrorPipeHandle.DangerousGetHandle();

The pipes (and their handles) are created during that call (just true if redirected). So there should be no sharing in this case.

[stephentoub]

As far as I can see these handles are submitted explicitly in the startup info

They are; that's how the child process knows which handle to use for what. But in order to actually use the handles, it needs access to them, so the handles are created as inheritable. If the handles weren't inheritable, the handle values passed to the child would be meaningless. It'd be like me saying to you "please read from the email I just emailed you" but then not actually sending you an email.

Look in the code just below the three lines you copied:

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs

Lines 519 to 523 in 4d3ec15

	startupInfo.hStdInput = childInputPipeHandle.DangerousGetHandle();
	startupInfo.hStdOutput = childOutputPipeHandle.DangerousGetHandle();
	startupInfo.hStdError = childErrorPipeHandle.DangerousGetHandle();
	startupInfo.dwFlags = Interop.Advapi32.StartupInfoOptions.STARTF_USESTDHANDLES;

then see the docs for that STARTF_USESTDHANDLES option:
https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/ns-processthreadsapi-startupinfoa
"The hStdInput, hStdOutput, and hStdError members contain additional information. If this flag is specified when calling one of the process creation functions, the handles must be inheritable and the function's bInheritHandles parameter must be set to TRUE. For more information, see Handle Inheritance."

[bernd5]

That was fast.
I have already read these articles. As far as I understood. The flag "bInheritHandle" makes parent handles accessable from the child process. That's fine.

So we could use for example file handles in the child application which could be submitted via Window Message.

But if
startInfo.RedirectStandardInput && startInfo.RedirectStandardOutput && startInfo.RedirectStandardError == true

Then there is no conflict with other starting processes.

[stephentoub]

Then there is no conflict with other starting processes.

There is.

If you could guarantee that every child process the parent creates has all of its RedirectXx properties set to false, then the lock wouldn't be necessary.

But the moment it's possible that even just one child process gets created with one of its RedirectXx properties set to true, the lock is needed. In that case, the parent process will be creating a handle to pass to the child. It makes that handle inheritable so that the child will have access to it, and it stores the handle value into the hStdXx field so that the child will know which handle it should use for that purpose. But because that handle was made inheritable, it's inheritable not just to that child, but to any other child created while the handle is alive. As such, Process needs to make sure that no other child process is created while that handle is alive... hence the lock.

[bernd5]

I would say just the opposite.

The child gets handles to the pipes and not to the normal standard XXX if RedirectXXX is true.
These pipes are shared only between the parent and the child.
In addition it would be okay if the child process would create another child which would inherit that handle...

A second call to process start would create three new pipes which have no relationship to the other pipes.

if (startInfo.RedirectXXX) { CreatePipe(out parentXXXPipeHandle, out childXXXPipeHandle, true); } else { childInputPipeHandle = new SafeFileHandle(Interop.Kernel32.GetStdHandle(Interop.Kernel32.HandleTypes.STD_XXX_HANDLE), false); }

[bernd5]

It's true that these (private) handles are inherited as well. But open file handles, sockets..., too.
But as long as the handle value is not submitted anyhow they are effectively not usable.
(There might be some enum-procs provided by the kernel)

[stephentoub]

But as long as the handle value is not submitted anyhow they are effectively not usable

The problem is that these other child processes can end up keeping the handle alive, which can then cause the parent and actual child that's using the handle to misbehave, e.g. the actual child can close the handle to signal completion to the parent, but the parent won't see it closed until all children that inherited the handle close it as well, which they won't do (since as you say they don't know the handle value) until they exit.

But open file handles, sockets..., too.

Most handles are created as non-inheritable by default. In general you need to opt-in to them being inheritable. For example, sockets are created with WSA_FLAG_NO_HANDLE_INHERIT:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs

Line 44 in 4d3ec15

IntPtr handle = Interop.Winsock.WSASocketW(addressFamily, socketType, protocolType, IntPtr.Zero, 0, Interop.Winsock.SocketConstructorFlags.WSA_FLAG_OVERLAPPED | Interop.Winsock.SocketConstructorFlags.WSA_FLAG_NO_HANDLE_INHERIT);;

and FileStreams are created with a FileShare, which needs to explicitly specify Inheritable for the handle to be created as such:
https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/IO/FileShare.cs

[bernd5]

But you can't know what the child is doing.
It could have an endless loop and never return. If it is the case, Okay. But it's a bug in the child.

Of course I don't know how these handles are manged internally. But I can Imagine that a call to DuplicateHandle just increases a reference count to support multiple Close calls before freeing the underling data structures.

In addition just the childXXXPipeHandle is disposed in the lock. This means that that the parent can't write/read somthing from/in the childXXX Pipe. This is Okay because the parent just want's to get the results / outputs. It never writes into the output stream or reads from the input stream!

The "real" handles remain open and are stored in the FileStreams / StreamReaders (outside the lock!).

You can see a working example (without locks) here:
https://docs.microsoft.com/en-us/windows/win32/ProcThread/creating-a-child-process-with-redirected-input-and-output
(It's is not the cleanest code because of global vars, but...)

[stephentoub]

I feel like we're either talking past each other or I'm not explaining things well. I will let someone else chime in. Regardless, the lock can't be removed.

[danmoseley]

I think this issue can be closed in favor of #13943 -- it seems the way forward is a new API.

[danmoseley]

similar on Linux https://github.com/dotnet/corefx/issues/33069

[svick]

@stephentoub

Process needs to make sure that no other child process is created while that handle is alive... hence the lock.

Would it make sense to use a readers-writer lock instead of a mutex here? That way, multiple processes that don't need the handle can be started in parallel, while if a process does need the handle, it's guaranteed no other process is starting in the meantime.

[stephentoub]

Would it make sense to use a readers-writer lock instead of a mutex here?

It's an interesting idea that could be experimented with, but I'm a little skeptical it'll be worthwhile.

Process.Start calls where no redirection is requested would take the reader lock. If any of the RedirectXx properties was true, it would take the writer lock. And I suspect that a typical app launching processes at enough scale for this to matter is going to be redirecting, which means they'd be serialized anyway. If switching to a reader/writer lock were free, then we might as well, but it's not, in particular in that such locks have measurably higher overhead than normal locks. But, if someone wants to experiment with it, it'd be interesting to see the numbers.