Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hasFiredAsync parameter when AsyncPlayerSendCommandsEvent is called #10896

Merged

Conversation

willkroboth
Copy link
Contributor

AsyncPlayerSendCommandsEvent is fired twice, once async, then sync. The javadocs suggest using the logic if (event.isAsynchronous() || !event.hasFiredAsync()) { // do stuff } to only react to the async event.


However, this check will currently pass during both times the event fires. As seen using this simple plugin:

public final class BukkitTestPlugin extends JavaPlugin implements Listener {
    @Override
    public void onEnable() {
        Bukkit.getServer().getPluginManager().registerEvents(this, this);
    }

    @EventHandler
    public void onCommandsSentToPlayer(AsyncPlayerSendCommandsEvent<?> event) {
        getLogger().info("AsyncPlayerSendCommandsEvent{" +
                "isAsynchronous=" + event.isAsynchronous() +
                ", hasFiredAsync=" + event.hasFiredAsync() + "}"
        );
    }
}

When a player logs in, the event is fired like so:

[07:51:27] [Server thread/INFO]: willkroboth joined the game
[07:51:27] [Server thread/INFO]: willkroboth[/127.0.0.1:41644] logged in with entity id 15 at ([world]-276.5, 72.0, 27.5)
[07:51:27] [Paper Async Command Builder Thread Pool - 0/INFO]: [BukkitTestPlugin] AsyncPlayerSendCommandsEvent{isAsynchronous=true, hasFiredAsync=false}
[07:51:27] [Server thread/INFO]: [BukkitTestPlugin] AsyncPlayerSendCommandsEvent{isAsynchronous=false, hasFiredAsync=false}
[07:52:04] [Server thread/INFO]: Checking version, please wait...
[07:52:05] [Thread-5/INFO]: This server is running Paper version 1.20.6-145-ver/1.20.6@fe7043e (2024-06-15T21:16:04Z) (Implementing API version 1.20.6-R0.1-SNAPSHOT)
You are running the latest version

Both times the event is called, the hasFiredAsync value is false. Therefore !event.hasFiredAsync() is always true, and // do stuff would always run.


This PR simply makes it so the second time the event is called, hasFiredAsync is true:

[08:27:10] [Server thread/INFO]: willkroboth joined the game
[08:27:10] [Server thread/INFO]: willkroboth[/127.0.0.1:52662] logged in with entity id 126 at ([world]-276.5, 72.0, 27.5)
[08:27:10] [Paper Async Command Builder Thread Pool - 0/INFO]: [BukkitTestPlugin] AsyncPlayerSendCommandsEvent{isAsynchronous=true, hasFiredAsync=false}
[08:27:10] [Server thread/INFO]: [BukkitTestPlugin] AsyncPlayerSendCommandsEvent{isAsynchronous=false, hasFiredAsync=true}
[08:27:19] [Server thread/INFO]: Checking version, please wait...
[08:27:19] [Thread-5/INFO]: This server is running Paper version 1.20.6-DEV-fix-CommandSend-hasFiredAsync@79e2cb6 (2024-06-17T12:19:25Z) (Implementing API version 1.20.6-R0.1-SNAPSHOT)
Unknown version
Previous version: 1.20.6-145-fe7043e (MC: 1.20.6)

With this, if (event.isAsynchronous() || !event.hasFiredAsync()) { // do stuff } will only do stuff during the async event, as the javadocs suggest.

I imagine this is a copy-paste error back from when the event was first added in this commit: fdf41b7#diff-1b1388b7c287d608bf170cd9f7f3ac9477793ac427c444b8968f86e9f4477468R20-R28.


The javadocs mention If for some reason we are unable to send this asynchronously in the future, only the sync method will fire. If the async call is eventually removed, I imagine the sync event call should have hasFiredAsync set to false (which would then be a correct statement). I believe that is the intended purpose of suggesting the if (event.isAsynchronous() || !event.hasFiredAsync()) { // do stuff } logic, so that // do stuff is still run if the async call is ever removed.

@willkroboth willkroboth requested a review from a team as a code owner June 17, 2024 12:53
@kennytv kennytv changed the base branch from master to ver/1.20.6 June 17, 2024 19:13
@kennytv kennytv merged commit e41d44f into PaperMC:ver/1.20.6 Jun 17, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

3 participants