Fix save on stop on Folia - #39
Open
bridgelol wants to merge 1 commit into
Open
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 fromonDisable:TickRegions.getScheduler().halt(...)has stopped every region thread. Nothing is left to run a region task.JavaPlugin.setEnabledflipsisEnabledto false before it callsonDisable.FoliaEntitySchedulerchecksplugin.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.
/stopprintsSaving 'testworld'...and then nothing, forever:The fix
ShutdownExecutoris a small queue that the stopping thread drains itself. While it is running,FoliaUtilhands tasks to it instead of to a scheduler, andonDisableruns the queue until the save future completes.Doing the work on that thread is safe. Folia's
TickThread.isTickThreadFor(...)falls back toisShutdownThread()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.getBlockEntitylooks 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.executereturns false when the entity has already been removed, and then runs neither the task nor the retired callback.scheduleOnEntityIfFoliapassednullfor retired and ignored the return value, soentityToBytesleft its future uncompleted andEntitiesWorldAccess.saveChunkDatablocked onjoin(). It now takes a retired callback and invokes it when scheduling fails.saveAsPassengerretry leftsuccessfulFutureuncompleted 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 throughscheduleOnEntityIfFolia, which runs inline when the caller already owns the entity.The block entity task in
PolarChunk.convertalso 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:
Paper 26.2 build 112, same setup, as a check that the normal path is unaffected:
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:
The file also loads again on the next start (
Loading polar world: testworld).