Fix #1912 and #1972: inventory updates in transactions (#1979)

checkstyle.xml

@@ -203,5 +203,10 @@
 		<module name="AtclauseOrder">
 			<property name="tagOrder" value="@param,@return,@throws,@deprecated"/>
 		</module>
+
+        <!-- Prevent var for all cases other than new instance creation -->
+        <module name="MatchXpath">
+            <property name="query" value="//VARIABLE_DEF[./TYPE/IDENT[@text='var'] and not(./ASSIGN/EXPR/LITERAL_NEW)]"/>
+        </module>
 	</module>
 </module>

fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/TransferApiImpl.java

@@ -62,4 +62,9 @@ public class TransferApiImpl {
 			return 0;
 		}
 	};
+
+	/**
+	 * Not null when writing to an inventory in a transaction, null otherwise.
+	 */
+	public static final ThreadLocal<Object> SUPPRESS_SPECIAL_LOGIC = new ThreadLocal<>();
 }

fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/item/InventorySlotWrapper.java

@@ -21,6 +21,7 @@ import net.minecraft.item.ItemStack;
 import net.fabricmc.fabric.api.transfer.v1.item.base.SingleStackStorage;
 import net.fabricmc.fabric.api.transfer.v1.item.ItemVariant;
 import net.fabricmc.fabric.api.transfer.v1.transaction.TransactionContext;
+import net.fabricmc.fabric.impl.transfer.TransferApiImpl;
 
 /**
  * A wrapper around a single slot of an inventory.
@@ -48,7 +49,13 @@ class InventorySlotWrapper extends SingleStackStorage {
 
 	@Override
 	protected void setStack(ItemStack stack) {
-		storage.inventory.setStack(slot, stack);
+		TransferApiImpl.SUPPRESS_SPECIAL_LOGIC.set(Boolean.TRUE);
+
+		try {
+			storage.inventory.setStack(slot, stack);
+		} finally {
+			TransferApiImpl.SUPPRESS_SPECIAL_LOGIC.remove();
+		}
 	}
 
 	@Override
@@ -79,6 +86,10 @@ class InventorySlotWrapper extends SingleStackStorage {
 		ItemStack original = lastReleasedSnapshot;
 		ItemStack currentStack = getStack();
 
+		if (storage.inventory instanceof SpecialLogicInventory specialLogicInv) {
+			specialLogicInv.fabric_onFinalCommit(slot, original, currentStack);
+		}
+
 		if (!original.isEmpty() && original.getItem() == currentStack.getItem()) {
 			// None is empty and the items match: just update the amount and NBT, and reuse the original stack.
 			original.setCount(currentStack.getCount());

fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/item/SpecialLogicInventory.java

@@ -0,0 +1,30 @@
+/*
+ * Copyright (c) 2016, 2017, 2018, 2019 FabricMC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package net.fabricmc.fabric.impl.transfer.item;
+
+import org.jetbrains.annotations.ApiStatus;
+
+import net.minecraft.item.ItemStack;
+
+/**
+ * Internal class that allows inventory instances to defer special logic until {@link InventorySlotWrapper#onFinalCommit()} is called.
+ * Special logic should be suppressed when {@link net.fabricmc.fabric.impl.transfer.TransferApiImpl#SUPPRESS_SPECIAL_LOGIC} is true.
+ */
+@ApiStatus.Internal
+public interface SpecialLogicInventory {
+	void fabric_onFinalCommit(int slot, ItemStack oldStack, ItemStack newStack);
+}

fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/mixin/transfer/AbstractFurnaceBlockEntityMixin.java

