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: define task_local in underlying future Drop #4604

Closed
wants to merge 3 commits into from

Conversation

l4l
Copy link
Contributor

@l4l l4l commented Apr 7, 2022

Motivation

Currently task_local-s cannot be accessed at the Drop::drop, which seems quite strange behavior.

Solution

Current solution wraps the underlying future with ManuallyDrop and requires some unsafe (not really sure about soundness). Other option is using Option<Fut> that have additional overhead though requires no unsafe blocks.

For some reason, a test within tokio doesn't reproduce the issue but it works for playground/separate crate (thanks @loyd for that one), link.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-task Module: tokio/task labels Apr 7, 2022
@Darksonn
Copy link
Contributor

Darksonn commented Apr 7, 2022

I don't understand the motivation for this. Can you please post some example code that uses this new feature, along with an explanation of what the code does?

@loyd
Copy link
Contributor

loyd commented Apr 7, 2022

We're storing the current trace ID in TLS and accessing it, e.g. for logging. However, in the Drop instances it can fail if it's being called during runtime termination.

@l4l
Copy link
Contributor Author

l4l commented Apr 11, 2022

I added a test for the case finally. Overall the previous behavior looks quite unexpected. Especially when you see in debugger LocalKey::scope at previous function frames but the task local value is unavailable that's one is really confusing.

@Darksonn
Copy link
Contributor

Darksonn commented Jun 29, 2022

The motivation for your PR makes a lot of sense, but it isn't quite implemented in the way I would want it to, and I also noticed a bunch of other minor issues in LocalKey. Therefore, I've opened the PR #4795 containing those changes.

Thanks for taking the time to submit a PR.

@Darksonn Darksonn closed this Jun 29, 2022
@l4l l4l deleted the tls-on-drop branch June 29, 2022 15:52
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-task Module: tokio/task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants