Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Set panic to abort in release mode #1762

Merged
merged 3 commits into from Feb 25, 2021

Conversation

ObsidianMinor
Copy link
Contributor

This should fix cases where we spawn threads that panic and get us in an invalid state that requires us to get killed anyway.

This should fix cases where we spawn threads
that panic and get us in an invalid state that
requires us to get killed anyway.
@ObsidianMinor ObsidianMinor requested a review from a team as a code owner February 8, 2021 19:13
@ObsidianMinor
Copy link
Contributor Author

We currently don't handle unwinds so switching to abort shouldn't cause any issues. If we do want to start handling unwinds, we'll need to remove this.

@ags799
Copy link
Contributor

ags799 commented Feb 10, 2021

This means that if a thread panics other threads can continue? Can you give an example where that would be useful?

@ObsidianMinor
Copy link
Contributor Author

That's the current behavior. Unwind causes panics to unwind the thread. If the panic happens on a thread that's not the main thread, the thread just returns an error on join, but if you drop the join handle (like we do every time we spawn a thread), the thread just dies and everything continues as normal.

That's not what we want to happen, since we want real errors (failure::Error), not panics. Abort changes the behavior to process abort which can never leave us in a bad state if a thread panics. For example, if the fs change notifier thread panics, the process won't be left hanging doing nothing.

@xortive
Copy link
Contributor

xortive commented Feb 25, 2021

@ObsidianMinor not super familiar with this, this won't cause errors to not be printed when a thread panics right?

@ObsidianMinor
Copy link
Contributor Author

this won't cause errors to not be printed when a thread panics right?

Yes, panics still go through the panic handler, but instead of the panic handler unwinding, it simply aborts after printing.

@xortive xortive merged commit 5fac2b0 into cloudflare:master Feb 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants