Skip to content

Run TLS destructors at process exit on all platforms #134085

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

Closed
wants to merge 11 commits into 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
Collaborator

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.

@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
@surban
Copy link
Contributor Author

surban commented Jan 13, 2025

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.

You are right.

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.

I implemented it using references to LazyKeys.

@surban surban requested a review from joboet January 13, 2025 21:12
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 13, 2025
@surban surban marked this pull request as draft January 13, 2025 21:15
@surban
Copy link
Contributor Author

surban commented Jan 13, 2025

I now have the problem that I need to register the LazyKey with the thread local list of destructors for every thread that uses it. Thus I need to call the register_dtor function once per thread that calls LazyKey::force.

Do you have an idea how I could achieve that without registering a second TLS key per LazyKey that keeps track whether it has been added to the destructors list? An option might be to store a sentinel value (for example 1 or 2) as the TLS value to indicate that the LazyKey been registered. However, I am unsure if this is acceptable practice. What do you think?

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@surban surban marked this pull request as ready for review January 14, 2025 13:37
@surban
Copy link
Contributor Author

surban commented Jan 14, 2025

I now have the problem that I need to register the LazyKey with the thread local list of destructors for every thread that uses it. Thus I need to call the register_dtor function once per thread that calls LazyKey::force.

I solved this by modifying os::Storage::get so that it checks if the value is being initialized for a particular thread. If so, it calls a function to add the TLS key to the thread-local destructor list. I hope this approach is okay.

This should be ready for review now.

@rust-log-analyzer

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).
@joboet
Copy link
Member

joboet commented Jan 24, 2025

I have some concerns about the implementation, but before that, I think T-libs should have a look at this to ensure that they are okay with the behaviour change: atexit doesn't just get run when main exits, but also when exit is called, which might be a bit unexpected.

@rustbot label +I-libs-nominated

@rustbot rustbot added the I-libs-nominated Nominated for discussion during a libs team meeting. label Jan 24, 2025
@surban
Copy link
Contributor Author

surban commented Jan 27, 2025

atexit doesn't just get run when main exits, but also when exit is called, which might be a bit unexpected.

Note that this is the same behavior as on currently stable Rust on Linux.

struct Foo;

impl Drop for Foo {
    fn drop(&mut self) {
        println!("wut");
    }
}

thread_local!(static FOO: Foo = Foo);

fn main() {
    FOO.with(|_| {});
    unsafe { libc::exit(0) };
}

The program above will print wut on rustc 1.84.0 (9fc6b4312 2025-01-07) on x86_64-unknown-linux-gnu with glibc 2.35.

@m-ou-se
Copy link
Member

m-ou-se commented Feb 5, 2025

We briefly discussed this in the libs meeting. We'd like to understand the motivation better. Is this just for consistency? Or are there concrete bugs that are solved by this PR?

@m-ou-se m-ou-se 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 Feb 5, 2025
@surban
Copy link
Contributor Author

surban commented Feb 10, 2025

There are several reasons for this:

  • Consistency:
    Currently, TLS destruction on process exit is highly inconsistent—not only across different platforms but even between different glibc versions on Linux. See this comment. This inconsistency complicates testing and may introduce subtle bugs due to unexpected platform differences.

  • Memory Leak Checking:
    Some tools report unfreed TLS memory as possibly lost (see issue #135608). While this is not a problem for the process itself since it is exiting anyway, it adds noise when debugging memory leaks—especially in scenarios involving unsafe code or linked C libraries. Avoiding such noise would improve the effectiveness of memory leak detection tools.

  • Cross-Platform Exit Handlers:
    If TLS destructors were reliably executed on process exit, user code could use them as exit notifications, effectively enabling atexit-style callbacks without requiring unsafe or platform-specific code. This would benefit crates like tempfile, allowing them to perform cleanup even when local destructors are not executed (e.g., when std::process::exit is called).

Alternative: Never Run TLS Destructors

An alternative approach would be to never run TLS destructors on process exit. While this is not my preferred solution due to the reasons mentioned above, it would at least provide consistency and be easy to implement. This approach would also align with the behavior of static items.

@surban
Copy link
Contributor Author

surban commented Feb 10, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 10, 2025
@the8472 the8472 removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 12, 2025
@the8472
Copy link
Member

the8472 commented Feb 12, 2025

We discussed this again during today's libs meeting. Opinions were mixed, leaning towards close. The following issues were raised during the discussion:

  • using TLS destructors as an exit handler is misguided because they would only run for the thread calling exit, other threads wouldn't get the desired cleanup behavior. So they should use atexit instead.
  • consistency may become a burden, e.g. on platforms where atexit does not work reliably or causes other issues

Leak checking wasn't discussed, but from previous issues I have the impression that only limited effort is spent on making leak-checkers happy. Genuine leaks need to be fixed, false positives (quasi-static things) are better handled through exceptions in the leak checker.

@rfcbot fcp close

@rfcbot
Copy link
Collaborator

rfcbot commented Feb 12, 2025

Team member @the8472 has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 12, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Feb 13, 2025

🔔 This is now entering its final comment period, as per the review above. 🔔

@m-ou-se m-ou-se removed the I-libs-nominated Nominated for discussion during a libs team meeting. label Feb 19, 2025
use crate::mem;

pub unsafe fn at_process_exit(cb: unsafe extern "C" fn()) {
// Miri does not support atexit.
Copy link
Member

Choose a reason for hiding this comment

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

Could you file an issue for this in the Miri repo? If std starts using this, we should consider supporting it.

Copy link
Member

@RalfJung RalfJung Feb 21, 2025

Choose a reason for hiding this comment

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

This could use a few more comments, explaining what enable_thread / enable_process are doing. The names sound like they enable a thread/process but that's not quite right I think.

}

/// On platforms with key-based TLS, the system runs the destructors for us.
/// We still have to make sure that [`crate::rt::thread_cleanup`] is called,
/// however. This is done by defering the execution of a TLS destructor to
/// the next round of destruction inside the TLS destructors.
///
/// POSIX systems do not run TLS destructors at process exit.
Copy link
Member

Choose a reason for hiding this comment

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

Are they guaranteed to not run? Or are they just not guaranteed to run? Does the implementation deal gracefully with implementations that run them anyway?

@RalfJung
Copy link
Member

Oh never mind, I saw this in the FCP list but the list didn't say that this is "FCP close".

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Feb 23, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Feb 23, 2025

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

@Amanieu Amanieu closed this Feb 23, 2025
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.