From c01050f20c4a0ffca071315ae2ac5660fa82014c Mon Sep 17 00:00:00 2001 From: aromaa Date: Sun, 17 Dec 2023 21:57:01 +0200 Subject: [PATCH] Fix deadlock in world gen threads on world unload --- .../IOWorker$PendingStoreAccessor.java | 36 ++++++ .../resources/mixins.sponge.accessors.json | 1 + .../chunk/SpongeUnloadedChunkException.java | 33 ++++++ .../world/level/chunk/ChunkStatusMixin.java | 90 +++++++++++++++ .../level/chunk/storage/IOWorkerMixin.java | 107 ++++++++++++++++++ src/mixins/resources/mixins.sponge.core.json | 1 + 6 files changed, 268 insertions(+) create mode 100644 src/accessors/java/org/spongepowered/common/accessor/world/level/chunk/storage/IOWorker$PendingStoreAccessor.java create mode 100644 src/main/java/org/spongepowered/common/world/level/chunk/SpongeUnloadedChunkException.java create mode 100644 src/mixins/java/org/spongepowered/common/mixin/core/world/level/chunk/ChunkStatusMixin.java diff --git a/src/accessors/java/org/spongepowered/common/accessor/world/level/chunk/storage/IOWorker$PendingStoreAccessor.java b/src/accessors/java/org/spongepowered/common/accessor/world/level/chunk/storage/IOWorker$PendingStoreAccessor.java new file mode 100644 index 00000000000..a073cbc0cfd --- /dev/null +++ b/src/accessors/java/org/spongepowered/common/accessor/world/level/chunk/storage/IOWorker$PendingStoreAccessor.java @@ -0,0 +1,36 @@ +/* + * This file is part of Sponge, licensed under the MIT License (MIT). + * + * Copyright (c) SpongePowered + * Copyright (c) contributors + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package org.spongepowered.common.accessor.world.level.chunk.storage; + +import net.minecraft.nbt.CompoundTag; +import org.checkerframework.checker.nullness.qual.Nullable; +import org.spongepowered.asm.mixin.Mixin; +import org.spongepowered.asm.mixin.gen.Accessor; + +@Mixin(targets = "net/minecraft/world/level/chunk/storage/IOWorker$PendingStore") +public interface IOWorker$PendingStoreAccessor { + + @Accessor("data") @Nullable CompoundTag accessor$data(); +} diff --git a/src/accessors/resources/mixins.sponge.accessors.json b/src/accessors/resources/mixins.sponge.accessors.json index f2590cc7b39..e852ed58552 100644 --- a/src/accessors/resources/mixins.sponge.accessors.json +++ b/src/accessors/resources/mixins.sponge.accessors.json @@ -180,6 +180,7 @@ "world.level.chunk.LevelChunk$RebindableTickingBlockEntityWrapperAccessor", "world.level.chunk.LevelChunkAccessor", "world.level.chunk.storage.ChunkStorageAccessor", + "world.level.chunk.storage.IOWorker$PendingStoreAccessor", "world.level.dimension.DimensionTypeAccessor", "world.level.entity.EntityTickListAccessor", "world.level.entity.PersistentEntitySectionManagerAccessor", diff --git a/src/main/java/org/spongepowered/common/world/level/chunk/SpongeUnloadedChunkException.java b/src/main/java/org/spongepowered/common/world/level/chunk/SpongeUnloadedChunkException.java new file mode 100644 index 00000000000..9a16a17fbd6 --- /dev/null +++ b/src/main/java/org/spongepowered/common/world/level/chunk/SpongeUnloadedChunkException.java @@ -0,0 +1,33 @@ +/* + * This file is part of Sponge, licensed under the MIT License (MIT). + * + * Copyright (c) SpongePowered + * Copyright (c) contributors + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package org.spongepowered.common.world.level.chunk; + +public final class SpongeUnloadedChunkException extends Exception { + + public static final SpongeUnloadedChunkException INSTANCE = new SpongeUnloadedChunkException(); + + private SpongeUnloadedChunkException() { + } +} diff --git a/src/mixins/java/org/spongepowered/common/mixin/core/world/level/chunk/ChunkStatusMixin.java b/src/mixins/java/org/spongepowered/common/mixin/core/world/level/chunk/ChunkStatusMixin.java new file mode 100644 index 00000000000..53d6f8eb4ae --- /dev/null +++ b/src/mixins/java/org/spongepowered/common/mixin/core/world/level/chunk/ChunkStatusMixin.java @@ -0,0 +1,90 @@ +/* + * This file is part of Sponge, licensed under the MIT License (MIT). + * + * Copyright (c) SpongePowered + * Copyright (c) contributors + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package org.spongepowered.common.mixin.core.world.level.chunk; + +import com.mojang.datafixers.util.Either; +import net.minecraft.server.level.ChunkHolder; +import net.minecraft.server.level.ServerLevel; +import net.minecraft.server.level.ThreadedLevelLightEngine; +import net.minecraft.server.level.WorldGenRegion; +import net.minecraft.world.level.chunk.ChunkAccess; +import net.minecraft.world.level.chunk.ChunkGenerator; +import net.minecraft.world.level.chunk.ChunkStatus; +import net.minecraft.world.level.chunk.ProtoChunk; +import net.minecraft.world.level.levelgen.blending.Blender; +import net.minecraft.world.level.levelgen.structure.templatesystem.StructureTemplateManager; +import org.spongepowered.asm.mixin.Mixin; +import org.spongepowered.asm.mixin.Overwrite; +import org.spongepowered.common.world.level.chunk.SpongeUnloadedChunkException; + +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.Executor; +import java.util.function.Function; + +@Mixin(ChunkStatus.class) +public abstract class ChunkStatusMixin { + + /** + * @author aromaa - December 17th, 2023 - 1.19.4 + * @reason Fixes a deadlock when the world is unloading/unloaded. + * The Blender#of method calls to the IOWorker#isOldChunkAround which + * submits a task to the main thread while blocking the current thread + * to wait for a response. This fails when the IOWorker has been closed + * as it no longer responds to further work and causes the thread to block + * indefinitely. Fixes this by special casing the IOWorker#isOldChunkAround + * to throw a special exception when the IOWorker has finished up its work + * and catches it here to convert it to a ChunkLoadingFailure. + * + * See IOWorkerMixin#createOldDataForRegion + */ + @Overwrite + private static CompletableFuture> lambda$static$6( + final ChunkStatus $$0, final Executor $$1, final ServerLevel $$2, final ChunkGenerator $$3, final StructureTemplateManager $$4, + final ThreadedLevelLightEngine $$5, final Function>> $$6, + final List $$7, final ChunkAccess $$8, final boolean $$9) { + if (!$$9 && $$8.getStatus().isOrAfter($$0)) { + return CompletableFuture.completedFuture(Either.left($$8)); + } else { + final WorldGenRegion $$10 = new WorldGenRegion($$2, $$7, $$0, -1); + try { //Sponge: Add try + return $$3.createBiomes($$1, $$2.getChunkSource().randomState(), Blender.of($$10), $$2.structureManager().forWorldGenRegion($$10), $$8) + .thenApply($$1x -> { + if ($$1x instanceof ProtoChunk) { + ((ProtoChunk) $$1x).setStatus($$0); + } + + return Either.left($$1x); + }); + } catch (final Exception e) { //Sponge start: Add catch + if (e.getCause() != SpongeUnloadedChunkException.INSTANCE) { + throw e; + } + + return ChunkHolder.UNLOADED_CHUNK_FUTURE; + } //Sponge end + } + } +} diff --git a/src/mixins/java/org/spongepowered/common/mixin/core/world/level/chunk/storage/IOWorkerMixin.java b/src/mixins/java/org/spongepowered/common/mixin/core/world/level/chunk/storage/IOWorkerMixin.java index 7207b6cb68e..f053b41f2c8 100644 --- a/src/mixins/java/org/spongepowered/common/mixin/core/world/level/chunk/storage/IOWorkerMixin.java +++ b/src/mixins/java/org/spongepowered/common/mixin/core/world/level/chunk/storage/IOWorkerMixin.java @@ -24,27 +24,60 @@ */ package org.spongepowered.common.mixin.core.world.level.chunk.storage; +import com.mojang.datafixers.util.Either; +import net.minecraft.nbt.CompoundTag; +import net.minecraft.nbt.IntTag; +import net.minecraft.nbt.Tag; +import net.minecraft.nbt.visitors.CollectFields; +import net.minecraft.nbt.visitors.FieldSelector; import net.minecraft.resources.ResourceKey; +import net.minecraft.util.thread.ProcessorMailbox; +import net.minecraft.util.thread.StrictQueue; import net.minecraft.world.level.ChunkPos; import net.minecraft.world.level.Level; import net.minecraft.world.level.chunk.storage.IOWorker; +import net.minecraft.world.level.chunk.storage.RegionFileStorage; import org.checkerframework.checker.nullness.qual.MonotonicNonNull; +import org.slf4j.Logger; import org.spongepowered.api.event.SpongeEventFactory; import org.spongepowered.api.event.world.chunk.ChunkEvent; +import org.spongepowered.asm.mixin.Final; import org.spongepowered.asm.mixin.Mixin; +import org.spongepowered.asm.mixin.Overwrite; +import org.spongepowered.asm.mixin.Shadow; import org.spongepowered.asm.mixin.injection.At; import org.spongepowered.asm.mixin.injection.Coerce; import org.spongepowered.asm.mixin.injection.Inject; import org.spongepowered.asm.mixin.injection.callback.CallbackInfo; import org.spongepowered.common.SpongeCommon; +import org.spongepowered.common.accessor.world.level.chunk.storage.IOWorker$PendingStoreAccessor; import org.spongepowered.common.bridge.world.level.chunk.storage.IOWorkerBridge; import org.spongepowered.common.event.ShouldFire; import org.spongepowered.common.event.tracking.PhaseTracker; +import org.spongepowered.common.world.level.chunk.SpongeUnloadedChunkException; import org.spongepowered.math.vector.Vector3i; +import java.util.BitSet; +import java.util.Map; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Supplier; + @Mixin(IOWorker.class) public abstract class IOWorkerMixin implements IOWorkerBridge { + // @formatter:off + @Shadow @Final private static Logger LOGGER; + + @Shadow @Final private AtomicBoolean shutdownRequested; + @Shadow @Final private ProcessorMailbox mailbox; + @Shadow @Final private RegionFileStorage storage; + @Shadow @Final private Map pendingWrites; + + @Shadow protected abstract boolean shadow$isOldChunk(CompoundTag $$0); + @Shadow protected abstract void shadow$tellStorePending(); + // @formatter:on + @MonotonicNonNull private ResourceKey impl$dimension; //We only set this for chunk related IO workers @Override @@ -65,4 +98,78 @@ public abstract class IOWorkerMixin implements IOWorkerBridge { SpongeCommon.post(postSave); } } + + /** + * @author aromaa - December 17th, 2023 - 1.19.4 + * @reason Fixes a deadlock when the world is unloading/unloaded. + * This method is called from a non server threads and there is a chance + * that the IOWorker is closed before it has the chance to schedule it. + * After the IOWorker has been closed it no longer completes any further + * tasks causing the join to wait indefinitely. Fixes this by submitting + * the task directly instead of indirectly from the background executor + * and not blocking inside of it. + */ + @Overwrite + private CompletableFuture createOldDataForRegion(final int x, final int z) { + //The impl$submitTaskCancellable is related to another fix for a separate deadlock case. + //The returned future is used inside the isOldChunkAround which tries to wait for the + //completion but also ends up deadlocking when the IOWorker closes. This is fixed by + //throwing a special exception when the IOWorker is no longer accepting new tasks. + //See ChunkStatusMixin + return this.impl$submitTaskCancellable(() -> { //Sponge: Use submitTask instead + try { + final ChunkPos min = ChunkPos.minFromRegion(x, z); + final ChunkPos max = ChunkPos.maxFromRegion(x, z); + final BitSet oldDataRegions = new BitSet(); + ChunkPos.rangeClosed(min, max).forEach(pos -> { + final CollectFields fields = new CollectFields(new FieldSelector(IntTag.TYPE, "DataVersion"), new FieldSelector(CompoundTag.TYPE, "blending_data")); + + try { + //Sponge start: unwrapped the scanChunk content to not submit another task + final IOWorker$PendingStoreAccessor store = this.pendingWrites.get(pos); + if (store != null) { + if (store.accessor$data() != null) { + store.accessor$data().acceptAsRoot(fields); + } + } else { + this.storage.scanChunk(pos, fields); + } + //Sponge end + } catch (final Exception e) { + IOWorkerMixin.LOGGER.warn("Failed to scan chunk {}", pos, e); + return; + } + + final Tag result = fields.getResult(); + if (result instanceof final CompoundTag compoundTag && this.shadow$isOldChunk(compoundTag)) { + final int regionIndex = pos.getRegionLocalZ() * 32 + pos.getRegionLocalX(); + oldDataRegions.set(regionIndex); + } + }); + return Either.left(oldDataRegions); + } catch (final Exception e) { + return Either.right(e); + } + }); + } + + private CompletableFuture impl$submitTaskCancellable(final Supplier> supplier) { + final CompletableFuture future = this.mailbox.askEither(processor -> new StrictQueue.IntRunnable(0, () -> { + if (!this.shutdownRequested.get()) { + processor.tell(supplier.get()); + } else { + processor.tell(Either.right(SpongeUnloadedChunkException.INSTANCE)); //Sponge: Complete exceptionally if shutdown was requested + } + + this.shadow$tellStorePending(); + })); + + //Sponge start: Complete exceptionally if shutdown was requested + if (this.shutdownRequested.get()) { + future.completeExceptionally(SpongeUnloadedChunkException.INSTANCE); + } + //Sponge end + + return future; + } } diff --git a/src/mixins/resources/mixins.sponge.core.json b/src/mixins/resources/mixins.sponge.core.json index 42ac1dd5c52..0f88acc869c 100644 --- a/src/mixins/resources/mixins.sponge.core.json +++ b/src/mixins/resources/mixins.sponge.core.json @@ -210,6 +210,7 @@ "world.level.block.state.BlockStateMixin", "world.level.border.WorldBorderMixin", "world.level.chunk.ChunkGeneratorMixin", + "world.level.chunk.ChunkStatusMixin", "world.level.chunk.LevelChunkMixin", "world.level.chunk.storage.IOWorkerMixin", "world.level.dimension.DimensionTypeMixin",