Skip to content

Commit

Permalink
Rework async-commands fix for all Paper version
Browse files Browse the repository at this point in the history 
Use some sneaky reflection to intercept `net.minecraft.server.CommandDispatcher`'s task scheduling and enforce our own read-write access

Updates #501
Fixes #494 and #503
  • Loading branch information
@willkroboth
willkroboth committed Dec 19, 2023
1 parent 1782863 commit c457c8c
Show file tree
Hide file tree
Showing 20 changed files with 845 additions and 77 deletions.
148 changes: 76 additions & 72 deletions ...pi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/CommandAPIBukkit.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -472,48 +472,50 @@ public void preCommandRegistration(String commandName) {
@Override
public void postCommandRegistration(RegisteredCommand registeredCommand, LiteralCommandNode<Source> resultantNode, List<LiteralCommandNode<Source>> aliasNodes) {
if(!CommandAPI.canRegister()) {
// Usually, when registering commands during server startup, we can just put our commands into the
// `net.minecraft.server.MinecraftServer#vanillaCommandDispatcher` and leave it. As the server finishes setup,
// it and the CommandAPI do some extra stuff to make everything work, and we move on.
// So, if we want to register commands while the server is running, we need to do all that extra stuff, and
// that is what this code does.
// We could probably call all those methods to sync everything up, but in the spirit of avoiding side effects
// and avoiding doing things twice for existing commands, this is a distilled version of those methods.

CommandMap map = paper.getCommandMap();
String permNode = unpackInternalPermissionNodeString(registeredCommand.permission());
RootCommandNode<Source> root = getResourcesDispatcher().getRoot();

// Wrapping Brigadier nodes into VanillaCommandWrappers and putting them in the CommandMap usually happens
// in `CraftServer#setVanillaCommands`
Command command = wrapToVanillaCommandWrapper(resultantNode);
map.register("minecraft", command);

// Adding permissions to these Commands usually happens in `CommandAPIBukkit#onEnable`
command.setPermission(permNode);

// Adding commands to the other (Why bukkit/spigot?!) dispatcher usually happens in `CraftServer#syncCommands`
root.addChild(resultantNode);
root.addChild(namespaceNode(resultantNode));

// Do the same for the aliases
for(LiteralCommandNode<Source> node: aliasNodes) {
command = wrapToVanillaCommandWrapper(node);
paper.modifyCommandTreesAndAvoidPaperCME(() -> {
// Usually, when registering commands during server startup, we can just put our commands into the
// `net.minecraft.server.MinecraftServer#vanillaCommandDispatcher` and leave it. As the server finishes setup,
// it and the CommandAPI do some extra stuff to make everything work, and we move on.
// So, if we want to register commands while the server is running, we need to do all that extra stuff, and
// that is what this code does.
// We could probably call all those methods to sync everything up, but in the spirit of avoiding side effects
// and avoiding doing things twice for existing commands, this is a distilled version of those methods.

CommandMap map = paper.getCommandMap();
String permNode = unpackInternalPermissionNodeString(registeredCommand.permission());
RootCommandNode<Source> root = getResourcesDispatcher().getRoot();

// Wrapping Brigadier nodes into VanillaCommandWrappers and putting them in the CommandMap usually happens
// in `CraftServer#setVanillaCommands`
Command command = wrapToVanillaCommandWrapper(resultantNode);
map.register("minecraft", command);

// Adding permissions to these Commands usually happens in `CommandAPIBukkit#onEnable`
command.setPermission(permNode);

root.addChild(node);
root.addChild(namespaceNode(node));
}
// Adding commands to the other (Why bukkit/spigot?!) dispatcher usually happens in `CraftServer#syncCommands`
root.addChild(resultantNode);
root.addChild(namespaceNode(resultantNode));

// Adding the command to the help map usually happens in `CommandAPIBukkit#onEnable`
updateHelpForCommands(List.of(registeredCommand));
// Do the same for the aliases
for(LiteralCommandNode<Source> node: aliasNodes) {
command = wrapToVanillaCommandWrapper(node);
map.register("minecraft", command);

// Sending command dispatcher packets usually happens when Players join the server
for(Player p: Bukkit.getOnlinePlayers()) {
p.updateCommands();
}
command.setPermission(permNode);

root.addChild(node);
root.addChild(namespaceNode(node));
}

// Adding the command to the help map usually happens in `CommandAPIBukkit#onEnable`
updateHelpForCommands(List.of(registeredCommand));

// Sending command dispatcher packets usually happens when Players join the server
for(Player p: Bukkit.getOnlinePlayers()) {
p.updateCommands();
}
});
}
}

Expand All @@ -536,7 +538,7 @@ private LiteralCommandNode<Source> namespaceNode(LiteralCommandNode<Source> orig

@Override
public LiteralCommandNode<Source> registerCommandNode(LiteralArgumentBuilder<Source> node) {
return getBrigadierDispatcher().register(node);
return paper.modifyCommandTreesAndAvoidPaperCME(() -> getBrigadierDispatcher().register(node));
}

@Override
Expand All @@ -563,52 +565,54 @@ public static void unregister(String commandName, boolean unregisterNamespaces,
}

private void unregisterInternal(String commandName, boolean unregisterNamespaces, boolean unregisterBukkit) {
CommandAPI.logInfo("Unregistering command /" + commandName);
paper.modifyCommandTreesAndAvoidPaperCME(() -> {
CommandAPI.logInfo("Unregistering command /" + commandName);

if(!unregisterBukkit) {
// Remove nodes from the Vanilla dispatcher
// This dispatcher doesn't usually have namespaced version of commands (those are created when commands
// are transferred to Bukkit's CommandMap), but if they ask, we'll do it
removeBrigadierCommands(getBrigadierDispatcher(), commandName, unregisterNamespaces, c -> true);
if(!unregisterBukkit) {
// Remove nodes from the Vanilla dispatcher
// This dispatcher doesn't usually have namespaced version of commands (those are created when commands
// are transferred to Bukkit's CommandMap), but if they ask, we'll do it
removeBrigadierCommands(getBrigadierDispatcher(), commandName, unregisterNamespaces, c -> true);

// Update the dispatcher file
CommandAPIHandler.getInstance().writeDispatcherToFile();
}
// Update the dispatcher file
CommandAPIHandler.getInstance().writeDispatcherToFile();
}

if(unregisterBukkit || !CommandAPI.canRegister()) {
// We need to remove commands from Bukkit's CommandMap if we're unregistering a Bukkit command, or
// if we're unregistering after the server is enabled, because `CraftServer#setVanillaCommands` will have
// moved the Vanilla command into the CommandMap
Map<String, Command> knownCommands = commandMapKnownCommands.get((SimpleCommandMap) paper.getCommandMap());
if(unregisterBukkit || !CommandAPI.canRegister()) {
// We need to remove commands from Bukkit's CommandMap if we're unregistering a Bukkit command, or
// if we're unregistering after the server is enabled, because `CraftServer#setVanillaCommands` will have
// moved the Vanilla command into the CommandMap
Map<String, Command> knownCommands = commandMapKnownCommands.get((SimpleCommandMap) paper.getCommandMap());

// If we are unregistering a Bukkit command, DO NOT unregister VanillaCommandWrappers
// If we are unregistering a Vanilla command, ONLY unregister VanillaCommandWrappers
boolean isMainVanilla = isVanillaCommandWrapper(knownCommands.get(commandName));
if(unregisterBukkit ^ isMainVanilla) knownCommands.remove(commandName);
// If we are unregistering a Bukkit command, DO NOT unregister VanillaCommandWrappers
// If we are unregistering a Vanilla command, ONLY unregister VanillaCommandWrappers
boolean isMainVanilla = isVanillaCommandWrapper(knownCommands.get(commandName));
if(unregisterBukkit ^ isMainVanilla) knownCommands.remove(commandName);

if(unregisterNamespaces) {
removeCommandNamespace(knownCommands, commandName, c -> unregisterBukkit ^ isVanillaCommandWrapper(c));
if(unregisterNamespaces) {
removeCommandNamespace(knownCommands, commandName, c -> unregisterBukkit ^ isVanillaCommandWrapper(c));
}
}
}

if(!CommandAPI.canRegister()) {
// If the server is enabled, we have extra cleanup to do
if(!CommandAPI.canRegister()) {
// If the server is enabled, we have extra cleanup to do

// Remove commands from the resources dispatcher
// If we are unregistering a Bukkit command, ONLY unregister BukkitCommandWrappers
// If we are unregistering a Vanilla command, DO NOT unregister BukkitCommandWrappers
removeBrigadierCommands(getResourcesDispatcher(), commandName, unregisterNamespaces,
c -> !unregisterBukkit ^ isBukkitCommandWrapper(c));
// Remove commands from the resources dispatcher
// If we are unregistering a Bukkit command, ONLY unregister BukkitCommandWrappers
// If we are unregistering a Vanilla command, DO NOT unregister BukkitCommandWrappers
removeBrigadierCommands(getResourcesDispatcher(), commandName, unregisterNamespaces,
c -> !unregisterBukkit ^ isBukkitCommandWrapper(c));

// Help topics (from Bukkit and CommandAPI) are only setup after plugins enable, so we only need to worry
// about removing them once the server is loaded.
getHelpMap().remove("/" + commandName);
// Help topics (from Bukkit and CommandAPI) are only setup after plugins enable, so we only need to worry
// about removing them once the server is loaded.
getHelpMap().remove("/" + commandName);

// Notify players
for (Player p : Bukkit.getOnlinePlayers()) {
p.updateCommands();
// Notify players
for (Player p : Bukkit.getOnlinePlayers()) {
p.updateCommands();
}
}
}
});
}

private void removeBrigadierCommands(CommandDispatcher<Source> dispatcher, String commandName,
Expand Down
38 changes: 37 additions & 1 deletion ...ukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/PaperImplementations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,14 @@
import com.mojang.brigadier.exceptions.SimpleCommandExceptionType;
import dev.jorel.commandapi.exceptions.WrapperCommandSyntaxException;
import dev.jorel.commandapi.nms.NMS;
import dev.jorel.commandapi.paper.CommandDispatcherReadWriteManager;
import io.papermc.paper.event.server.ServerResourcesReloadedEvent;
import net.kyori.adventure.text.Component;
import net.kyori.adventure.text.serializer.legacy.LegacyComponentSerializer;
import net.md_5.bungee.api.chat.TextComponent;

import java.util.function.Supplier;

import org.bukkit.Bukkit;
import org.bukkit.ChatColor;
import org.bukkit.command.CommandMap;
Expand All @@ -21,6 +25,7 @@ public class PaperImplementations {
private final boolean isFoliaPresent;
private final NMS<?> nmsInstance;
private final Class<? extends CommandSender> feedbackForwardingCommandSender;
private final CommandDispatcherReadWriteManager commandDispatcherReadWriteManager;

/**
* Constructs a PaperImplementations object
Expand All @@ -41,8 +46,10 @@ public PaperImplementations(boolean isPaperPresent, boolean isFoliaPresent, NMS<
} catch (ClassNotFoundException e) {
// uhh...
}

this.feedbackForwardingCommandSender = tempFeedbackForwardingCommandSender;

this.commandDispatcherReadWriteManager = new CommandDispatcherReadWriteManager();
if(isPaperPresent) nmsInstance.setupPaperCommandDispatcherReadWriteManager(this.commandDispatcherReadWriteManager);
}

/**
Expand Down Expand Up @@ -121,4 +128,33 @@ public WrapperCommandSyntaxException getExceptionFromString(String message) {
}
}

/**
* Waits to make sure Paper is not reading from the Brigadier CommandDispatchers before running a task that
* modifies those CommandDispatchers. This ensures that the command trees are not modified while Paper is
* building a Commands packet asynchronously, which may cause a ConcurrentModificationException.
* <p>
* If Paper isn't building Commands packets async (probably because we're on Spigot),
* the task is run immediately, since there isn't any chance of conflict anyway.
*
* @param modifyTask The task to run that modifies the command trees.
*/
public void modifyCommandTreesAndAvoidPaperCME(Runnable modifyTask) {
this.commandDispatcherReadWriteManager.runWriteTask(modifyTask);
}

/**
* Waits to make sure Paper is not reading from the Brigadier CommandDispatchers before running a task that
* modifies those CommandDispatchers. This ensures that the command trees are not modified while Paper is
* building a Commands packet asynchronously, which may cause a ConcurrentModificationException.
* <p>
* If Paper isn't building Commands packets async (probably because we're on Spigot),
* the task is run immediately, since there isn't any chance of conflict anyway.
*
* @param modifyTask The task to run that modifies the command trees.
* @return The result of running the {@code modifyTask}.
* @param <T> The class of the object returned by the {@code modifyTask}.
*/
public <T> T modifyCommandTreesAndAvoidPaperCME(Supplier<T> modifyTask) {
return this.commandDispatcherReadWriteManager.runWriteTask(modifyTask);
}
}
20 changes: 20 additions & 0 deletions .../commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/nms/NMS.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,10 @@
import com.mojang.brigadier.exceptions.CommandSyntaxException;
import com.mojang.brigadier.suggestion.SuggestionProvider;

import dev.jorel.commandapi.PaperImplementations;
import dev.jorel.commandapi.arguments.ArgumentSubType;
import dev.jorel.commandapi.arguments.SuggestionProviders;
import dev.jorel.commandapi.paper.CommandDispatcherReadWriteManager;
import dev.jorel.commandapi.wrappers.FloatRange;
import dev.jorel.commandapi.wrappers.FunctionWrapper;
import dev.jorel.commandapi.wrappers.IntegerRange;
Expand Down Expand Up @@ -457,4 +459,22 @@ String getScoreHolderSingle(CommandContext<CommandListenerWrapper> cmdCtx, Strin

Message generateMessageFromJson(String json);

/**
* Since <a href="https://github.com/PaperMC/Paper/pull/3116">PaperMC/Paper#3116</a>, Paper sometimes reads
* from the Brigadier CommandDispatcher asyncronously. This can cause ConcurrentModificationExceptions if the
* CommandAPI tries to create a command and write to the dispatcher at the same time.
* <p>
* This method hooks our read-write control into the appropriate location. This action is version-specific
* since Paper has altered thier implementation slightly over time.
* <p>
* Calling this method is unecessary if {@link PaperImplementations#isPaperPresent()} returns false, since Spigot doesn't
* build Commands packets async at all, so there is never any conflict to deal with. This method may assume that if it
* is called, it is being called on a Paper server, so calling this method when {@link PaperImplementations#isPaperPresent()}
* returns false is also discouraged.
* <p>
* See <a href="https://github.com/JorelAli/CommandAPI/pull/501">CommandAPI#501</a> for more details.
*
* @param commandDispatcherReadWriteManager The read-write manager being used
*/
void setupPaperCommandDispatcherReadWriteManager(CommandDispatcherReadWriteManager commandDispatcherReadWriteManager);
}
46 changes: 46 additions & 0 deletions ...kkit-core/src/main/java/dev/jorel/commandapi/paper/CommandDispatcherReadWriteManager.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package dev.jorel.commandapi.paper;

import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.function.Supplier;

/**
* Since <a href="https://github.com/PaperMC/Paper/pull/3116">PaperMC/Paper#3116</a>, Paper sometimes reads
* from the Brigadier CommandDispatcher asyncronously. This can cause ConcurrentModificationExceptions if the
* CommandAPI tries to create a command and write to the dispatcher at the same time.
* <p>
* This class handles locking read and write access at appropriate times.
* <p>
* See <a href="https://github.com/JorelAli/CommandAPI/pull/501">CommandAPI#501</a> for more details.
*/
public class CommandDispatcherReadWriteManager {
private ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock();

public void runWriteTask(Runnable writeTask) {
readWriteLock.writeLock().lock();
try {
writeTask.run();
} finally {
readWriteLock.writeLock().unlock();
}
}

public <T> T runWriteTask(Supplier<T> writeTask) {
readWriteLock.writeLock().lock();
try {
return writeTask.get();
} finally {
readWriteLock.writeLock().unlock();
}
}

public Runnable wrapReadTask(Runnable originalTask) {
return () -> {
readWriteLock.readLock().lock();
try {
originalTask.run();
} finally {
readWriteLock.readLock().unlock();
}
};
}
}
Loading

0 comments on commit c457c8c

Please sign in to comment.