Fix race condition when handling unknown packets. (#3508)

fabric-networking-api-v1/src/client/java/net/fabricmc/fabric/mixin/networking/client/ClientCommonNetworkHandlerMixin.java

@@ -16,7 +16,10 @@
 
 package net.fabricmc.fabric.mixin.networking.client;
 
+import org.slf4j.Logger;
+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;
@@ -32,6 +35,10 @@ import net.fabricmc.fabric.impl.networking.payload.RetainedPayload;
 
 @Mixin(ClientCommonNetworkHandler.class)
 public abstract class ClientCommonNetworkHandlerMixin implements NetworkHandlerExtensions {
+	@Shadow
+	@Final
+	private static Logger LOGGER;
+
 	@Inject(method = "onCustomPayload(Lnet/minecraft/network/packet/s2c/common/CustomPayloadS2CPacket;)V", at = @At("HEAD"), cancellable = true)
 	public void onCustomPayload(CustomPayloadS2CPacket packet, CallbackInfo ci) {
 		if (packet.payload() instanceof ResolvablePayload payload) {
@@ -45,14 +52,15 @@ public abstract class ClientCommonNetworkHandlerMixin implements NetworkHandlerE
 				throw new IllegalStateException("Unknown network addon");
 			}
 
-			if (handled) {
+			if (!handled && payload instanceof RetainedPayload retained && retained.buf().refCnt() > 0) {
-				ci.cancel();
+				// Duplicate the vanilla log message, as we cancel further processing.
-			} else if (payload instanceof RetainedPayload retained && retained.buf().refCnt() > 0) {
+				LOGGER.warn("Unknown custom packet payload: {}", payload.id());
-				// Vanilla forces to use the render thread for its payloads,
+
-				// that means this method can get called multiple times.
 				retained.buf().skipBytes(retained.buf().readableBytes());
 				retained.buf().release();
 			}
+
+			ci.cancel();
 		}
 	}
 }

fabric-networking-api-v1/src/testmod/java/net/fabricmc/fabric/test/networking/play/NetworkingPlayPacketTest.java

@@ -21,6 +21,7 @@ import static net.minecraft.server.command.CommandManager.argument;
 import static net.minecraft.server.command.CommandManager.literal;
 
 import java.util.List;
+import java.util.UUID;
 
 import com.mojang.brigadier.Command;
 import com.mojang.brigadier.CommandDispatcher;
@@ -38,6 +39,7 @@ import net.minecraft.util.Identifier;
 
 import net.fabricmc.api.ModInitializer;
 import net.fabricmc.fabric.api.command.v2.CommandRegistrationCallback;
+import net.fabricmc.fabric.api.event.lifecycle.v1.ServerTickEvents;
 import net.fabricmc.fabric.api.networking.v1.FabricPacket;
 import net.fabricmc.fabric.api.networking.v1.PacketByteBufs;
 import net.fabricmc.fabric.api.networking.v1.PacketType;
@@ -47,6 +49,7 @@ import net.fabricmc.fabric.test.networking.NetworkingTestmods;
 public final class NetworkingPlayPacketTest implements ModInitializer {
 	public static final Identifier TEST_CHANNEL = NetworkingTestmods.id("test_channel");
 	private static final Identifier UNKNOWN_TEST_CHANNEL = NetworkingTestmods.id("unknown_test_channel");
+	private static boolean spamUnknownPackets = false;
 
 	public static void sendToTestChannel(ServerPlayerEntity player, String stuff) {
 		ServerPlayNetworking.getSender(player).sendPacket(new OverlayPacket(Text.literal(stuff)), future -> {
@@ -55,7 +58,13 @@ public final class NetworkingPlayPacketTest implements ModInitializer {
 	}
 
 	private static void sendToUnknownChannel(ServerPlayerEntity player) {
-		ServerPlayNetworking.getSender(player).sendPacket(UNKNOWN_TEST_CHANNEL, PacketByteBufs.create());
+		PacketByteBuf buf = PacketByteBufs.create();
+
+		for (int i = 0; i < 20; i++) {
+			buf.writeUuid(UUID.randomUUID());
+		}
+
+		ServerPlayNetworking.getSender(player).sendPacket(UNKNOWN_TEST_CHANNEL, buf);
 	}
 
 	public static void registerCommand(CommandDispatcher<ServerCommandSource> dispatcher) {
@@ -71,6 +80,11 @@ public final class NetworkingPlayPacketTest implements ModInitializer {
 					sendToUnknownChannel(ctx.getSource().getPlayer());
 					return Command.SINGLE_SUCCESS;
 				}))
+				.then(literal("spamUnknown").executes(ctx -> {
+					spamUnknownPackets = true;
+					ctx.getSource().sendMessage(Text.literal("Spamming unknown packets state:" + spamUnknownPackets));
+					return Command.SINGLE_SUCCESS;
+				}))
 				.then(literal("bufctor").executes(ctx -> {
 					PacketByteBuf buf = PacketByteBufs.create();
 					buf.writeIdentifier(TEST_CHANNEL);
@@ -106,6 +120,19 @@ public final class NetworkingPlayPacketTest implements ModInitializer {
 		CommandRegistrationCallback.EVENT.register((dispatcher, registryAccess, environment) -> {
 			NetworkingPlayPacketTest.registerCommand(dispatcher);
 		});
+
+		ServerTickEvents.START_SERVER_TICK.register(server -> {
+			if (!spamUnknownPackets) {
+				return;
+			}
+
+			// Send many unknown packets, used to debug https://github.com/FabricMC/fabric/issues/3505
+			for (ServerPlayerEntity player : server.getPlayerManager().getPlayerList()) {
+				for (int i = 0; i < 50; i++) {
+					sendToUnknownChannel(player);
+				}
+			}
+		});
 	}
 
 	public record OverlayPacket(Text message) implements FabricPacket {