@@ -0,0 +1,89 @@
+/*
+ * Copyright (c) 2016, 2017, 2018, 2019 FabricMC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package net.fabricmc.fabric.mixin.transfer;
+
+import org.spongepowered.asm.mixin.Final;
+import org.spongepowered.asm.mixin.Mixin;
+import org.spongepowered.asm.mixin.Shadow;
+import org.spongepowered.asm.mixin.injection.At;
+import org.spongepowered.asm.mixin.injection.Inject;
+import org.spongepowered.asm.mixin.injection.callback.CallbackInfo;
+
+import net.minecraft.block.BlockState;
+import net.minecraft.block.entity.AbstractFurnaceBlockEntity;
+import net.minecraft.block.entity.BlockEntityType;
+import net.minecraft.block.entity.LockableContainerBlockEntity;
+import net.minecraft.inventory.Inventory;
+import net.minecraft.item.ItemStack;
+import net.minecraft.recipe.AbstractCookingRecipe;
+import net.minecraft.recipe.RecipeType;
+import net.minecraft.util.collection.DefaultedList;
+import net.minecraft.util.math.BlockPos;
+import net.minecraft.world.World;
+
+import net.fabricmc.fabric.impl.transfer.TransferApiImpl;
+import net.fabricmc.fabric.impl.transfer.item.SpecialLogicInventory;
+
+/**
+ * Defer cook time updates for furnaces, so that aborted transactions don't reset the cook time.
+ */
+@Mixin(AbstractFurnaceBlockEntity.class)
+public abstract class AbstractFurnaceBlockEntityMixin extends LockableContainerBlockEntity implements SpecialLogicInventory {
+	@Shadow
+	protected DefaultedList<ItemStack> inventory;
+	@Shadow
+	int cookTime;
+	@Shadow
+	int cookTimeTotal;
+	@Final
+	@Shadow
+	private RecipeType<? extends AbstractCookingRecipe> recipeType;
+
+	protected AbstractFurnaceBlockEntityMixin(BlockEntityType<?> blockEntityType, BlockPos blockPos, BlockState blockState) {
+		super(blockEntityType, blockPos, blockState);
+		throw new AssertionError();
+	}
+
+	@Inject(at = @At("HEAD"), method = "setStack", cancellable = true)
+	public void setStackSuppressUpdate(int slot, ItemStack stack, CallbackInfo ci) {
+		if (TransferApiImpl.SUPPRESS_SPECIAL_LOGIC.get() != null) {
+			inventory.set(slot, stack);
+			ci.cancel();
+		}
+	}
+
+	@Override
+	public void fabric_onFinalCommit(int slot, ItemStack oldStack, ItemStack newStack) {
+		if (slot == 0) {
+			ItemStack itemStack = oldStack;
+			ItemStack stack = newStack;
+
+			// Update cook time if needed. Code taken from AbstractFurnaceBlockEntity#setStack.
+			boolean bl = !stack.isEmpty() && stack.isItemEqualIgnoreDamage(itemStack) && ItemStack.areNbtEqual(stack, itemStack);
+
+			if (!bl) {
+				this.cookTimeTotal = getCookTime(this.world, this.recipeType, this);
+				this.cookTime = 0;
+			}
+		}
+	}
+
+	@Shadow
+	private static int getCookTime(World world, RecipeType<? extends AbstractCookingRecipe> recipeType, Inventory inventory) {
+		throw new AssertionError();
+	}
+}

fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/mixin/transfer/LootableContainerBlockEntityMixin.java

@@ -0,0 +1,41 @@
+/*
+ * Copyright (c) 2016, 2017, 2018, 2019 FabricMC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package net.fabricmc.fabric.mixin.transfer;
+
+import org.spongepowered.asm.mixin.Mixin;
+import org.spongepowered.asm.mixin.injection.At;
+import org.spongepowered.asm.mixin.injection.Redirect;
+
+import net.minecraft.block.entity.LootableContainerBlockEntity;
+
+import net.fabricmc.fabric.impl.transfer.TransferApiImpl;
+
+/**
+ * Defer markDirty until the outer transaction close callback when setStack is called from an inventory wrapper.
+ */
+@Mixin(LootableContainerBlockEntity.class)
+public class LootableContainerBlockEntityMixin {
+	@Redirect(
+			at = @At(value = "INVOKE", target = "Lnet/minecraft/block/entity/LootableContainerBlockEntity;markDirty()V"),
+			method = "setStack(ILnet/minecraft/item/ItemStack;)V"
+	)
+	public void fabric_redirectMarkDirty(LootableContainerBlockEntity self) {
+		if (TransferApiImpl.SUPPRESS_SPECIAL_LOGIC.get() == null) {
+			self.markDirty();
+		}
+	}
+}

fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/mixin/transfer/SimpleInventoryMixin.java

@@ -0,0 +1,41 @@
+/*
+ * Copyright (c) 2016, 2017, 2018, 2019 FabricMC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package net.fabricmc.fabric.mixin.transfer;
+
+import org.spongepowered.asm.mixin.Mixin;
+import org.spongepowered.asm.mixin.injection.At;
+import org.spongepowered.asm.mixin.injection.Redirect;
+
+import net.minecraft.inventory.SimpleInventory;
+
+import net.fabricmc.fabric.impl.transfer.TransferApiImpl;
+
+/**
+ * Defer markDirty until the outer transaction close callback when setStack is called from an inventory wrapper.
+ */
+@Mixin(SimpleInventory.class)
+public class SimpleInventoryMixin {
+	@Redirect(
+			at = @At(value = "INVOKE", target = "Lnet/minecraft/inventory/SimpleInventory;markDirty()V"),
+			method = "setStack(ILnet/minecraft/item/ItemStack;)V"
+	)
+	public void fabric_redirectMarkDirty(SimpleInventory self) {
+		if (TransferApiImpl.SUPPRESS_SPECIAL_LOGIC.get() == null) {
+			self.markDirty();
+		}
+	}
+}

