Feature: Better error handling/reporting #1670
Conversation
|
Hey, cool to see this coming along.
I'm not sure I understand the purpose of this? I remember we talked about switching methods with |
Good question. That's because I expect that in some cases, like for modules in The idea is that moving from This way we can contain the changes in error handling to a single module, without having to traverse up to all other modules on the way. So you're not a fan of that idea? |
|
Not particularly, I admit. I'm very on board with doing this incrementally, but I feel this will both create more work and give us a false sense of progress. I do get what you're saying about not wanting to traverse the whole codebase - but I don't share your pessimism. I think if we find one particular function that is called in many different places, we can do a PR specifically for that function. Remember: we don't need to bubble a Result all the way up in order to make progress. We can change all the |
But that's exactly what I said, no? the Edit: Note that it doesn't make the panics go away, they just happen further up the codepath (somewhere in the plugins) now, with an |
My issue is mostly with the expect. I've been personally burned in this code-base by maintainers leaving something "for someone else to do later" and that someone never coming along (or being me when I absolutely would prefer to do something else :) ). So - do this as you wish, but as a personal request: please only leave unfinished stuff in the code-base if you yourself intend on seeing them through in the near future. |
|
While testing with the debug plugins I had the impression that the tab bar has become very slow now, but that may just be the lack of wasm-opt... To the best of my knowledge all plugins are now free from panics caused by While provoking an error in the status bar I noticed that the plugins now sort of die gracefully - They just don't render any text at all. It's better than crashing (which is what happened previously), but I'm not sure whether that's ideal. Also the plugins recover from errors now. I tested this by causing an error when displaying the "LOCKED" indication in the second line of the status bar: When I enter "LOCKED" mode the status bar stays blank, when I leave "LOCKED" mode it recovers. I'm now thinking of a way to display the error to the user. Given that Plugin -> Server communication is somewhat limited, I guess the best option is to print the error to the bit of "canvas" we have and maybe log it, too. Or do you have a better suggestion? |
|
Oh and on a related note: Who calls these functions defined by |
|
I've found a solution now, here's what it looks like in action:
Of course it can only print error information on lines that do exist, so if the tab bar dies we will only see the last error in the chain. However, it will also log the error: And in the case that the plugin does panic (while loading, or by failing to What do you think? |
har7an
force-pushed
the
feature/better-error-reporting
branch
from
7bd5073
to
d095e22
Compare
zellij-tile/src/lib.rs
Outdated
| /// Catch non-fatal errors and print their content to the plugins stdout and stderr. This way, the | ||
| /// user sees the error and can even report it, but it will not crash the application. Hence, this | ||
| /// works only on the empty type `()`. | ||
| pub fn catch(ret: anyhow::Result<()>) { | ||
| match ret { | ||
| Ok(_) => (), | ||
| Err(err) => { | ||
| println!("ERROR: {}", err); | ||
| eprintln!("ERROR: {}", &err); | ||
| err.chain().skip(1).for_each(|cause| { | ||
| let msg = format!("because: {cause}"); | ||
| println!("{}", msg); | ||
| eprintln!("{}", &msg); | ||
| }); | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| /// Return the inner value if `Ok(_)` and panic the application otherwise. Before panicking, the | ||
| /// error is written to the logfile. | ||
| pub fn catch_fatal<T>(ret: anyhow::Result<T>) -> T { | ||
| match ret { | ||
| Ok(val) => val, | ||
| Err(err) => { | ||
| eprintln!("ERROR: {}", &err); | ||
| err.chain().skip(1).for_each(|cause| { | ||
| let msg = format!("because: {cause}"); | ||
| eprintln!("{}", &msg); | ||
| }); | ||
| panic!("{:#?}", err); | ||
| }, | ||
| } |
There was a problem hiding this comment.
@tlinford Maybe you have an idea how this bit of code can be written prettier? I wanted to put this into a trait and implement that on std::result::Result but I failed miserably. Makes the code below pretty unreadable, too...
|
@har7an - what do you think about (also) sending the error to the log? I think with plugins if you write to STDERR (eg. with |
har7an
force-pushed
the
feature/better-error-reporting
branch
3 times, most recently
from
81a2ee9
to
f9d58dc
Compare
|
@imsnif You mean the Shifted the code around a little and managed to make the whole I think this is a very solid foundation to build the error handling upon now. In theory we could slap a I'll leave the PR hanging around for another day or two to see if anything else that could be improved comes to my mind. I think it's a big step in the right direction, even if only because an error while rendering plugins won't crash the whole application any more and all plugin errors are now logged, even if zellij crashes. |
|
@har7an - briefly going over this, I notice that you also change the plugin API. Note that this is a breaking change for all plugins (the ones we know about and the ones we don't :) ), will require a documentation change on the docs website, and will likely require at least some amount of continuing support to people who use custom plugins and want to upgrade to the latest version. |
|
Also, does this affect non-rust plugins? We've got a budding Zig ecosystem too. |
har7an
force-pushed
the
feature/better-error-reporting
branch
from
e3bb4d5
to
8cae215
Compare
even for wasm targets.
and move everything that can't run in wasm into a separate submodule.
The module is now structured such that all code incompatible with wasm targets lives in its own submodule that isn't included when compiling for wasm targets.
har7an
force-pushed
the
feature/better-error-reporting
branch
from
94817d4
to
9ccc72e
Compare
|
Good hint about the plugin API change, thanks! I changed the plugin API back to what it was before, but I kept the modifications in the plugins and am now handling the errors inside the plugins, too. This makes handling the errors in the plugins completely optional, but still allows us to communicate to the user when things went wrong instead of directly crashing zellij (at least in some cases, and for the core plugins only, of course). As discussed recently @imsnif, I also went ahead and updated the public API of All of the errors are now extended by a I'll update the module docs in Overall I think it's a great (and convenient) addition to zellij. I'll do a few more tests in the next days to see how it works out in practice. More on this soon... |
that informs the reader about the endeavour to improve error handling throughout the zellij code base.
now that the panic calls have been removed.
that allow for easy/concise logging of `anyhow::Result` types and panicking the application when they are fatal or non-fatal.
that applications can import to conveniently access the error handling functionality part of the module. Re-exports some parts and macros from anyhow and the `LoggableError` and `FatalError` traits.
and make all fallible functions from the public API return a `Result`. Append error contexts in all functions that can come across error types to allow tracing where an error originates and what lead there.
and make them `fatal`. This will log the errors first, before unwrapping on the error type and panicking the application.
and unwrap on the `Result` types introduced from the new error handling.
in calls to `fatal`, so we keep track of the location where the panic really originates. Otherwise, any call to `fatal` will show the code in `errors` as source, which of course isn't true. Also change the error formatting and do not call `to_log` for fatal errors anymore, because the panic is already logged and contains much more information.
har7an
force-pushed
the
feature/better-error-reporting
branch
from
aa7bfeb
to
1a136f8
Compare
|
For the record: I tried implementing sending errors that do not change the servers state to the clients to crash only the offending client instead of the whole application in certain situations. To do that I need something that implements Note to self: In the next meeting I'd like to discuss the usage of trait objects vs. generics. |
explain some error handling facilities and the motivation behind using them.
functions that are part of the public API.
that explains how we plan to change error handling in zellij and invites new contributors to join the fun. Details the currently existent error handling capabilities and gives a bunch of examples taken from zellij-org#1670.
that explains how we plan to change error handling in zellij and invites new contributors to join the fun. Details the currently existent error handling capabilities and gives a bunch of examples taken from zellij-org#1670.
* docs: Add ERROR_HANDLING that explains how we plan to change error handling in zellij and invites new contributors to join the fun. Details the currently existent error handling capabilities and gives a bunch of examples taken from #1670. * utils/errors: Shorten docblock by moving previous content under "Help Wanted" to the new `docs/ERROR_HANDLING` document and link to the document instead.
As discussed in the dev meeting yesterday, here's a proposal on how to improve the error handling in zellij.
Here's what it currently does:
zellij_utils::errorsso we can share the contents (at least some) with code targeted atwasmexpect()statementszellij_utils::errorsthat invites users to join in on the fun and improve the error handling with uszellij_tilesubcrateLet me know what you think!
Maybe we can also link to this very commit from the docs in the
errorssubmodule so users have an example what the "improvement" may look like?