New windows implementation strategies from wepoll #1034

PerfectLaugh · 2019-07-15T06:12:56Z

Currently this passed most of the test suite. Except for a test in Token documentation.

https://gist.github.com/PerfectLaugh/07fb4766be01e797a678357706828f1d

Changed readv, writev function signature from ```&self``` to ```&mut self```

Added possible fix line on Token documentation.

Thomasdezeeuw

Thank you for doing this @PerfectLaugh. This one of the major blocking points for v0.7 so I've been looking forward to this for a while.

I've left some comments, but I didn't review the actual implementation as I don't know enough about IOCP to review this.

src/poll.rs

src/sys/windows/mod.rs

src/token.rs

src/waker.rs

src/net/tcp.rs

src/sys/windows/waker.rs

Cargo.toml

src/lib.rs

src/sys/windows/selector.rs

src/sys/windows/afd.rs

carllerche · 2019-07-15T16:11:32Z

cc @piscisaureus

PerfectLaugh · 2019-07-15T18:47:52Z

Now all the test suite is passed.

piscisaureus · 2019-07-15T18:48:34Z

src/sys/windows/selector.rs

-        trace!("returning");
-        Ok(())
-    }
+        // Codes to emulate ET in mio


@PerfectLaugh
Can you elaborate a bit on how this works?
In wepoll there's no way I can emulate edge-triggered, because I don't know when the user has attempted to read/write from a socket, so I don't know when to re-submit the AFD_POLL overlappped operation. I think in mio's case it might be possible because we can know when a socket read/write results in WSAEWOULDBLOCK.
But I don't see how that works here.

This work is in tcp.rs udp.rs. where I hooked all possible calls the WouldBlock could appear.

