Move event down a little

While this disrupts signed command handling a bit, general behavior stays the same, and most importantly chat can continue to be handled off-main. It is necessary for the message decoding to happen in order, which previously was not guaranteed with the method call being in the netty event loop for chat and on main for signed commands.

build-data/paper.at

@@ -122,7 +122,6 @@ public net.minecraft.tags.TagEntry id
 public net.minecraft.tags.TagEntry required
 public net.minecraft.tags.TagEntry tag
 public net.minecraft.util.datafix.fixes.BlockStateData register(ILcom/mojang/serialization/Dynamic;[Lcom/mojang/serialization/Dynamic;)V
-public net.minecraft.util.datafix.fixes.EntityCustomNameToComponentFix fixCustomName(Lcom/mojang/serialization/DynamicOps;Ljava/lang/String;Ljava/lang/String;)Lcom/mojang/serialization/Dynamic;
 public net.minecraft.util.datafix.fixes.ItemIdFix ITEM_NAMES
 public net.minecraft.util.datafix.fixes.ItemSpawnEggFix ID_TO_ENTITY
 public net.minecraft.world.BossEvent color

paper-server/patches/sources/net/minecraft/server/network/ServerGamePacketListenerImpl.java.patch

@@ -1395,7 +1395,7 @@
              throw new IllegalArgumentException("Expected packet sequence nr >= 0");
          } else {
              this.ackBlockChangesUpTo = Math.max(sequence, this.ackBlockChangesUpTo);
-@@ -1356,20 +_,38 @@
+@@ -1356,23 +_,37 @@
      @Override
      public void handleSetCarriedItem(ServerboundSetCarriedItemPacket packet) {
          PacketUtils.ensureRunningOnSameThread(packet, this, this.player.serverLevel());
@@ -1425,16 +1425,15 @@
  
      @Override
      public void handleChat(ServerboundChatPacket packet) {
-+        // CraftBukkit start - async chat
-+        // SPIGOT-3638
-+        if (this.server.isStopped()) {
-+            return;
-+        }
-+        // CraftBukkit end
++        if (this.server.isStopped()) return; // CraftBukkit - SPIGOT-3638
          Optional<LastSeenMessages> optional = this.unpackAndApplyLastSeen(packet.lastSeenMessages());
          if (!optional.isEmpty()) {
              this.tryHandleChat(packet.message(), () -> {
-@@ -1381,25 +_,45 @@
++                if (this.player.hasDisconnected()) return; // CraftBukkit
+                 PlayerChatMessage signedMessage;
+                 try {
+                     signedMessage = this.getSignedMessage(packet, optional.get());
+@@ -1381,10 +_,14 @@
                      return;
                  }
  
@@ -1442,31 +1441,30 @@
 -                Component component = this.server.getChatDecorator().decorate(this.player, signedMessage.decoratedContent());
 -                this.chatMessageChain.append(completableFuture, filteredText -> {
 -                    PlayerChatMessage playerChatMessage = signedMessage.withUnsignedContent(component).filter(filteredText.mask());
-+                CompletableFuture<FilteredText> completableFuture = this.filterTextPacket(signedMessage.signedContent()).thenApplyAsync(java.util.function.Function.identity(), this.server.chatExecutor); // CraftBukkit - async chat
-+                CompletableFuture<Component> componentFuture = this.server.getChatDecorator().decorate(this.player, null, signedMessage.decoratedContent()); // Paper - Adventure
++                // Paper start - Adventure
++                // Make decoration async too, just message decoding has to stay in order, unfortunately blocked by commands
++                CompletableFuture<FilteredText> completableFuture = this.filterTextPacket(signedMessage.signedContent()).thenApplyAsync(java.util.function.Function.identity(), this.server.chatExecutor);
++                CompletableFuture<Component> componentFuture = this.server.getChatDecorator().decorate(this.player, null, signedMessage.decoratedContent());
 +
-+                this.chatMessageChain.append(CompletableFuture.allOf(completableFuture, componentFuture), ($) -> { // Paper - Adventure
-+                    PlayerChatMessage playerChatMessage = signedMessage.withUnsignedContent(componentFuture.join()).filter(completableFuture.join().mask()); // Paper - Adventure
++                this.chatMessageChain.append(CompletableFuture.allOf(completableFuture, componentFuture), ($) -> {
++                    PlayerChatMessage playerChatMessage = signedMessage.withUnsignedContent(componentFuture.join()).filter(completableFuture.join().mask());
++                    // Paper end - Adventure
                      this.broadcastChatMessage(playerChatMessage);
                  });
--            });
-+            }, false); // CraftBukkit - async chat
-         }
-     }
- 
+             });
+@@ -1394,12 +_,31 @@
      @Override
      public void handleChatCommand(ServerboundChatCommandPacket packet) {
          this.tryHandleChat(packet.command(), () -> {
 +            // CraftBukkit start - SPIGOT-7346: Prevent disconnected players from executing commands
-+            if (this.player.hasDisconnected()) {
++            if (this.player.hasDisconnected() || this.player.isRemoved()) {
 +                return;
 +            }
 +            // CraftBukkit end
              this.performUnsignedChatCommand(packet.command());
 -            this.detectRateSpam();
--        });
 +            this.detectRateSpam("/" + packet.command()); // Spigot
-+        }, true); // CraftBukkit - sync commands
+         });
      }
  
      private void performUnsignedChatCommand(String command) {
@@ -1478,101 +1476,79 @@
 +
 +        PlayerCommandPreprocessEvent event = new PlayerCommandPreprocessEvent(this.getCraftPlayer(), prefixedCommand, new org.bukkit.craftbukkit.util.LazyPlayerSet(this.server));
 +        this.cserver.getPluginManager().callEvent(event);
-+
 +        if (event.isCancelled()) {
 +            return;
 +        }
++
 +        command = event.getMessage().substring(1);
 +        // CraftBukkit end
          ParseResults<CommandSourceStack> parseResults = this.parseCommand(command);
          if (this.server.enforceSecureProfile() && SignableCommand.hasSignableArguments(parseResults)) {
              LOGGER.error(
-@@ -1416,28 +_,57 @@
+@@ -1416,13 +_,21 @@
          Optional<LastSeenMessages> optional = this.unpackAndApplyLastSeen(packet.lastSeenMessages());
          if (!optional.isEmpty()) {
              this.tryHandleChat(packet.command(), () -> {
-+                // CraftBukkit start - SPIGOT-7346: Prevent disconnected players from executing commands
-+                if (this.player.hasDisconnected()) {
-+                    return;
-+                }
-+                // CraftBukkit end
++                if (this.player.hasDisconnected()) return; // CraftBukkit - SPIGOT-7346: Prevent disconnected players from executing commands
                  this.performSignedChatCommand(packet, optional.get());
 -                this.detectRateSpam();
--            });
 +                this.detectRateSpam("/" + packet.command()); // Spigot
-+            }, true); // CraftBukkit - sync commands
+             });
          }
      }
  
      private void performSignedChatCommand(ServerboundChatCommandSignedPacket packet, LastSeenMessages lastSeenMessages) {
-+        // CraftBukkit start
-+        String command = "/" + packet.command();
-+        if (org.spigotmc.SpigotConfig.logCommands) { // Paper - Add missing SpigotConfig logCommands check
-+            LOGGER.info("{} issued server command: {}", this.player.getScoreboardName(), command);
-+        } // Paper - Add missing SpigotConfig logCommands check
-+
-+        PlayerCommandPreprocessEvent event = new PlayerCommandPreprocessEvent(this.getCraftPlayer(), command, new org.bukkit.craftbukkit.util.LazyPlayerSet(this.server));
-+        this.cserver.getPluginManager().callEvent(event);
-+        command = event.getMessage().substring(1);
++        // Paper start
++        final String slashCommand = "/" + packet.command();
++        if (org.spigotmc.SpigotConfig.logCommands) {
++            LOGGER.info("{} issued server command: {}", this.player.getScoreboardName(), slashCommand);
++        }
++        // Paper end
 +
          ParseResults<CommandSourceStack> parseResults = this.parseCommand(packet.command());
  
          Map<String, PlayerChatMessage> map;
-         try {
-+            // Paper - Always parse the original command to add to the chat chain
-             map = this.collectSignedArguments(packet, SignableCommand.of(parseResults), lastSeenMessages);
-         } catch (SignedMessageChain.DecodeException var6) {
-             this.handleMessageDecodeFailure(var6);
+@@ -1433,11 +_,30 @@
              return;
          }
  
-+        // Paper start - Fix cancellation and message changing
-+        if (event.isCancelled()) {
-+            // Only now are we actually good to return
++        // Paper start
++        // Parsing commands to be on main :cryingandshaking: - if that weren't the case, both this and chat could be pushed off to the chat message chain immediately
++        // Always decode message arguments before cancelling, else the chat chain will be broken
++        if (this.player.isRemoved()) {
 +            return;
 +        }
 +
-+        // Remove signed parts if the command was changed
-+        if (!command.equals(packet.command())) {
-+            parseResults = this.parseCommand(command);
++        final org.bukkit.event.player.PlayerCommandPreprocessEvent event = new org.bukkit.event.player.PlayerCommandPreprocessEvent(this.getCraftPlayer(), slashCommand, new org.bukkit.craftbukkit.util.LazyPlayerSet(this.server));
++        if (!event.callEvent()) {
++            return;
++        }
++
++        final String updatedCommand = event.getMessage().substring(1);
++        if (!updatedCommand.equals(packet.command())) {
++            // Remove signed parts if the command was changed
++            parseResults = this.parseCommand(updatedCommand);
 +            map = Collections.emptyMap();
 +        }
-+        // Paper end - Fix cancellation and message changing
-+
++        // Paper end
          CommandSigningContext commandSigningContext = new CommandSigningContext.SignedArguments(map);
          parseResults = Commands.mapSource(
              parseResults, commandSourceStack -> commandSourceStack.withSigningContext(commandSigningContext, this.chatMessageChain)
          );
 -        this.server.getCommands().performCommand(parseResults, packet.command());
-+        this.server.getCommands().performCommand(parseResults, command); // CraftBukkit
++        this.server.getCommands().performCommand(parseResults, updatedCommand); // Paper
      }
  
      private void handleMessageDecodeFailure(SignedMessageChain.DecodeException exception) {
-@@ -1501,14 +_,20 @@
-         return dispatcher.parse(command, this.player.createCommandSourceStack());
-     }
+@@ -1503,7 +_,7 @@
  
--    private void tryHandleChat(String message, Runnable handler) {
-+    private void tryHandleChat(String message, Runnable handler, boolean sync) { // CraftBukkit
+     private void tryHandleChat(String message, Runnable handler) {
          if (isChatMessageIllegal(message)) {
 -            this.disconnect(Component.translatable("multiplayer.disconnect.illegal_characters"));
--        } else if (this.player.getChatVisibility() == ChatVisiblity.HIDDEN) {
 +            this.disconnectAsync(Component.translatable("multiplayer.disconnect.illegal_characters"), org.bukkit.event.player.PlayerKickEvent.Cause.ILLEGAL_CHARACTERS); // Paper - add proper async disconnect
-+        } else if (this.player.isRemoved() || this.player.getChatVisibility() == ChatVisiblity.HIDDEN) { // CraftBukkit - dead men tell no tales
+         } else if (this.player.getChatVisibility() == ChatVisiblity.HIDDEN) {
              this.send(new ClientboundSystemChatPacket(Component.translatable("chat.disabled.options").withStyle(ChatFormatting.RED), false));
          } else {
-             this.player.resetLastActionTime();
--            this.server.execute(handler);
-+            // CraftBukkit start
-+            if (sync) {
-+                this.server.execute(handler);
-+            } else {
-+                handler.run();
-+            }
-+            // CraftBukkit end
-         }
-     }
- 
 @@ -1520,7 +_,7 @@
                  var10000 = Optional.of(lastSeenMessages);
              } catch (LastSeenMessagesValidator.ValidationException var5) {
@@ -2607,7 +2583,7 @@
          this.chatMessageChain
              .append(
                  () -> {
-+                    server.executeBlocking(() -> { // Paper - Broadcast chat session update sync
++                    server.executeBlocking(() -> { // Paper - Broadcast chat session update sync, the used broadcast method isn't thread safe
                      this.player.setChatSession(chatSession);
                      this.server
                          .getPlayerList()

paper-server/patches/sources/net/minecraft/util/datafix/DataFixers.java.patch

@@ -1,16 +1,22 @@
 --- a/net/minecraft/util/datafix/DataFixers.java
 +++ b/net/minecraft/util/datafix/DataFixers.java
-@@ -541,6 +_,18 @@
+@@ -541,6 +_,24 @@
          Schema schema44 = builder.addSchema(1456, SAME_NAMESPACED);
          builder.addFixer(new EntityItemFrameDirectionFix(schema44, false));
          Schema schema45 = builder.addSchema(1458, V1458::new);
 +        // CraftBukkit start
-+        builder.addFixer(new com.mojang.datafixers.DataFix(schema45, false) {
++        // API allows setting player custom names, so we need to convert them.
++        // This does *not* handle upgrades in any other version, but generally those shouldn't need conversion.
++        builder.addFixer(new DataFix(schema45, false) {
 +            @Override
-+            protected com.mojang.datafixers.TypeRewriteRule makeRule() {
-+                return this.fixTypeEverywhereTyped("Player CustomName", this.getInputSchema().getType(References.PLAYER), (typed) -> {
-+                    return typed.update(DSL.remainderFinder(), (dynamic) -> {
-+                        return EntityCustomNameToComponentFix.fixCustomName(dynamic.getOps(), dynamic.get("CustomName").asString(""), "minecraft:player");
++            protected TypeRewriteRule makeRule() {
++                return this.fixTypeEverywhereTyped("Player CustomName", this.getInputSchema().getType(References.PLAYER), typed -> {
++                    return typed.update(DSL.remainderFinder(), dynamic -> {
++                        final String customName = dynamic.get("CustomName").asString("");
++                        if (customName.isEmpty()) {
++                            return dynamic.remove("CustomName");
++                        }
++                        return dynamic.set("CustomName", LegacyComponentDataFixUtils.createPlainTextComponent(dynamic.getOps(), customName));
 +                    });
 +                });
 +            }