Make Event a wrapper around a platform event #983

Thomasdezeeuw · 2019-06-15T09:19:56Z

Now Event is just a wrapper around a platform specific event to provide
a nice API, with methods such as is_readable.

This removes the Ready type and moves all its methods to the Event type.

TODO:

Now Event is just a wrapper around a platform specific event to provide a nice API, with methods such as is_readable. This removes the Ready type and moves all its methods to the Event type.

Uses Event::new and that is gone now.

Ready is gone. Also fixes some type-o's while in the Waker docs.

Thomasdezeeuw · 2019-06-15T10:18:42Z

Windows is left, the other platforms work.

This basically moves the old Event and Ready into the windows module. Someone with more knowledge about IOCP and Windows should really look at this, the currently situation isn't great.

Thomasdezeeuw · 2019-06-15T11:04:54Z

I've made an attempt at fixing Windows, basically moving Event and Ready into the windows module and pretend nothing changed. We already wanted a major rewrite of the Window's side of things, I just added to the mess here and hopefully that's acceptable for now.

Thomasdezeeuw · 2019-06-15T11:09:42Z

waker_multiple_wakeups_same_thread fails on Windows because it explicitly check for a single event, but Windows creates three. @carllerche do we just want to make the test less strict?

Now it accept mutltiple waker events, rather then just one.

Thomasdezeeuw · 2019-06-17T10:43:40Z

With 31e271a Windows also works.

Thomasdezeeuw · 2019-06-17T10:45:45Z

src/sys/unix/epoll.rs

+        // TODO: add readiness.
+        f.debug_struct("Event")
+            .field("token", &self.token())
+            //.field("readiness", &self.readiness())


Can we do with readiness? I could create a function epoll_event/kevent -> Ready, but is worth the effort?

I don't understand the question / problem. Instead of readiness, we could also include the details like "readable", "writable", "lio", ...

I've moved the Debug impl to event_imp.rs.

Thomasdezeeuw · 2019-06-17T10:47:10Z

src/sys/mod.rs

@@ -1,14 +1,16 @@
 #[cfg(unix)]
 pub use self::unix::{
-    pipe, set_nonblock, EventedFd, Events, Io, Selector, TcpListener, TcpStream, UdpSocket, Waker,
+    pipe, set_nonblock, Event, EventedFd, Events, Io, RawEvent, Selector, TcpListener, TcpStream,


Not sure if I like the name RawEvent, would SysEvent be better?

SysEvent is probably best as we are going for "sys" terminology.

I mentioned it in an in-progress review, but what about sys::Event? It is a crate private type anyway.

sys::Event and SysEvent are two different types. sys::Event is a wrapper around SysEvent to implement token, is_readable, etc. and SysEvent is just an alias for epoll_event/kevent.

Thomasdezeeuw · 2019-06-17T10:48:49Z

test/test_tcp_shutdown.rs


    println!("SHUTTING DOWN");
    // Now, shutdown the write half of the socket.
    socket.shutdown(Shutdown::Write).unwrap();

-    wait!(poll, is_readable);
+    wait!(poll, is_readable, true);


This is a function change.

Before this wouldn't expect hup here.

Thomasdezeeuw · 2019-06-17T10:49:19Z

test/test_waker.rs


    expect_no_events(&mut poll, &mut events);

    handle1.join().unwrap();
    handle2.join().unwrap();
 }
