Commit graph

12 commits

Author SHA1 Message Date
Mahmoud Al-Qudsi
4e3dc51bc4 Prevent test suite from hanging on panic 2024-11-11 16:45:13 -06:00
Johannes Altmanninger
30cba03bf9 Make SIGTERM handler async-signal-safe again 2024-10-21 09:30:47 +02:00
Johannes Altmanninger
3d5ef2bcf5 Fix inverted condition in panic handler
Fixes 139d204c (Restore terminal state again in panic handler, 2024-10-12).
2024-10-19 22:04:54 +02:00
Johannes Altmanninger
a139d204c0 Restore terminal state again in panic handler
Our panic handler attempts a blocking read from stdin and only exits
after the user presses Enter.

This is unconventional behavior and might cause surprise but there is a
significant upside: crashes become more visible for terminals that don't
already detect crashes (see ecdc9ce1d (Install a panic handler to avoid
dropping crash stacktraces, 2024-03-24)).

As reported in 4d0aa2b5d (Fix panic handler, 2024-08-28), the panic handler
failed to exit fish if the panic happens on background threads.  It would
only exit the background thread (like autosuggestion/highlight/history-pager
performer) itself. The fix was to abort the whole process.
Aborting has the additional upside of generating a coredump.

However since abort() skips stack unwinding, 4d0aa2b5d makes us no longer
restore the terminal on panic. In particular, if the terminal supports kitty
progressive enhancements, keys like ctrl-p will no longer work in say,
a Bash parent shell.  So it broke 121680147 (Use RAII for restoring term
modes, 2024-03-24).

Fix this while still aborting to create coredumps.  This means we can't use
RAII (for better or worse).  The bad part is that we have to deal with added
complexity; we need to make sure that we set the AT_EXIT handler only after
all its inputs (like TERMINAL_MODE_ON_STARTUP) are initialized to a safe
value, but also before any damage has been done to the terminal. I guess we
can add a bunch of assertions.

Unfortunately, if a background thread panics, I haven't yet figured out how
to tell the main thread to do the blocking read.  So the trick of "Press
Enter to exit", which allows users to attach a debugger doesn't yet work for
panics in background threads.  We can probably figure that out later. Maybe
use pthread_kill(3)?  Of course we still create coredumps, so that's fine.
As a temporary workaround, let's sleep for a bit so the user can at least
see that there is a crash & stacktrace.

One ugly bit here is that unit tests run AT_EXIT twice but it should be
idempotent.
2024-10-12 13:28:55 +02:00
Johannes Altmanninger
468849dd54 Minor refactoring in panic handler
I don't think I really get why this newline is here. It moves the cursor
from the end of the newline to the beginning of the next line.  Maybe it
was added only for panics in background threads?  Either way it's fine.
2024-10-12 12:18:50 +02:00
Mahmoud Al-Qudsi
5278259312 Don't break out of panic handler
The previous control flow logic wasn't sound and would leave the shell in a hung
state when `break` would be encountered.

The behavior is now straightforward, the shell reads until <Enter> or <q> is
pressed, at which point it aborts.
2024-08-28 17:38:05 -05:00
Mahmoud Al-Qudsi
4d0aa2b5dd Fix panic handler
It wasn't actually exiting and would basically block indefinitely after reading
from stdin.
2024-08-28 17:29:56 -05:00
Johannes Altmanninger
a216b3cf6a Print panic message to stderr, like the stack trace 2024-04-02 07:34:19 +02:00
Johannes Altmanninger
1232cfd3bb Fix typo 2024-04-02 07:33:07 +02:00
Johannes Altmanninger
af6dc9221f Use panic::set_hook instead of catch_unwind to help debug panics 2024-04-02 07:27:22 +02:00
Johannes Altmanninger
1216801474 Use RAII for restoring term modes
In particular, this allows restoring the terminal on crashes, which is
feasible now that we have the panic handler.  Since std::process::exit() skips
destructors, we need to reshuffle some code.  The "exit_without_destructors"
semantics (which std::process::exit() als has) was mostly necessary for C++
since Rust leaks global variables by default.
2024-03-24 16:34:36 +01:00
Johannes Altmanninger
ecdc9ce1dd Install a panic handler to avoid dropping crash stacktraces
When fish crashes due to a panic, the terminal window is closed.  Some
terminals keep the window around when the crash is due to a fatal signal,
but today we don't exit via fatal signal on panic.

There is the option to set «panic = "abort"» in Cargo.toml, which
would give us coredumps but also worse stacktraces on stderr.
More importantly it means that we don't unwind, so destructors are skipped
I don't think we want that because we should use destructors to
restore the terminal state.

On crash in interactive fish, read one more line before exiting, so the
stack trace is always visible.

In future, we should move this "read one line before exiting" logic to where
we call "panic!", so I can attach a debugger and see the stacktrace.
2024-03-24 13:36:59 +01:00