rustix icon indicating copy to clipboard operation
rustix copied to clipboard

Modernize epoll API

Open SUPERCILEX opened this issue 1 year ago • 4 comments

This PR:

  • Unifies the epoll backends
  • Implements the ideas in #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 avatar Aug 15 '24 04:08 SUPERCILEX

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.

SUPERCILEX avatar Aug 15 '24 20:08 SUPERCILEX

Aw, I thought the dependent type theory stuff was the cool part. :grin: 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>

SUPERCILEX avatar Aug 16 '24 18:08 SUPERCILEX

cc https://github.com/bytecodealliance/rustix/pull/908, which implemented a trait similar to EventBuffer but more generic in scope

notgull avatar Aug 16 '24 18:08 notgull

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.

SUPERCILEX avatar Aug 16 '24 19:08 SUPERCILEX

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?

SUPERCILEX avatar Nov 02 '24 22:11 SUPERCILEX

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

notgull avatar Nov 03 '24 01:11 notgull

I've now updated #1290 to implement the basic idea here.

sunfishcode avatar Feb 26 '25 00:02 sunfishcode

I'm going to go ahead and close this as #1290 contains the generalization of this code for general-purpose use. Thanks for the idea here, and thanks for your patience!

sunfishcode avatar Feb 26 '25 01:02 sunfishcode

Sounds good, I'm still reviewing #1290.

SUPERCILEX avatar Feb 26 '25 01:02 SUPERCILEX