Lsp: plugin_loaded initializes LSP-* plugins before they have a chance to run plugin_loaded

[rchl]

Using LSP and LSP-eslint.

When plugin_loaded function in LSP is called, it initializes all instances of LanguageHandler (so one added by LSP-eslint for example). The problem is that this runs before LSP-eslint's plugin_loaded has a chance to run (the order which one runs first is probably undefined but in my case LSP's runs first).

I want to initialize some paths from plugin_loaded (sublime.cache_path() that is only valid when called after plugin_loaded runs) and so current behavior causes issues.

I was wondering if it would be possible for LSP to trigger a short timeout (for the purpose of running from "next tick" or "next message loop cycle") from plugin_loaded and only start initializing after that. It would potentially cause no harm and make sure that all other plugin_loaded handlers fully executed (at least that the synchronous code ran there).

[rwols]

Perhaps we can lazily instantiate these handlers when they are needed (so when starting a potential Session). This way, I hope we can do without a “next tick” workaround. In fact, I don’t think there is any guarantee that an LSP-* will load in the next cycle of the sublime message loop. Maybe we can take a peek at how SublimeLinter handles plugins?

[rchl]

I don’t think there is any guarantee that an LSP-* will load in the next cycle of the sublime message loop

I don't know for sure of course, but I would imagine ST doing something like:

for plugin in plugins:
  plugin.plugin_loaded()

and that would give the guarantee that all plugin_loaded functions are called in the same message loop cycle.

[rchl]

The code is even "public" so could be checked manually but I don't have time / patience / knowledge to do it ;). But this is small part of the code:

    if len(module_plugins) > 0:
        m.__plugins__ = module_plugins

    if api_ready:
        if "plugin_loaded" in m.__dict__:
            try:
                m.plugin_loaded()
            except:
                traceback.print_exc()

(whether m.__plugins__ contains all plugins initially or there are other cases, I don't know. :))

[rchl]

As for initializing lazily, we probably need to get config right away to know for which files/syntaxes it should initialize. So I'm not sure we can do it more lazily than just from short timeout (which could probably also cause timing issues with config not being available on time).

[deathaxe]

A LSP-client plugin should propably register and unregister itself by a certain API call within the plugin_loaded() and plugin_unloaded() functions.

This way LSP could stop all running server instances and unregister its handlers once a client plugin is unloaded. Unloading may be triggered by plugin updates by package control or by disabling the client package.

In the case of LSP-lemminx client it is currently not possible to update the language-server binary via plugin update automatically if a server instance is already running. The binary is locked and can't be removed or replaced.

[rwols]

Yes, this is also my train of thought. How would a scheme look like that enables this? Basically the goal is to not dig into the __subclasses__ from within the base package.

[rchl]

There would need to be a global manager with public register/unregister methods, right?

In the case of LSP-lemminx client it is currently not possible to update the language-server binary via plugin update automatically if a server instance is already running. The binary is locked and can't be removed or replaced.

That's the case also for NodeJS-based servers on Windows.

[deathaxe]

Something like that, yes.

As we need to hook into ST's plugin loading/unloading process, the only solution on the client side was to call something within ST's hooks. I'd abstract a possible manager class via plain API functions.

Maybe something like that.

def plugin_loaded() -> None:
    LspXMLServer.setup()
    register_language_handler(LspXMLPlugin)


def plugin_unloaded() -> None:
    unregister_language_handler(LspXMLPlugin)
    LspXMLServer.cleanup()

I am a bit confused when it comes to server side implementation and handling all the startup/shutdown stuff. Means I can't yet/anymore follow all the object orientated vs. functional code structure of LSP.

Event handlers are registered to global dicts, while other things are pushed to some registry objects interleaved by functions creating objects calling functions creating objects ..... .

[rwols]

I’ve removed some of the (unneeded) complexity in my “rewrite transports” PR. It’s still a WIP though.

[rwols]

If we rename this package to "SublimeLSP", then it'll be loaded after all the LSP-foo packages have been loaded.

[rchl]

That should really not be necessary and even if it would work, there surely gonna be some outliers with a name of their own. It's brittle to rely on the name for loading order.

Why would it be necessary anyway to have certain loading order? We can always put stuff on queue if plugin is not ready yet and execute them later.

[rwols]

Can you think of another method to have plugin_loaded of LSP-x be called before plugin_loaded of LSP?

It's brittle to rely on the name for loading order.

That's the reality of Package Control and Sublime Text.

[rchl]

The reality is that there is a certain order to loading of packages but IMO nobody should rely on any specifics of it and code in a way that handles any order.

Can you think of another method to have plugin_loaded of LSP-x be called before plugin_loaded of LSP?

Please explain why would that be necessary.
Let's say that plugin doesn't verify server version (to make it more tricky for the sake of the example) and just immediately tries to register itself. It will be able to import a register_plugin function from LSP already at that point and call it. It would be the responsibility of that function to queue the "register" action if LSP hasn't initialized yet. If there is a problem with that then I'm just not aware what it is.

EDIT: And I'm talking mostly in context of new API so I guess it might seem a bit out of place when just reading this thread.

[rwols]

Please explain why would that be necessary.

We want server Foo enabled if and only if the ST package LSP-Foo is enabled (i.e. not in "disabled_packages" of Preferences.sublime-settings). So:

1) Don't register Foo when LSP-Foo is disabled at startup
2) Unregister Foo when LSP-Foo is disabled during runtime
3) Register Foo when it gets enabled again (via Package Control: Enable Package for instance)

