7782: Support as many abstract datagram unix domain socket operations as possible#75
7782: Support as many abstract datagram unix domain socket operations as possible#75martin-augment wants to merge 3 commits intomasterfrom
Conversation
Deduplicates the path2socket logic into our `SocketAddr` constructor. Where MIO currently doesn’t support using a STD Unix SocketAddr, a warning for this inconsistent behaviour is added to the documentation.
WalkthroughThe changes introduce a centralized ✨ Finishing Touches
🧪 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances Tokio's support for abstract Unix domain sockets, primarily on Linux and Android. It centralizes the logic for parsing socket addresses, including abstract paths, into a dedicated Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Code ReviewOverall this is a clean refactor. Consolidating abstract socket path logic into SocketAddr::from_path eliminates real duplication across listener.rs, stream.rs, and datagram/socket.rs. The FIXME comments are appropriate and clearly document the limitations pending upstream MIO support. Bug: Incorrect Android import in test file In tokio/tests/uds_datagram.rs the test imports std::os::linux::net::SocketAddrExt for both Linux and Android. However std::os::linux does not exist on Android, so this would cause a compile failure on Android targets. The correct pattern (already used in socketaddr.rs lines 3-6 and 73-76) is separate cfg attributes for each OS, importing std::os::android::net::SocketAddrExt on Android and std::os::linux::net::SocketAddrExt on Linux. Error kind inconsistency The abstract-path guards added to connect, try_send_to, send_to, and poll_send_to all return io::ErrorKind::Other. io::ErrorKind::InvalidInput is more appropriate since the caller passed an invalid argument. This would also be consistent with what the non-Linux branch of the recv_abstract test already asserts (Some(io::ErrorKind::InvalidInput)). Avoid suppressing bool_assert_comparison lint The test uses #[allow(clippy::bool_assert_comparison)] alongside assert_eq!(addr.is_unnamed(), true). Use assert!(addr.is_unnamed()) directly instead of suppressing the lint. SocketAddr::from_path documentation from_path is now public API (re-exported via pub use socketaddr::SocketAddr). The current doc comment is minimal. It should include a # Errors section (describing failure conditions such as interior null bytes or abstract paths on non-Linux) and a # Examples block, consistent with the rest of the public API. Missing tests for send-path rejection The new test covers bind + recv_from on abstract sockets, and bind rejection on non-Linux. There are no tests for the rejection logic added to connect, send_to, and try_send_to on Linux. A test asserting these return an error for abstract paths would prevent silent regressions when the FIXME is addressed and the guard is removed. Minor: .0 field access let addr = SocketAddr::from_path(path)?.0; accesses the pub(super) inner field directly. A small pub(crate) fn into_std(self) accessor on SocketAddr would make the call sites clearer. Minor style point only. Summary: The refactor direction is right and the overall quality is good. The Android import is the only issue that would cause a build failure; the rest are quality and consistency improvements worth addressing before merge. |
🤖 Augment PR SummarySummary: Extends Tokio’s Unix datagram support to handle Linux/Android abstract-namespace addresses where possible. Changes:
Technical Notes: Abstract namespace support is currently asymmetric (bind/recv works; connect/send is explicitly rejected) and centralized path parsing reduces duplicated OS-specific logic. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| use std::io; | ||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| use std::os::linux::net::SocketAddrExt; | ||
| #[cfg(any(target_os = "linux", target_os = "android"))] |
There was a problem hiding this comment.
#[cfg(any(target_os = "linux", target_os = "android"))] combined with std::os::linux::net::SocketAddrExt is not portable: on Android the extension trait is under std::os::android::net, so this import will fail to compile for target_os = "android".
Severity: high
🤖 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 IT test should use one import for Linux and a different one for Android. Since the IT tests are executed only on Linux in the Github Actions CI environment there is no issue at the moment because it is always Linux and the imported file matches the OS. A problem would occur when someone tries to run the test on Android.
| /// a socket path within the abstract namespace. | ||
| pub fn from_path<P>(path: P) -> io::Result<Self> | ||
| where | ||
| P: AsRef<Path>, |
There was a problem hiding this comment.
The doc for SocketAddr::from_path says “On Linux …”, but the implementation is enabled for target_os = "android" as well (cfg(any(linux, android))), which makes the docs/intent slightly misleading.
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:bug; feedback: The Augment AI reviewer is correct! The method is available for both Linux and Android (Android is based on Linux but still there are differences), so the documentation should be more explicit that Android is also supported.
| if target.as_ref().as_os_str().as_bytes().starts_with(b"\0") { | ||
| return Poll::Ready(Err(io::Error::new( | ||
| io::ErrorKind::Other, | ||
| "UnixDatagram::send_to(): abstract paths currently unsupported", |
There was a problem hiding this comment.
In poll_send_to, the error string says UnixDatagram::send_to() even though the check is in poll_send_to, which could be confusing when diagnosing failures from this API.
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:bug; feedback: The Augment AI reviewer is correct! The error message is copy/pasted from another method and the method name is still mentioning the source of the copy. The method name should be updated to "poll_send_to()" to avoid confusion in the developer debugging an issue.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| use std::future::poll_fn; | ||
| use std::io; | ||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| use std::os::linux::net::SocketAddrExt; |
There was a problem hiding this comment.
Wrong platform import for Android in test file
Medium Severity
The import std::os::linux::net::SocketAddrExt is gated behind cfg(any(target_os = "linux", target_os = "android")), but std::os::linux only exists on Linux — not Android. This will fail to compile when targeting Android. The correct pattern (used in socketaddr.rs and uds_stream.rs) uses separate cfg attributes: #[cfg(target_os = "android")] use std::os::android::net::SocketAddrExt and #[cfg(target_os = "linux")] use std::os::linux::net::SocketAddrExt.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Bugbot AI reviewer is correct! The IT test should use one import for Linux and a different one for Android. Since the IT tests are executed only on Linux in the Github Actions CI environment there is no issue at the moment because it is always Linux and the imported file matches the OS. A problem would occur when someone tries to run the test on Android.
| if target.as_ref().as_os_str().as_bytes().starts_with(b"\0") { | ||
| return Poll::Ready(Err(io::Error::new( | ||
| io::ErrorKind::Other, | ||
| "UnixDatagram::send_to(): abstract paths currently unsupported", |
There was a problem hiding this comment.
Error message references wrong method name in poll_send_to
Low Severity
The error message in poll_send_to says "UnixDatagram::send_to()" instead of "UnixDatagram::poll_send_to()". All other methods in this file include the correct method name in their error messages (connect → connect(), try_send_to → try_send_to()), so this appears to be a copy-paste oversight that could confuse users debugging the error.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Bugbot AI reviewer is correct! The error message is copy/pasted from another method and the method name is still mentioning the source of the copy. The method name should be updated to "poll_send_to()" to avoid confusion in the developer debugging an issue.
![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 introduces support for abstract Unix domain socket paths for datagrams, primarily by refactoring path-to-address conversion into a new SocketAddr::from_path method. This is a clean and effective way to centralize the logic. The changes also correctly add temporary blocks in connect and send_to variants to prevent usage with abstract paths, citing limitations in mio. I have a few suggestions to improve the error handling in these new blocks for better consistency and clarity.
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| if path.as_ref().as_os_str().as_bytes().starts_with(b"\0") { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::Other, |
There was a problem hiding this comment.
For consistency and to provide a more descriptive error, it would be better to use io::ErrorKind::InvalidInput here instead of io::ErrorKind::Other. This indicates that the input path type (abstract socket) is not valid for this operation. The new test recv_abstract also expects InvalidInput for a similar case on non-Linux platforms.
| io::ErrorKind::Other, | |
| io::ErrorKind::InvalidInput, |
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! Since some UDP related methods do not support abstract paths it would be better to return an InvalidInput kind of error to better tell the caller what is the issue.
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| if target.as_ref().as_os_str().as_bytes().starts_with(b"\0") { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::Other, |
There was a problem hiding this comment.
Similar to other functions in this file, it would be better to use io::ErrorKind::InvalidInput here instead of io::ErrorKind::Other to provide a more specific error when an unsupported path type is used.
| io::ErrorKind::Other, | |
| io::ErrorKind::InvalidInput, |
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! Since some UDP related methods do not support abstract paths it would be better to return an InvalidInput kind of error to better tell the caller what is the issue.
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| if target.as_ref().as_os_str().as_bytes().starts_with(b"\0") { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::Other, |
There was a problem hiding this comment.
To be consistent with other error handling in this file and provide a more descriptive error, please consider using io::ErrorKind::InvalidInput instead of io::ErrorKind::Other.
| io::ErrorKind::Other, | |
| io::ErrorKind::InvalidInput, |
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! Since some UDP related methods do not support abstract paths it would be better to return an InvalidInput kind of error to better tell the caller what is the issue.
| return Poll::Ready(Err(io::Error::new( | ||
| io::ErrorKind::Other, | ||
| "UnixDatagram::send_to(): abstract paths currently unsupported", | ||
| ))); |
There was a problem hiding this comment.
There are two small issues here:
- The error message refers to
UnixDatagram::send_to(), but this function ispoll_send_to(). The message should be updated to reflect the correct function name. - For consistency, it would be better to use
io::ErrorKind::InvalidInputinstead ofio::ErrorKind::Otherto indicate that abstract socket paths are not a valid input for this function at this time.
| return Poll::Ready(Err(io::Error::new( | |
| io::ErrorKind::Other, | |
| "UnixDatagram::send_to(): abstract paths currently unsupported", | |
| ))); | |
| return Poll::Ready(Err(io::Error::new( | |
| io::ErrorKind::InvalidInput, | |
| "UnixDatagram::poll_send_to(): abstract paths currently unsupported", | |
| ))); |
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! Since some UDP related methods do not support abstract paths it would be better to return an InvalidInput kind of error to better tell the caller what is the issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/net/unix/datagram/socket.rs`:
- Around line 577-580: The docs incorrectly state "Linux abstract namespace
paths" while the runtime checks also include Android; update the doc comments
that currently read "This method currently does not allow specifying Linux
abstract namespace paths." (and the similar occurrences at the other locations)
to mention Android as well — e.g. "Linux and Android abstract namespace paths"
or "Linux/Android abstract namespace paths"; make this change for all four
methods where the runtime checks use cfg(any(target_os = "linux", target_os =
"android")) (and the additional occurrences you noted) so the documentation
matches the actual cfg-gated behavior.
In `@tokio/tests/uds_datagram.rs`:
- Around line 11-14: The imports currently use
std::os::linux::net::SocketAddrExt for both Linux and Android which breaks on
Android; split the cfg imports so that #[cfg(target_os = "linux")] imports
SocketAddrExt from std::os::linux::net and #[cfg(target_os = "android")] imports
SocketAddrExt from std::os::android::net, while keeping the existing cfg wrapper
for std::os::unix::net as std_net; update the cfg attributes around the
SocketAddrExt and std_net uses (symbols: SocketAddrExt, std_net) so each target
pulls the correct platform module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7327b25-6ff5-4ed8-8b85-ba898dbe3ee9
📒 Files selected for processing (5)
tokio/src/net/unix/datagram/socket.rstokio/src/net/unix/listener.rstokio/src/net/unix/socketaddr.rstokio/src/net/unix/stream.rstokio/tests/uds_datagram.rs
| /// # Limitations | ||
| /// | ||
| /// This method currently does not allow specifying Linux abstract | ||
| /// namespace paths. |
There was a problem hiding this comment.
The limitation docs should mention Android too.
The runtime checks for abstract paths are gated with cfg(any(target_os = "linux", target_os = "android")) at Lines 618-624, 761-767, 1145-1151, and 1283-1289, so the current "Linux abstract namespace paths" wording is incorrect on Android across all four methods.
Also applies to: 715-718, 1102-1105, 1256-1259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tokio/src/net/unix/datagram/socket.rs` around lines 577 - 580, The docs
incorrectly state "Linux abstract namespace paths" while the runtime checks also
include Android; update the doc comments that currently read "This method
currently does not allow specifying Linux abstract namespace paths." (and the
similar occurrences at the other locations) to mention Android as well — e.g.
"Linux and Android abstract namespace paths" or "Linux/Android abstract
namespace paths"; make this change for all four methods where the runtime checks
use cfg(any(target_os = "linux", target_os = "android")) (and the additional
occurrences you noted) so the documentation matches the actual cfg-gated
behavior.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! The method is available for both Linux and Android (Android is based on Linux but still there are differences), so the documentation should be more explicit that Android is also supported.
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| use std::os::linux::net::SocketAddrExt; | ||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| use std::os::unix::net as std_net; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
On Rust Android targets, is the abstract Unix socket extension trait available as std::os::android::net::SocketAddrExtrather thanstd::os::linux::net::SocketAddrExt?
💡 Result:
Yes. On Android targets the abstract Unix-socket extension trait is exposed as std::os::android::net::SocketAddrExt (and on Linux as std::os::linux::net::SocketAddrExt), as shown by the std source and the rendered docs’ platform-specific import example. [1], [2]
Practical pattern:
#[cfg(target_os = "linux")]
use std::os::linux::net::SocketAddrExt;
#[cfg(target_os = "android")]
use std::os::android::net::SocketAddrExt;[1] (doc.rust-lang.org) [2] (doc.rust-lang.org)
Citations:
- 1: https://doc.rust-lang.org/beta/src/std/os/net/linux_ext/addr.rs.html
- 2: https://doc.rust-lang.org/beta/std/os/linux/net/trait.SocketAddrExt.html
Split the import by platform for Android compatibility.
Lines 11-14 compile for both Linux and Android, but SocketAddrExt is platform-specific. The code attempts to import from std::os::linux::net::SocketAddrExt for both targets, which fails on Android. Android requires std::os::android::net::SocketAddrExt instead. Mirror the split import pattern in tokio/src/net/unix/socketaddr.rs.
🔧 Proposed fix
-#[cfg(any(target_os = "linux", target_os = "android"))]
-use std::os::linux::net::SocketAddrExt;
+#[cfg(target_os = "android")]
+use std::os::android::net::SocketAddrExt;
+#[cfg(target_os = "linux")]
+use std::os::linux::net::SocketAddrExt;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[cfg(any(target_os = "linux", target_os = "android"))] | |
| use std::os::linux::net::SocketAddrExt; | |
| #[cfg(any(target_os = "linux", target_os = "android"))] | |
| use std::os::unix::net as std_net; | |
| #[cfg(target_os = "android")] | |
| use std::os::android::net::SocketAddrExt; | |
| #[cfg(target_os = "linux")] | |
| use std::os::linux::net::SocketAddrExt; | |
| #[cfg(any(target_os = "linux", target_os = "android"))] | |
| use std::os::unix::net as std_net; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tokio/tests/uds_datagram.rs` around lines 11 - 14, The imports currently use
std::os::linux::net::SocketAddrExt for both Linux and Android which breaks on
Android; split the cfg imports so that #[cfg(target_os = "linux")] imports
SocketAddrExt from std::os::linux::net and #[cfg(target_os = "android")] imports
SocketAddrExt from std::os::android::net, while keeping the existing cfg wrapper
for std::os::unix::net as std_net; update the cfg attributes around the
SocketAddrExt and std_net uses (symbols: SocketAddrExt, std_net) so each target
pulls the correct platform module.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! The IT test should use one import for Linux and a different one for Android. Since the IT tests are executed only on Linux in the Github Actions CI environment there is no issue at the moment because it is always Linux and the imported file matches the OS. A problem would occur when someone tries to run the test on Android.
value:good-to-have; category:bug; feedback: The Claude AI reviewer is correct! The IT test should use one import for Linux and a different one for Android. Since the IT tests are executed only on Linux in the Github Actions CI environment there is no issue at the moment because it is always Linux and the imported file matches the OS. A problem would occur when someone tries to run the test on Android. |
value:good-to-have; category:bug; feedback: The Claude AI reviewer is correct! Since some UDP related methods do not support abstract paths it would be better to return an InvalidInput kind of error to better tell the caller what is the issue. |
3 participants
7782: To review by AI