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

Defer World modification to when the worlds are not being ticked on Paper #2802

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

willkroboth
Copy link

@willkroboth willkroboth commented Aug 19, 2022

PaperMC/Paper/issues/8300 was resolved as: "plugins have to deal with this difference between Spigot and Paper," so this PR is dealing with the fact that Spigot allows worlds to be loaded/unloaded while the worlds are being ticked, while Paper will throw an IllegalStateException. This PR would resolve #2560.

First, this PR adds the method MVWorldManager#addOrRemoveWorldSafely. The implementation for this method in MVWorldManager looks like this:

public void addOrRemoveWorldSafely(String worldName, String operationName, Runnable worldModification) {
        if (safeToAddOrRemoveWorld()) {
            // Operation is fine to do now
            worldModification.run();
        } else {
            // Operation needs to be delayed until worlds are not being ticked

            Logging.fine("Worlds were being ticked while attempting to %s %s. Trying again in the next tick", operationName, worldName);
            new BukkitRunnable() {
                public void run() {
                    worldModification.run();
                }
            }.runTask(plugin);
        }
    }

If it is safe to add or remove a world when this method is called, it will run the requested worldModification immediately. If it is not safe (because we're on a Paper server and the worlds are being ticked), it will schedule the worldModification to happen later using a BukkitRunnable. All the commands (8 by my count) that may load or unload worlds were changed to use this method to make sure their task always happens when it is safe.

To tell if it is currently safe to add or remove a world, this method is used:

private boolean safeToAddOrRemoveWorld(){
        Method isTickingWorlds;
        try {
            isTickingWorlds = Bukkit.class.getMethod("isTickingWorlds");
        } catch (NoSuchMethodException e) {
            // Paper fixes aren't active, so it is always considered safe to proceed
            return true;
        }
        // Paper fixes are active, so we need to and can check Bukkit.isTickingWorlds
        try {
            return !(boolean) isTickingWorlds.invoke(null);
        } catch (InvocationTargetException | IllegalAccessException e) {
            // Shouldn't happen, I know I'm using the method correctly
            throw new RuntimeException(e);
        }
    }

If the method can't find Paper fixes, it will always return true because Spigot always considers it safe to load or unload a world. If Paper is present, the method checks the value of Bukkit.isTickingWorlds() to know when it is safe.

This PR is currently a draft because I am waiting for PaperMC/Paper#8316 to be accepted, which will add the method Bukkit.isTickingWorlds() to the Paper API. I'm submitting this pull request now for feedback on the way I solved the issue.

These changes were tested on Spigot 1.19.2 and Paper 1.19.2 (with the changes added by PaperMC/Paper#8316), and no issues were noticed.

@willkroboth willkroboth changed the title Fix #2560 Allow command blocks to load and unload worlds on Spigot and Paper Aug 21, 2022
@willkroboth
Copy link
Author

This PR no longer depends on PaperMC/Paper#8316 being accepted. Instead, the isIteratingOverLevels boolean is directly accessed via reflection like this:

private boolean safeToAddOrRemoveWorld(){
    Server server = Bukkit.getServer();
    Logging.finest("Using reflection to test for Paper build after PR #7653");
    try {
        // basically doing ((CraftServer) Bukkit.getServer()).getServer().isIteratingOverLevels;
        Method getConsole = server.getClass().getMethod("getServer");
        Object console = getConsole.invoke(server);

        Field isTickingWorlds = console.getClass().getField("isIteratingOverLevels");
        boolean isTicking = isTickingWorlds.getBoolean(console);

        Logging.finest("Paper fix active");
        return !isTicking;
    } catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException e) {
        Logging.finest("%sUnexpected exception: %s", ChatColor.RED, e.getMessage());
        Logging.finest("Assuming Paper fix is inactive");
        // If the Paper fix actually is active it should become obvious when Paper complains
        // about a world being loaded/unloaded while being ticked
        // If that happens, this method needs to be fixed
        return true;
    } catch (NoSuchFieldException ignored) {
        // Expected to fail when field isIteratingOverLevels doesn't exist
        // Therefore, Paper fixes aren't active, so it is always considered safe to proceed
        Logging.finest("Paper fix inactive");
        return true;
    }
}

Since we're no longer waiting on Paper, this PR is ready to be reviewed and merged.

@willkroboth willkroboth marked this pull request as ready for review September 17, 2022 21:22
@willkroboth
Copy link
Author

Paper has aligned with Spigot and currently allows worlds to be loaded/unloaded while being ticked. While this technically makes this PR unnecessary, they have asked to consider deferring world load/unload to when the worlds are not being ticked, and this PR still does that.

@willkroboth willkroboth changed the title Allow command blocks to load and unload worlds on Spigot and Paper Defer World modification to when the worlds are not being ticked on Paper Mar 31, 2023
willkroboth and others added 6 commits April 11, 2023 18:14
Addresses Multiverse#2560

Commands that load or unload worlds trigger an IllegalStateException if they are run via a command block while the worlds are being ticked.

Using a BukkitRunnable, the operation can be delayed until the next tick at a time when the worlds are not being ticked.

MVWorldManager#addOrRemoveWorldSafely performs this logic, either running the operation now or delaying it.

8 commands were modified to use MVWorldManager#addOrRemoveWorldSafely, which I think are all the relevant commands.

Unfortunately I haven't found a way to tell when the worlds are being ticked on a Spigot server. There probably won't be any way to do this until [SPIGOT-7089](https://hub.spigotmc.org/jira/browse/SPIGOT-7089) resolves
…nload worlds

Note: this code was designed under the assumption that PaperMC/Paper#8316 will be accepted, so it won't work unless the commit from that PR is included in the Paper build.
(PaperMC/Paper#8316 was accepted, so this works now)
Also some more logging messages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Commands blocks can’t create or unload worlds
2 participants