Multiple instances of Event class for the same event cause events being missed #2323
Description
Oni Version: 0.3.4 and 0.3.5 from latest git
Neovim Version (Linux only): 0.3.0
Operating System: Arch-Linux
Describe your issue
When navigating with j,k a lot of errors are thrown in the console window because editorManager.activeEditor.activeBuffer.
is null.
This happens because NeovimEditor's lastBufferId is null which in turn happens because
onBufEnter event is seemingly never called.
Now that is the part things get complicated.
Event handling happens through oni-types Event class.
So the oni-types Event class instantiates a new EventEmitter for each instance of the Event class.
Because of this for subscribe and dispatch to work, the same instance of the Event class must be used by the subscriber(s) and the dispatcher(s) basically meaning that all events must be singletons, although no measures are taken to enforce this.
(This behaviour also eliminates the need for the name parameter, since it is ignored and not used anyway.)
Of course if the name argument was provided for all events a single EventEmitter would be enough and the whole problem would be "patched", but the core of the problem is not the Event system by itself.
In the current state of the code, some events have two instances. One instance is dispatching, while the Editor is subscribed to the other. As of now I'm pretty sure that this happens with atleast all NeovimAutoCommands/Editor related Events.
A NeovimAutoCommands instance is created only in NeovimInstance.
Now things get even more complicated and confusing.
Two NeovimInstance instances are created that result in two NeovimProcessSpawner::startNeovim calls which basically runs nvim two times and I'm not sure if this is intended. Again it looks like NeovimInstance should be a singleton, but it isn't.
The first NeovimInstance is created in NeovimEditor which is a SuperClass of OniEditor and is instantiated in startEditors.
The second one is created in SharedNeovimInstance which is a NeovimInstance wrapper.
Now both startEditors and SharedNeovimInstance reside in App.ts so I guess the intended behaviour would be to pass the SharedNeovimInstance's NeovimInstance to startEditors or something similar.
I would make a PR but I'm not sure how to proceed since the intended behaviour is not that clear with this.
Expected behaviour
Events handled and program working smoothly.
Actual behaviour
Hundreds of errors spamming the console and at least all NeovimAutoCommand events are missed.
Steps to reproduce
Open any file, navigate with j,k in visual mode, with developer tools open.
Activity
[-]Two NeovimEditor instances, two nvim processes results in events being missed[/-][+]Two NeovimInstance instances, two nvim processes results in events being missed[/+][-]Two NeovimInstance instances, two nvim processes results in events being missed[/-][+]Two NeovimInstance instances, two nvim processes, events being missed[/+]CrossR commentedon Jun 15, 2018
I'm away at the mo (and really I'm not the best person to answer this anyways), but one thing I thought I should mention in case you don't know is that Oni does spawn multiple instances of Neovim.
That is, we have the main instance that we use for the editor, but there is also a second instance that is shared by all the UI. The explorer and more use it in their back end so that movements etc all work as expected and more. I think that at least explains the differene between the Neovim Editor (main editor instance) and the SharedNeovimInstance (shared across the UI). There is the possibility with the upcoming PRs on Neovim core for multiple windows that we'd be able to remove that, but currently its not the case, and potentially still not wanted (ie having a clean neovim instance is nice since we can not load any config etc, so plugins can't break the UI).
This potentially gets more complicated when you consider the "Oni split mode" shown over in #1682. That is, having multiple instances of the Neovim editor class, which may help explain a bit of the other parts of the design.
I'm also unable to reproduce any errors in the console, though I'm not running Nvim 0.3.0 on my laptop and am on Windows, so when I'm home I can test both on my Arch box for a bit of a better representation.
GeorgeTG commentedon Jun 16, 2018
Well since multiple instances of neovim are ok, the event system is at fault then.
Quick way to demonstrate the situation.
Modify the index.js (or tsx) from
oni-types
and include some logging in the Event class like so:Subscribe:
console.log(["Subscribe", this._name, callback, this._eventObject]);
Dispatch:
console.log(["Dispatch", this._name, val, this._eventObject]);
Constructor:
console.log(" In constructor for event: " + name);
Then into
NeovimAutoCommands
actually pass the event name to one event constructor like so:private _onBufEnterEvent = new Event<BufferEventContext>("onBufEnterEvent")
To be able to filter more easily.
On my machine I get this:

As you can see there are two different instances of the
Event
class so they have differentEventEmitter
instances and all dispatch calls are lost.This of course causes #2250 and probably more parts of the application to break.
A fix for this would be to pass the event name on all Event constructors properly and change Event to actually cache the EventEmitter instances with the event name as key. I've tried it on my machine and events are handled correctly, also no more error messages.
I can have a PR for this tomorrow if it's acceptable.
[-]Two NeovimInstance instances, two nvim processes, events being missed[/-][+]Multiple instances of Event class for the same event cause events being missed[/+]GeorgeTG commentedon Jun 19, 2018
@CrossR did you check into this by any chance?
CrossR commentedon Jun 19, 2018
Oops nope, I've just got back home today, so could possibly have a look later today.
Its probably also worth seeing if @bryphe has any insights since he wrote these parts I think, and is more aware of the ongoing architecture to support multiple Nvim instances and such.
akinsho commentedon Jun 24, 2018
@GeorgeTG thanks for raising this seems like an issue with the
Event
utility class as you point out,I think we would love to have a PR (only if your up for it no pressure of course if really great to have your investigation as its a really good jumping off point for anyone to tackle this), the
Event
class is here although as you suggest re making sure all Event constructors are passed an event name this though would involve adding names at all the event construction locations in Oni I dont believe there are too many but a PR could cover as much as your comfortable with/ feel are crucial and we can look out for the rest.GeorgeTG commentedon Jun 24, 2018
Well if all constructors get an event name, then
oni-types
can be changed to cache theEventEmitter
instance based on the event name, and that will be a fix. I am gonna make a PR for the constructors first.GeorgeTG commentedon Jun 25, 2018
By the way @akin909 I'm wondering why does a 44 line file needs its own repo. Doesn't that make development a bit more complicated?
akinsho commentedon Jun 25, 2018
@GeorgeTG I believe the long term idea behind it is to keep oni-core lean and modular and separate out different bits to be worked on in isolation to prevent core from becoming a monolith (@bryphe would be a better person to answer questions about the architectural path for oni, cheekily @'ing him although his is quite busy so might not be able to respond here, hopefully I've done it justice)
GeorgeTG commentedon Jun 25, 2018
Okay I've changed all constructors to pass EventNames #2355 .
This PR can be merged by itself without impacting anything.
If combined with the
oni-types
PR it should fix the errors and events will be handled properly.5 remaining items