We have the guarantee that:

1) plugin_loaded is called at startup, and when a package is enabled during runtime
2) plugin_unloaded is called when a package is disabled (or when it is reloaded)

Hence, we would like to say:

from LSP.plugin import register_plugin, unregister_plugin

def plugin_loaded():
    register_plugin(FooPlugin)

def plugin_unloaded():
    unregister_plugin(FooPlugin)

And, when LSP-Foo is loaded before LSP, this guarantees that Foo is enabled if and only if LSP-Foo is an enabled package.

Furthermore this allows LSP-Foo to do server binary setup and server binary teardown in exactly one place, instead of many awkward places:

from LSP.plugin import register_plugin, unregister_plugin

server = FooServer()

def plugin_loaded():
    server.setup()
    register_plugin(FooPlugin)

def plugin_unloaded():
    unregister_plugin(FooPlugin)
    server.teardown()

[rchl]

I agree that it's the way to go and I didn't argue with that.

I've only questioned why it would be necessary to rename any package to achieve that.

And, when LSP-Foo is loaded before LSP, this guarantees that Foo is enabled if and only if LSP-Foo is an enabled package.

And why mention disabled/enabled? The code of disabled packages is not even executed so I would expect that we are talking about enabled packages.

In any case, if we assume all are enabled, what would be the problem if LSP-Foo would be plugin_loaded before or after LSP?

[rwols]

I guess the deeper problem is that config handling is very static right now. They're collected once and that's that. I suppose when we make config handling dynamic we can let LSP load before LSP-foo packages.

So for instance, adding a new config via register_plugin should re-evaluate all views to check if any new sesions should be started, and unregister_plugin should also check which sessions should die.

[rwols]

Just wanted to remark that I haven't forgotten about this :)

[rwols]

This problem is already solved with AbstractPlugin.

1) Don't use plugin_loaded to install things
2) Use AbstractPlugin.update_or_install instead
3) Uninstall files in plugin_unloaded
4) Call LSP.plugin.register_plugin in plugin_loaded to ensure correct behavior when your package is enabled by the user
5) Call LSP.plugin.unregister_plugin in plugin_unloaded to ensure correct behavior when your package is disabled by the user

Example skeleton:

from LSP.plugin import register_plugin
from LSP.plugin import unregister_plugin
from LSP.plugin import AbstractPlugin
import shutil
import subprocess


class MyServer(AbstractPlugin):

    @classmethod
    def name(cls) -> str:
        return "my-server"

    @classmethod
    def needs_update_or_installation(cls) -> bool:
        return True

    @classmethod
    def install_or_update(cls) -> None:
        subprocess.check_call(["npm", "install" "--prefix", "asdf"])

    @classmethod
    def cleanup(cls) -> None:
        shutil.rmtree("asdf")


def plugin_loaded() -> None:
    register_plugin(MyServer)


def plugin_unloaded() -> None:
    unregister_plugin(MyServer)
    MyServer.cleanup()

[rchl]

lsp_utils doesn't call register_plugin but it still works. What's the catch there?

[rwols]

The catch is that LSP has to find all final subclasses in any case (which it does now in plugin_loaded). Because LSP itself may be disabled, and then later enabled. In which case LSP has to find all plugins again.

Not calling register_plugin means that the following scenario doesn't work:
1) user disables LSP-foo package
2) user enables LSP-foo packages <-- this does nothing now, but with register_plugin it is supposed to work

[rchl]

If LSP still forcefully registers plugins from its own plugin_loaded then I don't see how this fixes the original issue. The plugin's plugin_loaded will still run after LSP has triggered plugin's initialization.

BTW. In your example the def unregister_plugin() should be called def plugin_unloaded().

[rwols]

If LSP still forcefully registers plugins from its own plugin_loaded then I don't see how this fixes the original issue. The plugin's plugin_loaded will still run after LSP has triggered plugin's initialization.

The point is: don't use plugin_loaded for anything but register_plugin. There are no load order guarantees.

BTW. In your example the def unregister_plugin() should be called def plugin_unloaded().

Thanks, fixed.

[rchl]

OK, sounds good.

[rwols]

I'm trying this out on an LSP-* package but register_plugin doesn't quite yet do what it is supposed to do :) but I'll probably have a PR for that soon.

[rchl]

One part of this that was mentioned but is still not ideal is that the plugin gets asked for configuration immediately from plugin registration. That means that we need to provide the command path to run right away and that might require expensive IO and blocking subprocess calls (checking npm version for example).

This could be solved by only requiring language configuration initially as that is needed to know when to start the server. The rest of the options could probably be requested in a separate step when starting the plugin.

[rwols]

This could be solved by only requiring language configuration initially as that is needed to know when to start the server. The rest of the options could probably be requested in a separate step when starting the plugin.

You can use can_start for that. See e.g. LSP-flow or LSP-Dart.