Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Late composer installation for plugin libraries #2811

Open
SOF3 opened this Issue Mar 12, 2019 · 15 comments

Comments

Projects
None yet
6 participants
@SOF3
Copy link
Member

SOF3 commented Mar 12, 2019

Motivation

This issue serves as a milder alternative to #2755, which radically changed plugin installation methods.

Compared to #2755, this issue does not aim to resolve issues with NIH class loading and built-in plugin manager (#2504). The sole purpose is to allow automatic installation of library dependencies (not plugin dependencies) to prevent problems of (unwarned) conflicting library versions while making it easier to use libraries in plugins.

Mechanism

This proposal introduces a new optional file composer.json (or composer.lock, depending on the scenario) in the standard plugin structure. The only attribute used in the file would be the require atrribute. (All other attributes would be ignored) The server shall merge all the required packages into a (cachable) temp folder known as ad-hoc vendor and install composer dependencies inside.

The responsibility of the server includes the merging of such dependencies, and the selection/detection of incompatibility of shared libraries.

This shall enable single-instance libraries, which currently requires a plugin format known as "API plugins".

Concerns

Conflict with PMMP dependencies

There are two options:

  1. PMMP also loads dependencies via this system. This implies that PMMP must not use any composer packages without loading all plugins first.
  2. Append the PMMP dependencies into the ad-hoc vendor such that plugins cannot require these dependencies indirectly. This results in double loading of the same libraries. I am not sure if composer API and PHP autoloading API allow us to overcome potential problems here.

Plugins must be loaded on startup

Do we really need to allow loading plugins after the server starts? I even suggest removing the /reload command. Is it so hard to restart the server?

Packaged performance

To emulate the performance boost currently offered by packaging plugins in phar files, it is possible to lazily package the ad-hoc vendor directory into a phar too.

Editing plugins

Plugins can still be edited, but libraries cannot be edited without the danger of being overwritten. Nonetheless, it is not our responsibility to ensure that plugins (much less only libraries) can be edited. Observing that editing libraries is not prevalent right now, and it is usually unnecessary to edit libraries (because they are usually better-coded than plugins and do not have direct impact on users), this concern is dismissed. Users may still fork the library and produce their own version if they are professional enough; but they won't notice they want to edit a library otherwise anyway.

Libraries must be open-source

This is not true. Composer supports local/VCS-based library requirement.

Arbitrary updates

Plugin approval systems like Poggit will run into problems, because libraries are installed from a source that only its author has access to change.

It is possible for such approval platforms to modify the composer.json such that the library version is locked at a specific version, but this would result in problems when merging multiple plugins using the same library.

Another issue is https://www.google.com/amp/s/www.theregister.co.uk/AMP/2016/03/23/npm_left_pad_chaos/ But let's go the dangerous way of thinking: since npm is doing this for years, it should be ok.

Existing deployment workflow

This is a pure feature addition. Existing workflows do not have to be broken.

Debugging libraries

Consider adding extra include paths as a config in pocketmine.yml or alike.

@nathfreder

This comment has been minimized.

Copy link

nathfreder commented Mar 12, 2019

@SOF3 This sounds much more reasonable than #2755

@lukeeey

This comment has been minimized.

Copy link
Contributor

lukeeey commented Mar 20, 2019

Removing /reload is not useful in testing scenarios. All i have to do to change chat format or adjust some coordinates for something is edit the code then reload and i can test instantly, whereas a full restart means i must leave the game and rejoin.

@dktapps

This comment has been minimized.

Copy link
Member

dktapps commented Mar 20, 2019

@lukeeey

This comment has been minimized.

Copy link
Contributor

lukeeey commented Mar 20, 2019

@dktapps when using DevTools folder plugin loader it seems to. I get the onEnable messages in the console and my code changes apply

@dktapps

This comment has been minimized.

Copy link
Member

dktapps commented Mar 20, 2019

it seems to

here's the problem.

It may appear that some of your code changes are indeed reloaded when you run /reload, but this is not what is actually happening.
What may happen is that some classes which were not loaded prior to the reload may get autoloaded, since class autoloading happens on-the-fly. You'll notice that your code will never be "reloaded" twice (unless you change the namespace or something of that nature).

@Anders233

This comment has been minimized.

Copy link

Anders233 commented Mar 20, 2019

Can I reload the plugin code without shutting down the server?
This problem has plagued me for a long time.

@dktapps

