zellij-server: improve thread_bus error handling

[arriven]

WIP: #1753

As suggested in my previous PR, moving thread_bus error changes to a separate one

[har7an]

Thanks for continuing your work on this!

I left a remark below, do you mind applying it to your other changes, too? That'd be awesome (and very helpful for debugging).

[har7an]

Do you think we can get rid of the unwrap() attached to as_ref, too? Now that we already return a Result we may as well report that error case.

Also what do you think about adding the instruction we sent to the error message, similar to how you already do in send_to_plugin? It may not be helpful for the user, but maybe a developer can make sense of all the content.

[arriven]

Good point about .as_ref().unwrap()

As for including instruction in other errors, I was assuming that their existing serialization already does that but let me check to be sure

[arriven]

upd: crossbeam::channel::SendError doesn't format the content so yes, that pattern would be helpful for devs. Also I've noticed you've added it as a function on the type so I've moved the usage of .to_anyhow() into those thread_bus functions, hope you don't mind

[har7an]

Don't mind at all, well done. And thanks for doing the research!

[har7an]

Great, it looks very polished now!

One last thing: I implemented to_anyhow back then for use with to_client, hence the message states "failed to send message to client ...". Could you reword it to something more generic like "failed to send instruction: ..." instead? That way it doesn't feel "out of place" when e.g. we really tried to send to the server.

Now I'm curious: Do you think you can provoke an error in one of the senders and paste it here so we get a glimpse of what it looks like? For the sake of testing it, you could inject an early return into one of the ScreenInstruction handling functions, maybe in screen.rs:1787:

   2                 screen.render()?;
   1                 screen.unblock_input()?;
1788                 return Ok(());
   1             },
   2             ScreenInstruction::RightClick(point, client_id) => {

and then inject a fatal in route.rs:494 like so:

  5         Action::LeftMouseRelease(point) => {
  4             use zellij_utils::errors::FatalError;
  3             session
  2                 .senders
  1                 .send_to_screen(ScreenInstruction::LeftMouseRelease(point, client_id))
495                 .fatal();
  1         },

and then start zellij and do a left click somewhere in the terminal. That should cause an error.

[har7an]

Good catch, initially I implemented it as a crutch for the changes in tab, as you noticed correctly. Thank you for removing it in tab!

[arriven]

I mean, it refused to compile otherwise since anyhow::Result<T> != Result<T, crossbeam::channel::SendError> but thanks! 😅

[arriven]

sure, the error looks like this

 
Error occurred in server:

  × Thread 'server_router' panicked.
  ├─▶ Originating Thread(s)
  │     1. stdin_handler_thread: AcceptInput
  │   
  ├─▶ At zellij-server/src/route.rs:495:18
  ╰─▶ Program terminates: a fatal error occured
      
      Caused by:
          0: failed to send message to screen
          1: Originating Thread(s)
                1. stdin_handler_thread: AcceptInput
      
          2: failed to send message to channel: LeftMouseRelease(
                 Position {
                     line: Line(
                         3,
                     ),
                     column: Column(
                         54,
                     ),
                 },
                 1,
             )
      

It's even better in the terminal due to coloring but I don't think github would be able to show that 😅

[har7an]

Nice, I think it looks great now. Thanks for your contribution!