Skip to content
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

io: clean up I/O driver shutdown process #3481

Open
carllerche opened this issue Jan 28, 2021 · 2 comments
Open

io: clean up I/O driver shutdown process #3481

carllerche opened this issue Jan 28, 2021 · 2 comments
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. E-hard Call for participation: Experience needed to fix: Hard / a lot E-help-wanted Call for participation: Help is requested to fix this issue. M-io Module: tokio/io

Comments

@carllerche
Copy link
Member

The I/O driver needs some restructuring in order to clean up the shutdown process.

Currently, internals are structured such that there is a cycle between ScheduledIo (internal I/O resource state) and io::driver::Inner. The driver's internal struct holds a slab containing all the resource's ScheduledIo instances. Each ScheduleIo tracks wakers that should be notified when I/O events arrive. The wakers hold a reference to the driver's inner structure in order to unpark an I/O driver blocked in poll.

                        +---------------+
       +----------------+    Wakers     |
       |                +-------+-------+
       |                        ^
       v                        |
+------+------+         +-------+-------+
| Inner/Slab  +-------->|  ScheduledIo  |
+------+------+         +-------+-------+
       ^                        ^
       |                        |
+------+------+         +-------+------+
|    Driver   |         | Registration |
+-------------+         +--------------+

#3477 fixes the memory leak due to the cycle by ensuring wakers are dropped when a Registration is dropped. This breaks the cycle. However, Wakers may hold arbitrary structures. It is possible to implement a Waker that also holds a Registration. In this case, the cycle would not be broken leading again to a memory leak. This is a fairly convoluted scenario and probably does not happen much in practice, but it should still be fixed.

To fix this, we need the I/O driver shutdown process to clear any wakers being held by ScheduledIo instances. To do this without introducing a race condition is a bit tricky. First, a "is_shut_down" flag needs to be set atomically in a way that ensures no further ScheduledIo instances may be created. Then, each ScheduledIo must be iterated and a shutdown flag must be set on it. This can be set as part of the "readiness" field. If a waker is stored in the ScheduledIo it should be notified & dropped.

@carllerche carllerche added C-bug Category: This is a bug. A-tokio Area: The main tokio crate M-io Module: tokio/io E-hard Call for participation: Experience needed to fix: Hard / a lot E-help-wanted Call for participation: Help is requested to fix this issue. labels Jan 28, 2021
@zaharidichev
Copy link
Contributor

@carllerche Can I take this one? Should #3564, merge first?

@carllerche
Copy link
Member Author

I think this is fairly standalone. It mostly requires adding the ability to shutdown the internal slab itself. Feel free to take a stab at it and ask Qs on discord 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. E-hard Call for participation: Experience needed to fix: Hard / a lot E-help-wanted Call for participation: Help is requested to fix this issue. M-io Module: tokio/io
Projects
None yet
Development

No branches or pull requests

2 participants