daa84 / neovim-lib

Rust library for Neovim clients
GNU Lesser General Public License v3.0
192 stars 26 forks source link

Log an error instead of panicking when send fails #20

Closed Olical closed 6 years ago

Olical commented 6 years ago

Possible "fix" for #19, unless you think this isn't really the issue and this is a weird workaround / you want it to panic at that point.

This does mean an error will still be logged if someone calls echo but doesn't acknowledge it in Neovim before the timeout. If this is PR is completely invalid, feel free to close. Just wanted to suggest it :smile:

Olical commented 6 years ago

Maybe if it was a warning this would actually be okay... may want the same for async too I guess.

Olical commented 6 years ago

The scope of this has expanded to spelling corrections and exposing RequestHandler, I couldn't get my project to compile otherwise. I ended up with this for my handler:

impl neovim_lib::Handler for Handler {
    fn handle_notify(&mut self, name: &str, args: Vec<Value>) {
        let event = Event::from(name, &args);

        if let Err(msg) = self.tx.send(event) {
            error!("Could not send event through channel: {}", msg);
        }
    }
}

impl neovim_lib::RequestHandler for Handler {
    fn handle_request(&mut self, _name: &str, _args: Vec<Value>) -> Result<Value, Value> {
        error!("Requests are not supports, use notify");
        Err(Value::Nil)
    }
}
Olical commented 6 years ago

With this branch my program writes the following to the log file when I echo and let it timeout:

11:05:41 [ERROR] Command failed (echo "[bar] 127.0.0.1:5555 for files matching 'clj$'
[foo] 127.0.0.1:5555 for files matching '.clj'"): Unknown error type: Wait timeout (nvim_command)
11:05:46 [DEBUG] neovim_lib::rpc::client: Get message RpcResponse { msgid: 2, error: Nil, result: Nil }
11:05:46 [WARN] Send returned Err: sending on a closed channel

That last warn is where it would have panicked in the past.

daa84 commented 6 years ago

Think send to closed channel is a panic and application can't work after this, better crash here.

Olical commented 6 years ago

So maybe that change is invalid, but I think the exposing and spelling corrections would still be useful. I fixed my original issue by using the async API. Happy to close this if you want, but I had to make some of the changes you see in this PR to even get my code to compile. I think some of it could be useful.

daa84 commented 6 years ago

spelling corrections is ok, you can update PR and I apply it or i can update code with this fixes later