Revxrsal / Lamp

A powerful, extendable, flexible yet simple to use commands annotation framework.
MIT License
171 stars 33 forks source link

Implement an @Async annotation to make commands be executed asynchronously #88

Closed paaulhier closed 3 months ago

paaulhier commented 3 months ago

Create an @Async annotation to make commands be executed asynchronously

Revxrsal commented 3 months ago

There were a few small discussions in the Discord server about introducing something like this to Lamp. In my previous command framework, cub, there was a @RunAsync annotation which did exactly that. However, it brought many subtle issues with it and introduced a lot of design problems. To name a few:

  1. What is the async part exactly? Is it running the command? Resolving parameters? What if the command had an error? Would handling that error be async or not? What about the command response? What if a command blocks entirely? Etc.
  2. What's the appropriate "async" implementation to use?

It is also, admittedly, easy to misuse, which violates one of Lamp's core principles, to be misuse-resistant. For example:

  1. It is easy for users to assume that anything @Async is better, and give in to the magic of asynchronous stuff
  2. A single command that blocks (for example, network calls or file IO) may cause other commands to wait indefinitely without any visible effect on the server.
  3. Many APIs are not async-friendly, especially Bukkit ones. It's very easy to slip calls to the non-async-safe Bukkit API in an async command, and there is no way to catch such errors early on. This may cause lots of bugs.
  4. This involves too much magic, and in many cases will cause command behaviors to be unpredictable.

Besides that, Lamp does indeed support many asynchronous features that avoid most of these pitfalls:

  1. If you have a command that returns T, you can make it return CompletionStage<T> or CompletableFuture<T> and Lamp will automatically deal with it appropriately.
  2. Lamp supports Kotlin suspend functions (by calling the CommandHandler.supportSuspendFunctions() extension function)

For example, compare these two examples:

@Command("save-entities-in-world")
public CompletableFuture<Component> saveEntitiesInWorld(CommandSender sender, @Default("self") World world) {
    // These functions must be called on the main thread
    sender.sendMessage("Saving...");
    List<Entity> entities = world.getEntities();
    return CompletableFuture.supplyAsync(() -> {
        File file = new File("entities-" + world.getName() + ".json");
        // save entities here...
        return Component.text("Entities saved successfully");
    });
}

In this one, the asynchronous part is explicit and clear, and it's hard to accidentally call non-async-safe functions asynchronously. This is fully supported and it is the recommended way for doing asynchronous things.

@Async
@Command("save-entities-in-world")
public Component saveEntitiesInWorld(CommandSender sender, @Default("self") World world) {
    // Calling functions that are not async-safe may cause undefined behavior
    sender.sendMessage("Saving...");
    List<Entity> entities = world.getEntities();

    File file = new File("entities-" + world.getName() + ".json");
    // save entities here
    return Component.text("Entities saved successfully");
}

This one, while it looks nicer, is very bug-prone and it's easy to mistakenly call APIs that are not supported asynchronously. I find myself always overlooking the @Async annotation, and the code can be unpredictable at times. By the time the developer finds out about the bug, they will have to revert to using something similar to the first example. So it's time and effort wasted.

Such problems are also easy to avoid in Kotlin coroutines:

@Command("save-entities-in-world")
suspend fun saveEntitiesInWorld(sender: CommandSender, @Default("self") world: World) {
    // calls in the main thread
    val entities = world.getEntities()

    // calls on the IO dispatcher
    withContext(Dispatchers.IO) {
        val file = File(("entities-" + world.getName()).toString() + ".json")
        // save entities here
    }
}

Here, it's also clear and explicit, and the developer has complete control over which parts are executed asynchronously and which ones are not.

While I'm not entirely against introducing such an annotation, it may require more work and more explicitness than merely a marker on a function.