Oni: Is neovimInitialised check fails causing explorer to hang on first launch

[akinsho]

Following a change its unclear which possibly #1800 we are once again experiencing the bug where methods are being called on the neovim instance before it is initialised. Specifically this line

I believe the isNeovimInitialised check does not work correctly as I have seen this fail to guard against functionality being called on an undefined neovim instance. It was intially resolved by causing shellEnv to block initially on oni starting but something else seems to possibly have regressed this.

[akinsho]

@bryphe Incredibly unusually I've tried logging to see how this error is occuring and here are the logs

I've logged the neovim instance just before the function was called and it seems to exist yet for some very unclear reason (probably the log is async) the function that call eval seems to get null for the neovim instance, does anything re what could possibly be happening here come to mind? this seems to occur on trying to move across the side bar from explorer to sidebar items and back

[bryphe]

Thanks for starting to look into this!

I think there may be a _race_ between the setItems call and the release method on Binding. In particular, if release is called while the setItems promise is in progress, it'll cause a crash because _neovimInstance gets set to null.

In other words - its possible that release is getting called _between_ those neovimInstance calls, and in that case, it's like the rug gets pulled out from underneath the function - now, neovimInstance is null.

It might be helpful to add logging in the release method and the beginning / end of the inner function in setItems, so we could confirm that hypothesis.

From there, I think the best bet is to make release a Promise, such that it 'drains' the remaining work. We may also need to make the bindToMenu method a promise (so that it could release any existing binding). It's hard to reason about the function if the state can change underneath it, so I think it's better to refactor those methods to wait for any pending jobs to go through.

[akinsho]

Ah bit of logging seems to very possibly corroborate this its hard to pin down whats going on here, but this seems very likely - although 🙇 i'm not sure I follow re the promises for release and bind to menu?

[bryphe]

i'm not sure I follow re the promises for release and bind to menu?

Well, you could imagine two ways to fix this. One would be to check _every single time_ between the await if NeovimInstance is null or not. That could work, but it would quickly become untenable and difficult to maintain!

If we don't do that - then the only other approach is to _enforce_ that nothing can change the state of NeovimInstance during the course of that promise. The bindings get created with the bindToMenu call, and released with the release call (for example, if I'm navigating through the explorer -> sidebar icons, first, the explorer will call bindToMenu, and then release when I leave the explorer window, then the sidebar pane will call bindToMenu.). The problem is that these state transitions can occur while that setItems call is still in progress. We could, instead, queue up requests, such that we wait for the existing 'bound menu' to finish what its doing before handing it over. If we did that, that would necessitate changing the interface for release and bindToMenu, because they could no longer be synchronous - we might need to wait for the current operation to complete.

Hope that helps!

[akinsho]

@bryphe thanks that does help a lot, i've been working on a solution which seems promising although there seems to have been an actual regression in navigating the file explorer I wanted to confirm if you're experiencing this as well I've run a build straight off master I'm trying to identify where it happened but wanted to make sure it isn't related to something I've done locally but the problem still occurs running the master build with my config and init.vim turned off.

By can't navigate, I mean most of my input is ignored whilst trying to navigate the file explorer