Skip to content

Windows: Use anonymous pipes in Command #142517

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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented Jun 14, 2025

When setting Stdio::pipe on Command we want to create an anonymous pipe that can be used asynchronously (at least on our end). Usually we'd use CreatePipe to open anonymous pipes but unfortunately it opens pipes for synchronous access. The alternative is to use CreateNamedPipeW which does allow asynchronous access but that requires giving a file name to the pipe. So we currently have this awful hack where we attempt to emulate anonymous pipes using CreateNamedPipeW by attempting to create a unique name and looping until we find one that doesn't already exist.

The better option is to use the lower level NtCreateNamedPipeFile (which is used internally by both CreatePipe and CreateNamedPipeW). This function wasn't documented until a few years ago but now that it is it's ok for us to use it.

try-job: msvc
try-job: mingw

@rustbot
Copy link
Collaborator

rustbot commented Jun 14, 2025

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 14, 2025
@ChrisDenton

This comment was marked as outdated.

@rust-bors

This comment was marked as outdated.

rust-bors bot added a commit that referenced this pull request Jun 14, 2025
Windows: Use anonymous pipes in Command

When setting `Stdio::pipe` on `Command` we want to create an anonymous pipe that can be used asynchronously (at least on our end). Usually we'd use [`CreatePipe`](https://learn.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-createpipe) to open anonymous pipes but unfortunately it opens pipes for synchronous access. The alternative is to use [`CreateNamedPipeW`](https://learn.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-createnamedpipew) which does allow asynchronous access but that requires giving a file name to the pipe. So we currently have this awful hack where we attempt to emulate anonymous pipes using `CreateNamedPipeW` by attempting to create a unique name and looping until we find one that doesn't already exist.

