Concurrent error no longer occurs on Spigot when command blocks load or unload worlds

[willkroboth]

Is your feature request related to a problem?

#6072 and #8080 raised the issues that command blocks loading/unloading worlds would cause the server to crash. Paper fixed this in #7653 and #8081 by throwing an IllegalStateException if code attempted to load/unload worlds while the worlds were being ticked.

The same issue was raised in SPIGOT-7089, which was recently resolved. Now, if code attempts to load/unload a world while the worlds are being ticked on a Spigot server it continues as normal.

Describe the solution you'd like.

I think the Spigot fix here is better. Throwing the IllegalStateException isn't necessary anymore because worlds can now be loaded/unloaded while the worlds are being ticked.

I think all that needs to be done is remove the changes implemented by #7653 and #8081 and make sure craftbukkit is updated (might already be done since 0ddd20c).

Describe alternatives you've considered.

The alternative is that the Paper fix is kept and worlds would not be able to be loaded or unloaded while the worlds were being ticked.

Other

No response

[electronicboy]

loading I can semi get behind, but, somewhat iffy on due to long term goals to properly support vanilla dimensions stuff which loading worlds in play is already going to become a headache

unloading while ticking sounds like an unmitigated disaster likely to create its own set of stupid issues

[willkroboth]

Actually, an alternative I posted on SPIGOT-7089 might work as well:

3. Let CraftServer#createWorld and CraftServer#unloadWorld do their thing but delay them from modifying MinecraftServer.levels until the worlds are no longer being ticked.

This one would be the most complicated, and might
look something like this:

// do everything else
if(!worldsAreBeingTicked){
    Modify MinecraftServer.levels now because it is safe
} else {
    Add world to list so it can be loaded/unload later
}
This would allow createWorld and unloadWorld to be called while the worlds are being ticked, but not actually modify the Map of levels until it is safe to do so.

If the worlds are being ticked when CraftServer#createWorld or CraftServer#unloadWorld is called, instead of canceling the method with an IllegalStateException the action could be remembered and run later. This solves the problem of command blocks not being able to load or unload worlds while avoiding potential problems from modifying the world list while it is being iterated through.

Whatever happens, it would be best if Spigot and Paper at least agreed on what should happen in this situation, either the worlds are modified or they aren't.

[willkroboth]

Okay, I figured out what Spigot actually did to solve this issue. They added these methods to net.minecraft.server.MinecraftServer:

// CraftBukkit start
public void addLevel(ServerLevel level) {
    Map<ResourceKey<Level>, ServerLevel> oldLevels = this.levels;
    Map<ResourceKey<Level>, ServerLevel> newLevels = Maps.newLinkedHashMap(oldLevels);
    newLevels.put(level.dimension(), level);
    this.levels = Collections.unmodifiableMap(newLevels);
}

public void removeLevel(ServerLevel level) {
    Map<ResourceKey<Level>, ServerLevel> oldLevels = this.levels;
    Map<ResourceKey<Level>, ServerLevel> newLevels = Maps.newLinkedHashMap(oldLevels);
    newLevels.remove(level.dimension());
    this.levels = Collections.unmodifiableMap(newLevels);
}
// CraftBukkit end

Instead of modifying the levels map directly, they make a copy, modify the copy, and store the copy back to levels. I think that's actually super smart. The original object being iterated through is never modified, but levels is updated immediately, ready for the next tick.

I think this means that the Paper safeguards added by 0900-Throw-exception-on-world-create-while-being-ticked.patch can be removed without introducing any serious problems.

[electronicboy]

it's just a basic Copy on write alternative, it avoids the exception being thrown but doesn't put a single care into the longevity or impl decisions elsewhere;

as said, I can get behind loading, I don't think that there is anything here which creates issues which aren't going to have to be solved for loading worlds in general in the future;

unloading worlds is an entirely different situation, however, unloading worlds is already leaky and broken as heck due to mojangs implementation decisions, right now spigot introduces a fun precarious issue where you can unload a world and close all its underlying engineering right in the middle of that world being ticked, which brings up a lot of concerns around this and the potential to crash the server

