-
Notifications
You must be signed in to change notification settings - Fork 340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add std::os::unix::{register, unregister, Interest} #626
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leo60228 thanks so much for starting this work! I think this is an interesting step. I've got a few design notes though:
- We don't want to expose
mio::Evented
as part of the public interface. - Any design here should provide people access to the
epoll
instance that we're running; do you have an idea how you would go about this? - Why did you choose to expose
Entry
instead ofWatcher
?
I'm thinking exposing std::os::unix::Watcher
might make more sense than exposing std::os::Entry
for most cases. In many ways Entry
is purely an implementation detail that we don't want to expose to end-users. I really like the idea of having an async_std::os::unix
version of our watcher abstraction, and another for async_std::os::windows
. That way we can differentiate between RawFd
and RawSocket
in constructor interfaces.
edit: to answer my own question: the design in #293 (comment) doesn't mention Watcher
but instead only talks about register/unregister/Interest
. I guess that's what this is based on. Though I do feel that having a Watcher
would likely be closer to what we would want to expose.
src/net/driver/mod.rs
Outdated
reactor.notify_token = entry.token; | ||
|
||
Ok(reactor) | ||
} | ||
|
||
/// Registers an I/O event source and returns its associated entry. | ||
fn register(&self, source: &dyn Evented) -> io::Result<Arc<Entry>> { | ||
pub fn register(&self, source: &dyn Evented, fd: Option<c_int>) -> io::Result<Arc<Entry>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Evented
shouldn't be part of our pub interface, and fd
should be RawFd
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module is pub(crate)
, and RawFd is Unix-only. I didn't want to copy/paste the method and struct, though perhaps that's preferable for memory usage? It'd be heavily preferable to use Option<!>
instead of having to put every call in a cfg, though.
@@ -82,7 +87,7 @@ impl Reactor { | |||
} | |||
|
|||
/// Deregisters an I/O event source associated with an entry. | |||
fn deregister(&self, source: &dyn Evented, entry: &Entry) -> io::Result<()> { | |||
pub fn deregister(&self, source: &dyn Evented, entry: &Entry) -> io::Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Evented
shouldn't be part of our pub interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module is pub(crate)
.
Note that you will also want to support |
How does that work? Is there an EventedHandle in mio like there is EventedFd? |
mio doesn't seem to have a way to register a RawHandle. |
d8d18c4
to
7660dd5
Compare
FWIW, I switched from |
Would it be possible to also expose methods on |
It would be great if we could have access to what was the actual event. |
This will need a different approach, now that we are no longer using mio, and use smol as the underlying runtime. |
Yeah, this design doesn't make sense anymore, especially since you can just use |
Closes #293, based on #293 (comment)