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

Modernize epoll API #1110

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

SUPERCILEX
Copy link
Contributor

This PR:

  • Unifies the epoll backends
  • Implements the ideas in Weird epoll comment around close #1100, meaning the user can now pass in a Vec and uninits with the same API
  • Enables using epoll::wait in non-alloc crates

cc @notgull for extra review

@sunfishcode this buffer trait idea seems pretty cool, maybe we should use this pattern in other areas that take uninits?

All of this should be fully backwards compatible.

@SUPERCILEX SUPERCILEX force-pushed the epoll branch 12 times, most recently from f5f739c to db893ae Compare August 15, 2024 20:13
@SUPERCILEX
Copy link
Contributor Author

SUPERCILEX commented Aug 15, 2024

Ok, I think the remaining questions are:

  • Should we clear events in the Vec API? I have a slight preference against this, but I can also see not clearing leading to weird bugs.
  • What do we do if the Vec has zero capacity? Do we auto-alloc for the user or let the syscall error out. I'm leaning towards let the syscall error out, but again this could be confusing so I could see wanting to make the API friendlier.

Copy link
Contributor

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Big fan of the idea here, but a few notes.

src/event/epoll.rs Show resolved Hide resolved
src/event/epoll.rs Outdated Show resolved Hide resolved
@SUPERCILEX SUPERCILEX mentioned this pull request Aug 16, 2024
30 tasks
Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

I need some more time to think about this.

The EventBuffer trait as a way to migrate away from EventVec and toward just using a plain Vec or array makes sense.

That said, I have reservations about the use of B::Out return value, which means that the value you get back, which determines how you use this function, changes significantly depending on what type of thing you pass in, and it's not clear from the public interface how users would figure out this relationship.

I like how eg. read and read_uninit have intuitive signatures that don't require you to learn about a trait and look up what types implement it, and I like how they give simple error messages if you use the wrong type.

src/event/epoll.rs Outdated Show resolved Hide resolved
@SUPERCILEX
Copy link
Contributor Author

Aw, I thought the dependent type theory stuff was the cool part. 😁 I do agree that it's pretty high on the complexity scale though. I've tried tweaking it so the docs are actually usable:
image

And the error looks like this which is honestly pretty decent IMO:

error[E0277]: the trait bound `{integer}: EventBuffer` is not satisfied
   --> src/event/mod.rs:33:22
    |
33  |     epoll::wait(CWD, 0, 0);
    |     -----------      ^ the trait `EventBuffer` is not implemented for `{integer}`
    |     |
    |     required by a bound introduced by this call
    |
    = help: the following other types implement trait `EventBuffer`:
              &'a mut [Event]
              &'a mut [MaybeUninit<Event>]
              &mut EventVec
              &mut Vec<Event>

@notgull
Copy link
Contributor

notgull commented Aug 16, 2024

cc #908, which implemented a trait similar to EventBuffer but more generic in scope

@SUPERCILEX
Copy link
Contributor Author

Doh! I'd totally forgotten about that, sorry. And here I thought this was a new idea lol.

So if this space was already explored, then maybe we should just add a wait_uninit method instead and expect people to use spare_capacity_mut? I'm not a huge fan of that approach for Vecs because it means the caller has to take on the unsafety of setting the length if they want to pass the vec along rather than using the returned slice.

If we do want to try the fancy pantsy approach, I can make it look a little more like @notgull's original proposal while maintaining the Internal type so nobody can use it but us.

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@SUPERCILEX
Copy link
Contributor Author

I've been working with getxattr and @notgull's approach seems more and more convenient. It'd be really nice to be able to pass in both a MaybeUninit and plain buffer depending on the needs. I think I'm in favor of resurrecting some variant of the dependent typing stuff... thoughts?

@notgull
Copy link
Contributor

notgull commented Nov 3, 2024

I still advocate for my Buffer trait approach. It simplifies and extends so many use cases.

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

Successfully merging this pull request may close these issues.

3 participants