[willkroboth]

spigot introduces a fun precarious issue

Since Spigot introduced the issue, would it be their problem to fix it? Either way, testing this out on Spigot, nothing bad seems to be happening when unloading worlds while they are being ticked. Doing something like unloading a world with a command block in that world seems to work as expected, though if there is a way to crash the server I'd like to find it.

I did notice that Paper does actually have problems with unloading worlds while they are being ticked, which I think is caused by this Paper change in CraftServer#unloadWorld:

if (save) {
    handle.save(null, true, false); // Paper - don't disable saving
}

On Spigot unloading the world happens almost instantly, while Paper gets hung up saving the world for about a minute.

[Lulu13022002]

But this is a spigot bug: https://hub.spigotmc.org/jira/projects/SPIGOT/issues/SPIGOT-6967?filter=allopenissues
it just skip the save of the world on spigot which is dumb ...
And personally i see no issue here, it's just normal to throw when it's unsafe, also what is the use case of unloading a world with command block instead of a plugin ?

[electronicboy]

I mean, I don't have the patience to test every dozen theory, I just see that there is a precarious state where a world is ticking and parts of the system behind it have been shut down, what happens if something trips a chunk load in that world after the world is unloaded? if it doesn't blow up, is it even remotely safe to assume that that behavior is going to remain?

and, yea, saving stuff is slow, I wouldn't be surprised if replacing that logic is part of leafs work given how slow and overengineered some of that mess is

[willkroboth]

Huh, that is weird that there is a save method that has a flag for disabling the saving. Weirder still that there are no comments on the Spigot issue, but I guess they were just distracted by other issues. It seems that on Paper and Spigot the method does at least do this when that flag is set:

this.serverLevelData.setWorldBorder(worldserver1.getWorldBorder().createSettings());
this.serverLevelData.setCustomBossEvents(this.server.getCustomBossEvents().save());
this.convertable.saveDataTag(this.server.registryHolder, this.serverLevelData, this.server.getPlayerList().getSingleplayerData());

though still strange. I wonder if they did that intentionally or got confused and thought the flag enabled saving when it was first implemented.

It doesn't seem to me that unloading a world while they are being ticked is an unsafe operation anymore. It used to crash, and that's why Paper put in the throw, but now that Spigot has made its own resolution and implemented a copy-on-write alternative it doesn't seem to be unsafe anymore. I at least haven't managed to crash Spigot, though I'd be happy to make it happen to see that it is an unsafe operation. If it is unsafe I think it would be good to get Spigot to realize that so Spigot and Paper can agree on what happens when worlds are loaded or unloaded while being ticked.

I think technically the world unload is always being done by a plugin since I don't think vanilla supports creating and unloading worlds. As this comment mentions a world unload during world ticking could also be triggered by other things, such as certain events, not just command blocks. But I digress.

The original use case is mentioned in Multiverse/Multiverse-Core#2560, where I was trying to use the Multiverse command /mvregen (which calls unloadWorld and createWorld) to generate new worlds for each game of a manhunt-inspired minigame. I didn't know how to make plugins back then, so I used command blocks to manage the game. That crashed the server, so I made a report. Regardless of the use case though, this still was a potential crash.

In summary, after SPIGOT-7089 resolved, loading and unloading worlds while the worlds are being ticked doesn't seem to be an unsafe operation anymore. Therefore, I think Paper's fixes for the crash from #7653 and #8081 are obsolete and can be removed.

If it is unsafe to load/unload worlds while they are ticking, Spigot should probably know about that and throw an exception as well. Spigot should add a way to tell if the worlds are being ticked so that plugins can reschedule their attempts to load/unload worlds to avoid trying to do unsafe operations.

I suppose the core conflict that needs to be resolved here is that Spigot thinks this is safe, while Paper thinks it is unsafe.

[electronicboy]

