8001: Surface errors from SO_ERROR on recv for UDP sockets on Linux#90
8001: Surface errors from SO_ERROR on recv for UDP sockets on Linux#90martin-augment wants to merge 3 commits intomasterfrom
SO_ERROR on recv for UDP sockets on Linux#90Conversation
WalkthroughThis pull request updates AI agent configuration rules across three files ( ✨ 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 |
Code ReviewSummaryThis PR fixes a real behavior issue on Linux where Issues1. Fix is incomplete — other async recv methods are not updatedThe change only covers
The 2. Test is not platform-gatedThe test The test should be gated: #[cfg(target_os = "linux")]
#[tokio::test]
async fn send_to_recv_closed_err_kind_refused() ...If the behavior is known to also apply on other Unix platforms, 3. The source change itself may warrant a platform gateThe Minor Observations
VerdictThe core idea and fix are correct. Please address:
|
There was a problem hiding this comment.
Code Review
This pull request modifies UdpSocket::recv to include Interest::ERROR for surfacing socket errors and adds a test case for connection refusal. It also updates AI agent configuration rules. Feedback highlights a critical bug where the bitwise combination of interests may cause async_io to hang due to limitations in Interest::mask. Furthermore, it is recommended to apply these changes consistently across all receive methods and expand test coverage accordingly.
| self.io | ||
| .registration() | ||
| .async_io(Interest::READABLE, || self.io.recv(buf)) | ||
| .async_io(Interest::READABLE | Interest::ERROR, || self.io.recv(buf)) |
There was a problem hiding this comment.
While adding Interest::ERROR is intended to surface SO_ERROR events (such as ICMP unreachable) on connected UDP sockets, this change as implemented has two significant issues:
-
Broken Readiness Logic: In
tokio/src/io/interest.rs, theInterest::maskmethod uses amatchstatement that does not handle bitwise combinations. CombiningInterest::READABLE | Interest::ERRORwill result inReady::EMPTYbeing returned bymask(). This will causeasync_ioto hang indefinitely because the underlying readiness future will be waiting for an empty set of events. A fix forInterest::maskininterest.rsis required to support combined interests. -
Inconsistency: This change only updates the
recvmethod. Other receive methods such asrecv_from,recv_buf,recv_buf_from, and theirtry_*counterparts (e.g.,try_recv,try_recv_from) still useInterest::READABLEonly. This leads to inconsistent behavior across the API where some methods surface errors while others do not.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The Gemini AI reviewer is correct! The issue is fixed for only one of the recv_** methods. There are few more similar methods which will need the same fix. Prevents fixing an issue only partially
| async fn send_to_recv_closed_err_kind_refused() -> std::io::Result<()> { | ||
| let sender = UdpSocket::bind("127.0.0.1:0").await?; | ||
| let receiver = UdpSocket::bind("127.0.0.1:0").await?; | ||
|
|
||
| let receiver_addr = receiver.local_addr()?; | ||
| drop(receiver); | ||
| sender.connect(receiver_addr).await?; | ||
| sender.send(MSG).await?; | ||
|
|
||
| let mut recv_buf = [0u8; 32]; | ||
| let err = time::timeout(Duration::from_secs(5), sender.recv(&mut recv_buf)) | ||
| .await | ||
| .expect("timed out instead of returning error") | ||
| .unwrap_err(); | ||
|
|
||
| assert_eq!(err.kind(), io::ErrorKind::ConnectionRefused); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The new test case send_to_recv_closed_err_kind_refused is valuable for verifying that errors are surfaced. However, to ensure consistency and prevent regressions, you should add similar test cases for other receive methods, such as recv_from, try_recv, and recv_buf. This is especially important given that these methods currently lack the Interest::ERROR flag in the implementation and would likely fail this test or time out.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The Gemini AI reviewer is correct! The issue is fixed for only one of the recv_** methods. There are few more similar methods which will need the same fix. Prevents fixing an issue only partially
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/udp.rs`:
- Around line 50-68: The test send_to_recv_closed_err_kind_refused exhibits
Linux-specific behavior and must be gated to Linux; add a platform cfg attribute
(e.g. #[cfg(target_os = "linux")] or #[cfg(all(unix, target_os = "linux"))])
directly above the #[tokio::test] for the send_to_recv_closed_err_kind_refused
function so the test only runs on Linux targets, leaving the test body and
assertions unchanged.
🪄 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: 09d17b8b-487f-4b26-a41c-178660e0897f
📒 Files selected for processing (5)
.cursor/rules.mdAGENTS.mdCLAUDE.mdtokio/src/net/udp.rstokio/tests/udp.rs
| #[tokio::test] | ||
| async fn send_to_recv_closed_err_kind_refused() -> std::io::Result<()> { | ||
| let sender = UdpSocket::bind("127.0.0.1:0").await?; | ||
| let receiver = UdpSocket::bind("127.0.0.1:0").await?; | ||
|
|
||
| let receiver_addr = receiver.local_addr()?; | ||
| drop(receiver); | ||
| sender.connect(receiver_addr).await?; | ||
| sender.send(MSG).await?; | ||
|
|
||
| let mut recv_buf = [0u8; 32]; | ||
| let err = time::timeout(Duration::from_secs(5), sender.recv(&mut recv_buf)) | ||
| .await | ||
| .expect("timed out instead of returning error") | ||
| .unwrap_err(); | ||
|
|
||
| assert_eq!(err.kind(), io::ErrorKind::ConnectionRefused); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Gate this regression test to Linux to avoid cross-platform failures.
Line [50] currently enables this test everywhere, but the asserted behavior (Line [66], ConnectionRefused surfaced on recv) is Linux-specific per the PR intent. On non-Linux targets, this can fail or be flaky.
Suggested fix
-#[tokio::test]
+#[cfg(target_os = "linux")]
+#[tokio::test]
async fn send_to_recv_closed_err_kind_refused() -> std::io::Result<()> {📝 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.
| #[tokio::test] | |
| async fn send_to_recv_closed_err_kind_refused() -> std::io::Result<()> { | |
| let sender = UdpSocket::bind("127.0.0.1:0").await?; | |
| let receiver = UdpSocket::bind("127.0.0.1:0").await?; | |
| let receiver_addr = receiver.local_addr()?; | |
| drop(receiver); | |
| sender.connect(receiver_addr).await?; | |
| sender.send(MSG).await?; | |
| let mut recv_buf = [0u8; 32]; | |
| let err = time::timeout(Duration::from_secs(5), sender.recv(&mut recv_buf)) | |
| .await | |
| .expect("timed out instead of returning error") | |
| .unwrap_err(); | |
| assert_eq!(err.kind(), io::ErrorKind::ConnectionRefused); | |
| Ok(()) | |
| } | |
| #[cfg(target_os = "linux")] | |
| #[tokio::test] | |
| async fn send_to_recv_closed_err_kind_refused() -> std::io::Result<()> { | |
| let sender = UdpSocket::bind("127.0.0.1:0").await?; | |
| let receiver = UdpSocket::bind("127.0.0.1:0").await?; | |
| let receiver_addr = receiver.local_addr()?; | |
| drop(receiver); | |
| sender.connect(receiver_addr).await?; | |
| sender.send(MSG).await?; | |
| let mut recv_buf = [0u8; 32]; | |
| let err = time::timeout(Duration::from_secs(5), sender.recv(&mut recv_buf)) | |
| .await | |
| .expect("timed out instead of returning error") | |
| .unwrap_err(); | |
| assert_eq!(err.kind(), io::ErrorKind::ConnectionRefused); | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tokio/tests/udp.rs` around lines 50 - 68, The test
send_to_recv_closed_err_kind_refused exhibits Linux-specific behavior and must
be gated to Linux; add a platform cfg attribute (e.g. #[cfg(target_os =
"linux")] or #[cfg(all(unix, target_os = "linux"))]) directly above the
#[tokio::test] for the send_to_recv_closed_err_kind_refused function so the test
only runs on Linux targets, leaving the test body and assertions unchanged.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| self.io | ||
| .registration() | ||
| .async_io(Interest::READABLE, || self.io.recv(buf)) | ||
| .async_io(Interest::READABLE | Interest::ERROR, || self.io.recv(buf)) |
There was a problem hiding this comment.
Inconsistent Interest::ERROR across similar recv methods
Medium Severity
Interest::ERROR was added to recv but not to the other async receive methods — recv_buf, recv_from, and recv_buf_from — which all use async_io(Interest::READABLE, ...) with the same underlying recv syscalls. This means SO_ERROR (e.g. ConnectionRefused) will only be surfaced when callers use recv, but not when they use any of the other receive variants, creating inconsistent behavior across functionally equivalent APIs.
Additional Locations (2)
There was a problem hiding this comment.
value:useful; category:bug; feedback: The Bugbot AI reviewer is correct! The issue is fixed for only one of the recv_** methods. There are few more similar methods which will need the same fix. Prevents fixing an issue only partially
🤖 Augment PR SummarySummary: Prevents connected UDP Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
| self.io | ||
| .registration() | ||
| .async_io(Interest::READABLE, || self.io.recv(buf)) | ||
| .async_io(Interest::READABLE | Interest::ERROR, || self.io.recv(buf)) |
There was a problem hiding this comment.
tokio/src/net/udp.rs:788: This adds Interest::ERROR for recv, but other connected receive APIs like recv_buf (and the poll-based poll_recv / poll_recv_ready paths) still appear to wait only on readable readiness. If they can also observe an SO_ERROR-only readiness state, they may still hang or delay surfacing the error.
Severity: medium
Other Locations
tokio/src/net/udp.rs:989tokio/src/net/udp.rs:744tokio/src/net/udp.rs:815
🤖 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 issue is fixed for only one of the recv_** methods. There are few more similar methods which will need the same fix. Prevents fixing an issue only partially
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn send_to_recv_closed_err_kind_refused() -> std::io::Result<()> { |
There was a problem hiding this comment.
tokio/tests/udp.rs:51: This test asserts ErrorKind::ConnectionRefused, but UDP ICMP/error reporting (and the mapped ErrorKind) varies across platforms, and the PR description/title suggests the behavior is Linux-specific. Without a platform cfg, this may be flaky or fail on non-Linux CI.
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 "connection refused" error is Unix specific. Windows uses ConnectionReset instead. The test should improved to expect the correct error for the respective OS. Prevents failed CI due to test failures on Windows and WASM.
value:useful; category:bug; feedback: The Claude AI reviewer is correct! The issue is fixed for only one of the |
value:useful; category:bug; feedback: The Claude AI reviewer is correct! The "connection refused" error is Unix specific. Windows uses ConnectionReset instead. The test should improved to expect the correct error for the respective OS. Prevents failed CI due to test failures on Windows and WASM. |


8001: To review by AI