fabric-transfer-api-v1/src/main/resources/fabric-transfer-api-v1.mixins.json

@@ -3,11 +3,14 @@
   "package": "net.fabricmc.fabric.mixin.transfer",
   "compatibilityLevel": "JAVA_16",
   "mixins": [
+    "AbstractFurnaceBlockEntityMixin",
     "BucketItemAccessor",
     "DoubleInventoryAccessor",
     "DropperBlockMixin",
     "FluidMixin",
     "HopperBlockEntityMixin",
-    "ItemMixin"
+    "ItemMixin",
+    "LootableContainerBlockEntityMixin",
+    "SimpleInventoryMixin"
   ]
 }

fabric-transfer-api-v1/src/testmod/java/net/fabricmc/fabric/test/transfer/gametests/VanillaStorageTests.java

@@ -0,0 +1,122 @@
+/*
+ * Copyright (c) 2016, 2017, 2018, 2019 FabricMC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package net.fabricmc.fabric.test.transfer.gametests;
+
+import net.minecraft.block.Blocks;
+import net.minecraft.block.ComparatorBlock;
+import net.minecraft.block.entity.ChestBlockEntity;
+import net.minecraft.block.entity.FurnaceBlockEntity;
+import net.minecraft.item.ItemStack;
+import net.minecraft.item.Items;
+import net.minecraft.test.GameTest;
+import net.minecraft.test.GameTestException;
+import net.minecraft.test.TestContext;
+import net.minecraft.util.math.BlockPos;
+import net.minecraft.util.math.Direction;
+import net.minecraft.world.World;
+
+import net.fabricmc.fabric.api.gametest.v1.FabricGameTest;
+import net.fabricmc.fabric.api.transfer.v1.item.InventoryStorage;
+import net.fabricmc.fabric.api.transfer.v1.item.ItemVariant;
+import net.fabricmc.fabric.api.transfer.v1.transaction.Transaction;
+import net.fabricmc.fabric.test.transfer.mixin.AbstractFurnaceBlockEntityAccessor;
+
+public class VanillaStorageTests {
+	/**
+	 * Regression test for https://github.com/FabricMC/fabric/issues/1972.
+	 * Ensures that furnace cook time is only reset when extraction is actually committed.
+	 */
+	@GameTest(structureName = FabricGameTest.EMPTY_STRUCTURE)
+	public void testFurnaceCookTime(TestContext context) {
+		BlockPos pos = new BlockPos(0, 1, 0);
+		context.setBlockState(pos, Blocks.FURNACE.getDefaultState());
+		FurnaceBlockEntity furnace = (FurnaceBlockEntity) context.getBlockEntity(pos);
+		AbstractFurnaceBlockEntityAccessor accessor = (AbstractFurnaceBlockEntityAccessor) furnace;
+
+		ItemVariant rawIron = ItemVariant.of(Items.RAW_IRON);
+		furnace.setStack(0, rawIron.toStack(64));
+		furnace.setStack(1, new ItemStack(Items.COAL, 64));
+		InventoryStorage furnaceWrapper = InventoryStorage.of(furnace, null);
+
+		context.runAtTick(5, () -> {
+			if (accessor.getCookTime() <= 0) {
+				throw new GameTestException("Furnace should have started cooking.");
+			}
+
+			try (Transaction transaction = Transaction.openOuter()) {
+				if (furnaceWrapper.extract(rawIron, 64, transaction) != 64) {
+					throw new GameTestException("Failed to extract 64 raw iron.");
+				}
+			}
+
+			if (accessor.getCookTime() <= 0) {
+				throw new GameTestException("Furnace should still cook after simulation.");
+			}
+
+			try (Transaction transaction = Transaction.openOuter()) {
+				if (furnaceWrapper.extract(rawIron, 64, transaction) != 64) {
+					throw new GameTestException("Failed to extract 64 raw iron.");
+				}
+
+				transaction.commit();
+			}
+
+			if (accessor.getCookTime() != 0) {
+				throw new GameTestException("Furnace should have reset cook time after being emptied.");
+			}
+
+			context.complete();
+		});
+	}
+
+	/**
+	 * Tests that containers such as chests don't update adjacent comparators until the very end of a committed transaction.
+	 */
+	@GameTest(structureName = FabricGameTest.EMPTY_STRUCTURE)
+	public void testChestComparator(TestContext context) {
+		World world = context.getWorld();
+
+		BlockPos pos = new BlockPos(0, 2, 0);
+		context.setBlockState(pos, Blocks.CHEST.getDefaultState());
+		ChestBlockEntity chest = (ChestBlockEntity) context.getBlockEntity(pos);
+		InventoryStorage storage = InventoryStorage.of(chest, null);
+
+		BlockPos comparatorPos = new BlockPos(1, 2, 0);
+		// support block under the comparator
+		context.setBlockState(comparatorPos.offset(Direction.DOWN), Blocks.GREEN_WOOL.getDefaultState());
+		// comparator
+		context.setBlockState(comparatorPos, Blocks.COMPARATOR.getDefaultState().with(ComparatorBlock.FACING, Direction.WEST));
+
+		try (Transaction transaction = Transaction.openOuter()) {
+			storage.insert(ItemVariant.of(Items.DIAMOND), 1000000, transaction);
+
+			// uncommitted insert should not schedule an update
+			if (world.getBlockTickScheduler().isQueued(context.getAbsolutePos(comparatorPos), Blocks.COMPARATOR)) {
+				throw new GameTestException("Comparator should not have a tick scheduled.");
+			}
+
+			transaction.commit();
+
+			// committed insert should schedule an update
+			if (!world.getBlockTickScheduler().isQueued(context.getAbsolutePos(comparatorPos), Blocks.COMPARATOR)) {
+				throw new GameTestException("Comparator should have a tick scheduled.");
+			}
+		}
+
+		context.complete();
+	}
+}