The better option is to use the lower level [`NtCreateNamedPipeFile`](https://learn.microsoft.com/en-us/windows/win32/devnotes/nt-create-named-pipe-file) (which is used internally by both `CreatePipe` and `CreateNamedPipeW`). This function wasn't documented until a few years ago but now that it is it's ok for us to use it.

try-job: *msvc*
try-job: *mingw*
@ChrisDenton

This comment was marked as outdated.

@rust-bors

This comment was marked as outdated.

@ChrisDenton

This comment was marked as outdated.

@rust-bors

This comment was marked as outdated.

@ChrisDenton
Copy link
Member Author

ChrisDenton commented Jun 14, 2025

@bors try

@bors
Copy link
Collaborator

bors commented Jun 14, 2025

⌛ Trying commit 82f68e0 with merge 4b91445...

bors added a commit that referenced this pull request Jun 14, 2025
Windows: Use anonymous pipes in Command

When setting `Stdio::pipe` on `Command` we want to create an anonymous pipe that can be used asynchronously (at least on our end). Usually we'd use [`CreatePipe`](https://learn.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-createpipe) to open anonymous pipes but unfortunately it opens pipes for synchronous access. The alternative is to use [`CreateNamedPipeW`](https://learn.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-createnamedpipew) which does allow asynchronous access but that requires giving a file name to the pipe. So we currently have this awful hack where we attempt to emulate anonymous pipes using `CreateNamedPipeW` by attempting to create a unique name and looping until we find one that doesn't already exist.

The better option is to use the lower level [`NtCreateNamedPipeFile`](https://learn.microsoft.com/en-us/windows/win32/devnotes/nt-create-named-pipe-file) (which is used internally by both `CreatePipe` and `CreateNamedPipeW`). This function wasn't documented until a few years ago but now that it is it's ok for us to use it.

try-job: *msvc*
try-job: *mingw*
@bors
Copy link
Collaborator

bors commented Jun 14, 2025

☀️ Try build successful - checks-actions
Build commit: 4b91445 (4b91445f185574b3733649977aa2409da29f288b)

@@ -65,89 +60,108 @@ pub fn anon_pipe(ours_readable: bool, their_handle_inheritable: bool) -> io::Res
// operations. Instead, we create a "hopefully unique" name and create a
// named pipe which has overlapped operations enabled.
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems stale?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh right, yes. I'll fix that.


// A negative timeout value is a relative time (rather than an absolute time).
// The time is given in 100's of nanoseconds so this is 50 milliseconds.
let timeout = (50_i64 * 10000).neg() as u64;
Copy link
Member

Choose a reason for hiding this comment

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

Can you say more about why 50ms? I think this is new -- right?

I would sort of expect that we wouldn't timeout at all...

Copy link
Member Author

Choose a reason for hiding this comment

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

We were using 0 from CreateNamedPipeW which is a documented as using the default of 50 ms (see nDefaultTimeOut):

A value of zero will result in a default time-out of 50 milliseconds.

The timeout only has an effect when using wait functions like WaitNamedPipeW We don't actually make use of it ourselves but it is possible for users to get at the underlying handle via Child and calling as_handle on one of the fields. We don't make any guarantees here but I think it's worth not changing unless we need to.

Copy link
Member

Choose a reason for hiding this comment

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

I see, agreed that this isn't the PR to change that. Maybe worth adding a note to the comment reflecting where the 50ms came from?

@Mark-Simulacrum
Copy link
Member

This function wasn't documented until a few years ago but now that it is it's ok for us to use it.

The function is guaranteed to have existed on all supported Windows targets?

@ChrisDenton
Copy link
Member Author

This function wasn't documented until a few years ago but now that it is it's ok for us to use it.

The function is guaranteed to have existed on all supported Windows targets?

Yes. It goes back to at least XP and almost certainly before that.

@Mark-Simulacrum
Copy link
Member

r=me with nits/comments fixed

@ChrisDenton
Copy link
Member Author

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Jun 15, 2025

📌 Commit 5dc235f has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 15, 2025
@ChrisDenton
Copy link
Member Author

Oh, wait I didn't push the second comment.

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 15, 2025
@ChrisDenton
Copy link
Member Author

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Jun 15, 2025

📌 Commit 0e1db54 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 15, 2025
jhpratt added a commit to jhpratt/rust that referenced this pull request Jun 17, 2025
…lacrum

Windows: Use anonymous pipes in Command

When setting `Stdio::pipe` on `Command` we want to create an anonymous pipe that can be used asynchronously (at least on our end). Usually we'd use [`CreatePipe`](https://learn.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-createpipe) to open anonymous pipes but unfortunately it opens pipes for synchronous access. The alternative is to use [`CreateNamedPipeW`](https://learn.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-createnamedpipew) which does allow asynchronous access but that requires giving a file name to the pipe. So we currently have this awful hack where we attempt to emulate anonymous pipes using `CreateNamedPipeW` by attempting to create a unique name and looping until we find one that doesn't already exist.

The better option is to use the lower level [`NtCreateNamedPipeFile`](https://learn.microsoft.com/en-us/windows/win32/devnotes/nt-create-named-pipe-file) (which is used internally by both `CreatePipe` and `CreateNamedPipeW`). This function wasn't documented until a few years ago but now that it is it's ok for us to use it.

try-job: *msvc*
try-job: *mingw*
bors added a commit that referenced this pull request Jun 17, 2025
Rollup of 11 pull requests

Successful merges:

 - #140809 (Reduce special casing for the panic runtime)
 - #141608 (Add support for repetition to `proc_macro::quote`)
 - #141864 (Handle win32 separator for cygwin paths)
 - #142216 (Miscellaneous RefCell cleanups)
 - #142517 (Windows: Use anonymous pipes in Command)
 - #142570 (Reject union default field values)
 - #142584 (Handle same-crate macro for borrowck semicolon suggestion)
 - #142585 (Update books)
 - #142586 (Fold unnecessary `visit_struct_field_def` in AstValidator)
 - #142595 (Revert overeager warning for misuse of `--print native-static-libs`)
 - #142598 (Set elf e_flags on ppc64 targets according to abi)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jun 17, 2025
…lacrum

Windows: Use anonymous pipes in Command

When setting `Stdio::pipe` on `Command` we want to create an anonymous pipe that can be used asynchronously (at least on our end). Usually we'd use [`CreatePipe`](https://learn.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-createpipe) to open anonymous pipes but unfortunately it opens pipes for synchronous access. The alternative is to use [`CreateNamedPipeW`](https://learn.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-createnamedpipew) which does allow asynchronous access but that requires giving a file name to the pipe. So we currently have this awful hack where we attempt to emulate anonymous pipes using `CreateNamedPipeW` by attempting to create a unique name and looping until we find one that doesn't already exist.

The better option is to use the lower level [`NtCreateNamedPipeFile`](https://learn.microsoft.com/en-us/windows/win32/devnotes/nt-create-named-pipe-file) (which is used internally by both `CreatePipe` and `CreateNamedPipeW`). This function wasn't documented until a few years ago but now that it is it's ok for us to use it.

try-job: *msvc*
try-job: *mingw*
bors added a commit that referenced this pull request Jun 17, 2025
Rollup of 10 pull requests

Successful merges:

 - #138538 (Make performance description of String::{insert,insert_str,remove} more precise)
 - #141946 (std: refactor explanation of `NonNull`)
 - #142216 (Miscellaneous RefCell cleanups)
 - #142371 (avoid `&mut P<T>` in `visit_expr` etc methods)
 - #142377 (Try unremapping compiler sources)
 - #142517 (Windows: Use anonymous pipes in Command)
 - #142542 (Manually invalidate caches in SimplifyCfg.)
 - #142563 (Refine run-make test ignores due to unpredictable `i686-pc-windows-gnu` unwind mechanism)
 - #142570 (Reject union default field values)
 - #142584 (Handle same-crate macro for borrowck semicolon suggestion)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-bors bot added a commit that referenced this pull request Jun 17, 2025
Rollup of 10 pull requests

Successful merges:

 - #138538 (Make performance description of String::{insert,insert_str,remove} more precise)
 - #141946 (std: refactor explanation of `NonNull`)
 - #142216 (Miscellaneous RefCell cleanups)
 - #142371 (avoid `&mut P<T>` in `visit_expr` etc methods)
 - #142377 (Try unremapping compiler sources)
 - #142517 (Windows: Use anonymous pipes in Command)
 - #142542 (Manually invalidate caches in SimplifyCfg.)
 - #142563 (Refine run-make test ignores due to unpredictable `i686-pc-windows-gnu` unwind mechanism)
 - #142570 (Reject union default field values)
 - #142584 (Handle same-crate macro for borrowck semicolon suggestion)

r? `@ghost`
`@rustbot` modify labels: rollup
<!-- homu-ignore:start -->
[Create a similar rollup](https://bors.rust-lang.org/queue/rust?prs=138538,141946,142216,142371,142377,142517,142542,142563,142570,142584)
<!-- homu-ignore:end -->
try-job: dist-aarch64-apple
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants