Some updates to networking

[willkroboth]

Some updates to the networking logic as suggested by @Timongcraft on Discord.

The CommandAPI Bukkit plugin now provides CommandAPINetworking. So, if a plugin depends on CommandAPINetworking, it will be able to load if CommandAPINetworking or CommandAPI is present. This makes sense, since CommandAPI provides the same networking support that CommandAPINetworking does. Provides also allows plugins to load classes from CommandAPI like they would from CommandAPINetworking, which technically doesn't apply here since CommandAPINetworking has some unique classes not present in CommandAPI. I don't currently expect anyone to load classes from CommandAPINetworking though, so this is not a problem right now.

CommandAPI Bukkit and commandapi Velocity both now have the report-failed-packet-sends config option. By default this is true, and the CommandAPI will throw a runtime exception if it cannot send a packet because the receiver doesn't have a new enough CommandAPI networking implementation. If set to false, these attempts will fail silently. Technically, Bukkit doesn't use this option since it doesn't send any packets right now, but it was easy enough to do this way with the config system, and it might be used eventually. Bukkit Networking doesn't have this option and will always throw since it also doesn't currently send any packets, and I didn't bother adding a config file to that.

Looking for feedback on whether report-failed-packet-sends: false should log a warning/error message instead of failing silently. It would be less intrusive than an unhandled exception, and it could still be turned off with verbose-outputs or silent-logs. In this case, the option name should probably be changed to something like error-on-failed-packet-sends. (After typing that I kinda convinced myself a warning which you can turn off with silent-logs is the best option :P)

[willkroboth]

I did convince myself and changed the config option to be error-on-failed-packet-sends. So when true it throws an error, and false logs a warning.

[Timongcraft]

Why not have a option to disable the communication attempt directly instead of ignoring the error?

[willkroboth]

updateRequirements can't do anything on Velocity if it can't communicate with the backend, so if it is expected to fail, I don't think there is any reason to try calling it. If you do assume it will always work, I think there should at least be a warning when a method does not do what it is supposed to. When updateRequirements fails, it can cause confusing desyncs where the client tells a player they can access a command when they really can't.

[Timongcraft]

I didn't think correctly, making this as disable-warning is good, sorry.

[willkroboth]

No problem :) I wasn't exactly sure what you meant before

[Timongcraft]

But why is it that the error is downgraded to a warning? I mean, if you set a config option to disable an exception I'd except it to be completely silenced. I understand that you still want a warning because something went wrong, but then why have a config option in the first place?

[willkroboth]

My thoughts were based on this test setup I made.

Set<Player> canUse = new HashSet<>();

new CommandAPICommand("toggle")
    .executesPlayer(info -> {
        Player player = info.sender();

        if (canUse.contains(player)) {
            canUse.remove(player);
        } else {
            canUse.add(player);
        }
        CommandAPI.updateRequirements(player);

        boolean useState = canUse.contains(player);
        player.sendPlainMessage("Can use: " + useState);
    })
    .register();

new CommandAPICommand("test")
    .withRequirement(source -> {
        if (!(source instanceof Player player)) return true;

        return canUse.contains(player);
    })
    .executes(info -> {
        info.sender().sendPlainMessage("Success!");
    })
    .register();

The difference between an exception and a warning is that the exception interrupts the control flow. In this example, if updateRequirements throws an exception, then the player#sendPlainMessage step gets bypassed. If updateRequirements just logs a warning, then the surrounding logic can continue as if it did work.

The warning message can also be ignored by turning on the silent-logs option, so I thought this provided more possibilities. You can have error, warning, or nothing. Although, yeah, that does maybe conflict with other messages since you'd be turning off all other warnings and info logs as well.

I guess another option which does not show the warning is the user checking in advance whether the method would fail:

if (player.getCurrentServer()
    .map(server -> CommandAPIVelocity.get().getMessenger()
        .getConnectionProtocolVersion(server) > 0
    )
    .orElse(false)
) {
    CommandAPI.updateRequirements(player);
} else {
    // `updateRequirements` would have failed
}

This does give the most control to the developer what to do. I concede it isn't very good though since it's not documented and very implementation dependent, so a developer wouldn't know to do this and it could change. Maybe this logic could be put into a willUpdateRequirementsWork helper method?

Do you have use case where a different solution would make more sense?

[Timongcraft]

The warning message can also be ignored by turning on the silent-logs option, so I thought this provided more possibilities.

I did not think of that. Well then I have nothing to say. Apologies

[willkroboth]

👍 No problem! Thanks for clarifying.

[DerEchtePilz]

I just realized that this needs a docs PR to update the default config to include the new value.