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

feat: Expose other kqueue filters #112

Merged
merged 8 commits into from
Apr 13, 2023
Merged

Conversation

notgull
Copy link
Member

@notgull notgull commented Feb 3, 2023

This PR builds on the work done in smol-rs/polling#83 in order to be able to wait on kqueue events aside from file descriptor readiness. It accomplishes this by replacing the raw field in Source with a Registration field that, on kqueue platforms, accommodates all of the currently defined filters. This allows things like signals and Childs to be handled by Async<T> as well.

Draft until there is a new release of polling. I also need to test it on a BSD virtual machine.

@notgull notgull force-pushed the notgull/kqueue-other-filters branch from 4ee339f to 0a367ca Compare March 8, 2023 19:49
@notgull
Copy link
Member Author

notgull commented Mar 8, 2023

The new polling version is out now! I've marked this as ready

@notgull notgull marked this pull request as ready for review March 8, 2023 19:50
@notgull notgull requested a review from taiki-e April 6, 2023 22:37
@taiki-e taiki-e requested a review from fogti April 10, 2023 08:12
src/reactor.rs Outdated Show resolved Hide resolved
src/reactor/registration.rs Outdated Show resolved Hide resolved
@notgull
Copy link
Member Author

notgull commented Apr 10, 2023

Now that I look at it, maybe tying this to Async<T> isn't the best idea. The API is kind of obtuse this way, since you need to call readable(), and writable() will just silently hang forever. Maybe it would be better to have another type that just has a single ready() method.

Thoughts? Maybe we can call this type Filter<T>.

@fogti
Copy link
Member

fogti commented Apr 11, 2023

writable() will just hang forever

That sounds pretty bad, it should terminate either with {an I/O error or a panic} if possible...

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.

imo all one-lines which can end up in a hot path (e.g. Registration::modify) should be marked #[inline]

src/os/kqueue.rs Show resolved Hide resolved
src/os/kqueue.rs Show resolved Hide resolved
src/reactor/kqueue.rs Outdated Show resolved Hide resolved
@taiki-e taiki-e removed their request for review April 13, 2023 08:23
@notgull notgull merged commit 913b236 into master Apr 13, 2023
@notgull notgull deleted the notgull/kqueue-other-filters branch April 13, 2023 13:07
@notgull notgull mentioned this pull request Sep 25, 2023
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