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

Add level and edge triggered modes to the poller #59

Merged
merged 3 commits into from
Dec 30, 2022
Merged

Conversation

notgull
Copy link
Member

@notgull notgull commented Dec 19, 2022

At the moment, polling only supports oneshot-triggered event polling. However, most of the backends polling uses also supports level-triggered operation and some even support edge-triggered operations. While oneshot polling works for the async-io usecase, other users may want to use other polling modes. This PR adds alternative polling modes and avenues to use to react if one isn't available.

Note that we may be able to support level-triggered operation on Solarish, with some extra elbow grease. However, unless there's another way I couldn't see, the only way I could see to do this would be to keep track of the level-trigger file descriptors through a HashSet (wrapped in a Mutex) and that would add some performance burden to the polling operation.

Copy link
Member

@fogti fogti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks overall ok, but I dislike the code duplication in error cases.

src/lib.rs Show resolved Hide resolved
src/poll.rs Outdated Show resolved Hide resolved
src/poll.rs Outdated Show resolved Hide resolved
@fogti
Copy link
Member

fogti commented Dec 28, 2022

the only way I could see to do this would be to keep track of the level-trigger file descriptors through a HashSet (wrapped in a Mutex)

this makes stuff way more complicated, because it introduces (additional) cases where the set of file descriptors can become out-of-sync with the OS view of file descriptors (e.g. a file descriptor is closed, and a new one is opened with the same ID), where the reactor would need to be notified; on other OSes this is handled mostly automatically by the OS view the native interface (although epoll on Linux has some cases regarding cloned file descriptors where this doesn't happen in the way usually expected); but it has a realistic chance of making it worse.

@notgull
Copy link
Member Author

notgull commented Dec 28, 2022

this makes stuff way more complicated, because it introduces (additional) cases where the set of file descriptors can become out-of-sync with the OS view of file descriptors (e.g. a file descriptor is closed, and a new one is opened with the same ID), where the reactor would need to be notified; on other OSes this is handled mostly automatically by the OS view the native interface (although epoll on Linux has some cases regarding cloned file descriptors where this doesn't happen in the way usually expected); but it has a realistic chance of making it worse.

I see, thanks for confirming the gut feeling I had as to why we shouldn't do this. However, if this is an issue, isn't the poll() fallback backend also affected by this?

#51 is probably related.

src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@fogti fogti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks now ok overall.

But could we add some tests to make sure that it works?

@notgull
Copy link
Member Author

notgull commented Dec 30, 2022

It looks like there's an issue with kqueue, at least on MacOS. Let me spin up a FreeBSD VM so that I can check if it's there as well.

@notgull
Copy link
Member Author

notgull commented Dec 30, 2022

Hmm, it looks like there's some subtlety with EV_CLEAR and changing modes in kqueue that I'm not aware of. On FreeBSD, for some reason, when I switch it to oneshot mode, it takes a while after the write to the TCP pipe for the new notification to be registered, so I have to add a non-zero timeout to the wait calls.

It's probably something obvious that I'm missing. I'll take another run at it in the morning.

@notgull
Copy link
Member Author

notgull commented Dec 30, 2022

I think I've figured out the issue; sometimes, on kqueue, it takes a moment for the new events to be registered into queue after they're used. We just have to account for this in the tests.

At this stage, I think all of the issues with this PR are resolved.

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

Successfully merging this pull request may close these issues.

2 participants