fabric-transfer-api-v1/src/testmod/java/net/fabricmc/fabric/test/transfer/mixin/AbstractFurnaceBlockEntityAccessor.java

@@ -0,0 +1,28 @@
+/*
+ * Copyright (c) 2016, 2017, 2018, 2019 FabricMC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package net.fabricmc.fabric.test.transfer.mixin;
+
+import org.spongepowered.asm.mixin.Mixin;
+import org.spongepowered.asm.mixin.gen.Accessor;
+
+import net.minecraft.block.entity.AbstractFurnaceBlockEntity;
+
+@Mixin(AbstractFurnaceBlockEntity.class)
+public interface AbstractFurnaceBlockEntityAccessor {
+	@Accessor
+	int getCookTime();
+}

fabric-transfer-api-v1/src/testmod/java/net/fabricmc/fabric/test/transfer/unittests/ItemTests.java

@@ -45,6 +45,7 @@ class ItemTests {
 		testInventoryWrappers();
 		testLimitedStackCountInventory();
 		testLimitedStackCountItem();
+		testSimpleInventoryUpdates();
 	}
 
 	private static void testStackReference() {
@@ -230,4 +231,42 @@ class ItemTests {
 			throw new AssertionError(error);
 		}
 	}
+
+	/**
+	 * Ensure that SimpleInventory only calls markDirty at the end of a successful transaction.
+	 */
+	private static void testSimpleInventoryUpdates() {
+		var simpleInventory = new SimpleInventory(2) {
+			boolean throwOnMarkDirty = true;
+			boolean markDirtyCalled = false;
+
+			@Override
+			public void markDirty() {
+				if (throwOnMarkDirty) {
+					throw new AssertionError("Unexpected markDirty call!");
+				}
+
+				markDirtyCalled = true;
+			}
+		};
+		InventoryStorage wrapper = InventoryStorage.of(simpleInventory, null);
+		ItemVariant diamond = ItemVariant.of(Items.DIAMOND);
+
+		// Simulation should not trigger notifications.
+		try (Transaction tx = Transaction.openOuter()) {
+			wrapper.insert(diamond, 1000, tx);
+		}
+
+		// But commit after modification should.
+		try (Transaction tx = Transaction.openOuter()) {
+			wrapper.insert(diamond, 1000, tx);
+
+			simpleInventory.throwOnMarkDirty = false;
+			tx.commit();
+		}
+
+		if (!simpleInventory.markDirtyCalled) {
+			throw new AssertionError("markDirty should have been called when committing.");
+		}
+	}
 }

fabric-transfer-api-v1/src/testmod/resources/fabric-transfer-api-v1-testmod.mixins.json

@@ -0,0 +1,8 @@
+{
+  "required": true,
+  "package": "net.fabricmc.fabric.test.transfer.mixin",
+  "compatibilityLevel": "JAVA_8",
+  "mixins": [
+    "AbstractFurnaceBlockEntityAccessor"
+  ]
+}

fabric-transfer-api-v1/src/testmod/resources/fabric.mod.json

@@ -15,6 +15,12 @@
     ],
     "client": [
       "net.fabricmc.fabric.test.transfer.ingame.client.FluidVariantRenderTest"
+    ],
+    "fabric-gametest": [
+      "net.fabricmc.fabric.test.transfer.gametests.VanillaStorageTests"
     ]
-  }
+  },
+  "mixins": [
+    "fabric-transfer-api-v1-testmod.mixins.json"
+  ]
 }