Skip to content
This repository was archived by the owner on Apr 1, 2020. It is now read-only.
This repository was archived by the owner on Apr 1, 2020. It is now read-only.

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

Open
onivim/oni-types
#4
@GeorgeTG

Description

@GeorgeTG
Contributor

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

changed the title [-]Two NeovimEditor instances, two nvim processes results in events being missed[/-] [+]Two NeovimInstance instances, two nvim processes results in events being missed[/+] on Jun 15, 2018
changed the title [-]Two NeovimInstance instances, two nvim processes results in events being missed[/-] [+]Two NeovimInstance instances, two nvim processes, events being missed[/+] on Jun 15, 2018
CrossR

CrossR commented on Jun 15, 2018

@CrossR
Member

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

GeorgeTG commented on Jun 16, 2018

@GeorgeTG
ContributorAuthor

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:
EventEmitters
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.

changed the title [-]Two NeovimInstance instances, two nvim processes, events being missed[/-] [+]Multiple instances of Event class for the same event cause events being missed[/+] on Jun 16, 2018
GeorgeTG

GeorgeTG commented on Jun 19, 2018

@GeorgeTG
ContributorAuthor

@CrossR did you check into this by any chance?

CrossR

CrossR commented on Jun 19, 2018

@CrossR
Member

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

akinsho commented on Jun 24, 2018

@akinsho
Member

@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

GeorgeTG commented on Jun 24, 2018

@GeorgeTG
ContributorAuthor

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

GeorgeTG commented on Jun 25, 2018

@GeorgeTG
ContributorAuthor

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

akinsho commented on Jun 25, 2018

@akinsho
Member

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

linked a pull request that will close this issue on Jun 25, 2018
GeorgeTG

GeorgeTG commented on Jun 25, 2018

@GeorgeTG
ContributorAuthor

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

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Participants

      @GeorgeTG@CrossR@bryphe@akinsho

      Issue actions

        Multiple instances of Event class for the same event cause events being missed · Issue #2323 · onivim/oni