7992: io: AioSource now employs IO Safety#85
Conversation
WalkthroughThis pull request refactors file descriptor handling across Tokio to use borrowed and owned file descriptor types from the standard library instead of raw file descriptor primitives. The changes include: updating Cargo dependencies ( ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates several dependencies, including mio to 1.2.0, nix to 0.31.0, and mio-aio to version 2. Additionally, it refactors the AIO source registration and various test utilities to use safer file descriptor abstractions like BorrowedFd and OwnedFd from std::os::fd instead of RawFd. I have no feedback to provide.
|
PR Review - io: AioSource now employs IO Safety. This PR migrates AioSource from RawFd/AsRawFd to the IO-safe equivalents BorrowedFd/OwnedFd/AsFd (Rust 1.63+). Bumps mio to 1.2.0, nix to 0.31.0, mio-aio to v2. Overall well-scoped and correct. Key observations: (1) Correctness: LlSource/LioSource calling .as_raw_fd() on BorrowedFd for libc::sigevent is the idiomatic bridge between IO-safe Rust and raw C APIs. BorrowedFd lifetime guarantees validity for the call. (2) mem::forget ordering in io_async_fd_memory_leak.rs is correct - set_nonblocking borrow released before forget runs. (3) Retained RawFd/AsRawFd imports in the leak test are intentional and not dead code. (4) Breaking API change: AioSource::register signature change from RawFd to BorrowedFd is breaking for downstream implementors - even though FreeBSD-only and behind tokio_unstable, should be documented in CHANGELOG. (5) MioSource::reregister delegating to register is pre-existing intentional behavior. Verdict: Approve with minor suggestion - add CHANGELOG entry for the breaking AioSource::register trait signature change. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tokio/tests/io_async_fd.rs (1)
103-119:⚠️ Potential issue | 🟡 MinorFix panic labels in
set_nonblockingto match the actual fcntl ops.The
expect(...)messages referenceF_GETFD/F_SETFD, but the code callsF_GETFL/F_SETFL, which makes failure diagnostics misleading.Suggested patch
- let flags = nix::fcntl::fcntl(fd, F_GETFL).expect("fcntl(F_GETFD)"); + let flags = nix::fcntl::fcntl(fd, F_GETFL).expect("fcntl(F_GETFL)"); @@ - nix::fcntl::fcntl(fd, F_SETFL(flags)).expect("fcntl(F_SETFD)"); + nix::fcntl::fcntl(fd, F_SETFL(flags)).expect("fcntl(F_SETFL)");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tokio/tests/io_async_fd.rs` around lines 103 - 119, The panic messages in set_nonblocking refer to the wrong fcntl operations (they mention F_GETFD / F_SETFD) which is misleading; update the expect/panic strings to reference the actual ops used (F_GETFL and F_SETFL) and ensure the error context matches the calls to nix::fcntl::fcntl(fd, F_GETFL) and nix::fcntl::fcntl(fd, F_SETFL(flags)); adjust both the initial expect("fcntl(F_GETFD)") and the final expect("fcntl(F_SETFD)") to accurate messages like "fcntl(F_GETFL)" and "fcntl(F_SETFL)" (or similar descriptive text) so failures in set_nonblocking point to the correct syscall.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tokio/src/io/bsd/poll_aio.rs`:
- Around line 19-22: The change to AioSource::register to take BorrowedFd<'_>
breaks existing implementers; restore a non-breaking API by reintroducing the
original signature register(&mut self, fd: RawFd, token: usize) (or provide a
default method with that signature) and implement a small wrapper that converts
the RawFd into a BorrowedFd and calls the new BorrowedFd-based implementation
(or vice versa) so existing impls compile; update the trait to include both
signatures (one delegating to the other) and ensure the public trait symbol
AioSource::register remains usable by downstream code.
---
Outside diff comments:
In `@tokio/tests/io_async_fd.rs`:
- Around line 103-119: The panic messages in set_nonblocking refer to the wrong
fcntl operations (they mention F_GETFD / F_SETFD) which is misleading; update
the expect/panic strings to reference the actual ops used (F_GETFL and F_SETFL)
and ensure the error context matches the calls to nix::fcntl::fcntl(fd, F_GETFL)
and nix::fcntl::fcntl(fd, F_SETFL(flags)); adjust both the initial
expect("fcntl(F_GETFD)") and the final expect("fcntl(F_SETFD)") to accurate
messages like "fcntl(F_GETFL)" and "fcntl(F_SETFL)" (or similar descriptive
text) so failures in set_nonblocking point to the correct syscall.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb34b1f3-1f6c-4296-8815-4b7d1be58da6
📒 Files selected for processing (6)
tokio/Cargo.tomltokio/src/io/bsd/poll_aio.rstokio/tests/io_async_fd.rstokio/tests/io_async_fd_memory_leak.rstokio/tests/io_poll_aio.rstokio/tests/net_unix_pipe.rs
| pub trait AioSource { | ||
| /// Registers this AIO event source with Tokio's reactor. | ||
| fn register(&mut self, kq: RawFd, token: usize); | ||
| fn register(&mut self, kq: BorrowedFd<'_>, token: usize); | ||
|
|
There was a problem hiding this comment.
This is a public API break for AioSource implementers.
Changing register from raw-fd-style input to BorrowedFd<'_> will break downstream impls at compile time. If intentional, this should be treated as a breaking-change path (or have a compatibility strategy / explicit release-note callout).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tokio/src/io/bsd/poll_aio.rs` around lines 19 - 22, The change to
AioSource::register to take BorrowedFd<'_> breaks existing implementers; restore
a non-breaking API by reintroducing the original signature register(&mut self,
fd: RawFd, token: usize) (or provide a default method with that signature) and
implement a small wrapper that converts the RawFd into a BorrowedFd and calls
the new BorrowedFd-based implementation (or vice versa) so existing impls
compile; update the trait to include both signatures (one delegating to the
other) and ensure the public trait symbol AioSource::register remains usable by
downstream code.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The CodeRabbit AI reviewer is correct! The signature change is an API breakage and it cannot be made. A new method will need to be added to the trait with a default implementation, so that it does not break the implementers .
🤖 Augment PR SummarySummary: Make FreeBSD POSIX AIO integration use IO-safe file descriptor types.
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| pub trait AioSource { | ||
| /// Registers this AIO event source with Tokio's reactor. | ||
| fn register(&mut self, kq: RawFd, token: usize); | ||
| fn register(&mut self, kq: BorrowedFd<'_>, token: usize); |
There was a problem hiding this comment.
tokio::io::bsd::AioSource is publicly re-exported, so changing register from RawFd to BorrowedFd is a breaking API change for FreeBSD users; can you confirm this is acceptable for the release/changelog you’re targeting?
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The Augment AI reviewer is correct! The signature change is an API breakage and it cannot be made. A new method will need to be added to the trait with a default implementation, so that it does not break the existing implementations .
| # Everything else is optional... | ||
| bytes = { version = "1.2.1", optional = true } | ||
| mio = { version = "1.0.1", optional = true, default-features = false } | ||
| mio = { version = "1.2.0", optional = true, default-features = false } |
There was a problem hiding this comment.
These dependency bumps (mio/nix/mio-aio) could implicitly raise the effective MSRV above rust-version = "1.71"; consider ensuring CI still exercises/builds on 1.71 so this doesn’t regress older toolchains.
Severity: medium
Other Locations
tokio/Cargo.toml:115tokio/Cargo.toml:125tokio/Cargo.toml:160
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Augment AI reviewer is correct! The Minimum Supported Rust Version (MSRV) could change while updating dependencies. The Tokio project has a CI job that checks the build with 1.71, so such problems would be caught early!
value:useful; category:bug; feedback: The Claude AI reviewer is correct! The signature change is an API breakage and it cannot be made. A new method will need to be added to the trait with a default implementation, so that it does not break the implementers . |
2 participants
7992: To review by AI