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

Document panic from thread::current #107216

Closed
wants to merge 1 commit into from

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Jan 23, 2023

Playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=978be7b8cb9d857f33f217ab4ff13eca

extern "C" fn get_thread() {
    let _ = std::panic::catch_unwind(std::thread::current);
}

fn main() {
    unsafe { libc::atexit(get_thread) };
}
thread '<unnamed>' panicked at 'use of std::thread::current() is not possible after the thread's local data has been destroyed', library/std/src/thread/mod.rs:733:5
stack backtrace:
   5: core::option::Option<T>::expect
             at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/core/src/option.rs:741:21
   6: std::thread::current
             at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/std/src/thread/mod.rs:733:5
   7: core::ops::function::FnOnce::call_once
             at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/core/src/ops/function.rs:251:5
  11: std::panic::catch_unwind
             at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/std/src/panic.rs:137:14
  12: playground::get_thread
             at ./[src/main.rs:2](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021#):13
  14: exit
  15: __libc_start_main
  16: _start

@rustbot
Copy link
Collaborator

rustbot commented Jan 23, 2023

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added 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. labels Jan 23, 2023
@dtolnay dtolnay changed the title Document panic from std::thread::current Document panic from thread::current Jan 23, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 23, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@dtolnay
Copy link
Member Author

dtolnay commented Jan 23, 2023

Not sure whether we're able to guarantee a safe panic in this situation, or if this use of atexit is unsound.

@rfcbot poll libs Guarantee panic?

@rfcbot
Copy link

rfcbot commented Jan 23, 2023

Team member @dtolnay has asked teams: T-libs, for consensus on:

Guarantee panic?

@thomcc
Copy link
Member

thomcc commented Jan 23, 2023

Guaranteeing anything after main is probably pretty difficult in general.

@thomcc
Copy link
Member

thomcc commented Jan 23, 2023

In particular, if an implementation ran TLS dtors for the main thread after calling atexit callbacks, I'm unsure how we would implement this if we wanted to guarantee panic in that case -- we could do it in the case where we control main(), but that's not guaranteed.

I think it's reasonable to document that this will panic in some cases, though.

@ChrisDenton
Copy link
Member

See also Windows.

In general I think it'd be good to actually document that the stdlib should not be used before/after main (or at least that your on your own if you try). But by the same token we can't really guarantee anything specific happens if you do do stuff before or after main. At least not in a cross-platform way.

@@ -717,6 +717,13 @@ where

/// Gets a handle to the thread that invokes it.
///
/// # Panics
///
/// Panics if called beyond the end of `main`, when the Rust standard library's
Copy link
Member

Choose a reason for hiding this comment

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

Why not just remove the guarantee?

Suggested change
/// Panics if called beyond the end of `main`, when the Rust standard library's
/// This may panic if called beyond the end of `main`, if the Rust standard library's

Comment on lines +724 to +725
/// platforms can hit this. In general much of the standard library is not okay
/// to use before or after `main`.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really the right place to mention this, as ChrisDenton said it would be good to have some module documentation (perhaps on the std top level module) explaining this and then linking to that section.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll look for a spot. Is it fair to say 100% of libcore is fine to use before/after main, but the default assumption for libstd stuff like std::thread and std::io is it's UB to use before/after main, with the observable behavior being indistinguishable from a panic in the best case? How about liballoc?

Copy link
Member

Choose a reason for hiding this comment

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

Some core and alloc functions could get poisoned through hooks like the global allocator or panic hooks.

I'll look for a spot.

Since this affects multiple modules maybe the top level?

the default assumption for libstd stuff like std::thread and std::io is it's UB to use before/after main, with the observable behavior being indistinguishable from a panic in the best case

That's a tough one, I guess it's true that there might be UB lurking somewhere but feels a bit like a "known to the state of california to cause cancer" statement.

Copy link
Member

Choose a reason for hiding this comment

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

Technically there are some functions that are probably UB on Windows. In particular when you're running before main on Windows, the loader lock is likely held, and we may call into functions that are UB if the loader lock is helt (but in practice these are likely to be just a deadlock).

We historically have taken measures to reduce the damage of UB (usually by mitigating it into a "does not work" state) from doing stuff to std before main, as people do do it. I would certainly like to discourage it, though.

Copy link
Member

Choose a reason for hiding this comment

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

with the observable behavior being indistinguishable from a panic in the best case

The best case is that it just works. But that depends on the platform and operation.

@dtolnay dtolnay 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 27, 2023
@dtolnay
Copy link
Member Author

dtolnay commented Apr 23, 2023

I filed #110708 with takeaways from the feedback above, and I'll close this PR since I agree this is not appropriate to document like this.

@dtolnay dtolnay closed this Apr 23, 2023
@dtolnay dtolnay deleted the threadpanic branch April 23, 2023 01:15
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-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.

7 participants