Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

errors: Don't panic in wasm_vm #1827

Merged
merged 6 commits into from
Oct 20, 2022

Conversation

har7an
Copy link
Contributor

@har7an har7an commented Oct 20, 2022

Removes calls to unwrap and panic in the server API of wasm_vm, replacing explicit calls to panic! with .fatal() instead. Leaves the plugin API unchanged.

The most notable change is that now, when we load incompatible plugins (older versions, whatever) we get pretty error messages. In the terminal it looks like this:

$ ./target/debug/zellij

Error occurred in server:

  × Thread 'screen' panicked.
  ├─▶ Originating Thread(s)
  │   	1. ipc_server: NewClient
  │   	2. pty_thread: NewTab
  │   	3. screen_thread: NewTab
  │   
  ├─▶ At zellij-server/src/panes/plugin_pane.rs:164:42
  ╰─▶ called `Result::unwrap()` on an `Err` value: RecvError
  help: If you are seeing this message, it means that something went wrong.
        Please report this error to the github issue.
        (https://github.com/zellij-org/zellij/issues)
        
        Also, if you want to see the backtrace, you can set the `RUST_BACKTRACE` environment variable to `1`

which is just an effect caused by the initial error (and the next thing on my TODO). The real error is now sitting in the logs:

INFO   |zellij_client            | 2022-10-20 10:29:21.592 [main      ] [zellij-client/src/lib.rs:133]: Starting Zellij client! 
INFO   |zellij_server            | 2022-10-20 10:29:21.597 [main      ] [zellij-server/src/lib.rs:213]: Starting Zellij server! 
INFO   |zellij_server::wasm_vm   | 2022-10-20 10:29:21.658 [wasm      ] [zellij-server/src/wasm_vm.rs:104]: Wasm main thread starts 
DEBUG  |tab-bar                  | 2022-10-20 10:29:21.825 [id: 0     ] thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error("expected value", line: 1, column: 372)', default-plugins/tab-bar/src/main.rs:30:1 
DEBUG  |tab-bar                  | 2022-10-20 10:29:21.825 [id: 0     ] note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace 
ERROR  |zellij_utils::errors::not| 2022-10-20 10:29:21.840 [wasm      ] [zellij-utils/src/errors.rs:435]: Panic occured:
             thread: wasm
             location: At zellij-server/src/lib.rs:722:92
             message: Program terminates: a fatal error occured

Caused by:
    0: If you're seeing this error the most likely cause is that your plugin versions
       don't match your current zellij version.
       
       If you're a user:
           Please contact the distributor of your zellij version and report this error
           to them.
       
       If you're a developer:
           Please run zellij with the updated plugins. The easiest way to achieve this
           is to build zellij with `cargo make install`. Also refer to the docs:
           https://github.com/zellij-org/zellij/blob/main/CONTRIBUTING.md#building
       
    1: failed to update plugin
    2: RuntimeError: unreachable
           at <unnamed> (<module>[190]:0x1d557)
           at <unnamed> (<module>[91]:0xe98a)
           at <unnamed> (<module>[96]:0xf010)
           at <unnamed> (<module>[451]:0x4cb23)
    3: unreachable 
ERROR  |zellij_utils::errors::not| 2022-10-20 10:29:21.840 [screen    ] [zellij-utils/src/errors.rs:435]: Panic occured:
             thread: screen
             location: At zellij-server/src/panes/plugin_pane.rs:164:42
             message: called `Result::unwrap()` on an `Err` value: RecvError 
ERROR  |zellij_utils::errors::not| 2022-10-20 10:29:21.844 [async-std/runti] [zellij-utils/src/errors.rs:435]: Panic occured:
             thread: async-std/runtime
             location: At zellij-server/src/terminal_bytes.rs:124:14
             message: called `Result::unwrap()` on an `Err` value: failed to send message to screen

Caused by:
    0: Originating Thread(s)
       
    1: failed to send message to channel

The runtime error about having reached unreachable code is still as useless as before, but that's out of our hands.

@imsnif Are you fine with the wording I chose? In particular this bit:

  If you're seeing this error the most likely cause is that your plugin versions
  don't match your current zellij version.
  
  If you're a user:
      Please contact the distributor of your zellij version and report this error
      to them.
  
  If you're a developer:
      Please run zellij with the updated plugins. The easiest way to achieve this
      is to build zellij with `cargo make install`. Also refer to the docs:
      https://github.com/zellij-org/zellij/blob/main/CONTRIBUTING.md#building

which is returned by calls to `lock` on various types of locks from
`std`. In our case, some of the locks we try to acquire in `wasm_vm` can
contain an `mpsc::Sender`, which is `!Send` and hence doesn't work with
`anyhow`. Turn the `PoisonError` into an error string instead and
returns that as `anyhow::Err`.
in the WASM VM codes server API. Note that this doesn't include the
Plugin APIs. Mark the error as `fatal` in `server/lib`, where the wasm
thread is created.

This will cause zellij to report a proper error (and log it) when any of
the plugin-related functions fails. Unfortunately, this closes the
channel to the WASM thread. Hence, when loading the plugins upon startup
fails, the error reported in the terminal (visible to the user) hints
towards a call in `plugin_pane` being the culprit. However, the real
error will be contained in the logs.

Also add an error message and print it to the user in case that the
plugin failure was caused by a plugin version mismatch.
@har7an har7an temporarily deployed to cachix October 20, 2022 08:47 Inactive
@imsnif
Copy link
Member

imsnif commented Oct 20, 2022

This is very cool!!!
I didn't look at the code, but I'm very happy to see this.

About the wording: I'd add something about clearing the plugin cache as a possible solution (in the PLUGIN_DIR from zellij setup --check). Otherwise it looks great.

Also: is there any chance we can show this in STDOUT as well as the logs? A lot of users wouldn't know to look there.

@har7an
Copy link
Contributor Author

har7an commented Oct 20, 2022

Also: is there any chance we can show this in STDOUT as well as the logs? A lot of users wouldn't know to look there.

Not quite yet. As you see on stdout, zellij dies trying to receive a message from the wasm_vm, which of course fails (because by then the VM is dead). My next task after this one will be to rewrite plugin_pane and find a way to handle this. I'll do that outside this PR, because it requires me to touch a trait function from Pane, which requires changes in all the pane handling code, you can imagine...

Ideally I'll manage to send a message to the plugin_pane and then handle it there. I'll play around with it. As a quickfix (in case I don't make it until the release, and I don't want to block that), we can change the existing call to unwrap in plugin_pane with something like .expect("Failed to receive message from plugins. Please check the logs.").

Edit: Here's what it looks like:

Error occurred in server:

  × Thread 'screen' panicked.
  ├─▶ Originating Thread(s)
  │   	1. ipc_server: NewClient
  │   	2. pty_thread: NewTab
  │   	3. screen_thread: NewTab
  │   
  ├─▶ At zellij-server/src/panes/plugin_pane.rs:166:18
  ╰─▶ Failed to receive reply from plugin. Please check the logs: RecvError
  help: If you are seeing this message, it means that something went wrong.
        Please report this error to the github issue.
        (https://github.com/zellij-org/zellij/issues)
        
        Also, if you want to see the backtrace, you can set the `RUST_BACKTRACE` environment variable to `1`.

when failing to receive a message from the plugins for rendering pane
contents.
@har7an har7an temporarily deployed to cachix October 20, 2022 09:29 Inactive
@har7an har7an merged commit a56723f into zellij-org:main Oct 20, 2022
har7an added a commit that referenced this pull request Oct 20, 2022
which removes calls to `unwrap` from `wasm_vm` and prints sensible error messages when errors in the plugins occur.
@har7an har7an deleted the errors/dont-panic-in-wasm_vm branch October 23, 2022 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants