Add windows NamedPipe #1351
Add windows NamedPipe #1351
Conversation
|
The reason I am opting to add this to mio proper is because #1047 is not done, so |
|
Want me to move https://github.com/Thomasdezeeuw/mio-pipe into Mio as well? I was planning on proposing that for a while. |
|
Sure! In general, I think it is fair to have mio constructs for common OS primitives. |
|
@carllerche created an issue for the Mio-pipe move: #1354. |
|
Ok, I got this building and tests passing. I would appreciate a review. This is an upgrade of the |
tests/win_named_pipe.rs
Outdated
| use std::task::{Context, Poll}; | ||
| use std::time::Duration; | ||
|
|
||
| // use mio::{Events, Poll, PollOpt, Ready, Token}; |
|
@Thomasdezeeuw Ok, the feedback should be addressed. I opted to stay w/ |
|
Ok, I tested that this works for Tokio. 👍 |
If I understood correctly, the only benefit |
| } | ||
|
|
||
| fn flush(&mut self) -> io::Result<()> { | ||
| Ok(()) |
There was a problem hiding this comment.
This combined with the scheduled write above might give issues were a user thinks the bytes have been written to the pipe and closes the application, but it's not actually the case.
| @@ -0,0 +1,267 @@ | |||
| #![cfg(windows)] | |||
There was a problem hiding this comment.
Nit: These tests can use some of the test utilities in tests/util, such as expect_events, but that can be done later.
| } | ||
|
|
||
| #[test] | ||
| fn write_then_drop() { |
There was a problem hiding this comment.
Re: not implementing flush (or a no-op implementation) above. I think running this test with the client in a different process that stops right after the write (line 194) would fail.
|
True, but
I don't think this is correct.
This is true, but if we're using |
|
@Thomasdezeeuw I have seen this in practice and the documentation indicates as such. |
|
Look at the ASM generated by this: use std::sync::atomic::AtomicUsize;
use std::sync::atomic::Ordering::{Acquire, Release};
static CELL: AtomicUsize = AtomicUsize::new(0);
pub fn main() -> usize {
CELL.store(1, Release);
CELL.load(Acquire)
}example::main:
mov qword ptr [rip + example::CELL], 1
mov rax, qword ptr [rip + example::CELL]
ret
example::CELL:
.zero 8Plain |
|
Alright! CI passes 🎉 let me investigate Tokio now. |
The For cas operations it doesn't matter and I incorrectly assumed the same to be true for load/store operations. use std::sync::atomic::{AtomicUsize, Ordering};
static CELL: AtomicUsize = AtomicUsize::new(0);
pub fn main() {
CELL.fetch_add(1, Ordering::AcqRel);
CELL.fetch_add(1, Ordering::SeqCst);
CELL.load(Ordering::SeqCst);
}example::main:
lock add qword ptr [rip + example::CELL], 1
lock add qword ptr [rip + example::CELL], 1
mov rax, qword ptr [rip + example::CELL]
ret
example::CELL:
.zero 8 |
|
I've deprecated and directed users of the old mio-named-pipes to mio 0.7. Thanks! |
This builds on tokio-rs/mio#1351 and introduces the tokio::net::windows::named_pipe module which provides low level types for building and communicating asynchronously over windows named pipes. Named pipes require the `net` feature flag to be enabled on Windows. Co-authored-by: Alice Ryhl <[email protected]>
Tokio needs windows named pipe support in order to upgrade to Mio 0.7. I thought I would take a quick initial stab at this.
It is based on https://github.com/bbqsrc/mio-named-pipes, but simplified some. Instead of shoehorning this into the
Sourcetrait, I dod registration innew. Also, since theRegistrationinternal queue is gone in Mio 0.6, I built the API on top ofstd::task::Wakerinstead. As this is essentially adding an IOCP based API on top of Mio in order to power Tokio, it seemed silly to add a shim to make the API "seem" like epoll and then layer wakers back on.