Windows: Use a pipe relay for chaining pipes

[ChrisDenton]

Fixes #95759

This fixes the issue by chaining pipes synchronously and manually pumping messages between them. It's not ideal but it has the advantage of not costing anything if pipes are not chained ("don't pay for what you don't use") and it also avoids breaking existing code that rely on our end of the pipe being asynchronous (which includes rustc's own testing framework).

Libraries can avoid needing this by using their own pipes to chain commands.

[rust-highfive]

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

[ChrisDenton]

I'm not sure if kenneytm is around at the moment and github is suggesting dtolnay so...

r? @dtolnay

Not to rush review or anything but a test is intermittently failing in CI while this issue remains unresolved.

[ChrisDenton]

Ok, so I'm guessing dtolnay is busy too and people are getting frustrated with the CI failures. So erm, let's try...

r? @m-ou-se

I'm totally available to answer any questions or concerns.

[m-ou-se]

rely on our end of the pipe being asynchronous (which includes rustc's own testing framework).

Could you elaborate a bit on how code relies on that?

[m-ou-se]

Can you point me to the right documentation about asynchronous pipes?

I did a quick search, but only found a page that says:

Asynchronous (overlapped) read and write operations are not supported by anonymous pipes.

[ChrisDenton]

rely on our end of the pipe being asynchronous (which includes rustc's own testing framework).

Could you elaborate a bit on how code relies on that?

It uses miow to do asynchronous I/O. See

rust/src/tools/compiletest/src/read2.rs

Lines 190 to 200 in f38c5c8

	pub fn read2(
	out_pipe: ChildStdout,
	err_pipe: ChildStderr,
	data: &mut dyn FnMut(bool, &mut Vec<u8>, bool),
	) -> io::Result<()> {
	let mut out = Vec::new();
	let mut err = Vec::new();
	let port = CompletionPort::new(1)?;
	port.add_handle(0, &out_pipe)?;
	port.add_handle(1, &err_pipe)?;

I guess it wouldn't be too hard to change but I'm worried that the wider ecosystem is also relying on these being opened for "overlapped" I/O as the handles are made available via the AsRawHandle trait.

Can you point me to the right documentation about asynchronous pipes?

We actually use named pipes

rust/library/std/src/sys/windows/pipe.rs

Line 89 in f38c5c8

let handle = c::CreateNamedPipeW(

The docs are here Synchronous and Overlapped Pipe I/O

[ChrisDenton]

This comment has a bit of background on the issue of async and sync pipe I/O: #38811 (comment)

After that, Rust switched to using synchronous I/O for the end of the pipe given to the child process. However, we continued to use "Overlapped" (aka asynchronous) I/O for our end. Which is fine until we give our end of the pipe to another process.

[m-ou-se]

We actually use named pipes

Ah, that explains my confusion.

After that, Rust switched to using synchronous I/O for the end of the pipe given to the child process. However, we continued to use "Overlapped" (aka asynchronous) I/O for our end. Which is fine until we give our end of the pipe to another process.

I see. I dug a bit through history, reading #38835 that switched one end of the pipe to synchonous code, to try to understand this. Here's my summary, please correct me if I'm wrong:

---

Just like on Unix, it's possible to use pipes for stdin/stdout/stderr of a child process. Windows supports basic anonymous pipes, but those don't support 'overlapped' aka asynchronous mode, which we use to be able to read from both stdout and stderr in parallel. So instead, we open a named pipe (\\.\pipe\__rust_anonymous_pipe1__.{pid}.{random}), because those do support overlapped mode.

Using a pipe end that's in overlapped mode as if it's not in overlapped mode might result in bad things, so we shouldn't pass those to child processes. So we open only one end of the pipe in overlapped mode: the end we keep. The comment on anon_pipe talks about 'our' and 'their' ends of the pipe, making the assumption that 'our' end will always stay on our side:

rust/library/std/src/sys/windows/pipe.rs

Lines 37 to 39 in ab33f71

	/// subtly different. Here we'll return two pipes in the `Pipes` return value,
	/// but one is intended for "us" where as the other is intended for "someone
	/// else".

However, this is a wrong assumption, because 'our' half of the pipe might be given to another subprocess, through .stdin(child.stdout) when spawning another process, possibly resulting in undefined behaviour in that process.

Now, the only reason we enable overlapping mode, is for read2, which reads from two pipes at once:

rust/library/std/src/sys/windows/pipe.rs

Line 331 in ab33f71

pub fn read2(p1: AnonPipe, v1: &mut Vec<u8>, p2: AnonPipe, v2: &mut Vec<u8>) -> io::Result<()> {

read2 is only used in one place: in Child::wait_with_output in the case both stdout and stderr are redirected:

rust/library/std/src/process.rs

Lines 1926 to 1927 in ab33f71

	(Some(out), Some(err)) => {
	let res = read2(out.inner, &mut stdout, err.inner, &mut stderr);

It might be possible to implement read2 without using overlapped mode. If we do that, we can use 'real' anonymous pipes rather than our \\.\pipe\__rust_anonymous_pipe1__ hack, and the issue with passing overlapped mode handles to subprocesses would be fixed.

However, there is at least one case of another crate assuming overlapped mode is enabled for subprocess pipes: our very own compiletest, which implements its own read2 function based on that assumption:

rust/src/tools/compiletest/src/read2.rs

Lines 190 to 200 in ab33f71

	pub fn read2(
	out_pipe: ChildStdout,
	err_pipe: ChildStderr,
	data: &mut dyn FnMut(bool, &mut Vec<u8>, bool),
	) -> io::Result<()> {
	let mut out = Vec::new();
	let mut err = Vec::new();
	let port = CompletionPort::new(1)?;
	port.add_handle(0, &out_pipe)?;
	port.add_handle(1, &err_pipe)?;

Other crates might depend on it too, for example if they use miow::iocp::CompletionPort::add_handle on child.stdout. We don't know if anything other than compiletest does that.

---

Based on that understanding, I have a few questions:

• Can we implement read2 for (anonymous) non-overlapped mode pipes?
• Is is possible to turn off overlapped mode on a handle, or get a clone/duplicate of a handle with overlapped mode disabled? (E.g. through ReOpenFile()?)
• In which ways would code break that assumes overlapped mode is enabled if we stop enabling it? Will they reliably error, or would that result in undefined/unpredictable behaviour?

[ChrisDenton]

• Can we implement read2 for (anonymous) non-overlapped mode pipes?

Yes. It would require spinning up a thread to make sure we don't block either pipe. If a pipe buffer is full then it'll be blocked until drained so we could otherwise end up deadlocked.

---

• Is is possible to turn off overlapped mode on a handle, or get a clone/duplicate of a handle with overlapped mode disabled? (E.g. through ReOpenFile()?)

No, it can't be turned off once it's on. Pipes use a server/client model. You can create more than one server for the same pipe (both overlapped and not) but once a client "connects" it must use the same server. It can reconnect to the pipe (thus potentially being given a different server) but the child process won't know that and in any case we can't coordinate a disconnect/reconnect. Essentially once we give a process the "theirs" handle we can't really change either side of the pipe.

---

• In which ways would code break that assumes overlapped mode is enabled if we stop enabling it? Will they reliably error, or would that result in undefined/unpredictable behaviour?

For the pipe server it should be a reliable error. I tested this by removing c::FILE_FLAG_OVERLAPPED from:

rust/library/std/src/sys/windows/pipe.rs

Line 82 in ab33f71

let mut flags = c::FILE_FLAG_FIRST_PIPE_INSTANCE | c::FILE_FLAG_OVERLAPPED;

This should be equivalent to using anonymous pipes because (from Anonymous Pipe Operations):

Anonymous pipes are implemented using a named pipe with a unique name.

When attempting to use overlapped I/O (i.e via miow), this line:

port.add_handle(0, &out_pipe)?;

Will give the error:

Os { code: 87, kind: InvalidInput, message: "The parameter is incorrect." }

EDIT: In code above, miow is using I/O completion ports which do error if given a handle that wasn't opened in overlapped mode. However, if using ReadFileEx, WriteFileEx, ReadFile or WriteFile without I/O completion ports, the call will simply block. This could be a problem for applications relying on asynchronous I/O.

[m-ou-se]

Yes. It would require spinning up a thread to make sure we don't block either pipe. If a pipe buffer is full then it'll be blocked until drained so we could otherwise end up deadlocked.

Ah, I see. It looks like that was exactly what we did before we switched to overlapped mode pipes.

So, it looks like we'll always have to spawn threads to handle certain situations:

• Either we spawn a thread in read2 when waiting on both stdout and stderr in Child::wait_with_output (what we did a long time ago);
• Or, we spawn a thread when attaching one child process' stdin/out/err to that of another child (what this PR does).

Neither is great, but it seems to me that option 2 (this PR) is the better one, since that's a less common scenario.

[m-ou-se]

Can we reduce the number of situations in which this is done by only using asynchronous pipes if both stderr and stdout of a child process are pipes? (And never doing it for stdin.)

[m-ou-se]

I'm approving this for now so the issue gets fixed, but I'd like to see if we can reduce the number of cases in which this workaround is necessary.

@bors r+ rollup=never

[bors]

📌 Commit 9013054 has been approved by m-ou-se

[ChrisDenton]

Can we reduce the number of situations in which this is done by only using asynchronous pipes if both stderr and stdout of a child process are pipes? (And never doing it for stdin.)

For sure, I can look into this. I'd still worry a bit about people relying on them being asynchronous pipes but I guess there are no guarantees when using as_raw_handle on stdlib types.

[m-ou-se]

Maybe we can add something like .use_async_pipes() to Windows' CommandExt, and enable that automatically only when both stderr and stdout are pipes. Then users that really need these pipes to be async in other situations can still manually enable it.

[ChrisDenton]

That does sound like a good compromise.

[bors]

⌛ Testing commit 9013054 with merge 9f434ff730b39d84ff9c40f47a1396a8fdd758db...

[bors]

💥 Test timed out

[ChrisDenton]

@bors retry timeout waiting for a runner to pick up aarch64-gnu, self-hosted, ARM64, linux

[RalfJung]

@bors p=10
higher priority than rollups, because this keeps blocking other PRs

[RalfJung]

Is this line the key bug? Stdio::Handle expects a sync pipe but pipe.into_handle() produces an async pipe?
That seems like maybe there should be comments in some places so that this is not reverted in the future when the context of this PR has been lost. (But let's add comments, if any, in a separate PR, so that this one can land ASAP.)

[ChrisDenton]

Good point. I'll add that in a follow up PR.

[bors]

⌛ Testing commit 9013054 with merge 69a5ae3...

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RalfJung]

(I think that is the log from the old failure, the timeout)

[ChrisDenton]

Indeed, CI looks like it is still running. It gave me a scare though. 😌

[bors]

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 69a5ae3 to master...

[rust-timer]

Finished benchmarking commit (69a5ae3): comparison url.

Summary:

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: mixed results

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	0	1	0	1	0
mean2	N/A	0.7%	N/A	-0.3%	N/A
max	N/A	0.7%	N/A	-0.3%	N/A

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. number of relevant changes ↩

2. the arithmetic mean of the percent change ↩