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

std: Don't assume thread::current() works on panic #24478

Merged
merged 2 commits into from
Apr 28, 2015

Conversation

alexcrichton
Copy link
Member

Inspecting the current thread's info may not always work due to the TLS value
having been destroyed (or is actively being destroyed). The code for printing
a panic message assumed, however, that it could acquire the thread's name
through this method.

Instead this commit propagates the Option outwards to allow the
std::panicking module to handle the case where the current thread isn't
present.

While it solves the immediate issue of #24313, there is still another underlying
issue of panicking destructors in thread locals will abort the process.

Closes #24313

@rust-highfive
Copy link
Collaborator

r? @pcwalton

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

@alexcrichton
Copy link
Member Author

r? @aturon

@rust-highfive rust-highfive assigned aturon and unassigned pcwalton Apr 15, 2015
@@ -18,7 +18,6 @@ use cell::UnsafeCell;

// Sure wish we had macro hygiene, no?
#[doc(hidden)]
#[unstable(feature = "thread_local_internals")]
Copy link
Member

Choose a reason for hiding this comment

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

Were these accidentally stripped?

Copy link
Member Author

Choose a reason for hiding this comment

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

The module at the top-level has an #[unstable] attribute which everything inside inherits from by default. (it was intentional)

@alexcrichton
Copy link
Member Author

I've opened #24479 about panicking destructors in TLS aborting the process, which I believe is a separable issue.

@aturon
Copy link
Member

aturon commented Apr 27, 2015

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 27, 2015

📌 Commit 3d04a33 has been approved by aturon

@bors
Copy link
Contributor

bors commented Apr 27, 2015

⌛ Testing commit 3d04a33 with merge 240ff7b...

@bors
Copy link
Contributor

bors commented Apr 27, 2015

💔 Test failed - auto-win-32-nopt-t

Don't need so much manual #[doc(hidden)] and #[unstable] as much of it is
inherited!
Inspecting the current thread's info may not always work due to the TLS value
having been destroyed (or is actively being destroyed). The code for printing
a panic message assumed, however, that it could acquire the thread's name
through this method.

Instead this commit propagates the `Option` outwards to allow the
`std::panicking` module to handle the case where the current thread isn't
present.

While it solves the immediate issue of rust-lang#24313, there is still another underlying
issue of panicking destructors in thread locals will abort the process.

Closes rust-lang#24313
@alexcrichton
Copy link
Member Author

@bors: r=aturon d98ab4f

@bors
Copy link
Contributor

bors commented Apr 28, 2015

⌛ Testing commit d98ab4f with merge 2b8c9b1...

bors added a commit that referenced this pull request Apr 28, 2015
Inspecting the current thread's info may not always work due to the TLS value
having been destroyed (or is actively being destroyed). The code for printing
a panic message assumed, however, that it could acquire the thread's name
through this method.

Instead this commit propagates the `Option` outwards to allow the
`std::panicking` module to handle the case where the current thread isn't
present.

While it solves the immediate issue of #24313, there is still another underlying
issue of panicking destructors in thread locals will abort the process.

Closes #24313
@bors bors merged commit d98ab4f into rust-lang:master Apr 28, 2015
@alexcrichton alexcrichton deleted the issue-24313 branch April 30, 2015 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panicking in the drop function of a struct that is stored in a thread local causes a stack overflow
6 participants