io: always cleanup AsyncFd registration list on deregister#7773
io: always cleanup AsyncFd registration list on deregister#7773Darksonn merged 36 commits intotokio-rs:masterfrom
Conversation
Fixes memory leak when fd is closed before AsyncFd drop. Fixes: tokio-rs#7563
ADD-SP
left a comment
There was a problem hiding this comment.
if OS deregister fails, the OS already doesn't track it, so cleanup doesn't break memory safety.
Could you please explain why? I'm curious on it.
There was a problem hiding this comment.
Only cleaning up the intrusive list may not be a proper fix of this issue. There are some questions we need to answer.
- Shall we propagate the error to the downstream user?
- When / When not propagate the error?
- How to propagate this error?
- Will our fix cause a breaking change?
|
Hi @ADD-SP, Thanks for the feedback! I've updated the PR to address your questions. Test Infrastructure AddedAdded test-only methods to verify the fix:
All gated with Test EvolutionInitial test only exercised the code path. Updated to check registration count after each iteration and assert it doesn't grow (indicating leak). Platform-Specific BehaviorThe leak cannot be reproduced on macOS. On Linux with epoll:
The test is Linux-specific for leak detection but exercises the code path on macOS too. Why Cleanup After OS Failure is Safe
When Remaining Questions
Per your comment in #7563, I see two options for into_inner(): (1)add try_into_inner() (non-breaking) or (2) panic on error. I recommend Option 1 for non-breaking behavior and user choice, but I can implement Option 2 if you prefer.
Thanks again! |
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
I'll be appreciated if you check my rss-based test: SummaryI tested several approaches as suggested:
Since this is a "logical leak" (objects stuck in a data structure, not truly unreachable), standard leak detectors don't work. Solution: RSS-based memory testI implemented a test that monitors RSS (Resident Set Size) before and after running 5000 iterations. No internal API access needed - just reads Linux results:
The test is gated with |
Instead of checking absolute RSS growth (which varies with allocator behavior), this test now runs multiple phases and checks if memory stabilizes. A real leak causes unbounded growth across all phases; fixed code stabilizes as memory is reused. This approach is more robust across different CI environments where allocator behavior may differ.
cc2358c to
3b8d349
Compare
|
Miri does support epoll. |
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Hi @Darksonn. test memory_leak_miri_check ... ok |
Are you saying that miri detects it, and that miri doesn't detect it when the fix is applied? That sounds like a pretty good way to test it. |
To clarify: Miri cannot detect this leak - neither with nor without the fix. Miri is designed to detect unreachable memory (orphaned allocations). But this is a logical leak where objects remain reachable through the internal registrations linked list - they're just never removed. Since they're still reachable, Miri sees no problem. Same reason LSan couldn't detect it. The RSS-based test is the practical solution since it measures actual memory growth.
|
|
Relying on RSS is going to be extremely fragile. Your memory allocator may keep memory around even if the program has freed it, and this still counts in RSS. If we're going the route of measuring memory, then please declare a The test needs to be moved to its own file so the |
@Darksonn, Implemented a custom #[global_allocator] that tracks allocations via a static AtomicUsize, forwarding to the system allocator. The test is in io_async_fd_memory_leak.rs so the allocator only affects that file. |
Darksonn
left a comment
There was a problem hiding this comment.
Thanks for adding the new test. One nit below.
- Add feature flag check to #[cfg] - Make allocated counter a struct field - Add error handling for fcntl in set_nonblocking
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.49.0` → `1.50.0` | --- ### Release Notes <details> <summary>tokio-rs/tokio (tokio)</summary> ### [`v1.50.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.50.0): Tokio v1.50.0 [Compare Source](tokio-rs/tokio@tokio-1.49.0...tokio-1.50.0) ### 1.50.0 (Mar 3rd, 2026) ##### Added - net: add `TcpStream::set_zero_linger` ([#​7837]) - rt: add `is_rt_shutdown_err` ([#​7771]) ##### Changed - io: add optimizer hint that `memchr` returns in-bounds pointer ([#​7792]) - io: implement vectored writes for `write_buf` ([#​7871]) - runtime: panic when `event_interval` is set to 0 ([#​7838]) - runtime: shorten default thread name to fit in Linux limit ([#​7880]) - signal: remember the result of `SetConsoleCtrlHandler` ([#​7833]) - signal: specialize windows `Registry` ([#​7885]) ##### Fixed - io: always cleanup `AsyncFd` registration list on deregister ([#​7773]) - macros: remove (most) local `use` declarations in `tokio::select!` ([#​7929]) - net: fix `GET_BUF_SIZE` constant for `target_os = "android"` ([#​7889]) - runtime: avoid redundant unpark in current\_thread scheduler ([#​7834]) - runtime: don't park in `current_thread` if `before_park` defers waker ([#​7835]) - io: fix write readiness on ESP32 on short writes ([#​7872]) - runtime: wake deferred tasks before entering `block_in_place` ([#​7879]) - sync: drop rx waker when oneshot receiver is dropped ([#​7886]) - runtime: fix double increment of `num_idle_threads` on shutdown ([#​7910], [#​7918], [#​7922]) ##### Unstable - fs: check for io-uring opcode support ([#​7815]) - runtime: avoid lock acquisition after uring init ([#​7850]) ##### Documented - docs: update outdated unstable features section ([#​7839]) - io: clarify the behavior of `AsyncWriteExt::shutdown()` ([#​7908]) - io: explain how to flush stdout/stderr ([#​7904]) - io: fix incorrect and confusing `AsyncWrite` documentation ([#​7875]) - rt: clarify the documentation of `Runtime::spawn` ([#​7803]) - rt: fix missing quotation in docs ([#​7925]) - runtime: correct the default thread name in docs ([#​7896]) - runtime: fix `event_interval` doc ([#​7932]) - sync: clarify RwLock fairness documentation ([#​7919]) - sync: clarify that `recv` returns `None` once closed and no more messages ([#​7920]) - task: clarify when to use `spawn_blocking` vs dedicated threads ([#​7923]) - task: doc that task drops before `JoinHandle` completion ([#​7825]) - signal: guarantee that listeners never return `None` ([#​7869]) - task: fix task module feature flags in docs ([#​7891]) - task: fix two typos ([#​7913]) - task: improve the docs of `Builder::spawn_local` ([#​7828]) - time: add docs about auto-advance and when to use sleep ([#​7858]) - util: fix typo in docs ([#​7926]) [#​7771]: tokio-rs/tokio#7771 [#​7773]: tokio-rs/tokio#7773 [#​7792]: tokio-rs/tokio#7792 [#​7803]: tokio-rs/tokio#7803 [#​7815]: tokio-rs/tokio#7815 [#​7825]: tokio-rs/tokio#7825 [#​7828]: tokio-rs/tokio#7828 [#​7833]: tokio-rs/tokio#7833 [#​7834]: tokio-rs/tokio#7834 [#​7835]: tokio-rs/tokio#7835 [#​7837]: tokio-rs/tokio#7837 [#​7838]: tokio-rs/tokio#7838 [#​7839]: tokio-rs/tokio#7839 [#​7850]: tokio-rs/tokio#7850 [#​7858]: tokio-rs/tokio#7858 [#​7869]: tokio-rs/tokio#7869 [#​7871]: tokio-rs/tokio#7871 [#​7872]: tokio-rs/tokio#7872 [#​7875]: tokio-rs/tokio#7875 [#​7879]: tokio-rs/tokio#7879 [#​7880]: tokio-rs/tokio#7880 [#​7885]: tokio-rs/tokio#7885 [#​7886]: tokio-rs/tokio#7886 [#​7889]: tokio-rs/tokio#7889 [#​7891]: tokio-rs/tokio#7891 [#​7896]: tokio-rs/tokio#7896 [#​7904]: tokio-rs/tokio#7904 [#​7908]: tokio-rs/tokio#7908 [#​7910]: tokio-rs/tokio#7910 [#​7913]: tokio-rs/tokio#7913 [#​7918]: tokio-rs/tokio#7918 [#​7919]: tokio-rs/tokio#7919 [#​7920]: tokio-rs/tokio#7920 [#​7922]: tokio-rs/tokio#7922 [#​7923]: tokio-rs/tokio#7923 [#​7925]: tokio-rs/tokio#7925 [#​7926]: tokio-rs/tokio#7926 [#​7929]: tokio-rs/tokio#7929 [#​7932]: tokio-rs/tokio#7932 </details> --- ### Configuration 📅 **Schedule**: Branch creation - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) in timezone Pacific/Auckland, Automerge - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) in timezone Pacific/Auckland. 🚦 **Automerge**: Disabled because a matching PR was automerged previously. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My41Mi4xIiwidXBkYXRlZEluVmVyIjoiNDMuNTIuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsicmVub3ZhdGUiXX0=--> Reviewed-on: https://harton.dev/project-neon/neonfs/pulls/53 Co-authored-by: Renovate Bot <bot@harton.nz> Co-committed-by: Renovate Bot <bot@harton.nz>
Fixes memory leak when fd is closed before AsyncFd drop.
Fixes: #7563
Motivation
When a file descriptor is closed before dropping
AsyncFd, OS deregistration fails and causes an early return, preventing cleanup of the internal registration list. This leaksScheduledIoobjects over time.Solution
Always clean up the internal registration list, even if OS deregistration fails. Store the OS result, perform cleanup, then return the error. This is safe because the list is Tokio's internal tracking - if OS deregister fails, the OS already doesn't track it, so cleanup doesn't break memory safety.
Includes a test reproducing the bug scenario.