diff --git a/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/CommandAPIBukkit.java b/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/CommandAPIBukkit.java index 83fada355f..6fa0d14e42 100644 --- a/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/CommandAPIBukkit.java +++ b/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/CommandAPIBukkit.java @@ -467,7 +467,7 @@ public void preCommandRegistration(String commandName) { @Override public void postCommandRegistration(RegisteredCommand registeredCommand, LiteralCommandNode resultantNode, List> aliasNodes) { if(!CommandAPI.canRegister()) { - paper.modifyCommandTreesSafely(() -> { + 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. @@ -533,7 +533,7 @@ private LiteralCommandNode namespaceNode(LiteralCommandNode orig @Override public LiteralCommandNode registerCommandNode(LiteralArgumentBuilder node) { - return paper.modifyCommandTreesSafely(() -> getBrigadierDispatcher().register(node)); + return paper.modifyCommandTreesAndAvoidPaperCME(() -> getBrigadierDispatcher().register(node)); } @Override @@ -560,7 +560,7 @@ public static void unregister(String commandName, boolean unregisterNamespaces, } private void unregisterInternal(String commandName, boolean unregisterNamespaces, boolean unregisterBukkit) { - paper.modifyCommandTreesSafely(() -> { + paper.modifyCommandTreesAndAvoidPaperCME(() -> { CommandAPI.logInfo("Unregistering command /" + commandName); if (!unregisterBukkit) { diff --git a/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/PaperImplementations.java b/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/PaperImplementations.java index 151d7a15b6..f785416c72 100644 --- a/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/PaperImplementations.java +++ b/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/PaperImplementations.java @@ -10,8 +10,8 @@ import dev.jorel.commandapi.nms.NMS; import io.papermc.paper.event.server.ServerResourcesReloadedEvent; -import java.util.List; -import java.util.concurrent.Callable; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; import java.util.concurrent.ThreadPoolExecutor; import java.util.function.Supplier; @@ -122,17 +122,11 @@ public Class getFeedbackForwardingCommandSender() { * * @param modifyTask The task to run that modifies the command trees. */ - public void modifyCommandTreesSafely(Runnable modifyTask) { - try { - modifyCommandTreesSafely((Callable) () -> { - modifyTask.run(); - return null; - }); - } catch (Exception e) { - // `modifyTask` should not be throwing any checked exceptions anyway - // since that would violate the signature of Runnable - throw new RuntimeException(e); - } + public void modifyCommandTreesAndAvoidPaperCME(Runnable modifyTask) { + modifyCommandTreesAndAvoidPaperCME(() -> { + modifyTask.run(); + return null; + }); } /** @@ -147,25 +141,38 @@ public void modifyCommandTreesSafely(Runnable modifyTask) { * @return The result of running the {@code modifyTask}. * @param The class of the object returned by the {@code modifyTask}. */ - public T modifyCommandTreesSafely(Supplier modifyTask) { - try { - return modifyCommandTreesSafely((Callable) modifyTask::get); - } catch (Exception e) { - // `modifyTask` should not be throwing any checked exceptions anyway - // since that would violate the signature of Supplier - throw new RuntimeException(e); - } - } - - private T modifyCommandTreesSafely(Callable modifyTask) throws Exception { - if(paperCommandSendingPool == null) { + public T modifyCommandTreesAndAvoidPaperCME(Supplier modifyTask) { + if(paperCommandSendingPool == null || paperCommandSendingPool.isShutdown()) { // If the server isn't building Commands packets async (probably because we're on Spigot), // it is safe to run the task immediately - return modifyTask.call(); + return modifyTask.get(); } // Otherwise, submit the modify task to the pool. - // The pool only runs one task at a time, so this ensures we don't modify - // the commands while a command-building process is reading them - return paperCommandSendingPool.invokeAll(List.of(modifyTask)).get(0).get(); - } + // The pool only runs one task at a time (see https://github.com/JorelAli/CommandAPI/pull/501#issuecomment-1773959895), + // so this ensures we don't modify the commands while a command-building process is reading them + CompletableFuture result = new CompletableFuture<>(); + + paperCommandSendingPool.execute(() -> result.complete(modifyTask.get())); + + try { + return result.get(); + } catch (ExecutionException e) { + Throwable cause = e.getCause(); + + // `modifyTask` may very well throw runtime exceptions, pass it on + if(cause instanceof RuntimeException runtimeExceptionCause) throw runtimeExceptionCause; + + // `modifyTask` should not throw a checked exception, since that violates the signature of Supplier + // This path shouldn't happen, so it is reasonable to freak out with a generic exception + throw new AssertionError("Unexpected exception. `Supplier modifyTask` should not throw a checked exception.", e); + } catch (InterruptedException e) { + // As far as I can tell, this is the best way to handle this exception (see https://stackoverflow.com/a/18926205). + // The InterruptedException means that this thread should stop whatever it's doing ASAP. + // I don't think it makes sense to add `throws InterruptedException` to the method signature since it's not + // going to be handled anywhere else, so we just have to wrap it in a RuntimeException and get out of here. + // The `modifyTask` probably did not complete correctly, so we don't have a reasonable result to return. + Thread.currentThread().interrupt(); + throw new RuntimeException("The commands could not be updated :(. The thread was interrupted.", e); + } + } }