Skip to content

Port to rustix 1.0.#230

Merged
fogti merged 13 commits intosmol-rs:masterfrom
sunfishcode:main
Apr 29, 2025
Merged

Port to rustix 1.0.#230
fogti merged 13 commits intosmol-rs:masterfrom
sunfishcode:main

Conversation

@sunfishcode
Copy link
Copy Markdown
Contributor

@sunfishcode sunfishcode commented Mar 18, 2025

rustix 1.0 introduced a new Buffer API which is used in epoll::wait, kqueue, and polling, so this patch updates the code to use the new spare_capacity mechanism for writing to the spare capacity of a Vec.

rustix 1.0 also changes timeout parameters from i32 milliseconds to Timespec seconds plus nanoseconds, which is more general and aligns with newer Linux syscalls such as epoll_pwait2.

This also fixes a bug in the wait function; in rustix 0.38, calling epoll::wait would clear the events list before adding new events, however polling's documentation states that "New events will be appended to events." Using spare_capacity in rustix 1.0, the list is not cleared first, so events are appended.

Closes #234.

@sunfishcode
Copy link
Copy Markdown
Contributor Author

sunfishcode commented Mar 18, 2025

I've now also tested this on illumos, to test the port backend. Everything passes except for the insert_twice test, however this doesn't appear to be a regression. The code is checking that inserting a fd twice gets an AlreadyExists error, however port_associate doesn't fail when the fd is already associated, and the port.rs code doesn't have any extra bookkeeping.

Copy link
Copy Markdown
Collaborator

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks! This is generally LGTM.

Comment thread src/epoll.rs Outdated
Comment thread src/poll.rs Outdated
Comment thread src/poll.rs Outdated
Comment thread src/port.rs Outdated
@taiki-e taiki-e requested review from fogti and notgull March 28, 2025 15:42
@fogti
Copy link
Copy Markdown
Member

fogti commented Mar 28, 2025

For the mentioned problem, I think it would be a better idea to put rustix::event::Timespec into our public API, and re-export it from the crate root, instead of wrapping the fallible conversion.

Alternatively, putting a wrapper type for that into the crate root, and expose appropriate conversions.

@taiki-e
Copy link
Copy Markdown
Collaborator

taiki-e commented Mar 30, 2025

I think the generally preferred behavior regarding time overflow is saturation or close to it.
smol-rs/async-io#87
smol-rs/async-io#86 (comment)

@sunfishcode
Copy link
Copy Markdown
Contributor Author

sunfishcode commented Apr 1, 2025

Makes sense; I've now changed the code to handle Duration->Timespec overflow by saturating to an infinite timeout.

The i686-unknown-freebsd failure looks real. I'm investigating.

@sunfishcode sunfishcode requested a review from taiki-e April 1, 2025 11:22
@sunfishcode
Copy link
Copy Markdown
Contributor Author

The i686-unknown-freebsd failure is real; I'm working on a fix.

Copy link
Copy Markdown
Collaborator

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks!

@fogti
Copy link
Copy Markdown
Member

fogti commented Apr 3, 2025

Sorry, I currently don't have time to properly review this (as I'm trying to finish a thesis); maybe after next week. I wish I'd knew how to test this for Hermit, but I currently don't.

rustix 1.0 introduced a new [`Buffer`] API which is used in `epoll::wait`,
`kqueue`, and `polling`, so this patch updates the code to use the new
[`spare_capacity`] mechanism for writing to the spare capacity of a `Vec`.

rustix 1.0 also changes timeout parameters from `i32` milliseconds to
`Timespec` seconds plus nanoseconds, which is more general and aligns with
newer Linux syscalls such as [`epoll_pwait2`].

This also fixes a bug in the `wait` function; in rustix 0.38, calling
`epoll::wait` would clear the events list before adding new events,
however polling's documentation [states] that "New events will be
appended to `events`." Using `spare_capacity` in rustix 1.0, the list is
not cleared first, so events are appended.

[`Buffer`]: https://docs.rs/rustix/latest/rustix/buffer/trait.Buffer.html
[`spare_capacity`]: https://docs.rs/rustix/latest/rustix/buffer/fn.spare_capacity.html
[`epoll_pwait2`]: https://www.man7.org/linux/man-pages/man2/epoll_pwait2.2.html
[states]: https://docs.rs/polling/latest/polling/struct.Poller.html#method.wait
Copy link
Copy Markdown
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.

Manual test suite run passes on x64_64-unknown-freebsd and x86_64-unknown-linux-gnu.

Please write a CHANGELOG.md entry for this, given that it changes how event vectors are handled: they aren't cleared anymore, this is a bugfix that brings the code back in line with what it should be, but that itself doesn't prevent downstream from accidentally ending up relying on the former, buggy behavior. (non-blocker)

@fogti fogti merged commit 8327466 into smol-rs:master Apr 29, 2025
25 of 26 checks passed
@fogti fogti mentioned this pull request Apr 29, 2025
@taiki-e
Copy link
Copy Markdown
Collaborator

taiki-e commented May 25, 2025

Published in 3.8.0.

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.

3 participants