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

Rework the API for I/O safety #123

Merged
merged 2 commits into from
Aug 4, 2023
Merged

Rework the API for I/O safety #123

merged 2 commits into from
Aug 4, 2023

Conversation

notgull
Copy link
Member

@notgull notgull commented Jun 25, 2023

This reworks the polling API with I/O safety in mind. See #38 and smol-rs/smol#244 for more information.

This is a breaking change that makes the add() family of functions unsafe. I am very open to alternatives to this change.

The indirect effects of this change is that is makes async_io::Async::get_mut and read/write_with_mut unsafe, as well as having implications for the AsyncRead/AsyncWrite impls as well. See smol-rs/async-io#142. I would appreciate community feedback on these changes.

src/epoll.rs Show resolved Hide resolved
src/epoll.rs Outdated Show resolved Hide resolved
@notgull
Copy link
Member Author

notgull commented Jul 13, 2023

@smol-rs/admins Can anyone review this PR? Given that this change will have ecosystem wide effects, I'd like to make sure this is what we want.

@fogti
Copy link
Member

fogti commented Jul 13, 2023

Make sure to also review the corresponding PR for async-io. I think most of it looks good. idk if/what it would do to code further downstream to async-io and others. (API changes like the new IoSafe trait should maybe documented better /+ more prominently.

@notgull notgull requested a review from taiki-e July 30, 2023 15:50
@notgull
Copy link
Member Author

notgull commented Jul 30, 2023

@taiki-e You have good judgement when it comes to this sort of thing, so I'll defer to you before I merge this.

@notgull
Copy link
Member Author

notgull commented Aug 4, 2023

I'll merge this now and we can discuss the changes in greater depth later.

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