Windows: Cache the pipe filesystem handle#155250
Windows: Cache the pipe filesystem handle#155250rust-bors[bot] merged 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
ChrisDenton
added
S-waiting-on-review
|
@bors r+ |
rust-bors
Bot
added
S-waiting-on-bors
…crum Windows: Cache the pipe filesystem handle Updates the anonymous pipe handling based on feedback from @lhecker (see rust-lang#142517 (comment)). This does two things: 1. Cache the handle to the pipe filesystem so we don't have to reopen it each time. 2. Use the `\Device\NamedPipe\` directly instead of the symlink to it.
…crum Windows: Cache the pipe filesystem handle Updates the anonymous pipe handling based on feedback from @lhecker (see rust-lang#142517 (comment)). This does two things: 1. Cache the handle to the pipe filesystem so we don't have to reopen it each time. 2. Use the `\Device\NamedPipe\` directly instead of the symlink to it.
…uwer Rollup of 15 pull requests Successful merges: - #152995 (ACP Implementation of PermissionsExt for Windows ) - #153457 (prevent deref coercions in `pin!`) - #155250 (Windows: Cache the pipe filesystem handle) - #155574 (Move `std::io::RawOsError` to `core::io`) - #155757 (macro_metavar_expr_concat: explain why idents are invalid) - #155823 (miri subtree update) - #155693 (Suggest enclosing format string with `""` under special cases) - #155707 (Fix minor panic-unsoundness in CString::clone_into) - #155719 (Suggest `.iter()` for shared projections) - #155779 (ssa_range_prop: use `if let` guards) - #155789 (Cleanups to `AttributeExt`) - #155805 (Mention `DEPRECATED_LLVM_INTRINSIC` lint for internal use) - #155806 (Remove the incomplete marker from `impl` restrictions) - #155820 (Avoid improper spans when `...` or `..=` is recovered from non-ASCII) - #155822 (Add default field values to diagnostic FormatArgs)
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.