Add support for connecting with handshake message

[fredizzimo]

Unexpected standard output, when starting the shell for example is causing a lot of confusion for the users of Neovide. This fixes that by making it possible to connect with a handshake message, which is sent immediately upon connecting. All output will be ignored until the expected response from the message is received (byte by byte), instead of trying to decode the invalid output.

I first considered implementing this completely on the Neovide side, but it's not possible, due to message id having to be constantly increasing. And I also think that other users might find this useful.

The tests are not very exhaustive, but they should cover the common cases. They are only run on unix, because it's a bit harder to craft extra output on Windows and I'm working on a Linux machine. They are also only run with Tokio, due to std-async, not supporting passing custom command lines.

[KillTheMule]

Hey thanks for that wellcrafted PR! Before going to deep into the code, I'd like to understand the motivation better. What should make the child cmd put out extra things to its stdout? Aren't you at neovide in full control of what you spawn? How does the user come into play here? Maybe you could just link a neovide issue where this is described :)

[fredizzimo]

Unfortunately, we don't have full control. When launching on WSL on Windows or when using macOS, we need to launch a non-interactive login shell, that runs nvim. And users are not necessarily configuring it correctly to not output anything and sometimes that's not even fully under their control, at least not without strong technical knowledge. Using sockets is also not an option, at least not on Windows. Unix sockets do not work between windows and WSL, and we can't guarantee that the network is setup to allow communication between WSL and Windows. The port selection would also be problematic, to reliably report which port was selected in the presence of potential extra output.

Another use case is when passing wrong parameters to neovim, so that no rpc server is even started. For example, even neovide -- --help, which internally starts neovim --embed --help and doesn't start the server. Currently, the only thing we can report in this case is that Neovim crashed.

Here are some issues, not all of them are completely related, some of them caused by the overly complex startup that we need currently to at least get some form of error reporting. Most are caused by stderr output though, which we haven't allowed, but in theory could, although with less reporting of what goes wrong when it does.
https://github.com/neovide/neovide/issues?q=is%3Aissue%20%22unexpected%20output%20%22%20

With these changes it will be possible to report both the stdout and stderr when Neovim doesn't start properly, either due to invalid shell configuration or something else.

[KillTheMule]

So my knowledge on non-linux is kinda limited, please bare with me :)

Unfortunately, we don't have full control. When launching on WSL on Windows or when using macOS, we need to launch a non-interactive login shell, that runs Neovide.

Did you mean to write "... that runs nvim"? Because I don't see what the shell that runs neovide would have to say here that ends out in our internal stdio.

... sockets ...

Yeah, let's not worry about those other connection options, stdio should definitely work for you ;)

Another use case is when passing wrong parameters to neovim, so that no rpc server is even started. For example, even neovide -- --help, which internally starts neovim --embed --help and doesn't start the server. Currently, the only thing we can report in this case is that Neovim crashed.

Ok, I can see how that's not working, but I'd assume you'd not call out to nvim-rs in that case anyways, but just run the command the standard way and capture its stdout. Would that solve this particular aspect?

[fredizzimo]

Did you mean to write "... that runs nvim"? Because I don't see what the shell that runs neovide would have to say here that ends out in our internal stdio.

Yes, nvim, sorry I wrote a bit too quickly

Ok, I can see how that's not working, but I'd assume you'd not call out to nvim-rs in that case anyways, but just run the command the standard way and capture its stdout. Would that solve this particular aspect?

We actually pass everything after the -- directly to nvim and try to connect nvim-rs, currently that includes --help and --version. They could be handled as special cases, but it does not really solve the problem if an invalid argument is passed for example.

[KillTheMule]

We actually pass everything after the -- directly to nvim and try to connect nvim-rs, currently that includes --help and --version. They could be handled as special cases, but it does not really solve the problem if an invalid argument is passed for example.

Ok I see, and with nvim supporting so many options, you don't want to take on validation in your code, surely.

But doesn't that "just" mean we need to improve the spawn function, maybe return an error if something went wrong? That would leave the --help issue open, but would feel much more correct. What do you think? Doesn't mean you need to do it, of course :)

But then we'd still need the handshake thingy to make sure our nvim is responding, which makes some kind of general sense indeed. As does swallowing up garbage from noninteractive shells and the like.

Which in a kinda longwinded way means I think this would be a welcome addition, so I'll look at the code, leave some inline comments, and then ponder changing the spawning functions in a way that let's the caller check if the nvim process spawned properly.

[fredizzimo]

I just finished up the Neovide side of it here neovide/neovide#3045

And yes, we report the errors, maybe not in the nicest way. But both the stdout and stderr is reported. Neovim actually uses stderr for most of it, which nvim-rs does not deal with, so I think it's out of scope here.

[KillTheMule]

Ok, sorry for taking some time ;) I left some comments, but overall this is a very nice PR, thanks.

I understand GHA does not support WSL really, but we have a macOS runner, so shouldn't it be kind of easy to "just" do what neovide does to trigger the extra command outputs? Certainly not a blocker, but it does feel kind of odd not to test at least one of the real major use cases.

As an aside, would you find it beneficial for neovide if I added sterr handling? It missing is "just" an oversight, I don't see why I shouldn't support it :)

[fredizzimo]

I addressed your review comments.

Unfortunately, it's not easy to fully realistically replicate the situation during the testing, since it depends on the user shell configuration. I think the tests that I added are enough, and I have done some real-world realistic testing through Neovide and WSL.

For the stderr handling, I'm not sure how it would fit. In Neovide it's done here https://github.com/neovide/neovide/pull/3045/files in session.rs

[fredizzimo]

I'm not sure why the Ubuntu Beta CI tests failed. I will investigate more tomorrow. And probably improve the assertion message as well.

Edit: I have not been able reproduce it locally without modifications, but most likely the test is flawed, and it fails already when sending the message.

[fredizzimo]

I fixed the unsuccessful_handshake_with_wrong_output tests, and added better failure reporting.

So, I think everything should be OK now.

[KillTheMule]

Thanks a bunch!

[fredizzimo]

@KillTheMule, do you think you can create a release at some point? We would like to release a new Neovide version quite soon.