the CME is an entirely seperate concern of what I'm saying, that's just because the list was being modified; making that a COW will solve the original CME but leaves huge concerns over the state of the world, i.e. now you've got a world which is ticking but is supposed to have been shut down, which is already a mess given how broken this system is between vanilla and upstream, meaning that things like the chunk management stuff should be shutdown and closed for this world, the question here is generally "can we be assured that a plugin interacting with a world in this oddball state isn't going to introduce further crash risks into the server, and can we even be sure that this is going to be a reliable bet in terms of long term API support", i.e. are we sure that we're not going to have to add back the protection against this when the new chunk system is merged

My concern is that the unloading of worlds is occurring at a weird point in time: i.e. when the world is still in the queue for ticking, like, in part, some of the risks are reduced because you can't unload a world with players actively in them, but, what if a plugin is doing something that induces a chunk load right after the world is shutdown, is there a risk that events are still fired for this world which would make plugins interact with a world in a dangerous state?

i.e. what happens if you unload a world fully, and then try to change a block in an unloaded chunk in that world? like, this introduces many dozen state concerns

[willkroboth]

Okay, unloading a world while it is being ticked sounds like a bad idea.

I suppose I can't know what projects that aren't my own want, but here's how I think the situation currently is and why I think there is a problem:

1. Plugins like Multiverse should be compatible with Spigot and Paper. Either the creators of Multiverse want this to be true so more people can use it, or Paper wants to support all plugins that are compatible with Spigot. I'm very sure the second point is true because it says so on Paper's website: "All the meanwhile, Paper retains compatibility with plugins written for Spigot and Bukkit."
2. Multiverse wants to provide commands that load and unload worlds. I'm very sure that's true because it says on Multiverse's Bukkit page: "Perform all per-world modifications with in-game commands! (Stop getting those YAML errors!)"
3. Multiverse wants its commands to work when put in command blocks. This may or may not be true. They could make it so that their commands do not work in command blocks. However, their command system seems to support command blocks, so I think this is true. This is further supported by the fact that Commands blocks can’t create or unload worlds Multiverse/Multiverse-Core#2560 was marked as an upstream issue.

So Multiverse wants to have commands that load and unload worlds work when put in command blocks. Maybe they could change that, and that would be one resolution to this problem.

In this respect, Multiverse is currently compatible with Spigot but not with Paper. Spigot lets them do this, but Paper does not, which seems to be against point 1. This could be resolved by Spigot by agreeing with Paper and throwing an exception, or it could be resolved by Paper by agreeing with Spigot and letting it happen.

It could also technically be resolved by Multiverse if Multiverse could tell when it was on a Spigot server and when it was on a Paper server. I've actually already made a commit that kinda does this. If it is unsafe to load/unload a world when the method addOrRemoveWorldSafely is called, it will do it later, but if it is safe it will do it now, like this:

if(worldsAreNotBeingTicked){
    modifyWorlds();
} else {
    new BukkitRunnable() {
        public void run() {
            modifyWorlds();
        }
    }.runTask(plugin);
}

The only thing it needs to resolve this issue is a way to tell when it is safe/unsafe, replacing WorldsAreNotBeingTicked in the example. Right now this would have to be something like isPaperServer && !MinecraftServer.isIteratingOverWorlds. If Paper let this happen it could become true. If Spigot throws an exception they should also add a method to check if the worlds are being ticked so WorldsAreNotBeingTicked could become that.

So, unless any of my previous points are wrong, there is a conflict, and it can be resolved in three ways:

1. Spigot can throw an exception
   I don't think Spigot will change their solution unless it is shown to be unsafe

2. Paper can let it happen
   I'm here to try to make this happen

3. Multiverse (stand-in for plugins in general) can change their behavior based on whether they are on a Spigot or Paper server
   I suppose I could switch and make this happen

As a user who is not any of these entities, I just want one of these solutions to be chosen.

Can you think of any other solutions? Which solution do you think I should pursue?

[willkroboth]

If there are no objections, I'm going to close this issue tomorrow with the resolution: plugins have to deal with the differences between Spigot and Paper.