Skip to content

Fix save on stop on Folia - #39

Open
bridgelol wants to merge 1 commit into
MinehubMC:26_2from
bridgelol:fix/folia-save-on-stop
Open

Fix save on stop on Folia#39
bridgelol wants to merge 1 commit into
MinehubMC:26_2from
bridgelol:fix/folia-save-on-stop

Conversation

@bridgelol

Copy link
Copy Markdown

Closes #37.

The problem

Saving a world schedules work on the region scheduler (block entities, in PolarChunk.convert) and on the entity scheduler (EntitySerializerImpl.entityToBytes), then blocks on the resulting futures. That is fine while the server is running, but not from onDisable:

  • Folia disables plugins from its region shutdown thread, and it gets there only after TickRegions.getScheduler().halt(...) has stopped every region thread. Nothing is left to run a region task.
  • Bukkit drops any task submitted by a plugin that is not enabled, and JavaPlugin.setEnabled flips isEnabled to false before it calls onDisable. FoliaEntityScheduler checks plugin.isEnabled() inside the task wrapper and returns early, so this part is not Folia specific.

So the tasks never ran, the futures never completed, and Polar.saveWorld(world).join() blocked forever. The world was not saved and the server never finished stopping, which is what the TODO on that line was about.

Reproduced on Folia 26.2 (build 4) with a blank polar world containing one loaded chunk. /stop prints Saving 'testworld'... and then nothing, forever:

"Region shutdown thread" #86 waiting on condition
	at java.util.concurrent.CompletableFuture.join(CompletableFuture.java:2138)
	at live.minehub.polarpaper.PolarPaper.onDisable(PolarPaper.java:107)
	at org.bukkit.plugin.java.JavaPlugin.setEnabled(JavaPlugin.java:285)
	...
	at net.minecraft.server.MinecraftServer.stopServer(MinecraftServer.java:1091)
	at io.papermc.paper.threadedregions.RegionShutdownThread.run(RegionShutdownThread.java:163)

The fix

ShutdownExecutor is a small queue that the stopping thread drains itself. While it is running, FoliaUtil hands tasks to it instead of to a scheduler, and onDisable runs the queue until the save future completes.

Doing the work on that thread is safe. Folia's TickThread.isTickThreadFor(...) falls back to isShutdownThread() when there is no current region, so the ownership checks pass. It is the same thread Folia saves its own chunks with a moment later. On Paper the stopping thread is just the main thread, so nothing changes there.

One thing does not survive the move: LevelChunk.getBlockEntity looks at the captured block entities first, and on Folia those live in region data that the shutdown thread cannot reach (getCurrentWorldData() returns null there). Block entities are only ever captured mid block placement, so while stopping the conversion reads the chunk's own block entity map instead.

Other hangs on the same path

Both of these could block a save forever on their own, so they are fixed here too:

  • EntityScheduler.execute returns false when the entity has already been removed, and then runs neither the task nor the retired callback. scheduleOnEntityIfFolia passed null for retired and ignored the return value, so entityToBytes left its future uncompleted and EntitiesWorldAccess.saveChunkData blocked on join(). It now takes a retired callback and invokes it when scheduling fails.
  • The saveAsPassenger retry left successfulFuture uncompleted when the retry itself threw, and it scheduled onto the entity from the entity's own thread on Folia, which deadlocks against itself. It now goes through scheduleOnEntityIfFolia, which runs inline when the caller already owns the entity.

The block entity task in PolarChunk.convert also completes its future exceptionally now rather than leaving it hanging if the loop throws.

Timeout

Saving on stop is bounded by SAVE_ON_STOP_TIMEOUT_SECONDS (60s, the same budget Folia gives itself to halt its schedulers). It should not be reachable now that the tasks actually run, but a stop that never returns is worse than a world that failed to save. Happy to change the number or drop it if you would rather it stayed unbounded.

Testing

Blank polar world with a force loaded chunk, a chest, a furnace and a cow, saveOnStop: true, then /stop.

Folia 26.2 build 4:

[polarpaper] Disabling polarpaper v2.1.3
[polarpaper] Clearing temp files for testworld
[polarpaper] Saving 'testworld'...
[polarpaper] Saved 'testworld' in 19ms

Paper 26.2 build 112, same setup, as a check that the normal path is unaffected:

[polarpaper] Saving 'testworld'...
[polarpaper] Saved 'testworld' in 27ms

No errors in either, shutdown continues normally and the server exits. Before the change the Folia run never gets past Saving 'testworld'....

Decompressing the two written files gives byte identical content, with the block entities and the entity present:

minecraft:chest    x3
minecraft:furnace  x3
minecraft:cow      x1

The file also loads again on the next start (Loading polar world: testworld).

Saving a world schedules work on the region scheduler (block entities in
PolarChunk.convert) and on the entity scheduler (EntitySerializerImpl),
then blocks on the resulting futures. That does not work from onDisable:
Folia halts its region schedulers before it disables plugins, and Bukkit
drops any task submitted by a plugin that is no longer enabled. The tasks
never ran, so Polar.saveWorld(world).join() blocked forever and the server
never finished stopping.

Run those tasks on the stopping thread instead, through ShutdownExecutor.
Folia disables plugins from its region shutdown thread, where
TickThread.isTickThreadFor falls back to isShutdownThread(), so region
ownership checks pass and the work is safe to do there. It is the same
thread Folia saves its own chunks with.

Also fixes two other ways a save could block forever:

- EntityScheduler.execute returns false when the entity has already been
  removed, and then runs neither callback. scheduleOnEntityIfFolia passed
  null for retired and ignored the result, so entityToBytes left its
  future uncompleted and saveChunkData blocked on join().
- The saveAsPassenger retry left successfulFuture uncompleted when the
  retry itself threw, and scheduled onto the entity from the entity's own
  thread on Folia, which deadlocks against itself.
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.

Folia does not support saveOnStop

1 participant