Multiple instances of Event class for the same event cause events being missed

[GeorgeTG]

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.

[CrossR]

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]

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 different EventEmitter 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.

[GeorgeTG]

@CrossR did you check into this by any chance?

[CrossR]

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]

@GeorgeTG thanks for raising this seems like an issue with the Event utility class as you point out,

I can have a PR for this tomorrow if it's acceptable.

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]

Well if all constructors get an event name, then oni-types can be changed to cache the EventEmitter instance based on the event name, and that will be a fix. I am gonna make a PR for the constructors first.

[GeorgeTG]

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]

@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]

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.

[bryphe]

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.

Do you have a callstack for this, or a custom configuration that uses editorManager.activeEditor.activeBuffer? I don't see these errors in my setup. I'm wondering if there is some customization that's exposing this?

If you could share your config.tsx that would be helpful!

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.

The events should not be singletons. We support having multiple editor instances, and as such, each editor instance should have its own buffer enter event. The activeEditor can change, especially if you have editor.split.mode set to Oni - so the best practice is to actually hook up events to editorManager.anyEditor - that proxies the buffer enter event so it is always listening to the buffer enter of the current editor.

Here's an example - I have editor.split.mode set to oni in my config, so when I press <c-w><c-v>, it actually creates a vertical split that is a second editor instance.

I then hook up separate event handlers:
Oni.editors.allEditors[0].onBufferEnter.subscribe(() => console.log("Buffer 0 enter"))
Oni.editors.allEditors[1].onBufferEnter.subscribe(() => console.log("Buffer 1 enter"))

This allows me to specifically work with a particular editor - it's useful if I need to add ephemeral hooks.

I didn't show this in the gif - but if I just want to subscribe to buffer enter events across the board, I could use:
Oni.editors.anyEditor.onBufferEnter.subscribe(() => console.log("any buffer entered"))

Hope that helps explain the behavior a bit.

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?

I use this code in other projects. I'd be open to bringing it here as long as we can still publish it as a separate npm package.

[GeorgeTG]

I just clone oni and build it, I have no config.tsx so I guess the defaults are loaded.
This is on my laptop, since I'm away from home.
Imgur

[bryphe]

Thanks @GeorgeTG for the screen capture! That's very helpful.

Looks like it's crashing here

oni/browser/src/Services/Browser/index.tsx

Line 177 in 7c09bcf

return (buffer as IBuffer).getLayerById<BrowserLayer>("oni.browser")

:

    const getLayerForBuffer = (buffer: Oni.Buffer): BrowserLayer => {
        return (buffer as IBuffer).getLayerById<BrowserLayer>("oni.browser")
    }

Which is called by:

oni/browser/src/Services/Browser/index.tsx

Line 194 in 7c09bcf

const layer = getLayerForBuffer(editorManager.activeEditor.activeBuffer)

It seems like the activeBuffer is null in this case - it's not clear to me how that is null. It does seem like there is something going wrong with the event flow in this case. Here's what should be happening:

1. A BufEnter autocmd is triggered in Neovim. This happens in our oni-core-interop's init.vim:

   oni/vim/core/oni-core-interop/plugin/init.vim

   Line 126 in c5d42ee

   autocmd! BufEnter * :call OniNotifyWithBuffers("BufEnter")

2. We pick up the BufEnter on the JS side here in NeovimInstance:

   oni/browser/src/neovim/NeovimInstance.ts

   Line 954 in 7c09bcf

   this._autoCommands.notifyAutocommand(eventName, eventContext)

   And at that point, we dispatch the onBufEnter event for that specific editor
3. The NeovimEditor listens to onBufEnter from the NeovimInstance it owns - this populates the activeBuffer property. This is handled here:

   oni/browser/src/Editor/NeovimEditor/NeovimEditor.tsx

   Line 472 in 7c09bcf

   this._onBufEnter(evt),

So something is busted along that pipeline - not sure where it is breaking down. It would be useful to know where it gets to, by putting breakpoints in.

We can also make calls to the NeovimInstance directly - I'd be curious to see what the result of this command is:

Oni.editors.activeEditor._neovimEditor._neovimInstance.getBufferIds().then((ids) => console.dir(ids))

I'm wondering if somehow activeEditor is pointing to something different?

[GeorgeTG]

I went over this in the original post. Tldr: The problem is that there are two instances of the events. Subscribe is called in one and events are emitted from the other. Check the post for details.

[bryphe]

The problem is that there are two instances of the events.

Two instances of events are expected - there are two NeovimInstances we spawn - once backing the NeovimEditor and one backing the SharedNeovimInstance - each one has its own instance of the buffer enter event. This means we'd expect there to be two logs to the constructor.

There is definitely something going wrong here in the pipeline that I outlined above - but having two event instances is not the root cause. There's something specifically going wrong with the event handling from nvim -> NeovimInstance -> NeovimEditor.

Note that - we don't see this get hit in any of our CiTests, and I can't repro it locally on Mac / Windows - so I believe there is at least something specific to your configuration that's triggering it. It would be a helpful / interesting data point if anyone else is hitting this, though!

Keep in mind, if this actually was a general issue with the event system, we'd see it reproduce in our automation (based on your gif... it's unfortunately really easy to repro on your machine!)