DRSchlaubi commented 5 days ago

Summary

Currently the plugin system is fairly basic, which makes it easy to to fall into some traps

Motivation

Currently the plugin classpath is unpredictable, which causes issues if plugins depend on each other and/or use the same dependencies.

For example lavasrc bundles certain files of lyrics.kt. Due to the implementation of the plugin loader every plugin shares the class path of the plugin which was loaded before it

Since the order of loading is semi-random running an identical configuration can result in different outcomes in different environments therefore on one server LavaSrc provides the class whilst on another server lyrics.kt does. Since both plugins might not be in sync this can cause runtime errors about different class versions.

This is also an issue for other dependencies like the Kotlin stdlib

Also the Gradle Plugin currently erases the META-INF folder from all of the plugins dependencies, which causes service discovery to fail, if the plugin depends on a library requiring it (like Ktor)

Goals

Isolate the classloaders for plugin files from their dependencies and other plugins
Provide legacy support for loading existing plugins
Provide the ability to update a plugin to the new format with minimal change

Non-goals

Change the plugin publishing mechanism in any way
Introduce a mechanism for downloading plugin dependencies automatically

Class loader isolation

Instead of having one class loader depending on all the others, each plugin should have one class loader for its own classes and one for libraries the plugin might depend on. This way a plugin will always lookup classes from its own class loader first, before asking other plugins classloaders or the top-level app class loader, so if two plugins share the same dependency in different versions, updating either of the plugins won't break the other

plugin.dependencies

When plugin class-loaders are isolated, this also means that APIs provided by other plugins, like the LavaSearch API, will no longer be accessible, therefore the plugin loader should parse the manifest for plugin dependencies and then calculate a hierarchy in which plugins should be loaded to 1) load a plugin's dependencies first 2) Provide those plugin classes as a classloader

A possible format for dependencies would be:

<pluginName>@<version>

By putting a ? after the plugin name, you can specify a dependency as optional, which means, that if the dependency is installed it will be loaded first, however if not the loader will not reject loading the plugin

The version can either be omitted, a version number or a semver range, this way we can provide support for different API versions in Plugins

New publishing format

In order for Class loader isolation to be possible, plugins need to differentiate between class files and dependencies, this would be possible by using the following structure

plugin.zip
  classes/ -> plugin classes
  lib/
    dep1.jar
    dep2.jar
lavalink-plugins/plugin-name.properties

In order to provide support for older plugins parsing the old jar format should still be supported, in this case dependencies will simply be in the same class loader as the plugins’ files. The new format can be introduced using the Gradle plugin so plugin authors simply need to recompile their plugin to switch to the new format

Extensions

Another problem is that currently plugins provide extension points without a real mechanism to do so. This causes the problem that plugins need to shade the dependency classes. As an example LavaSrc includes the classes of both LavaLyrics and LavaSearch to prevent classnotfound errors if those plugins are not installed.

An extension system would introduce a new @Extension annotation, that needs to be put on each spring bean, this allows said annotation to have a dependency argument, which allows the plugin loader to skip it when the dependency is not installed

PF4j

Most of the suggestions is already implemented in the very customizable pf4j library, which would allow us to focus on regular lavalink features instead of maintaining a complex plugin system

freyacodes commented 5 days ago

This is a very good presentation of the problem. I had not considered dependency issues like this when I designed the system.

I wonder how tricky it would be to integrate this in a reverse-compatible way with both systems working in parallel, and whether we might want to use pf4j-spring for that.

DRSchlaubi commented 5 days ago

I am confident we can make Pf4j load the current plugins, I can try implementing a prototype

I would not use Pf4j spring since that would require plugins to have a main class, which is a larger change for existing plugins

Also pf4j spring is really basic and offers almost not advantage over regular pf4j

DRSchlaubi commented 4 days ago

After implementing a proto type I found a new issue regarding the loading of plugin dependencies.

Whilst there is no official mechanism for doing that, they exist, therefore plugins like LavaSrc shade LavaLyrics and LavaSearch

An extension system which is in the update proposal would solve this issue

You can check the current implementation here: #1111

DRSchlaubi commented 2 days ago

After further investigation it might be smarter to force every spring extension to be annotated with @Extension to avoid confusion and complexity in the processor

lavalink-devs / Lavalink

Improve the plugin system #1110