From 8711d19703ea0f31fff26aee0b53a9753085e388 Mon Sep 17 00:00:00 2001 From: solo Date: Mon, 4 Oct 2021 08:14:58 -0400 Subject: [PATCH] Merge pull request #307 from solonovamax/fix/commands-completing-before-execution Fix cloud swallowing exceptions in `suspend` methods. --- ...ynchronousCommandExecutionCoordinator.java | 41 +++++++++---------- .../coroutines/KotlinAnnotatedMethods.kt | 17 ++++---- .../coroutines/KotlinAnnotatedMethodsTest.kt | 34 +++++++++++---- 3 files changed, 55 insertions(+), 37 deletions(-) diff --git a/cloud-core/src/main/java/cloud/commandframework/execution/AsynchronousCommandExecutionCoordinator.java b/cloud-core/src/main/java/cloud/commandframework/execution/AsynchronousCommandExecutionCoordinator.java index ee9e6117..c5b628f8 100644 --- a/cloud-core/src/main/java/cloud/commandframework/execution/AsynchronousCommandExecutionCoordinator.java +++ b/cloud-core/src/main/java/cloud/commandframework/execution/AsynchronousCommandExecutionCoordinator.java @@ -89,6 +89,8 @@ public final class AsynchronousCommandExecutionCoordinator 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 extends CommandExe final @NonNull Pair<@Nullable Command, @Nullable Exception> pair = this.getCommandTree().parse(commandContext, input); if (pair.getSecond() != null) { - final CompletableFuture> 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, @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, @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; } diff --git a/cloud-kotlin-extensions/src/main/kotlin/cloud/commandframework/kotlin/coroutines/KotlinAnnotatedMethods.kt b/cloud-kotlin-extensions/src/main/kotlin/cloud/commandframework/kotlin/coroutines/KotlinAnnotatedMethods.kt index 46e86e85..5a754bbe 100644 --- a/cloud-kotlin-extensions/src/main/kotlin/cloud/commandframework/kotlin/coroutines/KotlinAnnotatedMethods.kt +++ b/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( override fun executeFuture(commandContext: CommandContext): CompletableFuture { val instance = context().instance() val params = createParameterValues(commandContext, commandContext.flags(), false) + // We need to propagate exceptions to the caller. - return coroutineScope - .async(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 + } } } diff --git a/cloud-kotlin-extensions/src/test/kotlin/cloud/commandframework/kotlin/coroutines/KotlinAnnotatedMethodsTest.kt b/cloud-kotlin-extensions/src/test/kotlin/cloud/commandframework/kotlin/coroutines/KotlinAnnotatedMethodsTest.kt index bfbb4a77..0881d4ea 100644 --- a/cloud-kotlin-extensions/src/test/kotlin/cloud/commandframework/kotlin/coroutines/KotlinAnnotatedMethodsTest.kt +++ b/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 @@ -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 { + commandManager.executeCommand(TestCommandSender(), "test-exception").await() + } + } + private class TestCommandSender - private class TestCommandManager : - CommandManager( - AsynchronousCommandExecutionCoordinator.newBuilder() - .withExecutor(executorService) - .build(), - CommandRegistrationHandler.nullCommandRegistrationHandler() - ) { + private class TestCommandManager : CommandManager( + AsynchronousCommandExecutionCoordinator.newBuilder() + .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() } }