+
+fn expect_waker_event(poll: &mut Poll, events: &mut Events, token: Token) {


This because less strict, allow multiple waker events rather then only one.

carllerche

Looks pretty good 👍 thanks for putting this together. I had a few minor questions inline.

carllerche · 2019-06-17T17:48:28Z

src/event_imp.rs

-}
+    /// Get access to the platform specific event, the returned value differs
+    /// per platform.
+    pub fn raw_event(&self) -> &sys::RawEvent {


Is this function intentionally part of the public API?

Yes, this allows users to access the underlying system event for platform specific extensions. You previously mentioned accessing kevent for optimising Tokio.

Ah, my intent was not to expose RawEvent but have kqueue only fns directly on Event (conditionally defined). WDYT? Either way, we can punt exposing the raw event until we get to exposing kqueue specific data.

carllerche · 2019-06-17T17:49:57Z

src/event_imp.rs

-pub fn ready_from_usize(events: usize) -> Ready {
-    Ready::from_usize(events)
+    /// Create an `Event` from a platform specific event.
+    pub fn from_raw_event(raw_event: sys::RawEvent) -> Event {


Is this function intentionally part of the public API?

Also, if we are bikeshedding names, what about:

sys::Event

fn as_sys(&self) -> &sys::Event

fn from_sys(sys::Event)

carllerche · 2019-06-17T17:50:38Z

src/sys/mod.rs

@@ -1,14 +1,16 @@
 #[cfg(unix)]
 pub use self::unix::{
-    pipe, set_nonblock, EventedFd, Events, Io, Selector, TcpListener, TcpStream, UdpSocket, Waker,
+    pipe, set_nonblock, Event, EventedFd, Events, Io, RawEvent, Selector, TcpListener, TcpStream,


I mentioned it in an in-progress review, but what about sys::Event? It is a crate private type anyway.

carllerche · 2019-06-17T17:52:12Z

src/sys/unix/epoll.rs

+        // TODO: add readiness.
+        f.debug_struct("Event")
+            .field("token", &self.token())
+            //.field("readiness", &self.readiness())


I don't understand the question / problem. Instead of readiness, we could also include the details like "readable", "writable", "lio", ...

carllerche

Approving, but I recommend removing fn sys_event and fn from_sys_event for now and consider adding it back when we approach exposing kqueue specific data.

carllerche · 2019-06-17T21:44:02Z

Very nice job 👍

Thomasdezeeuw · 2019-06-17T21:50:39Z

I've removed the system event from the public API and opened #990.

Make Event a wrapper around a platform event

4d89c0b

Now Event is just a wrapper around a platform specific event to provide a nice API, with methods such as is_readable. This removes the Ready type and moves all its methods to the Event type.

Thomasdezeeuw requested review from carllerche and asomers June 15, 2019 09:19

Thomasdezeeuw added 7 commits June 15, 2019 11:28

Update waker tests

3db27e9

Fix Waker example

e2b7506

Remove Event example

4f72abd

Uses Event::new and that is gone now.

Fix doc links to Ready

6645a99

Ready is gone. Also fixes some type-o's while in the Waker docs.

Format the Waker tests

1417bad

Update epoll implementation

6262ccd

Remove more references to Ready from docs

629260c

Fix Windows platform

ec52389

This basically moves the old Event and Ready into the windows module. Someone with more knowledge about IOCP and Windows should really look at this, the currently situation isn't great.

Thomasdezeeuw added 2 commits June 17, 2019 12:39

Make expect_waker_event test less strict

31e271a

Now it accept mutltiple waker events, rather then just one.

Fix test_write_shutdown

8662b97

Thomasdezeeuw commented Jun 17, 2019

View reviewed changes

Only check for hup on kqueue platforms

9092a37

Thomasdezeeuw marked this pull request as ready for review June 17, 2019 11:37

Run rustfmt

8d379a6

This was referenced Jun 17, 2019

Consider Separating Registration Data from Signal Data #759

Closed

Reconsider providing hup as a readiness value for all *nix platforms. #941

Closed

Thomasdezeeuw added this to the v0.7 milestone Jun 17, 2019

carllerche reviewed Jun 17, 2019

View reviewed changes

Thomasdezeeuw added 3 commits June 17, 2019 23:09

Rename RawEvent to SysEvent

b648db9

Show readable, writable etc. in Debug impl for Event

6e14b5a

Rename Event::raw_event to sys_event

6866d53

carllerche approved these changes Jun 17, 2019

View reviewed changes

Thomasdezeeuw added 2 commits June 17, 2019 23:46

Remove Event::sys_event

9571c78

Make Event::from_sys_event private

bbd861c

Thomasdezeeuw mentioned this pull request Jun 17, 2019

Consider exposing OS specific event #990

Closed

carllerche merged commit 1eb579c into tokio-rs:master Jun 18, 2019

Thomasdezeeuw deleted the platform-event branch June 24, 2019 14:43

kleimkuhler mentioned this pull request Nov 8, 2019

Wrap kqueue::Events so that it can be Send and Sync #1130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Event a wrapper around a platform event #983

Make Event a wrapper around a platform event #983

Thomasdezeeuw commented Jun 15, 2019 •

edited

Loading

Thomasdezeeuw commented Jun 15, 2019

Thomasdezeeuw commented Jun 15, 2019

Thomasdezeeuw commented Jun 15, 2019

Thomasdezeeuw commented Jun 17, 2019

Thomasdezeeuw Jun 17, 2019

carllerche Jun 17, 2019

Thomasdezeeuw Jun 17, 2019

Thomasdezeeuw Jun 17, 2019

carllerche Jun 17, 2019

carllerche Jun 17, 2019

Thomasdezeeuw Jun 17, 2019

Thomasdezeeuw Jun 17, 2019

Thomasdezeeuw Jun 17, 2019

Thomasdezeeuw Jun 17, 2019

carllerche left a comment

carllerche Jun 17, 2019

Thomasdezeeuw Jun 17, 2019

carllerche Jun 17, 2019

carllerche Jun 17, 2019

carllerche Jun 17, 2019

carllerche Jun 17, 2019

carllerche left a comment

carllerche commented Jun 17, 2019

Thomasdezeeuw commented Jun 17, 2019

Make Event a wrapper around a platform event #983

Make Event a wrapper around a platform event #983

Conversation

Thomasdezeeuw commented Jun 15, 2019 • edited Loading

Thomasdezeeuw commented Jun 15, 2019

Thomasdezeeuw commented Jun 15, 2019

Thomasdezeeuw commented Jun 15, 2019

Thomasdezeeuw commented Jun 17, 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

carllerche left a 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

carllerche left a comment

Choose a reason for hiding this comment

carllerche commented Jun 17, 2019

Thomasdezeeuw commented Jun 17, 2019

Thomasdezeeuw commented Jun 15, 2019 •

edited

Loading