Multiverse-Core icon indicating copy to clipboard operation
Multiverse-Core copied to clipboard

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

Open willkroboth opened this issue 3 years ago • 2 comments

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 avatar Aug 19 '22 21:08 willkroboth

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 avatar Sep 17 '22 21:09 willkroboth

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 avatar Sep 24 '22 16:09 willkroboth