This comment has been minimized.

Copy link
Member

dktapps commented Mar 20, 2019

You can't. Please stop taking the thread off topic.

@TheClimbing

This comment has been minimized.

Copy link

TheClimbing commented Mar 20, 2019

Why do I think it should be possible for DevTools to reload a folder plugin entirely?

@pmmp pmmp locked as off topic and limited conversation to collaborators Mar 20, 2019

@SOF3

This comment has been minimized.

Copy link
Member Author

SOF3 commented Mar 20, 2019

No, not at all, otherwise it might cause other problems such as problems with statics that you might not 100% be aware of. Live code swap is always dangerous, and even software much more professional than PocketMine like Android is still having trouble implementing proper code swap that does not lead to crashes that otherwise wouldn't happen.
Server startup is usually fast enough. If it's slow for you, make sure you're using a phar distribution of PocketMine.
If you are referring to slow joining time of the client, you should have a look at plugins like Specter.
you might also want to write unit tests for your plugin before testing it with PocketMine code. That is, assuming your plugin is large enough to require unit tests.

@SOF3 SOF3 removed this from the 5.0 milestone Mar 23, 2019

@pmmp pmmp unlocked this conversation Mar 23, 2019

@Anders233

This comment has been minimized.

Copy link

Anders233 commented Mar 24, 2019

Because I often add some new features or urgently fix some bugs when the server is running normally.
So if you don't shut down and restart the server,
It's a nice feature to be able to reload the plugin code.
I don't want to tell the players that the server has to be restarted.
Rejoin the server later.

@SOF3

This comment has been minimized.

Copy link
Member Author

SOF3 commented Mar 24, 2019

It has NEVER been and WILL never be possible to reload plugin code without restarting the server. It is only possible to load absolutely new code, so this has nothing to do with urgent fixed.

This issue is discussing about the removal of the ability to load absolutely new plugins, which rarely happens. (If you are updating a plugin, you must restart anyway)
More servers have most of their plugins installed on the first few days, and this late enable ability is rarely useful after that. Since you won't have lots of players on the first few days, restarting the server would not be a big deal.

@dktapps

This comment has been minimized.

Copy link
Member

dktapps commented Mar 24, 2019

yOu cAn'T rElOaD pLugIn sOurCe coDe aT RuNtImE

how many times...

@nathfreder

This comment has been minimized.

Copy link

nathfreder commented Mar 24, 2019

yOu cAn'T rElOaD pLugIn sOurCe coDe aT RuNtImE

how many times...

It's pretty misleading though. /reload could mean anything.

  • Reload server
  • Reload plugin source
  • Reload vending machine
@dktapps

This comment has been minimized.

Copy link
Member

dktapps commented Mar 24, 2019

yes, this is exactly why it's been removed in 4.0.

@SOF3

This comment has been minimized.

Copy link
Member Author

SOF3 commented Mar 24, 2019

The proposal is still facing difficulties with plugin loaders.
Suppose a plugin called D registers a new plugin loader that is going to load T. D cannot be loaded without loading libraries required for D.
On the other hand, plugin T might also require libraries. However, they cannot be loaded because the composer stage has already been initialized in order to trigger T.

This is a very problematic issue. Fortunately, we don't have many plugins that load other plugins on startup (much less after startup - see #2825). The most popular example is DevTools (others like ZipPluginLoader and AllAPILoader are nonstandard and pretty much hated by some conservative members of the community). So, why don't we disallow late composer installation only for these specific plugins?

Hence, the solution is like this:
Add a new plugin load order PluginLoadOrder::NO_COMPOSER (or NO_LIBS, or SEARCHING_PLUGINS). Plugins that provide plugin loaders must use this load order, and they must not request libraries. Plugins loaded by these plugin loaders may still use PluginLoadOrder::NO_COMPOSER.

The pseudocode becomes like this:

plugins := []
for each plugin in "plugins/*.phar"
    push plugin to plugins[plugin.load_order]
while plugins[NO_COMPOSER] is not empty
    plugin := plugins[NO_COMPOSER].take
    load plugin // this may recursively append to plugins[NO_COMPOSER]

merge_libs := []
for each plugin in plugins[STARTUP] + plugins[POSTWORLD]
    append plugin.libs to merge_libs

composer_update merge_libs

for each plugin in plugins[STARTUP]
    load plugin
...
for each plugin in plugins[POSTWORLD]
    load plugin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.