Windows: Cache the pipe filesystem handle#155250
Windows: Cache the pipe filesystem handle#155250ChrisDenton wants to merge 2 commits intorust-lang:mainfrom
Conversation
rustbot
added
S-waiting-on-review
|
r? @ibraheemdev rustbot has assigned @ibraheemdev. Use Why was this reviewer chosen?The reviewer was selected based on:
|
Also use the `\Device\NamedPipe\` directly instead of the symlink to it.
|
r? rust-lang/libs |
|
The review request |
| let pipe_fs = { | ||
| let path = api::unicode_str!(r"\??\PIPE\"); | ||
| // Open a handle to the pipe filesystem (`\Device\NamedPipe\`) and cache it. | ||
| // This will be used when creating a new anonymous pipe. |
There was a problem hiding this comment.
Are we going to get complaints from valgrind (not sure that works on Windows) for a handle leak due to this? Anything preemptive that we could do to indicate it's intentional?
There was a problem hiding this comment.
We don't tend to get those types of complaints for Windows but. sure, I can add a comment explicitly noting this is intentional.
Edit: Windows doesn't have the tight fd limit that unix systems can have so leaking a single handle is less of an issue (the limit for handles is something north of 16 million).
| // Open a handle to the pipe filesystem (`\Device\NamedPipe\`) and cache it. | ||
| // This will be used when creating a new anonymous pipe. | ||
| static PIPE_FS: Atomic<c::HANDLE> = Atomic::<c::HANDLE>::new(ptr::null_mut()); | ||
| let pipe_fs = if let handle = PIPE_FS.load(Relaxed) |
There was a problem hiding this comment.
Hm, this feels like it needs to be Acquire coupled with a Release store below, no? Otherwise we're not necessarily seeing the memory initialization. Though probably OK in practice given the interesting memory is in the kernel... right?
There was a problem hiding this comment.
Yeah exactly: The NT file handle is kernel-mode only and so access is fully synchronized already.
There was a problem hiding this comment.
Agreed, all we need to do here is ensure the atomic containing the handle synchronises with itself. For which relaxed is sufficient. It would be a (rather serious, imo) bug in the OS if using a handle across threads risked synchronisation issues around the handle itself.
rustbot
added
S-waiting-on-author
5 participants
Updates the anonymous pipe handling based on feedback from @lhecker (see #142517 (comment)). This does two things:
\Device\NamedPipe\directly instead of the symlink to it.