Skip to content
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

Reduce Mio's portable API to only support edge triggered notifications (no oneshot). #928

Closed
carllerche opened this issue Apr 25, 2019 · 23 comments
Labels
rfc Request for comments.
Milestone

Comments

@carllerche
Copy link
Member

As far as I know, when used as a portable API, Mio is only used with edge triggered notifications. Additionally, Mio's support for portable oneshot notifications is incorrect.

Given this, I propose reducing Mio's portable API to simply support edge triggered notifications. Roughly (not factoring in all planned changes), Evented would be updated to:

pub trait Evented {

    fn register(
        &mut self, 
        register: &mut Register, 
        token: Token, 
        interests: Interests
    ) -> Result<()>;

    fn deregister(&mut self, register: &mut Register) -> Result<()>;
}

We could possibly keep a PollOption around for forwards compatibility.

Notably, it is no longer possible to change the interest after initial registration without deregistering first.

It would be possible to add platform specific APIs to access the other poll modes, but those would not be done via the Evented trait.

Thoughts?

cc/ @asomers @Thomasdezeeuw @stjepang

@asomers
Copy link
Collaborator

asomers commented Apr 25, 2019

+1 .

@Thomasdezeeuw
Copy link
Collaborator

So this would remove both oneshot and level triggers, As well as reregistering? I don't have a use case for oneshot trigger and if its incorrect in the first place I'm +1. However I do use both level trigger and reregistering, so I'm -1 on those. What are the reasons to remove them?

@carllerche
Copy link
Member Author

@Thomasdezeeuw re-registering has also been a source of portability problems because platforms behave differently...

Could you elaborate on your case for level & re-registering? It is always possible to impl those behaviors on top of edge triggered, but w/ some overhead.

@carllerche
Copy link
Member Author

@Thomasdezeeuw Are you available to talk in gitter? gitter.im/tokio-rs/mio

@seanmonstar
Copy link
Member

There's this issue in hyper asking for level-triggered events for their listener, as it's noticeably faster: hyperium/hyper#1493

I don't otherwise have an opinion on this, just thought I'd share what others have asked for.

@carllerche
Copy link
Member Author

@seanmonstar IIRC, the level vs. edge was unrelated.

@Thomasdezeeuw
Copy link
Collaborator

After talking with @carllerche on gitter we decided to remove level triggers.

@carllerche
Copy link
Member Author

@asomers can you think of any reason why reregister would cause problems w/ kqueue if oneshot is removed? kqueue uses separate events for readable vs. writable. Since we don't know the original registration when calling reregister, we always set both. Either setting interest or setting EV_DELETE (code).

Does this strategy seem correct?.

@asomers
Copy link
Collaborator

asomers commented Apr 27, 2019

No, I don't think there's any problem with reregister. In fact, I don't think reregister has problems even with oneshots. I think it's only deregister that causes problems with oneshots.

@ghost
Copy link

ghost commented Apr 29, 2019

Perfect, it seems this is a totally reasonable plan then and I'd be happy to see the API simplified! Shall we add this issue to the 0.7 milestone list?

@carllerche carllerche added this to the v0.7 milestone Apr 29, 2019
@carllerche
Copy link
Member Author

@stjepang ah yes... i forgot. it was easier for me to just add it then reply to you to add it though 👍

@ghost
Copy link

ghost commented May 9, 2019

I'll prepare a PR removing oneshot and level triggered notifications, as well as the reregister method.

@Thomasdezeeuw Thomasdezeeuw added the rfc Request for comments. label Jun 7, 2019
@Thomasdezeeuw
Copy link
Collaborator

@stjepang How far are you with a pr? @carllerche and I discussed this again today and we think we should remove PollOpt and always use edge triggers.

@Thomasdezeeuw
Copy link
Collaborator

I've created #975.

@Thomasdezeeuw
Copy link
Collaborator

This was done in #975.

@rom1v
Copy link
Contributor

rom1v commented Jul 10, 2019

Hi,

I just came across this issue after my issue #911 has been closed.

Mio is only used with edge triggered notifications.

Personnally, I currently use mio only in level-triggered mode (in a tool providing reverse-tethering for Android), for example here or here).

The implementation relies on poll() being level-triggered. By the way, I initially wrote the application in Java (multi-platform), which only provides level-triggered notifications (like select() or poll() in C).

I guess I will have to adapt the Rust version to use edge-triggered notifications.

After talking with @carllerche on gitter we decided to remove level triggers.

Is this discussion public?

@Thomasdezeeuw
Copy link
Collaborator

@rom1v I can take a look at your code to see how hard it is to use edge triggers. As for the discussion it was public on Gitter, likely around the same time I made that comment.

@carllerche
Copy link
Member Author

I'm not necessarily against adding back access to level triggered events, but not at the "portable API" level. As in, we can discuss ways to provide the full feature set of the various OSes at an OS specific level.

@carllerche
Copy link
Member Author

The specific discussion is about the API that mio should expose as the portable API (works on all supported platforms).

@rom1v
Copy link
Contributor

rom1v commented Jul 10, 2019

I'm not necessarily against adding back access to level triggered events, but not at the "portable API" level.

(I don't know how Java implements it, but it provides level-triggered events for every platform. See SelectableChannel)

@0xpr03
Copy link

0xpr03 commented Jan 26, 2021

So if I read this correctly there is currently no way to use level triggered events on linux ? My use case is notify-rs/notify#267 (coming from upgrading to 0.7 in notify-rs/notify#278 ) which leads to using level triggered events on linux to avoid missing data changes.

@Thomasdezeeuw
Copy link
Collaborator

@0xpr03 I've read through notify-rs/notify#278 and I think it's doing the right thing. The loop at https://github.com/notify-rs/notify/pull/278/files#diff-b38928aaabe2c7f4a3272295a3c8bb05873ab22a7275c330934cd4a0da1d5de5R206 should ensure that all events are read and that new ionotify events generate new Mio events. Is there a regression test for notify-rs/notify#267 somewhere? Because I think it would pass it.

@0xpr03
Copy link

0xpr03 commented Jan 26, 2021

@Thomasdezeeuw ah thanks, I think you're right! Sorry for the noise, maybe this can at least help other people.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc Request for comments.
Projects
None yet
Development

No branches or pull requests

6 participants