Merge pull request #307 from solonovamax/fix/commands-completing-before-execution

Fix cloud swallowing exceptions in `suspend` methods.

cloud-core/src/main/java/cloud/commandframework/execution/AsynchronousCommandExecutionCoordinator.java

@@ -89,6 +89,8 @@ public final class AsynchronousCommandExecutionCoordinator<C> extends CommandExe
                             resultFuture.completeExceptionally(new CommandExecutionException(throwable, commandContext));
                         }
                     }
+                    // Only complete when the execution is actually finished. See #306 for more info.
+                    resultFuture.complete(new CommandResult<>(commandContext));
                 });
             }
         };
@@ -97,30 +99,25 @@ public final class AsynchronousCommandExecutionCoordinator<C> extends CommandExe
             final @NonNull Pair<@Nullable Command<C>, @Nullable Exception> pair =
                     this.getCommandTree().parse(commandContext, input);
             if (pair.getSecond() != null) {
-                final CompletableFuture<CommandResult<C>> future = new CompletableFuture<>();
-                future.completeExceptionally(pair.getSecond());
-                return future;
+                resultFuture.completeExceptionally(pair.getSecond());
+            } else {
+                this.executor.execute(() -> commandConsumer.accept(pair.getFirst()));
             }
-            return CompletableFuture.supplyAsync(() -> {
-                commandConsumer.accept(pair.getFirst());
-                return new CommandResult<>(commandContext);
-            }, this.executor);
-        }
-
-        this.executor.execute(() -> {
-            try {
-                final @NonNull Pair<@Nullable Command<C>, @Nullable Exception> pair =
-                        this.getCommandTree().parse(commandContext, input);
-                if (pair.getSecond() != null) {
-                    resultFuture.completeExceptionally(pair.getSecond());
-                } else {
-                    commandConsumer.accept(pair.getFirst());
-                    resultFuture.complete(new CommandResult<>(commandContext));
+        } else {
+            this.executor.execute(() -> {
+                try {
+                    final @NonNull Pair<@Nullable Command<C>, @Nullable Exception> pair =
+                            this.getCommandTree().parse(commandContext, input);
+                    if (pair.getSecond() != null) {
+                        resultFuture.completeExceptionally(pair.getSecond());
+                    } else {
+                        commandConsumer.accept(pair.getFirst());
+                    }
+                } catch (final Exception e) {
+                    resultFuture.completeExceptionally(e);
                 }
-            } catch (final Exception e) {
-                resultFuture.completeExceptionally(e);
-            }
-        });
+            });
+        }
 
         return resultFuture;
     }

cloud-kotlin-extensions/src/main/kotlin/cloud/commandframework/kotlin/coroutines/KotlinAnnotatedMethods.kt

@@ -29,8 +29,8 @@ import cloud.commandframework.context.CommandContext
 import cloud.commandframework.execution.CommandExecutionCoordinator
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.GlobalScope
-import kotlinx.coroutines.async
-import kotlinx.coroutines.future.asCompletableFuture
+import kotlinx.coroutines.future.future
+import java.lang.reflect.InvocationTargetException
 import java.lang.reflect.Method
 import java.util.concurrent.CompletableFuture
 import java.util.function.Predicate
@@ -71,12 +71,15 @@ private class KotlinMethodCommandExecutionHandler<C>(
     override fun executeFuture(commandContext: CommandContext<C>): CompletableFuture<Void?> {
         val instance = context().instance()
         val params = createParameterValues(commandContext, commandContext.flags(), false)
+
         // We need to propagate exceptions to the caller.
-        return coroutineScope
-            .async<Void?>(this@KotlinMethodCommandExecutionHandler.coroutineContext) {
-                context().method().kotlinFunction?.callSuspend(instance, *params.toTypedArray())
-                null
+        return coroutineScope.future(this@KotlinMethodCommandExecutionHandler.coroutineContext) {
+            try {
+                context().method().kotlinFunction!!.callSuspend(instance, *params.toTypedArray())
+            } catch (e: InvocationTargetException) { // unwrap invocation exception
+                e.cause?.let { throw it } ?: throw e // if cause exists, throw, else rethrow invocation exception
             }
-            .asCompletableFuture()
+            null
+        }
     }
 }

cloud-kotlin-extensions/src/test/kotlin/cloud/commandframework/kotlin/coroutines/KotlinAnnotatedMethodsTest.kt

@@ -26,6 +26,7 @@ package cloud.commandframework.kotlin.coroutines
 import cloud.commandframework.CommandManager
 import cloud.commandframework.annotations.AnnotationParser
 import cloud.commandframework.annotations.CommandMethod
+import cloud.commandframework.exceptions.CommandExecutionException
 import cloud.commandframework.execution.AsynchronousCommandExecutionCoordinator
 import cloud.commandframework.internal.CommandRegistrationHandler
 import cloud.commandframework.meta.CommandMeta
@@ -36,13 +37,15 @@ import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.withContext
 import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.assertThrows
+import java.util.concurrent.ExecutorService
 import java.util.concurrent.Executors
 import java.util.concurrent.TimeUnit
 
 class KotlinAnnotatedMethodsTest {
 
     companion object {
-        val executorService = Executors.newSingleThreadExecutor()
+        val executorService: ExecutorService = Executors.newSingleThreadExecutor()
     }
 
     private lateinit var commandManager: CommandManager<TestCommandSender>
@@ -68,15 +71,27 @@ class KotlinAnnotatedMethodsTest {
         commandManager.executeCommand(TestCommandSender(), "test").await()
     }
 
+    @Test
+    fun `test suspending command methods with exception`(): Unit = runBlocking {
+        AnnotationParser(commandManager, TestCommandSender::class.java) {
+            SimpleCommandMeta.empty()
+        }
+            .also { it.installCoroutineSupport() }
+            .parse(CommandMethods())
+
+        assertThrows<CommandExecutionException> {
+            commandManager.executeCommand(TestCommandSender(), "test-exception").await()
+        }
+    }
+
     private class TestCommandSender
 
-    private class TestCommandManager :
-        CommandManager<TestCommandSender>(
-            AsynchronousCommandExecutionCoordinator.newBuilder<TestCommandSender>()
-                .withExecutor(executorService)
-                .build(),
-            CommandRegistrationHandler.nullCommandRegistrationHandler()
-        ) {
+    private class TestCommandManager : CommandManager<TestCommandSender>(
+        AsynchronousCommandExecutionCoordinator.newBuilder<TestCommandSender>()
+            .withExecutor(executorService)
+            .build(),
+        CommandRegistrationHandler.nullCommandRegistrationHandler()
+    ) {
 
         override fun hasPermission(sender: TestCommandSender, permission: String): Boolean = true
 
@@ -90,5 +105,8 @@ class KotlinAnnotatedMethodsTest {
             withContext(Dispatchers.Default) {
                 println("called from thread: ${Thread.currentThread().name}")
             }
+
+        @CommandMethod("test-exception")
+        public suspend fun suspendingCommandWithException(): Unit = throw IllegalStateException()
     }
 }