Error Creating PollEvented for RawFd #2413 #2419
Conversation
Darksonn
added
A-tokio
|
You have some warnings that caused the CI to fail. |
Darksonn
added
the
S-waiting-on-author
|
@Darksonn There's currently an unused method now in the PR. I don't think it would have any impact by removing it (seems to be internal to io), but wasn't sure if I should just remove it. |
|
Yeah, in this case it should be removed. |
|
@Darksonn Removed. |
Darksonn
removed
the
S-waiting-on-author
There was a problem hiding this comment.
@dbcfd Overall the change in code looks good to me!
My main hesitation with adding this to the API is that it may be a point of confusion for new users. Someone new to Tokio may see both PollEvented::new and Poll::new_with_ready. If they intend to only read from the source (that is also writable), they may wonder why use new instead of new_with_ready.
It would require understanding the implementations of both that new_with_ready will not handle registering interest in HUP--errors should still be triggered without explicitly registering interest.
That being said, using the PollEvented API is not that usual so chances are the user is already in a situation where they have an understanding of Mio and the IO driver.
I think this might be best addressed with documentation. Indicate that new_with_ready should be used if your source does not support both read and write. |
I would be good with that! If you can add that to the documentation then I should be good for approving this. |
There was a problem hiding this comment.
Looks good. Thanks for adding the clarification about differentiating between the two methods.
I'd still like to get a quick review from @carllerche since this is an API addition.
dbcfd
force-pushed
the
poll-evented-error-2413
branch
from
8cd75a0
to
8580e1a
Compare
Add additional methods to allow PollEvented to be created with an appropriate `mio::Ready` state so that it can be properly registered with the reactor.
dbcfd
force-pushed
the
poll-evented-error-2413
branch
from
8580e1a
to
e9ee6c4
Compare
|
I think we are going to have to deprecate PollEvented long term (as we look into things like uring). I'm ok w/ merging this now. That said, could we also try to solve the problem without PollEvented. Should we add a |
|
Would love just being able to hook a rawfd type up to reactor for polling. |
|
I am merging this per @carllerche's comments. Please see #2519 for solving the problem without |
Add additional methods to allow PollEvented to be created with an appropriate mio::Ready state, so that it can be properly registered with the reactor. Fixes tokio-rs#2413
Add additional methods to allow PollEvented to be created with an appropriate
mio::Readystate so that it can be properly registered with the reactor.Fixes #2413