Add method isTickingWorlds to Bukkit

[willkroboth]

It is unsafe for plugins to attempt loading or unloading a world while the worlds are being ticked. However, there currently isn't a practical way (as far as I could tell) for plugins to know whether or not it is currently unsafe to try to load/unload a world. Technically, they could do this:

try{
    Bukkit.createWorld()/unloadWorld();
} catch (IllegalStateException ignored){
    // whoops, I guess it wasn't a good idea to do this now
}

However, this isn't great, since if it was okay to create/unload a world right now, that would happen whether or not they actually wanted to do that right now. For example, if a plugin wants to do some expensive preprocessing before it creates the world, it could either check beforehand, like so:

try{
    Bukkit.createWorld("probe");
} catch (IllegalStateException ignored){
    // not safe, don't continue
    return;
}
// Now we have a new world to handle...
Bukkit.unloadWorld("probe")

// expensive processing

Bukkit.createWorld("real")

or do the work and get rejected:

// expensive processing

try{
    Bukkit.createWorld("real");
} catch (IllegalStateException ignored){
    // welp, guess that processing was for nothing
    return;
}

With this PR, plugins can use Bukkit.isTickingWorlds() to tell when the worlds are being ticked. The first example now looks like this:

if(!Bukkit.isTickingWorlds()) {
    // Alright, it's safe to go ahead
    Bukkit.createWorld()/unloadWorld();
} else {
    // Alright, I'll do it later
}

[PedroMPagani]

Have you thought about using both tick events of paper? ServerTickEnd is an example, this PR should kind of achieve the same as the event, shouldn't it?

[Lulu13022002]

This patch with the other pr is not really when the server tick but more when a CME can occur on the world level list which is more accurate (but the accuracy here is not really needed for the load/unload use case). The other pr is probably more important bc of the non async chat events that run into the process queue. Also you should change the documentation of the load/unload methods to explain the unsafe use (in the upstream javadoc patch bc it's still unsafe on spigot) and shouldn't assume that it's the only use case.

[willkroboth]

Neither this PR nor my other PR (#8315) is aiming to fix the CME; that was already fixed in Paper #7653 and #8081 by throwing an IllegalStateException if CraftServer#createWorld or CraftServer#unloadWorld is called while the value accessed by Bukkit.isTickingWorlds() is true.

Loading/unloading worlds while they are being ticked is considered safe on Spigot. Spigot doesn't throw an IllegalStateException if you try it. Instead, they avoided the crash by making the levels map effectively copy on write, which avoids the CME in a different way. This "fix" has been included in Paper since Paper build #216, but Paper decided to keep their solution in #8300.

I do agree with your comment on changing the Javadocs. Knowing if it's safe to load/unload worlds might not be the only use case of Bukkit.isTickingWorlds, and the load/unload methods should mention when they might throw an exception. I'll fix that now.

[willkroboth]

Since there doesn't seem to be any progress here, I'm guessing we're either waiting for the new chunk generation system, or this isn't going to get pulled. Going with it's not going to get pulled, would it at least be worth it to update the Javadocs for Bukkit#createWorld and Bukkit#unloadWorld to include the @throws IllegalStateException when {@link #isTickingWorlds() isTickingWorlds} is true part (or something equivalent)? I think it's important that developers who are developing for Paper to know that these methods could throw this exception, otherwise they might be confused why the exception is happening.

[electronicboy]

Dizzy and low priority, not 100% sure what the plan here is given that, ideally we'd line up with spigot, but my concerns over safety haven't really been addressed, especially given we've had issues with world unloading causing issues for years as-is

[willkroboth]

I figured out I could access the result of Bukkit.isTickingWorlds() using reflection like this:

Server server = Bukkit.getServer();
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);
    // Paper fix is active, and we have the value Bukkit.isTickingWorlds() would have returned in isTicking
} catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException ignored) {
    // These exceptions shouldn't happen
} catch (NoSuchFieldException ignored) {
    // isIteratingOverLevels field does not exist, so that Paper fix is not active
}

Considering isIteratingOverLevels is a public field anyways I think this method is valid. The reflection only really helps with not having to import CraftServer for every new minecraft version and handling the case where the server running this code is not paper, in which case the branch for NoSuchFieldException is triggered.

With that, apart from maybe the Javadocs updates, this pull request isn't necessary. If someone would like to copy the files and take over the PR, be my guest, but I'm closing this PR with the resolution: "just access the field yourself"

[Lulu13022002]

You don't need to use CraftServer since MinecraftServer has already a singleton:
MinecraftServer.getServer().isIteratingOverLevels

[electronicboy]

Heves the down low:

Loading worlds mid tick is not ever going to be officially supported, there will be no commitment to resolving issues that occur in doing this, however, given that the logic in the server generally seems to be sound enough, I am restoring this. There is no promise that this won't be reverted in the future, especially if state corruption issues are found, however, this will now work akin to spigot. I highly recommend using the boolean added in this patch to validate the current state before loading a world and deferring as appropriate.