7992: io: AioSource now employs IO Safety#91
Conversation
implementors Older implementors will continue to work without IO Safety. But newer implementors should implement AioSource::register_borrowed instead of AioSource::register.
WalkthroughThis pull request updates the tokio crate to support borrowed file descriptor registration for POSIX AIO sources. The main changes include: (1) bumping dependency versions in ✨ 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 |
|
TEST |
|
PR Review: IO Safety for AioSource Overall this is a well-structured change that correctly adopts IO safety idioms and updates dependencies in sync. The backwards-compatibility design (each default calling the other, with one panicking to break the cycle) is a known Rust pattern and is correctly implemented here. A few issues worth addressing: Bugs / Correctness The Typo in doc comment (poll_aio.rs, line 26): The text says "It's memory-safe safe, but not I/O safe." — double "safe" — should be "memory-safe". Misleading comment calls Design / API Runtime-only enforcement of the "implement at least one" contract (poll_aio.rs, line 19-20): The doc says "Implementors must implement at least one of Test Coverage No test for the backwards-compatibility path: The tests in Minor
Error message typo fix is a nice catch: The Summary:
|
![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 the mio, nix, and mio-aio dependencies and refactors the AioSource trait to utilize I/O-safe types like BorrowedFd and OwnedFd instead of RawFd. The register method is deprecated in favor of register_borrowed, with corresponding updates applied to the test suite. Feedback is provided regarding a minor typo in the documentation of the AioSource trait.
| /// | ||
| /// # Safety | ||
| /// | ||
| /// It's memory-safe safe, but not I/O safe. If the file referenced by `kq` gets dropped, then |
There was a problem hiding this comment.
There is a typo in the documentation: "memory-safe safe" should be "memory-safe".
| /// It's memory-safe safe, but not I/O safe. If the file referenced by `kq` gets dropped, then | |
| /// It's memory-safe, but not I/O safe. If the file referenced by `kq` gets dropped, then |
There was a problem hiding this comment.
value:good-to-have; category:documentation; feedback: The Gemini AI reviewer is correct! memory-safe safe sounds wrong. One of the occurrences of safe should be removed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tokio/tests/io_async_fd.rs (1)
106-114: Dead code:flags < 0check is unreachable.In nix 0.31.0,
fcntlreturnsResult<c_int, Errno>. The.expect()on line 106 already panics on error, so if execution reaches line 108,flagsis guaranteed to be the successful return value (valid file status flags, which are non-negative). Theif flags < 0branch can never execute.This is pre-existing dead code, but since you're touching this function, consider cleaning it up:
🧹 Suggested cleanup
fn set_nonblocking(fd: &OwnedFd) { use nix::fcntl::{OFlag, F_GETFL, F_SETFL}; let flags = nix::fcntl::fcntl(fd, F_GETFL).expect("fcntl(F_GETFL)"); - if flags < 0 { - panic!( - "bad return value from fcntl(F_GETFL): {} ({:?})", - flags, - nix::Error::last() - ); - } - let flags = OFlag::from_bits_truncate(flags) | OFlag::O_NONBLOCK; 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 106 - 114, The code performs nix::fcntl::fcntl(fd, F_GETFL).expect("fcntl(F_GETFL)") and then checks if the returned flags < 0, but expect() already panics on error and valid flags are non-negative, so remove the unreachable if flags < 0 { panic!(...) } branch; keep the single-line let flags = nix::fcntl::fcntl(fd, F_GETFL).expect("fcntl(F_GETFL)"); and any subsequent uses of flags unchanged (references: variable name flags and the fcntl call F_GETFL).tokio/tests/io_poll_aio.rs (1)
24-29: Please keep one fixture on the deprecated registration hook.Every
AioSourceimpl in this file now overridesregister_borrowed, so the compatibility fallback added intokio/src/io/bsd/poll_aio.rsis no longer exercised. A tiny legacy fixture that only implements the deprecatedregisterwould protect that path against regressions.Also applies to: 84-92, 225-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tokio/tests/io_poll_aio.rs` around lines 24 - 29, One of the tests removed the last use of the deprecated registration hook so the compatibility fallback in poll_aio.rs (the AioSource registration fallback path) is no longer exercised; restore a tiny legacy fixture that implements only the old deprecated register/deregister hook (instead of register_borrowed) so the fallback path is covered. Concretely, in the AioSource test fixtures (where implementations define register_borrowed and deregister), keep or add a minimal fixture that implements the deprecated register (and corresponding deregister) method signature only, ensuring the test suite hits the compatibility branch in poll_aio.rs that handles the deprecated registration API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tokio/tests/io_async_fd.rs`:
- Around line 106-114: The code performs nix::fcntl::fcntl(fd,
F_GETFL).expect("fcntl(F_GETFL)") and then checks if the returned flags < 0, but
expect() already panics on error and valid flags are non-negative, so remove the
unreachable if flags < 0 { panic!(...) } branch; keep the single-line let flags
= nix::fcntl::fcntl(fd, F_GETFL).expect("fcntl(F_GETFL)"); and any subsequent
uses of flags unchanged (references: variable name flags and the fcntl call
F_GETFL).
In `@tokio/tests/io_poll_aio.rs`:
- Around line 24-29: One of the tests removed the last use of the deprecated
registration hook so the compatibility fallback in poll_aio.rs (the AioSource
registration fallback path) is no longer exercised; restore a tiny legacy
fixture that implements only the old deprecated register/deregister hook
(instead of register_borrowed) so the fallback path is covered. Concretely, in
the AioSource test fixtures (where implementations define register_borrowed and
deregister), keep or add a minimal fixture that implements the deprecated
register (and corresponding deregister) method signature only, ensuring the test
suite hits the compatibility branch in poll_aio.rs that handles the deprecated
registration API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e1cb8a9-a970-43e5-863b-311485651dea
📒 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
value:valid-but-wont-fix; category:bug; feedback: The CodeRabbit AI reviewer is not correct! The idea is to deprecate the |
value:good-but-wont-fix; category:bug; feedback: The CodeRabbit AI reviewer is correct! The method returns Result, so any kind of problems should be returned as Err. But this is in tests code and it just makes sure that the error is not returned in C-style, i.e. a negative number. I do not do any harm, so there is no need to remove it. |
|
augment review |
value:good-to-have; category:documentation; feedback: The Claude AI reviewer is correct! |
value:valid-but-wont-fix; category:bug; feedback: The Claude AI reviewer is not correct! Changing the type of the argument of the old/existing |
🤖 Augment PR SummarySummary: This PR updates Tokio’s FreeBSD/POSIX AIO integration to use Rust’s I/O-safety APIs. Changes:
Technical Notes: The main behavioral intent is to reduce accidental misuse of raw file descriptors by propagating 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| /// Registers this AIO event source with Tokio's reactor. | ||
| fn register_borrowed(&mut self, kq: BorrowedFd<'_>, token: usize) { | ||
| // This default implementation serves to provide backwards compatibility with AioSource | ||
| // implementors written before 1.51.0 that only implemented the unsafe `register` method. |
There was a problem hiding this comment.
register_borrowed’s compat comment says older implementors “only implemented the unsafe register method”, but register isn’t declared unsafe here (it’s deprecated, but still safe). Consider adjusting the wording so the docs don’t imply an unsafe contract that doesn’t exist.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:incorrect-but-reasonable; category:bug; feedback: The Augment AI reviewer is not correct! Here unsafe is confused with Rust's unsafe keyword while it actually means that the usage of RawFD is more unsafe than using BorrowedFS/OwnedFD.
| fn register(&mut self, _kq: RawFd, _token: usize) { | ||
| // This default implementation exists so new AioSource implementors that implement the | ||
| // register_borrowed method can compile without the need to implement register. | ||
| unimplemented!("Use AioSource::register_borrowed instead") |
There was a problem hiding this comment.
tokio/src/io/bsd/poll_aio.rs:32: Because both register and register_borrowed have default implementations, an AioSource impl that forgets to override either will compile but panic at runtime when Tokio registers it (via this unimplemented!). Since AioSource is public, this is a fairly easy footgun for downstream implementors.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:incorrect-but-reasonable; category:bug; feedback: The Augment AI reviewer is not correct! The important function now is register_borrowed(). By default it just delegates to register(), so that all old implementors still work. They will see a deprecation warning but their build will still pass and the functionality will work at runtime. Any new implementors may not override neither of the functions because both of them have default implementations but they will quickly realize that they need to override register_borrowed() on the first attempt to use the function. Hopefully they will test the new implementation before using it in production.
| /// Registers this AIO event source with Tokio's reactor. | ||
| fn register_borrowed(&mut self, kq: BorrowedFd<'_>, token: usize) { | ||
| // This default implementation serves to provide backwards compatibility with AioSource | ||
| // implementors written before 1.51.0 that only implemented the unsafe `register` method. |
There was a problem hiding this comment.
tokio/src/io/bsd/poll_aio.rs:38: This comment describes the deprecated register as “unsafe”, but the method isn’t actually unsafe (it’s just not I/O-safe), which could mislead implementors reading the compatibility note.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:incorrect-but-reasonable; category:bug; feedback: The Augment AI reviewer is not correct! Here unsafe is confused with Rust's unsafe keyword while it actually means that the usage of RawFD is more unsafe than using BorrowedFS/OwnedFD.
| /// | ||
| /// # Safety | ||
| /// | ||
| /// It's memory-safe safe, but not I/O safe. If the file referenced by `kq` gets dropped, then |
There was a problem hiding this comment.
tokio/src/io/bsd/poll_aio.rs:26: Doc typo: “memory-safe safe” reads like an accidental duplication in the # Safety text.
Severity: low
🤖 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:documentation; feedback: The Augment AI reviewer is correct! memory-safe safe sounds wrong. One of the occurrences of safe should be removed.
2 participants
7992: To review by AI