-
Notifications
You must be signed in to change notification settings - Fork 0
7773: io: always cleanup AsyncFd registration list on deregister #33
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
base: master
Are you sure you want to change the base?
Changes from all commits
931635d
3d7d87d
b6452c7
bf5c706
e659d60
b44e56d
94f46b9
0a0f94e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,6 +53,33 @@ impl RegistrationSet { | |
| self.num_pending_release.load(Acquire) != 0 | ||
| } | ||
|
|
||
| /// TEST PURPOSE RELATED TO PR #7773 | ||
| #[cfg(feature = "full")] | ||
| pub(super) fn pending_release_count(&self) -> usize { | ||
| self.num_pending_release.load(Acquire) | ||
| } | ||
| /// TEST PURPOSE RELATED TO PR #7773 | ||
| #[cfg(feature = "full")] | ||
| pub(super) fn total_registration_count(&self, synced: &mut Synced) -> usize { | ||
| // Count by temporarily draining the list, then restoring it | ||
| // This is safe for test purposes | ||
| let mut items = Vec::new(); | ||
|
|
||
| // Drain all items | ||
| while let Some(item) = synced.registrations.pop_back() { | ||
| items.push(item); | ||
| } | ||
|
|
||
| let count = items.len(); | ||
|
|
||
| // Restore items in reverse order (since we popped from back) | ||
| for item in items.into_iter().rev() { | ||
| synced.registrations.push_front(item); | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: List restoration reverses registration orderThe
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:useful; category:bug; feedback:The Bugbot AI reviewer is correct! The reverse iteration while re-adding the items combined with push_front() reverses the final order of the items with the one before counting them. |
||
|
|
||
| count | ||
| } | ||
|
Comment on lines
+61
to
+81
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. List order is reversed after counting. The drain-and-restore logic reverses the list order. After popping from back into a Vec and then iterating in reverse while pushing to front, the final order is the reverse of the original. While this may not affect test correctness (since you're only counting), the comment "Restore items in reverse order" is misleading—it describes the iteration order, not the restoration goal. Consider using a simple traversal if the - // Restore items in reverse order (since we popped from back)
- for item in items.into_iter().rev() {
- synced.registrations.push_front(item);
+ // Restore items in original order
+ for item in items.into_iter() {
+ synced.registrations.push_back(item);
}Alternatively, if order truly doesn't matter for tests, update the comment to clarify that order is not preserved.
🤖 Prompt for AI Agents
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:useful; category:bug; feedback:The CodeRabbit AI reviewer is correct! The reverse iteration while re-adding the items combined with push_front() reverses the final order of the items with the one before counting them. |
||
|
|
||
| pub(super) fn allocate(&self, synced: &mut Synced) -> io::Result<Arc<ScheduledIo>> { | ||
| if synced.is_shutdown { | ||
| return Err(io::Error::new( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -947,12 +947,76 @@ async fn try_new() { | |
| assert!(Arc::ptr_eq(&original, &returned)); | ||
| } | ||
|
|
||
| /// Regression test for issue #7563 | ||
| /// | ||
| /// Reproduces the bug where closing fd before dropping AsyncFd causes | ||
| /// OS deregister to fail, preventing cleanup and leaking ScheduledIo objects. | ||
| #[tokio::test] | ||
| async fn try_with_interest() { | ||
| let original = Arc::new(InvalidSource); | ||
| async fn memory_leak_when_fd_closed_before_drop() { | ||
| use std::os::unix::io::{AsRawFd, RawFd}; | ||
| use std::sync::Arc; | ||
| use tokio::io::unix::AsyncFd; | ||
| use tokio::runtime::Handle; | ||
|
|
||
| let error = AsyncFd::try_with_interest(original.clone(), Interest::READABLE).unwrap_err(); | ||
| let (returned, _cause) = error.into_parts(); | ||
| use nix::sys::socket::{self, AddressFamily, SockFlag, SockType}; | ||
|
|
||
| assert!(Arc::ptr_eq(&original, &returned)); | ||
| struct RawFdWrapper { | ||
| fd: RawFd, | ||
| } | ||
|
|
||
| impl AsRawFd for RawFdWrapper { | ||
| fn as_raw_fd(&self) -> RawFd { | ||
| self.fd | ||
| } | ||
| } | ||
|
|
||
| let rt_handle = Handle::current(); | ||
| tokio::task::yield_now().await; | ||
| let initial_count = rt_handle.io_total_registration_count(); | ||
|
|
||
| const ITERATIONS: usize = 30; | ||
| let mut max_count_seen = initial_count; | ||
|
|
||
| for _ in 0..ITERATIONS { | ||
| let (fd_a, _fd_b) = socket::socketpair( | ||
| AddressFamily::Unix, | ||
| SockType::Stream, | ||
| None, | ||
| SockFlag::empty(), | ||
| ) | ||
| .expect("socketpair"); | ||
| let raw_fd = fd_a.as_raw_fd(); | ||
| set_nonblocking(raw_fd); | ||
| std::mem::forget(fd_a); | ||
|
|
||
| let afd = Arc::new(RawFdWrapper { fd: raw_fd }); | ||
| let async_fd = AsyncFd::new(ArcFd(afd.clone())).unwrap(); | ||
|
|
||
| unsafe { | ||
| libc::close(raw_fd); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The return value of 🤖 Was this useful? React with 👍 or 👎
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:good-to-have; category:bug; feedback:The Augment AI reviewer is correct! The system call result should be checked before continuing with the rest of the test because now it assumes that everything is always successful. |
||
| } | ||
|
|
||
| drop(async_fd); | ||
| tokio::task::yield_now().await; | ||
|
|
||
| let current_count = rt_handle.io_total_registration_count(); | ||
| max_count_seen = max_count_seen.max(current_count); | ||
| } | ||
|
|
||
| tokio::task::yield_now().await; | ||
| tokio::time::sleep(Duration::from_millis(100)).await; | ||
|
|
||
| let final_count = rt_handle.io_total_registration_count(); | ||
| max_count_seen = max_count_seen.max(final_count); | ||
|
|
||
| assert!( | ||
| final_count <= initial_count + 2 && max_count_seen <= initial_count + 2, | ||
| "Memory leak detected: final count {} (initial: {}), max seen: {}. \ | ||
| With bug, count would be ~{} ({} leaked objects).", | ||
| final_count, | ||
| initial_count, | ||
| max_count_seen, | ||
| initial_count + ITERATIONS, | ||
| max_count_seen.saturating_sub(initial_count) | ||
| ); | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Test for
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This drain/restore logic appears to reverse
synced.registrations(collecting viapop_back, then iteratingrev()andpush_front). Since this mutates driver state even in a test-only helper, it could subtly change behavior after calling the counter.🤖 Was this useful? React with 👍 or 👎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value:useful; category:bug; feedback:The Augment AI reviewer is correct! The reverse iteration while re-adding the items combined with push_front() reverses the final order of the items with the one before counting them.