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

task: Drop the join waker of a task eagerly when the JoinHandle gets dropped or the task completes #6986

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tglane
Copy link
Contributor

@tglane tglane commented Nov 23, 2024

Motivation

Currently, the waker registered with a JoinHandle is not dropped until the task allocation is dropped. This behaviour may cause the memory allocated by a task to not be freed when in the case of two tasks awaiting each others JoinHandle.

See #6505 for more details.

Solution

To prevent this we need to actively drop the waker when the JoinHandle gets dropped (or the task completes in some cases).

Closes #6505.

@github-actions github-actions bot added R-loom-current-thread Run loom current-thread tests on this PR R-loom-multi-thread Run loom multi-thread tests on this PR R-loom-multi-thread-alt Run loom multi-thread alt tests on this PR labels Nov 23, 2024
dropped or the task completes

Currently, the waker registered with a JoinHandle is not dropped until
the task allocation is dropped. This behaviour may cause the memory
allocated by a task to not be freed when in the case of two tasks
awaiting each others JoinHandle.

This commit changes the behaviour by actively dropping the waker when
the JoinHandle gets dropped (or the task completes in some cases).
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime labels Nov 25, 2024
tokio/src/runtime/task/mod.rs Outdated Show resolved Hide resolved
tokio/src/runtime/task/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be nice to mention somewhere that JOIN_WAKER represents whether the runtime needs to call wake_by_ref, and not whether the waker field is set. This way, it makes intuitive sense that the runtime can unset JOIN_WAKER without setting waker to None.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that it is not needed to add this because JOIN_WAKER acts as an access control bit to the waker field.
But I could add it to the documentation as well to make the use cases for the JOIN_WAKER bit more clear.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, the "control bit" wording is okay, but I think what I proposed would be even better.

Copy link
Contributor Author

@tglane tglane Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the following to the docs of the JOIN_WAKER:

//!    [...]After the runtime sets COMPLETE to one, it invokes the waker if there is one so in this
//!    case when a task completes the `JOIN_WAKER` bit implicates to the runtime
//!    whether it should invoke the waker or not. After the runtime is done with
//!    using the waker during task completion, it unsets the `JOIN_WAKER` bit to give
//!    the `JoinHandle` exclusive access again so that it is able to drop the waker
//!    at a later point.

Would that be enough for you?

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 M-runtime Module: tokio/runtime R-loom-current-thread Run loom current-thread tests on this PR R-loom-multi-thread Run loom multi-thread tests on this PR R-loom-multi-thread-alt Run loom multi-thread alt tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JoinHandle wakers are kept alive longer than necessary
2 participants