Serialized event handling

[Lyude]

This is in response to the comments on #25

Continuation of the work for serialized events. This also demonstrates how I was able to just combine the two futures we needed to run in parallel into one to make the interface simpler. Note that I've only tested this with my neovim-gtk fork so far, I'm going to try to make sure the nested requests example is still working hopefully this weekend and finish cleaning the rest of this up and implementing proper error handling.

Let me know what you think.

[KillTheMule]

So try_join seems to be able to reduce the futures the user needs to run from 2 to 1, good job! I've left two comments.

So, this is basically my branch serial's last commit, apart from the try_join, isn't it? Maybe you could reinstate this test (with single threaded runtimes, can we achive that with async-std as well?), just to make sure we're not missing anything.

[KillTheMule]

Oh, and we need to think about point 2 of my earliest PR and see if that problem is solved by this design. Maybe we can find a test.

[Lyude]

So try_join seems to be able to reduce the futures the user needs to run from 2 to 1, good job! I've left two comments.

So, this is basically my branch serial's last commit, apart from the try_join, isn't it?
Maybe you could reinstate this test (with single threaded runtimes, can we achive that with async-std as well?), just to make sure we're not missing anything.

Yeah - I've been cleaning it up a bit more locally: removing leftover comments, being a bit more careful with how we handle errors on the channels, etc. I was actually hoping to write some tests based off the examples as well though, but it looks like you've got some tests already though so I'd be happy to take a look at reinstating them to hopefully save myself some effort.
Another test I'm considering writing in addition to yours is one to actually start up a neovim Child, connect to it, then also connect to it with a second Neovim<W> instance that uses unix sockets and try interrupting sending/receiving on the second instance to make sure that we're propagating errors from the two joined futures correctly. More specifically: making sure that if one future ends pre-maturely because of a non-reader error and that results in the second future ending because of a reader error (which wouldn't be terribly useful to the user, since the reader error was just a result of the actual error from the first future that needs to be handled) that we forward the non-reader error to the user.

[Lyude]

Another test I'm considering writing in addition to yours is one to actually start up a neovim Child, connect to it, then also connect to it with a second Neovim<W> instance that uses unix sockets and try interrupting sending/receiving on the second instance to make sure that we're propagating errors from the two joined futures correctly. More specifically: making sure that if one future ends pre-maturely because of a non-reader error and that results in the second future ending because of a reader error (which wouldn't be terribly useful to the user, since the reader error was just a result of the actual error from the first future that needs to be handled) that we forward the non-reader error to the user.

Actually - I'm being silly here and forgetting that only one future is handling I/O with nvim and the other is just handling the channel we created to pass along incoming events. So this would probably be a bit overkill

[Lyude]

Actually - I'm being silly here and forgetting that only one future is handling I/O with nvim and the other is just handling the channel we created to pass along incoming events. So this would probably be a bit overkill

…and ignore this, whoops, because I realized we're still sending responses from handling requests to nvim in handle_rec() so we do still need to make sure errors get propogated correctly from both

[Lyude]

(next up on my TODO: see if we should still get the test you mentioned in KillTheMule/neovim-lib@62108d0 working)

[Lyude]

So try_join seems to be able to reduce the futures the user needs to run from 2 to 1, good job! I've left two comments.

So, this is basically my branch serial's last commit, apart from the try_join, isn't it? Maybe you could reinstate this test (with single threaded runtimes, can we achive that with async-std as well?), just to make sure we're not missing anything.

So I looked into trying to get this test working again, but I honestly couldn't figure out what this test would accomplish that we aren't already doing with other tests (other than also just testing ChildStdin, which seems like it might be better for a separate MR). Am I missing something here?

Also - I think the rest of this branch is definitely ready for review

[KillTheMule]

Alright, awesome! I'll review, but I've also followed along and think this will fit nicely :) Please note I've just had surgery on my right hand, so everything will be pretty slow for some time. Especially, since there are 2 things I'll want to do before merging that (add a specific test that should have existed all along, and try out this PR with neovide), but I'm not sure when I'll have recovered enough for that.

Am I missing something here?

Ah yeah, very sorry, that was a mistake on my part. There are 2 scenarios which would hang using neovim_lib, which prompted the creation of nvim-rs. I want a test for the second one, too, but I linked the one for the first scenario, which is indeed very much covered by nested_requests. I'll write one for the first myself when I can, but I also really don't see how this should be going wrong.

[KillTheMule]

Ok, I've made some comments, there basically nothing to do, although I've just "read" the code and could not yet try things locally. Hope I can get to that tomorrow :)

[Lyude]

Seems like I've gone through all of the review comments, so this should basically be ready for merge :)

[Kethku]

I'd like to test this in neovide. If I've been following this thread correctly, I ran into similar issues with an early version of neovide and have since been using a fork with a dumb serialization hack. I'm hopeful that this PR will also fix my issues

[KillTheMule]

Seems like I've gone through all of the review comments, so this should basically be ready for merge :)

Very awesome, thanks a lot! Unfortunately, due to my current circumstances (needed to be hospitalized again :( ) I won't be able to do some laste testing and playing around too soon, so I hope you're ok with me delaying the merge a bit. I sincerely hope I can get it done within the next week!

I tried to run neovide with it, but there were tokio incompatibilities, and I can't deal with stuff efficiently here, so I did not investigate further. Will try to do that later, too, though :)

[Lyude]

Very awesome, thanks a lot! Unfortunately, due to my current circumstances (needed to be hospitalized again :( ) I won't be able to do some laste testing and playing around too soon, so I hope you're ok with me delaying the merge a bit. I sincerely hope I can get it done within the next week!

I tried to run neovide with it, but there were tokio incompatibilities, and I can't deal with stuff efficiently here, so I did not investigate further. Will try to do that later, too, though :)

Oh no! I'm sorry to hear that. Health comes first, so I'm totally fine with waiting a bit. If there's anything I could help out with let me know

[KillTheMule]

Ok, I've written the test I've wanted and it works :) I couldn't push to your branch, so I'll just merge this afterwards. I hope tonight I can look at the neovide situation, and then I'll merge this.

[Kethku]

Don't block on neovide. I can test that after

[KillTheMule]

Well, I really want to see if it solves the problem neovide had with non-serialized messages. But locally, I can't reproduce said problem with current master, even when putting my system under heavy load (upgrading to tokio 1.1.1 was very easy). But neovide is very sluggish here anyways, so maybe it's sort of the graphics slowing it down so much that the notification pipeline appears serialized in effect? Could you try to reproduce that with current master? I can send you a PR for upgrading tokio.

[Kethku]

I will try to test this shortly

[Kethku]

I just tested both master and the serial fork and found issues in neither. That said, that sorta makes sense as I did some work recently to add a layer of serialization to neovim events on our side as well. This in combination with a refactoring a number of months ago which moves all of the processing of editor and renderer bits to their own threads means there is very little work happening in the actual event processing handler.

So I have closed the pr updating tokio on our side and created a new one which targets master nvim-rs. I will merge that pr once this PR has merged.

TLDR looks A-OK to me. Awesome stuff, very excited

[KillTheMule]

Awesome, thanks for checking! I'll make a new crates.io release soon.

Thanks for all the work and your peseverance, @Lyude !