And the codes here is to eliminate those events that have been read. So we can simulate proper ET behavior (probably?

Could you explain how this code eliminates events that have been read?

This code simply rely on assumption that mio can control and intercept every aspects that user will do to this socket. With this assumption (As long as rawsocket is not being manipulated by external library or raw api calls), we can wipe down the interests that user hasn't done it, simulating the ET behavior.

src/sys/windows/selector.rs

SergejJurecko · 2019-07-16T04:33:30Z

Strangely this compiles in debug but not release mode (x86_64-pc-windows-msvc)

PerfectLaugh · 2019-07-16T05:55:02Z

Strangely this compiles in debug but not release mode (x86_64-pc-windows-msvc)

Fixed.

src/sys/mod.rs

Thomasdezeeuw · 2019-07-16T09:54:41Z

src/sys/windows/sourcerawsocket.rs

+///     fn register(&self, registry: &Registry, token: Token, interests: Interests)
+///         -> io::Result<()>
+///     {
+///         SourceRawSocket::new(&self.raw_socket).register(registry, token, interests)


This usage is rather expensive if we're creating a new Arc<Mutex<..>> each call to register. If we can't do without the SockState we might need to go for a different design.

The problem is we need to simulate ET behavior. Then we do need SockState to maintain what interests have been read

That is fine, but would the example still work then? It throws about the same after each call to (re)register doesn't it?

Indeed. You are right. I cannot hold all the actions user took on the RawSocket.

Then the library will need to take level trigger approach rather than edge trigger

or just remove SourceRawSocket

Let's remove it for now then and we'll find another approach later.

src/sys/windows/tcp.rs

src/sys/windows/udp.rs

test/test_tcp.rs

piscisaureus · 2019-07-17T14:43:06Z

src/sys/windows/afd.rs

+
+    pub fn cancel(&self, iosb: &mut IO_STATUS_BLOCK) -> io::Result<()> {
+        unsafe {
+            if iosb.u.Status != STATUS_PENDING {


The IO_STATUS_BLOCK can be updated by the kernel any time when the operation is pending.
So it should probably be wrapped in an UnsafeCell and be pinned.

UnsafeCell has no Deref trait implemented therefore cannot be pinned. I used the Box instead.

If you can guarantee that certain memory doesn't move it can be pinned, then you don't need to use Box. But these kind of optimisations can be done later.

Uploaded new code. Please check

src/sys/windows/afd.rs

Merge mio master commits

PerfectLaugh · 2019-07-18T05:03:55Z

Please check the documentation I added also. My English is the worst of the world and I don't want any syntax/grammar error.

src/sys/windows/io_status_block.rs

Thomasdezeeuw · 2019-07-19T09:30:52Z

src/sys/windows/selector.rs

-    /// and once complete they'll be put back in.
-    buffers: Mutex<BufferPool>,
+pub struct SockState {
+    iosb: Pin<IoStatusBlock>,


I don't think this Pin is current. I didn't following the entire previous discussion but I think IoStatusBlock can't move right? If that is the case it need to implemented !Pin (by using std::marker::PhantomPinned). Next it needs to be allocated in memory where it won't move, likely on the heap. The easier way to do that is to use Pin<Box<IoStatusBlock>>. Later we can figure out if we can remove the box by making SockState pinned.

Also note that all methods on IoStatusBlock should accept Pin<&mut Self> rather then &mut self (see the std::future::Future trait for an example).

SockState is wrapped by Arc<Mutex> all the time. I think it is actually safe not using Pin though.
I will do Pin<Box> for now.

src/sys/windows/selector.rs

Thomasdezeeuw · 2019-07-19T09:37:16Z

src/sys/windows/selector.rs

-                }
-                events.events.push(Event::new(Ready::READABLE, token));
-                continue;
+        unsafe {


Can you reduce this unsafe block and add a comment why it is safe?

I cannot because the logic is connected. It is better to put this still.

// We use the status info in IO_STATUS_BLOCK to determine the socket poll status. It is unsafe to use a pointer of IO_STATUS_BLOCK.

src/sys/windows/udp.rs

src/sys/windows/mod.rs

src/sys/windows/afd.rs

Thomasdezeeuw · 2019-07-19T14:00:16Z

@PerfectLaugh can you not mention me via @ in commits, now I'm going to get an email from GitHub every time something happens with that commit, thanks.

PerfectLaugh · 2019-07-19T14:00:55Z

@PerfectLaugh can you not mention me via @ in commits, now I'm going to get an email from GitHub every time something happens with that commit, thanks.

sorry

carllerche · 2019-07-19T16:21:41Z

I would like to get this PR merged sooner than later. Any non critical follow up change can be tracked in a follow up issue.

@Thomasdezeeuw @piscisaureus could you list out any changes that are blocking merging this?

Thomasdezeeuw · 2019-07-19T16:28:20Z

I'm fine with merging now, all my comments can be fixed later.

carllerche · 2019-07-19T16:50:08Z

I opened #1041 to track changes. Once we get the 👍 from @piscisaureus, I will merge this and extract all remaining items & questions into #1041.

piscisaureus

LGTM

carllerche

🎉 Thanks for doing this @PerfectLaugh 👍 💯

Thomasdezeeuw · 2019-07-19T17:53:15Z

Thank you @PerfectLaugh you've helped us with one of the major blockers for Mio v0.7!

PerfectLaugh added 8 commits July 15, 2019 04:34

New windows strategy copied from wepoll.

94107aa

Changed readv, writev function signature from ```&self``` to ```&mut self```

Changes function signature in SelectorInner

a4bbd15

Changes &self to &mut self in UdpStream::recv_bufs

432638f

Modify selector a bit

a0556f0

Write a lot I cannot remember :(

781f5fc

Get rid of Poll

7456a31

Explicitly unwrap the update function.

b151fc9

Added possible fix line on Token documentation.

Remove comment on Token documentation

0241a36

Thomasdezeeuw requested changes Jul 15, 2019

View reviewed changes

PerfectLaugh added 3 commits July 15, 2019 19:49

Changed as requests

90cc6a0

Limit selector_arc function scope as windows only

d6e0ca0

Stupid github hide my comments :(

2d5b76f

This was referenced Jul 15, 2019

Rewrite of the Windows implementation #1024

Closed

Poll.deregister() has no effect on Windows #633

Closed

Simplify tcp.rs and udp.rs repetition with macro

1ae0789

Remove HashMap handling

67356cd

piscisaureus reviewed Jul 15, 2019

View reviewed changes

src/sys/windows/selector.rs Show resolved Hide resolved

Fix debug assertions (Please check in test_tcp)

ad7c7f8

PerfectLaugh added 3 commits July 16, 2019 15:39

Change fmt::Debug in several structs

1eea024

Indent a bit

67b6c1f

Fix sourcerawsocket.rs doc typo

cbb3679

Thomasdezeeuw mentioned this pull request Jul 16, 2019

Testing in release mode on CI? #1035

Closed

Thomasdezeeuw reviewed Jul 16, 2019

View reviewed changes

PerfectLaugh added 2 commits July 16, 2019 18:33

Changed as requests

e8ac202

Comply with new tests

957fe52

piscisaureus reviewed Jul 17, 2019

View reviewed changes

src/sys/windows/afd.rs Outdated Show resolved Hide resolved

PerfectLaugh and others added 5 commits July 18, 2019 01:31

Pinned IO_STATUS_BLOCK

367635d

move lazy_static to windows only

f83545e

Merge pull request #1 from tokio-rs/master

1030eee

Merge mio master commits

Remove extern crate

e99c1b9

Add documentations

65b4305

Rename MioSocketState to WindowsSocketState

be4f769

Thomasdezeeuw reviewed Jul 19, 2019

View reviewed changes

PerfectLaugh added 2 commits July 19, 2019 19:46

Fix connection shutdown event::is_writable == false

b931aad

Changes requested by Review ( @Thomasdezeeuw )

4a45b58

Remove old traits method from sys/unix/udp.rs

a8546e2

Thomasdezeeuw self-requested a review July 19, 2019 16:28

Thomasdezeeuw approved these changes Jul 19, 2019

View reviewed changes

carllerche mentioned this pull request Jul 19, 2019

Remaining wepoll items. #1041

Closed

16 tasks

piscisaureus approved these changes Jul 19, 2019

View reviewed changes

carllerche approved these changes Jul 19, 2019

View reviewed changes

carllerche merged commit 65f19dd into tokio-rs:master Jul 19, 2019

Thomasdezeeuw mentioned this pull request Jul 20, 2019

mio::net::UdpSocket::connect() fails on windows with error 10014 (invalid pointer) #721

Closed

This was referenced Aug 20, 2019

A memory leak in Windows #919

Closed

Memory leak in Trust-DNS client when used in Windows hickory-dns/hickory-dns#692

Closed

reqwest never close socket connection in windows ! seanmonstar/reqwest#284

Closed

bdbai mentioned this pull request Apr 5, 2021

Internal Windows APIs are not available for UWP apps #1484

Closed

New windows implementation strategies from wepoll #1034

New windows implementation strategies from wepoll #1034

Conversation

PerfectLaugh commented Jul 15, 2019

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

carllerche commented Jul 15, 2019

PerfectLaugh commented Jul 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergejJurecko commented Jul 16, 2019

PerfectLaugh commented Jul 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PerfectLaugh commented Jul 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thomasdezeeuw commented Jul 19, 2019

PerfectLaugh commented Jul 19, 2019

carllerche commented Jul 19, 2019

Thomasdezeeuw commented Jul 19, 2019

carllerche commented Jul 19, 2019

piscisaureus left a comment

Choose a reason for hiding this comment

carllerche left a comment

Choose a reason for hiding this comment

Thomasdezeeuw commented Jul 19, 2019