zellij-server/src/screen: improve error handling

[arriven]

Got rid of last unwrap() calls on result, replacing them with error propagation. The only unwrap() left are on Option type which would be converted into fatal() anyway since the code already checks for the corresponding condition in that same function (move_clients_from_closed_tab)

WIP #1753

[har7an]

Hi, thanks for picking this up!

I added some suggestions to the code. In particular: Could you please rephrase the error contexts you introduced? You can have a look at the docs if it helps you. Consider for example the message "failed to send message to server": You introduced it twice, in different portions of the code, but attached to the same function (send_to_server). If I see this error after zellij crashed and search for it, I will find it in two different locations. How do I know where the error happened then?

You observation on the Option types in move_clients_from_closed_tab is correct, the code here already checks for the necessary pre-conditions. I don't think we need to take further actions there.

[har7an]

I think we should rephrase it to say something about what we tried to do when we got that error (When in doubt, refer to the function name). Ideally, send_to_server already returns an error that tells us something like "failed to deliver message" or so.

I'll go on a bit of a hunt to see whether that's actually the case. If it isn't, I think we should attach this particular bit of context inside the call to send_to_server (to keep us from replicating it across the codebase. This function is used rather often).

[arriven]

It doesn't seem like send_to_server (and other similar functions) are attaching that kind of context to the error. In order to support that we'd need to change the return type for those but it'll definitely break patterns like these

zellij/zellij-server/src/screen.rs

Lines 835 to 840 in 716f606

	.or_else(|err| {
	let (_, error_context) = err.0;
	Err(anyhow!("failed to send data to plugins"))
	.context(error_context)
	.context("failed to update tabs")
	})?;

I'll probably try to see how many places with this type of usage you have

[arriven]

ok, it's just one place where it's used that way. Interestingly, I had to move this piece of code into send_to_pty because of missing traits for it's instruction. Not sure if it makes it better but I guess it's easier for you to take a look at the implementation and for me to revert that particular commit if it doesn't work that to operate on theory

[har7an]

@arriven Yeah, that's a funny story. It stems from the fact that the PluginInstruction we send there is an enum, where one of its variants holds a mpsc::Sender. anyhows .context & friends requires the error types they get to be 'static + Send + Sync (And in our case the error type is SendError<(PluginInstruction, _)>), but mpsc::Sender is !Sync. That's why I made use of this "crutch" instead and construct an anyhow::Error from scratch. It's not pretty to look at, but it needn't be imo.

[har7an]

Okay, your changes look very good to me! I like how you rephrased and rewrote the docs, I think it's much clearer now than it was before, thanks.

I would prefer if we handle the changes to thread_bus.rs in a separate PR. While they do relate to the screen, of course, I think they're a good candidate for a standalone PR. And if you decide to work on that, too, I think it would be great if we stuck with this code:

                .or_else(|err| {
                    let (_, error_context) = err.0;
                    Err(anyhow!("failed to send data to plugin"))
                        .context(error_context)
                })

but also make use of the _ (which is the instruction we tried to send) and include that in the error context as well. We have that bit of information after all, so we may as well show it to someone. As far as I know, all the instruction types we have should at least be Debug, so printing them should be possible.

Once you revert that commit and CI passes, this PR is good to go!

[har7an]

P.S.: I see the CI doesn't like your formatting. I recommend you run cargo make from the project root directory once, that'll fix all formatting-related issues for you. You needn't even install cargo make, if you don't want to. It has binary releases available on github: https://github.com/sagiegurari/cargo-make/releases/tag/0.36.1

Also refer to our documentation about building here: https://github.com/zellij-org/zellij/blob/main/CONTRIBUTING.md#building

[har7an]

Very good, thank you!

[arriven]

@har7an I had to use cargo make format-flow since cargo make fails on clippy warnings unrelated to my changes (mostly complaining about #[derive(PartialEq)] that doesn't derive Eq) and just using rustfmt didn't produce any result. Do you mind if I also open an issue about that and potentially fix the warnings?

[har7an]

Do you mind if I also open an issue about that and potentially fix the warnings?

That's interesting. I just ran cargo make on the latest main and everything passed. Are you absolutely sure it's unrelated to your changes? Does it also happen when you run cargo make on main?

[arriven]

Yes, I'm fairly certain that it's not related. All the errors are coming from zellij-utils and happen on main. It might be due to my cargo make version (0.36.0, the one you've linked is 0.36.1) but the errors seem quite plausible. Every error except one is

you are deriving `PartialEq` and can implement `Eq`

so that might be my cargo make not picking some kind of parameter that's supposed to disable that warning but there's also an error about using message.push_string(&format!(...)) when write!(&mut message, ...) is available (and in fact is used right at the line above the one producing this error)

[har7an]

That's weird... We'll see what the CI has to say about that. If all checks pass here, feel free to open the issue and have a closer look!

cargo make is really just a more buffed version of GNU make and calls other tools (like rustfmt) under the hood to perform all the build/lint/check tasks. So I think cargo make can be ruled out as the culprit in this case. Maybe for some reason you're not using the 1.61 rust toolchain version? What does rustfmt --version give you? I see this: rustfmt 1.4.38-stable (fe5b13d 2022-05-18)

[arriven]

Yeah, it's definitely something with my setup. I decided to look into that rabbit hole of errors and after fixing errors in zellij-utils, zellij-client, and zellij-server (each step was unlocked only after you complete previous one 😃 ) it started complaining about issues in dependencies, and on contrary to issues in actual workspace those ones don't seem to make a lot of sense, i.e.:

error[E0412]: cannot find type `Result` in this scope
   --> /home/arriven/.cargo/registry/src/github.com-1ecc6299db9ec823/once_cell-1.12.0/src/race.rs:271:28
    |
271 |             F: FnOnce() -> Result<Box<T>, E>,
    |                            ^^^^^^ not found in this scope

As for versions, I have the following:

• rustfmt 1.5.1-stable
• rustc 1.63.0
• cargo 1.63.0
• clippy 0.1.63
• cargo-make 0.36.0

I should probably try it with your nix setup to have proper versions of dependencies first

[har7an]

Yeah well that error really doesn't make any sense. Maybe try a cargo clean? I've encountered similar problems with embedded rust before, but I don't remember how I made those go away...

I'm sure you'll figure it out, though. Thanks for contributing! :)