Server: Remove panics in tab module
#1748
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Note to self: Must replace calls to |
har7an
force-pushed
the
errors/dont-panic-in-server-tab
branch
from
33537e8
to
54d7903
Compare
har7an
force-pushed
the
errors/dont-panic-in-server-tab
branch
from
d3b691b
to
445a51c
Compare
har7an
force-pushed
the
errors/dont-panic-in-server-tab
branch
from
445a51c
to
835b198
Compare
|
@imsnif I changed the dependencies here in that I added the zellij crash dump
We could drop the second stack trace (since IMO it doesn't really contain anything hinting towards the error source), but I thought we keep it in case the Another bonus is that since the backtrace is part of the zellij error log
So we have all useful information in the logs directly and it doesn't get lost. I like it much better now, what's your opinion? |
|
ping, @imsnif |
har7an
force-pushed
the
errors/dont-panic-in-server-tab
branch
from
835b198
to
452d030
Compare
|
I'll merge this now (if CI passes) since with the recent activity around error handling I expect merge conflicts to become much more frequent. We can still revert it before the next release when necessary, or I'll patch it as required. Note to self: Discuss whether we should drop |
|
@har7an - please please please wait for the KDL PR before merging! |
for converting `Result` types that don't satisfy `anyhow`s trait constraints (`Display + Send + Sync + 'static`) conveniently. An example of such a Result is the `SendError` returned from `send_to_plugins`, which sends `PluginInstruction`s as message type. One of the enum variants can contain a `mpsc::Sender`, which is `!Sync` and hence makes the whole `SendError` be `!Sync` in this case. Add an implementation for this case that takes the message and converts it into an error containing the message formatted as string, with the additional `ErrorContext` as anyhow context.
and apply error reporting via `anyhow` instead. Make all relevant functions return `Result`s where previously a panic could occur and attach error context.
to accept an optional 4th parameter, a literal "?". If present, this will append a `?` to the given closure verbatim to handle/propagate errors from within the generated macro code.
and apply appropriate error context and propagate errors further up.
created with `anyhow!`. Since these errors don't have an underlying cause, we describe the cause in the macro instead and then attach the error context as usual before `?`ing the error back up.
to capture error backtraces at the error origins (i.e. where we first receive an error and convert it to a `anyhow::Error`). Since we propagate error back up the call stack now, the place where we `unwrap` on errors doesn't match the place where the error originated. Hence, the callstack, too, is quite misleading since it contains barely any references of the functions that triggered the error. As a consequence, we have 2 backtraces now when zellij crashes: One from `anyhow` (that is implicitly attached to anyhows error reports), and one from the custom panic handler (which is displayed through `miette`).
in the output of miette. Since we record backtraces with `anyhow` now, we end up having two backtraces in the output: One from the `anyhow` error and one from the actual call to `panic`. Adds a comment explaining the situation and another "section" to the error output of miette: We print the backtrace from anyhow as "Stack backtrace", and the output from the panic handler as "Panic backtrace". We keep both for the (hopefully unlikely) case that the anyhow backtrace isn't existent, so we still have at least something to work with.
and leave the `panic`ing to the calling function instead.
which extended `active_tab!` by passing the client IDs to the closure. However, this isn't necessary because closures capture their environment already, and the client_id needn't be mutable.
har7an
force-pushed
the
errors/dont-panic-in-server-tab
branch
from
452d030
to
e4d8d6a
Compare
that defaults to some default client_id if it isn't valid (e.g. when the ScreenInstruction is sent via CLI).
har7an
force-pushed
the
errors/dont-panic-in-server-tab
branch
from
332b4b2
to
1f610a7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removes all calls to
unwrap()in the tab module ofzellij-server. Makes all relevant functions returnResults instead, attaching error context where possible to trace error origins better. Also updates the code inserver::screento accept the newResults and attach context there, too.Related to #1734.
I want to test what it looks like first by provoking some errors in the code to see whether it's really any good.