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

Run TLS destructors at process exit on all platforms #134085

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

Conversation

surban
Copy link
Contributor

@surban surban commented Dec 9, 2024

This calls TLS destructors for the thread that initiates a process exit. This is done by registering a process-wide
exit callback.

Previously UNIX platforms other than Linux and Apple did not destruct TLS variables on the thread that initiated the process exit (either by returning from main or calling std::process::exit).

This also adds a test to verify this behavior.

@joboet Can we run CI tests for all available platforms for this?

r? joboet

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 9, 2024
@bors
Copy link
Contributor

bors commented Dec 10, 2024

☔ The latest upstream changes (presumably #134108) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 10, 2024
@rustbot

This comment has been minimized.

This calls TLS destructors for the thread that initiates
a process exit. This is done by registering a process-wide
exit callback.

Previously UNIX platforms other than Linux and Apple
did not destruct TLS variables on the thread that
initiated the process exit (either by returning from
main or calling std::process::exit).
@surban surban force-pushed the tls-process-destruct branch from 62b9e7a to b18d55b Compare December 10, 2024 15:57
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 10, 2024
Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I've been quite busy recently.

At a first glance, the changes look mostly good. There is a problem with the TLS destructor registration however: the TLS key can be used as soon as it is initialized, so it is possible that another thread completes before the destructor is registered. Always registering the destructor is not an option either, since that might lead to cycles in the destructor list. Windows uses INIT_ONCE synchronization to resolve this issue, but replicating that might prove difficult on platforms without a native Once-like interface (Once itself cannot be used in the TLS code since it needs TLS itself). I think this is best solved by keeping a thread-local destructor list similar to the one used on platforms with native TLS, but using the keys (or perhaps references to the LazyKeys) instead of pointers to values. Or perhaps you can think of another way?

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants