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

Feature: Rework startup logic #4377

Merged
merged 24 commits into from
Feb 14, 2024

Conversation

onebeastchris
Copy link
Member

@onebeastchris onebeastchris commented Jan 4, 2024

Hey! This PR aims to resolve a few issues related to /geyser reload and general hacky inconsistencies on all bootstraps.

Bootstrap changes:
To achieve this, this PR would split the current onEnable and onDisable methods set by the GeyserBootstrap interface into separate methods to avoid some of the reload guards.

Instead of onEnable, there now would be a onGeyserInitialize and onGeyserEnable method. Similarly, there would be a onGeyserDisable / onGeyserShutdown method. The prefix should avoid conlicts with platform methods (such as spigots onLoad, or fabric onInitialize), and allow us to properly define loading behavior.

onGeyserInitialize would be called early, and only once (with spigot's reloading being the exception, but that's a different mess). This would primarily do things that would only need to be done once, for example registering event listeners, checking for platform incompatibilities, or similar things that would only be done once instead of multiple times.

onGeyserLoad would be called once when it's time to "start" Geyser. For e.g. spigot, that would be after all other plugins load, for Velocity, that would be called when the ListenerBoundEvent, and so forth. This method would also be called again for the Geyser reload command to basically re-start Geyser. Additionally, Bungee and Spigot platforms would now also load the config in onGeyserEnable, which will also re-load the config with the geyser reload command. (resolves #4299).

onGeyserDisable would be called exclusively for the geyser reload command, or indirectly by onGeyserShutdown. This would also ensure that we e.g. dont shut down the Geyser injector - if configured/present - since that wouldn't be re-created with a reload.

Internal changes

The bootstrap changes here require a few other internal adjustments too. For example, the command's in the command manager are no longer cleared with a disable - since we're not able to re-register commands.

Additionally, i've added a GeyserPreReloadEvent and GeyserPostReloadEvent to be called before and after Geyser reloads. You can think of these as being equivalents to the GeyserPreInitialize and GeyserPostInitialize events. In an ideal world, we would be able to shut down extensions fully and load them new in the reload event. This is unfortunately not possible - seeing that e.g. commands cannot be added or removed while the server is running. Hence, a better solution than to disable extensions is to keep them running, and indicate that we are reloading using these new events.

Another major change would be that we would start Geyser immediately on the spigot/paper variant with onEnable instead of waiting for other plugins - otherwise, we can't register extension commands properly.

Other changes:

  • Yeeted old velocity check, replaced with a check for outdated velocity using it's api methods
  • moved bungee query disabling to after the config was loaded - since changing the bedrock port (geyser's listening port) is possible with reload, we might need to re-do that
  • removed pre4.0.0 viaversion check - there is still a check for outdated viaversion

Possibilities/Goals:

  • This should make the ViaProxy implementation less hacky, since it can now split it's logic properly
  • Changing/Expanding geyser reload behavior: With this PR, it would leave extensions running, and apply all configuration changes (with the exception of direct connection and compression disabling, since reloading wouldnt re-inject). Do we also want to e.g. re-populate the item and block registries? Currently, only the resource pack registry would be re-populated.

TODO:

  • thorough testing on all platforms
    Initial testing seems to work, need to confirm once again after the structure is more finalized

  • ensure cloud pr compatibility - specifically, whether we can re-register extension commands easily. If not, that will either require per-platform handling, or a rework of how extensions and geyser reload work together

done - turns out that we can't add new or remove existing commands, with or without cloud. That's unfortunate, and opens the question of how we want to handle that.

  • verify spigot's reload command works, and if needed, adjust GeyserImpl#reload
    Works out of the box, doesn't even differ from reloading handling; need to commit that change though.

- rename Geyser onLoad/onDisable to geyser specific names
- Re-work startup logic on Fabric
- yeet `INITIALIZED` variable
- Change structure to fit the new stages

TODO: reload config with Geyser reload
# Conflicts:
#	bootstrap/bungeecord/src/main/java/org/geysermc/geyser/platform/bungeecord/GeyserBungeePlugin.java
#	bootstrap/spigot/src/main/java/org/geysermc/geyser/platform/spigot/GeyserSpigotPlugin.java
#	core/src/main/java/org/geysermc/geyser/GeyserBootstrap.java
#	core/src/main/java/org/geysermc/geyser/pack/SkullResourcePackManager.java
#	core/src/main/java/org/geysermc/geyser/registry/loader/BiomeIdentifierRegistryLoader.java
#	core/src/main/java/org/geysermc/geyser/registry/loader/CollisionRegistryLoader.java
#	core/src/main/java/org/geysermc/geyser/registry/loader/EffectRegistryLoader.java
#	core/src/main/java/org/geysermc/geyser/registry/loader/EnchantmentRegistryLoader.java
#	core/src/main/java/org/geysermc/geyser/registry/loader/NbtRegistryLoader.java
#	core/src/main/java/org/geysermc/geyser/registry/loader/SoundRegistryLoader.java
#	core/src/main/java/org/geysermc/geyser/registry/populator/BlockRegistryPopulator.java
#	core/src/main/java/org/geysermc/geyser/registry/populator/CreativeItemRegistryPopulator.java
#	core/src/main/java/org/geysermc/geyser/registry/populator/ItemRegistryPopulator.java
#	core/src/main/java/org/geysermc/geyser/registry/populator/RecipeRegistryPopulator.java
#	core/src/main/java/org/geysermc/geyser/util/FileUtils.java
# Conflicts:
#	bootstrap/fabric/src/main/java/org/geysermc/geyser/platform/fabric/mixin/client/IntegratedServerMixin.java
- properly re-fire PreInit event
- Fully load and unload extensions
todo: find out if cloud manages to register commands on geyser reload, because this currently sure can't
@onebeastchris onebeastchris added PR: Feature When a PR implements a new feature PR: Optimization When a PR doesn't necessarily add anything new or fix anything, but improves upon the code Extensions PR: Needs review Indicates that a PR is functional and review-ready. labels Jan 5, 2024
- Don't disable extensions at reload
- Fire GeyserPreReloadEvent and GeyserPostReloadEvent instead of the pre and post init events so extensions can do custom reloading

Unfortunately, we can't disable and enable extensions  without consequences - e.g. it is not possible to add or remove commands during runtime. Hence, it makes more sense to let extensions running during reloads, and provide them with information about the reload so extensions can implement custom reload handling.

In the future, some Geyser lifecycle events could be re-fired; for example the custom item or block registries. This however is outside the scope of this PR.

Further changes:
- split geyser.shutdown() into disable and shutdown respectively to better separate between reloading and stopping
- undo extension hot-loading
- don't re-create the GeyserImpl instance on fabric with reloads
- Listen to spigot's serverloadevent to determine if we need to end our shutdown process and start running again
- Add GeyserPreReloadEvent and GeyserPostReloadEvent to indicate reload status to extensions

TODO:
- testing all bootstraps
@onebeastchris onebeastchris marked this pull request as ready for review January 17, 2024 10:32
@Konicai
Copy link
Member

Konicai commented Feb 10, 2024

It's very possible that I simply missed it but I feel that the following needs more discussion:

Another major change would be that we would start Geyser immediately on the spigot/paper variant with onEnable instead of waiting for other plugins - otherwise, we can't register extension commands properly.

This will lead to Spigot plugins largely being unable to interact with Geyser's registries (custom blocks, items, etc)? IIRC we delayed starting for the purpose of allowing them to do that

@onebeastchris
Copy link
Member Author

Shouldn't plugins that rely on Geysers api simply declare dependencies in the plugin.yml to be loaded before Geyser?

@onebeastchris
Copy link
Member Author

The only alternative that I see to keep the current behavior where Geyser loads only after the server starts would be to create the command manager early on (assuming that doesn't depend on other things that occur after onGeyserEnable). However, that would also require loading the config early (and hence, twice), since we otherwise can't create the logger properly

Changes:
- fire preinit, loading of locale, and extension manager creation again in the GeyserImpl constructor.
- Load configs once early, and again when we are reloading
- Don't re-create the command manager on all platforms
@onebeastchris
Copy link
Member Author

Reverted the change where spigot/paper would start Geyser on onEnable() - instead, i'm now loading configs early on all platforms. Tested on all platforms with a few extensions; works fine.

Other neat details:

  • the GeyserDefineCommandsEvent should now be called before the registries are loaded
  • this behavior is consistent across platforms.

Also added some more details in the javadocs of GeyserImpl.

@onebeastchris onebeastchris merged commit 6a51d82 into GeyserMC:master Feb 14, 2024
1 check passed
@onebeastchris onebeastchris deleted the rework-startup-logic branch February 28, 2024 21:10
@onebeastchris onebeastchris removed the PR: Needs review Indicates that a PR is functional and review-ready. label Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Extensions PR: Feature When a PR implements a new feature PR: Optimization When a PR doesn't necessarily add anything new or fix anything, but improves upon the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Config Doesn't Reload when Running geyser reload, Reboot Required to Apply
3 participants