Create CommandAPI plugin messaging system (Fix CommandAPIVelocity#updateRequirements)

[willkroboth]

The main goal of this PR is to fix the method CommandAPIVelocity#updateRequirements, which currently does nothing.

When CommandAPI#updateRequirements is called, the Commands packet should be resent. This packet encodes a Brigadier RootCommandNode containing all the commands a player is allowed to run. If a player's requirements change, this packet should be resent to inform the player of those changes (see Updating requirements).

Unfortunately, Velocity cannot generate the Commands packet by itself. It can only inject its own commands into a Commands packet sent by a backend server. There is also no clean built-in way to trigger the backend server to resend the Commands packet. So, the CommandAPI needs to set up its own cross-server communication.

That gets to the big change of this PR, a CommandAPI plugin messaging system. The Minecraft protocol includes a Plugin Message packet. Any plugin can use this packet to send whatever data they want, and any plugin at the destination can listen for the packet to receive that data.

While the CommandAPIVelocity#updateRequirements problem could have been easily solved with a quick and dirty solution, this PR instead creates a general packet-based communication protocol that should expand to other platforms and new packets. This system is heavily based on what I saw in Minecraft's code for handling packets. To show how this system works, I'll explain the process that fixes CommandAPIVelocity#updateRequirements.

BE WARNED: Lots of code reading ahead and explanations that might only make sense to me :P. If you can't understand what's happening, that's probably because I don't understand what's happening and the code is very silly. I didn't know about most of this stuff until about 2 weeks ago, so there is probably some incorrect information.

Plugin setup/shutdown

Click to expand

The central class of this PR is CommandAPIMessenger. It is the main interface for sending and receiving data using the Plugin Message packet. This code starts the process of setting up and shutting down this system:

public class CommandAPIHandler {
	private CommandAPIMessenger<?, ?> messenger;

	public void onEnable() {
		platform.onEnable();
		// Setting up the network messenger usually requires registering
		//  events, which often does not work until onEnable
		messenger = platform.setupMessenger();
	}

	public void onDisable() {
		messenger.close();
		platform.onDisable();
		CommandAPIHandler.resetInstance();
	}
}

public class CommandAPIBukkit {
	private BukkitCommandAPIMessenger messenger;

	@Override
	public BukkitCommandAPIMessenger setupMessenger() {
		messenger = new BukkitCommandAPIMessenger(CommandAPIProtocol.CHANNEL_NAME, getConfiguration().getPlugin());
		return messenger;
	}

	public BukkitCommandAPIMessenger getMessenger() {
		return messenger;
	}
}

public class CommandAPIVelocity {
	private VelocityCommandAPIMessenger messenger;

	@Override
	public VelocityCommandAPIMessenger setupMessenger() {
		messenger = new VelocityCommandAPIMessenger(
			CommandAPIProtocol.CHANNEL_NAME,
			getConfiguration().getPlugin(),
			getConfiguration().getServer()
		);
		return messenger;
	}

	public VelocityCommandAPIMessenger getMessenger() {
		return messenger;
	}
}

The different platforms have their own ways of hooking into this system, so CommandAPIMessenger is extended by BukkitCommandAPIMessenger and VelocityCommandAPIMessenger. onEnable, the platforms set up their implementation and hand it to CommandAPIHandler. onDisable, CommandAPIHandler calls CommandAPIMessenger#close to disable the messenger.

Here is how the messengers are constructed and closed:

 public abstract class CommandAPIMessenger<InputChannel, OutputChannel> {
	private final CommandAPIPacketHandler<InputChannel> packetHandler;

	protected CommandAPIMessenger(CommandAPIPacketHandler<InputChannel> packetHandler) {
		this.packetHandler = packetHandler;
	}

	public abstract void close();
}

public class BukkitCommandAPIMessenger extends CommandAPIMessenger<Player, Player> implements PluginMessageListener {
	private final String channelName;
	private final JavaPlugin plugin;

	public BukkitCommandAPIMessenger(String channelName, JavaPlugin plugin) {
		super(new BukkitPacketHandler());
		this.channelName = channelName;
		this.plugin = plugin;

		// Register to listen for plugin messages
		Messenger messenger = Bukkit.getServer().getMessenger();
		messenger.registerIncomingPluginChannel(this.plugin, this.channelName, this);
		messenger.registerOutgoingPluginChannel(this.plugin, this.channelName);
	}

	@Override
	public void close() {
		// Unregister this listener
		Messenger messenger = Bukkit.getServer().getMessenger();
		messenger.unregisterIncomingPluginChannel(this.plugin, this.channelName);
		messenger.unregisterOutgoingPluginChannel(this.plugin, this.channelName);
	}

	@Override
	public void onPluginMessageReceived(String channel, @NotNull Player player, byte[] message) {
		...
	}
}

public class VelocityCommandAPIMessenger extends CommandAPIMessenger<ChannelMessageSource, ChannelMessageSink> {
	private final ChannelIdentifier channel;
	private final Object plugin;
	private final ProxyServer proxy;

	public VelocityCommandAPIMessenger(String channelName, Object plugin, ProxyServer proxy) {
		super(new VelocityPacketHandler());
		this.channel = MinecraftChannelIdentifier.from(channelName);
		this.plugin = plugin;
		this.proxy = proxy;

		// Register the channel so the PluginMessageEvent is fired
		this.proxy.getChannelRegistrar().register(this.channel);
		// Register the event handler to catch the PluginMessageEvent
		this.proxy.getEventManager().register(this.plugin, this);
	}

	@Override
	public void close() {
		// Unregister the channel and this listener
		this.proxy.getChannelRegistrar().unregister(this.channel);
		this.proxy.getEventManager().unregisterListener(this.plugin, this);
	}

	@Subscribe
	public void onPluginMessageEvent(PluginMessageEvent event) {
		...
	}
}

CommandAPIMessenger has two type parameters, InputChannel and OutputChannel. These types specify what classes on the platform may send messages to the plugin, and which classes the plugin can send messages to. On Bukkit, only Player sends messages, and messages can only be sent to Player. As a middle-man proxy, Velocity can send and receive messages from both Player and ServerConnection. Those two share the common interfaces ChannelMessageSource for receiving and ChannelMessageSink for sending.

This code shows a new class CommandAPIPacketHandler, which is again implemented per platform as BukkitPacketHandler and VelocityPacketHandler. This class will be inspected later in Handling Packets. What's important is that packets should be handled differently on different platforms, and the messengers set CommandAPIMessenger up with the correct implementation.

BukkitCommandAPIMessenger and VelocityCommandAPIMessenger use the objects passed into their constructors to hook into the PluginMessage events on their platforms. So, when the Plugin Message packet is received, Bukkit will call onPluginMessageReceived(String channel, @NotNull Player player, byte[] message) and Velocity will call onPluginMessageEvent(PluginMessageEvent event). The specifics of these methods will be inspected later in Receiving Packets.

Finally, the close method is abstract in CommandAPIMessenger, and implemented in the platform messengers. The messengers simply unregister their events and channels before the server shuts down.

The CommandAPI protocol

Click to expand

The second and third most important classes are CommandAPIProtocol and CommandAPIPacket. These classes define how data is encoded and decoded into a byte array. They were inspired by net.minecraft.network.ConnectionProtocol and net.minecraft.network.protocol.Packet respectively. We'll see how they come together in Sending Packets and Receiving Packets, but this is a brief overview of the methods they contain.

First, the simpler CommandAPIPacket:

public interface CommandAPIPacket {
	void write(ByteArrayDataOutput buffer);

	<InputChannel> void handle(InputChannel sender, CommandAPIPacketHandler<InputChannel> packetHandler);
}

This interface simply defines the write and handle method. However, implementations of CommandAPIPacket should also have two static methods, deserialize and create. For example, this is what a packet containing two ints might look like:

public class TestPacket implements CommandAPIPacket {
    public static TestPacket deserialize(ByteArrayDataInput input) {
        int a = input.readInt();
        int b = input.readInt();
        return new TestPacket(a, b);
    }

    public static TestPacket create(int a, int b) {
        return new TestPacket(a, b);
    }

    private final int a;
    private final int b;

    private TestPacket(int a, int b) {
        this.a = a;
        this.b = b;
    }

    @Override
    public void write(ByteArrayDataOutput buffer) {
        buffer.writeInt(a);
        buffer.writeInt(b);
    }

    @Override
    public <InputChannel> void handle(InputChannel sender, CommandAPIPacketHandler<InputChannel> packetHandler) {
        packetHandler.handle(sender, this);
    }
}

Here is what these methods do:

• void write(ByteArrayDataOutput buffer) - Writes the data for the packet to the given byte output buffer.
• static [packet class] deserialize(ByteArrayDataInput buffer) - Reads data from the given byte input buffer to create a new packet. This method can assume everything written by the write method will be present for it to read.
• static [packet class] create([parameters for the packet]) - Creates a new packet with the given parameters
• <InputChannel> void handle(InputChannel sender, CommandAPIPacketHandler<InputChannel> packetHandler) - Performs the appropriate action for receiving this packet. See more in Handling Packets.

Next, CommandAPIProtocol:

public final class CommandAPIProtocol {
	// Utility class should never be instantiated
	private CommandAPIProtocol() {}

	public static final String CHANNEL_NAME = "commandapi:plugin";

	private static final List<Function<ByteArrayDataInput, ? extends CommandAPIPacket>> idToPacket = new ArrayList<>();
	private static final Map<Class<? extends CommandAPIPacket>, Integer> packetToId = new HashMap<>();

	static {
		register(ClientToServerUpdateRequirementsPacket.class, ClientToServerUpdateRequirementsPacket::deserialize);
	}

	private static <Packet extends CommandAPIPacket> void register(Class<Packet> clazz, Function<ByteArrayDataInput, Packet> deserializer) {
		if (packetToId.containsKey(clazz)) {
			throw new IllegalStateException("Packet class \"" + clazz.getSimpleName() + "\" was already registered!");
		}

		int id = idToPacket.size();
		idToPacket.add(deserializer);
		packetToId.put(clazz, id);
	}

	public static CommandAPIPacket createPacket(int id, ByteArrayDataInput buffer) {
		if (id < 0 || id >= idToPacket.size()) return null;
		Function<ByteArrayDataInput, ? extends CommandAPIPacket> deserializer = idToPacket.get(id);
		return deserializer == null ? null : deserializer.apply(buffer);
	}

	public static int getId(Class<? extends CommandAPIPacket> clazz) {
		return packetToId.getOrDefault(clazz, -1);
	}
}

This class handles assigning 'ids' to the packets, which identifies packets in the byte input stream. Each packet class should be added to the protocol by putting it into the static initializer block at the top of the class. The register method registers a packet's class by giving it the next id, and linking that id to the deserialize method for that packet. The createPacket method can then be used to turn a byte input buffer into a packet, and getId will return the id for a given packet class. If the packet is not registered, these methods will return null and -1 respectively.

This class also holds the constant CHANNEL_NAME, which is set to "commandapi:plugin". This namespace identifier is included in the Plugin Message packet to show the packet is meant to be handled by the CommandAPI.

Sending Packets

Click to expand

Sending packets is handled by the CommandAPIMessenger#sendPacket method, which looks like this:

public abstract class CommandAPIMessenger<InputChannel, OutputChannel> {
	public void sendPacket(OutputChannel target, CommandAPIPacket packet) {
		// Inspired by net.minecraft.PacketEncoder
		int id = CommandAPIProtocol.getId(packet.getClass());
		if (id == -1) {
			throw new IllegalStateException(
				"Packet class \"" + packet.getClass().getSimpleName() +
					"\" is not registered in the CommandAPIProtocol! This packet cannot be sent."
			);
		}

		ByteArrayDataOutput output = ByteStreams.newDataOutput();

		// Write packet's id
		output.writeByte(id);
		// Write packet's data
		packet.write(output);

		// Send the bytes
		sendRawBytes(target, output.toByteArray());
	}
    
	protected abstract void sendRawBytes(OutputChannel target, byte[] bytes);
}

public class BukkitCommandAPIMessenger extends CommandAPIMessenger<Player, Player> {
    private final String channelName;
    private final JavaPlugin plugin;

    @Override
    public void sendRawBytes(Player target, byte[] bytes) {
        target.sendPluginMessage(this.plugin, this.channelName, bytes);
    }
}

public class VelocityCommandAPIMessenger extends CommandAPIMessenger<ChannelMessageSource, ChannelMessageSink> {
    private final ChannelIdentifier channel;

    @Override
    public void sendRawBytes(ChannelMessageSink target, byte[] bytes) {
        target.sendPluginMessage(this.channel, bytes);
    }
}

The sendPacket method uses CommandAPIProtocol#getId to get the id for the packet. If -1 is given as the id, then this packet is not registered and cannot be sent. Otherwise, the id is written to a byte array, followed by the packet's data. These bytes are then sent to the given target using the abstract sendRawBytes method. The platform implementation for each messenger takes care of getting the bytes sent.

Receiving Packets

Click to expand

The platform messengers know when a Plugin Message packet has been received by hooking into the events provided by the server APIs, as discussed in Plugin setup/shutdown. Those methods are implemented as so:

public class BukkitCommandAPIMessenger extends CommandAPIMessenger<Player, Player> implements PluginMessageListener {
    private final String channelName;

    @Override
    public void onPluginMessageReceived(String channel, @NotNull Player player, byte[] message) {
        // A plugin message was sent to Bukkit, check if it is for us
        if (!channel.equals(this.channelName)) return;

        // Handle the message
        messageReceived(player, message);
    }
}

public class VelocityCommandAPIMessenger extends CommandAPIMessenger<ChannelMessageSource, ChannelMessageSink> {
    private final ChannelIdentifier channel;

    @Subscribe
    public void onPluginMessageEvent(PluginMessageEvent event) {
        // A plugin message was sent to Velocity, check if it is for us
        if (!event.getIdentifier().equals(this.channel)) return;

        // We will handle this message, set the result so that it is not forwarded
        event.setResult(PluginMessageEvent.ForwardResult.handled());

        // Handle the message
        messageReceived(event.getSource(), event.getData());
    }
}

These methods verify the Plugin Message packet is meant for us by checking the channel identifier, then send the event source and data into CommandAPIMessenger#messageRecieved. That method looks like this:

public abstract class CommandAPIMessenger<InputChannel, OutputChannel> {
    private final CommandAPIPacketHandler<InputChannel> packetHandler;
    
    protected void messageReceived(InputChannel sender, byte[] input) {
        // Inspired by net.minecraft.PacketDecoder

        // Empty packet. Kinda weird, but nothing to do.
        if (input.length == 0) return;

        ByteArrayDataInput buffer = ByteStreams.newDataInput(input);

        int id;
        CommandAPIPacket packet;
        try {
            // Read the id
            id = buffer.readByte();
            // Use the id to find and use the correct deserialize method
            packet = CommandAPIProtocol.createPacket(id, buffer);
        } catch (IllegalStateException e) {
            throw new IllegalStateException("Exception while reading packet", e);
        }
        if (packet == null) throw new IllegalStateException("Unknown packet id: " + id);

        boolean extraBytes;
        try {
            buffer.readByte();
            extraBytes = true;
        } catch (IllegalStateException ignored) {
            // Buffer is out of bytes, as expected
            // There isn't a method to check this for some reason?
            // https://github.com/google/guava/issues/937
            extraBytes = false;
        }
        if (extraBytes) {
            throw new IllegalStateException(
                    "Packet was larger than expected! Extra bytes found after deserializing.\n" +
                            "Given: " + Arrays.toString(input) + ", Read: " + packet
            );
        }

        // Handle the packet
        packet.handle(sender, packetHandler);
    }
}

It reads one byte to be the packet id, then uses CommandAPIProtocol#createPacket to deserialize the packet with that id. This method will throw an exception if:

• CommandAPIProtocol#createPacket returns null, indicating the given packet id is unknown
• One of the read method calls on the ByteArrayDataInput throws an IllegalStateException, indicating that the packet tried reading past the available data
• There were bytes remaining in the input buffer after reading the packet data.

If the packet is successfully deserialized it will be handled. This is described in the next section, Handling Packets.

Handling Packets

Click to expand

The main class for handling packets is CommandAPIPacketHandler, which currently looks like this:

public interface CommandAPIPacketHandler<InputChannel> {
	void handleUpdateRequirementsPacket(InputChannel sender, ClientToServerUpdateRequirementsPacket packet);
}

Each packet that needs a platform-specific implementation should have a corresponding method here. For example, the ClientToServerUpdateRequirements packet calls the handleUpdateRequirementsPacket method, which is implemented like so:

public class ClientToServerUpdateRequirementsPacket implements CommandAPIPacket {
    @Override
    public <InputChannel> void handle(InputChannel sender, CommandAPIPacketHandler<InputChannel> packetHandler) {
        packetHandler.handleUpdateRequirementsPacket(sender, this);
    }
}

public class BukkitPacketHandler implements CommandAPIPacketHandler<Player> {
	@Override
	public void handleUpdateRequirementsPacket(Player sender, ClientToServerUpdateRequirementsPacket packet) {
		CommandAPI.updateRequirements(sender);
	}
}

public class VelocityPacketHandler implements CommandAPIPacketHandler<ChannelMessageSource> {
	@Override
	public void handleUpdateRequirementsPacket(ChannelMessageSource sender, ClientToServerUpdateRequirementsPacket packet) {
		// This client-to-server packet should never be sent by the server
		if (sender instanceof ServerConnection) return;

		// This packet may be sent to Velocity when the CommandAPI is a client mod
		// We shall update their requirements as requested
		Player player = (Player) sender;
		CommandAPI.updateRequirements(player);
	}
}

Both of these methods handle the packet by updating requirements for the client that sent the packet, though Velocity also has to account for the fact that ServerConnection might send it packets.

Fixing CommandAPIVelocity#updateRequirements

Click to expand

So, finally, CommandAPIVelocity can use this messaging system to fix its updateRequirements method. It does this using the ClientToServerUpdateRequirementsPacket, which looks like this:

public class ClientToServerUpdateRequirementsPacket implements CommandAPIPacket {
    public static ClientToServerUpdateRequirementsPacket deserialize(ByteArrayDataInput ignored) {
        // Nothing to read
        return new ClientToServerUpdateRequirementsPacket();
    }

    public static ClientToServerUpdateRequirementsPacket create() {
        // No parameters to write
        return new ClientToServerUpdateRequirementsPacket();
    }

    private ClientToServerUpdateRequirementsPacket() {

    }

    @Override
    public void write(ByteArrayDataOutput buffer) {
        // Nothing to write
    }

    @Override
    public <InputChannel> void handle(InputChannel sender, CommandAPIPacketHandler<InputChannel> packetHandler) {
        packetHandler.handleUpdateRequirementsPacket(sender, this);
    }
}

This packet is very simple, since it doesn't encode any data. It has nothing to read or write, and no parameters to create it. The server simply needs to receive this packet from the client, then should send the Commands packet back to that client.

CommandAPIVelocity uses this like so:

public class CommandAPIVelocity {
	private VelocityCommandAPIMessenger messenger;

	public VelocityCommandAPIMessenger getMessenger() {
		return messenger;
	}

	@Override
	public void updateRequirements(AbstractPlayer<?> playerWrapper) {
		Player player = (Player) playerWrapper.getSource();
		Optional<ServerConnection> optionalServer = player.getCurrentServer();

		// If the ServerConnection is not present, I think the player is not currently connected to any server.
		//  Therefore, they don't have any commands, so they don't need to be updated
		if(optionalServer.isEmpty()) return;

		getMessenger().sendPacket(optionalServer.get(), ClientToServerUpdateRequirementsPacket.create());
	}
}

It creates a new ClientToServerUpdateRequirementsPacket and sends it to the server the player is connected to. The system discussed previously handles this like so:

On Velocity

• CommandAPIMessenger#sendPacket is called
  • CommandAPIProtocol#getId returns 0 as the id for this packet
  • ClientToServerUpdateRequirementsPacket doesn't have anything to write, so the data is simply new byte[]{0}
• VelocityCommandAPIMessenger#sendRawBytes sends the data on the commandapi:plugin channel
• Velocity creates and sends a Plugin Message packet

On Bukkit

• Bukkit receives a Plugin Message packet
• BukkitCommandAPIMessenger#onPluginMessageReceived is called to handle the event
  • Verifies the message is on the commandapi:plugin channel
• CommandAPIMessenger#messageReceivied is called to handle the byte array new byte[]{0}
  • Reads packet id 0
  • CommandAPIProtocol#createPacket is called to deserialize the remaining data
    • CommandAPIProtocol finds the deserialize method for ClientToServerUpdateRequirementsPacket
    • ClientToServerUpdateRequirementsPacket#deserialize has nothing to read, so it simply creates its new packet
• CommandAPIPacket#handle is called
• CommandAPIPacketHandler#handleUpdateRequirementsPacket is called to handle this packet
• BukkitPacketHandler#handleUpdateRequirementsPacket is called to handle the packet given the player who sent it
  • CommandAPI#updateRequirements is called for the player who sent the packet
  • CommandAPIBukkit#updateRequirements triggers the Bukkit server to resend its Commands packet
• Bukkit rebuilds a Commands packet, reevaluating the player's requirements, then sends it

On Velocity

• Velocity intercepts the Commands packet
• Velocity re-injects its commands, reevaluating the player's requirements, then sends it

Other notes

CommandAPIVelocityConfig now requires the plugin object when it is being constructed, since VelocityCommandAPIMessenger needs it to register events.

There are tests for CommandAPIMessenger and related features in commandapi-bukkit-test-tests as CommandAPIMessengerTests. Taking a look at those might help clarify what this system is intended to do.

Todo

These are just some details to consider before releasing this PR. It would be annoying to change the protocol after releasing it while keeping backwards compatibility, so it would be good to find a solid system first.

Public protocol details:

• Is commandapi:plugin a good Plugin Message packet channel name? I'm not sure if there is a convention for channel names. Maybe commandapi and plugin are overly generic and could conflict with other channels.
• Is this a good encoding scheme? The packet id, data... system is used by Minecraft, but maybe there is a better way to encode the data.
• Dose it make sense to hand out packet ids sequentially? Right now, the first CommandAPIPacket registered in CommandAPIProtocol is given id 0, then id 1, etc. Again, this is how Minecraft does it, but maybe it would make sense to hand out ids in a way that is independent of the order they are registered.
• What should happen when a new packet is sent to an old version of the plugin that doesn't know about that packet? Right now, an exception would be thrown since the packet id is invalid.
• Should ClientToServerUpdateRequirementsPacket be given id 0? Perhaps there is a more important packet to put 'first'.
• What should happen if there is not a CommandAPI plugin on the backend server to receive the ClientToServerUpdateRequirementsPacket? Currently, nothing happens and the requirements are not updated.

And of course, code review! If you think there's something weird in the code, point it out and I'll try to fix it.

[JorelAli]

There are still two files I have yet to stare at in full detail (CommandAPIProtocol and CommandAPIMessengerTests). I have yet to stare at packet ID usage and figuring out how the tests work (looks like you've made some very good use of Mockito using features I've never seen before!)

[JorelAli]

We don't need this temporary variable, do we? This does the same thing:

packetToId.put(clazz, idToPacket.size());
idToPacket.add(deserializer);

[willkroboth]

This is still present in the code

CommandAPI/commandapi-core/src/main/java/dev/jorel/commandapi/network/CommandAPIProtocol.java

Lines 108 to 110 in a264fec

	int id = idToPacket.size();
	idToPacket.add(deserializer);
	packetToId.put(clazz, id);

I think it might make it clearer that the id is the next one in the sequence, though it's probably not much different.
I'm not sure if either is 'best', I could go either way.

[JorelAli]

To answer some of the questions in the original post:

Is commandapi:plugin a good Plugin Message packet channel name? I'm not sure if there is a convention for channel names. Maybe commandapi and plugin are overly generic and could conflict with other channels.

I think commandapi is fine, but plugin is a bit generic. Perhaps commandapi:update_requirements might be better - tailoring it specifically to what it does? (I foresee a future where we may want other CommandAPI channels and changing channel names in the future would be a nightmare, so being more specific is almost certainly a better thing to do at this stage)

Is this a good encoding scheme? The packet id, data... system is used by Minecraft, but maybe there is a better way to encode the data.

It works, but it might be in our best interest to provide a version byte as well, so we'd have version, packet id, data.... Just in case. We can choose to ignore this byte, but if we ever in a super far future ever need to track packet data structure versions, we'd have a mechanism to do so.

Does it make sense to hand out packet ids sequentially?

I'll get round to this after I stare at how we do sequential packet IDs.

What should happen when a new packet is sent to an old version of the plugin that doesn't know about that packet? Right now, an exception would be thrown since the packet id is invalid.

We should throw an exception if a new packet is sent to an old version of the plugin that doesn't know about that packet however I think I'm misunderstanding the question. How would the old plugin know the packet ID is invalid if the packet ID keeps incrementing?

Should ClientToServerUpdateRequirementsPacket be given id 0? Perhaps there is a more important packet to put 'first'.

I don't see any reason why id 0 is a bad thing, let's keep it for now. If we want to have "more important packets", we'd add a new channel (e.g. adding a commandapi:init channel - inspired by World downloader). I doubt we'd have to do this at this stage however, unless we want a handshaking system so we're not sending versions with every single packet?

What should happen if there is not a CommandAPI plugin on the backend server to receive the ClientToServerUpdateRequirementsPacket? Currently, nothing happens and the requirements are not updated.

Consider adding a timeout feature. If we send Velocity -> Bukkit -> Velocity and we don't hear the Bukkit -> Velocity response after some time, we should probably put a warning in Velocity's console.

[willkroboth]

I think commandapi is fine, but plugin is a bit generic. Perhaps commandapi:update_requirements might be better - tailoring it specifically to what it does?

Well, the system is meant to expand to other use cases, not just update_requirements. But yeah, be more specific to differentiate it from potential initialize or handshake channels. I would say this channel is supposed to be 'packets while the server is running'. Maybe commandapi:play, which is what I think Minecraft calls its 'packets while the server is running' protocol.

I'll get round to this after I stare at how we do sequential packet IDs.

We should throw an exception if a new packet is sent to an old version of the plugin that doesn't know about that packet however I think I'm misunderstanding the question. How would the old plugin know the packet ID is invalid if the packet ID keeps incrementing?

I explained this a bit more here on discord. It sounds like when people first hear 'sequential packet ids`, there is a misconception that each packet gets a new id. So, like:

INCORRECT

Send PacketA -> id 0
Send PacketA -> id 1
Send PacketB -> id 2
Send PacketA -> id 3

Instead, packet ids are assigned to each class of packet, and the same type of packet always gets the same id. So, like:

CORRECT

Register PacketA -> id 0
Register PacketB -> id 1

Send PacketA -> id 0
Send PacketA -> id 0
Send PacketB -> id 1
Send PacketA -> id 0

'sequential packet ids' happens when the CommandAPIProtocol class is statically initialized. The id is used to tell what type of packet needs to be created from the network. Both Velocity and Bukkit would load A -> 0, B -> 1, and they agree what the id bytes mean.

So, if version 1 loaded A -> 0 and version 2 loaded A -> 0, B -> 1, when V2 sends B to V1, V1 doesn't know about packet id 1 and throws an exception.

I doubt we'd have to do this at this stage however, unless we want a handshaking system so we're not sending versions with every single packet?

Consider adding a timeout feature. If we send Velocity -> Bukkit -> Velocity and we don't hear the Bukkit -> Velocity response after some time, we should probably put a warning in Velocity's console.

Yeah, a handshake system could be good for both the version problem and the no-response problem. A packet on commandapi:handshake could be used to establish what protocol version is being used. The plugin with a higher version could then make sure not to send any packets the older version couldn't understand. If the response never comes, then the plugin would know not to send any packets because they would never be understood. I really like this idea and think I have a good idea how to implement it.

[willkroboth]

Okay, there has been a massive commit: 196fc7e. Most of the basic ideas are still there, but here's everything new to know about.

---

Handling ClientToServerUpdateRequirementsPacket being ignored

Click to expand

The main point of this commit was to solve the problem where nothing would happen if the ClientToServerUpdateRequirementsPacket was sent to a backend server that wasn't running the CommandAPI. The Commands packet would not be sent in response, and there wouldn't be any errors to indicate what had happened.

Now, ClientToServerUpdateRequirementsPacket looks something like this:

public class ClientToServerUpdateRequirementsPacket implements CommandAPIPacket {
	@Override
	public void write(FriendlyByteBuffer buffer, Object target, int protocolVersion) throws ProtocolVersionTooOldException {
		if (protocolVersion == 0) {
			throw ProtocolVersionTooOldException.whileSending(target, protocolVersion,
				"CommandAPI version 9.0.4 or greater is required to receive ClientToServerUpdateRequirementsPacket"
			);
		}
		// Nothing to write
	}
}

It still doesn't have any data to write, but if the protocol version is 0 (more on that later), it will throw a ProtocolVersionTooOldException explaining that the backend server needs to install CommandAPI version 9.0.4 or later to receive messages. So, if the ClientToServerUpdateRequirementsPacket will be ignored, there is an exception as desired.

Protocol Version exchange

Click to expand

In order to know that the ClientToServerUpdateRequirementsPacket will be ignored, we need to know that the backend server has a "protocol version" of 0. This is handled by the subclasses of CommandAPIMessenger and the SetVersionPacket.

First, the SetVersionPacket, which looks like this:

public class SetVersionPacket implements CommandAPIPacket {
	public static SetVersionPacket deserialize(FriendlyByteBuffer buffer) {
		int protocolVersion = buffer.readVarInt();
		return new SetVersionPacket(protocolVersion);
	}

	public static SetVersionPacket create(int protocolVersion) {
		return new SetVersionPacket(protocolVersion);
	}

	private final int protocolVersion;

	private SetVersionPacket(int protocolVersion) {
		this.protocolVersion = protocolVersion;
	}

	public int getProtocolVersion() {
		return protocolVersion;
	}

	@Override
	public void write(FriendlyByteBuffer buffer, Object target, int protocolVersion) {
		buffer.writeVarInt(this.protocolVersion);
	}
}

This is pretty simple. The SetVersionPacket contains an int that represents a protocol version. The sender puts their protocol version in the packet and sends it to a target.

CommandAPIMessenger handles sending and storing the information from SetVersionPacket. The process depends on hooking into platform-specific events, so the platform implementations handle it. First, BukkitCommandAPIMessenger:

public class BukkitCommandAPIMessenger extends CommandAPIMessenger<Player, Player> implements PluginMessageListener, Listener {
	private final JavaPlugin plugin;
	private final Map<Player, Integer> protocolVersionPerPlayer;

	public BukkitCommandAPIMessenger(JavaPlugin plugin) {
		// ...
		this.plugin = plugin;
		this.protocolVersionPerPlayer = new HashMap<>();
		// ...

		Bukkit.getPluginManager().registerEvents(this, this.plugin);
	}

	@EventHandler
	public void onPlayerRegisterChannel(PlayerRegisterChannelEvent event) {
		this.sendPacket(event.getPlayer(), SetVersionPacket.create(CommandAPIProtocol.PROTOCOL_VERSION));
	}

	@EventHandler
	public void onPlayerLeave(PlayerQuitEvent event) {
		this.protocolVersionPerPlayer.remove(event.getPlayer());
	}

	public void setProtocolVersion(Player sender, int protocolVersion) {
		this.protocolVersionPerPlayer.put(sender, protocolVersion);
	}

	@Override
	public int getConnectionProtocolVersion(Player target) {
		return this.protocolVersionPerPlayer.getOrDefault(target, 0);
	}
}

When the player joins the server, they can't actually be sent plugin messages immediately. The PluginMessage packet is assigned to the PLAY protocol state. When events like Bukkit's PlayerJoinEvent are fired, the connection is still in a LOGIN state (There are Login Plugin packets, but we don't worry about that). So, you have to use some more "obscure" events.

On Bukkit, we listen for the PlayerRegisterChannelEvent. This event happens after a Player finishes logging onto the server when their plugin channels get registered, which actually allows messages to be sent. So, as soon as a Player can be sent plugin messages, we send them SetVersionPacket. The protocol version for the current instance is held as CommandAPIProtocol.PROTOCOL_VERSION, so that's what is put in the packet.

When Bukkit receives a SetVersionPacket from a Player, the packet handling code calls BukkitCommandAPIMessenger#setProtocolVersion. The received protocol version is put into the Map<Player, Integer> protocolVersionPerPlayer for future reference.

If anyone needs to know the protocol version for any Player, they can call the getConnectionProtocolVersion method. This retrieves the stored protocol version from the protocolVersionPerPlayer map. If the SetVersionPacket was never sent by this player, the default version is 0.

Finally, when the player disconnects (PlayerQuitEvent), they get removed from the protocolVersionPerPlayer map. If they're not connected, they won't receive messages.

Remember here that connections from both real Player clients and a proxy like a Velocity look the same to Bukkit. So, this does consider the future when CommandAPI might be ported to a client-side mod, but it also sends the version to Velocity.

VelocityCommandAPIMessenger is mostly the same ideas, just with the Velocity objects:

public class VelocityCommandAPIMessenger extends CommandAPIMessenger<ChannelMessageSource, ChannelMessageSink> {
	private final Object plugin;
	private final ProxyServer proxy;
	private final Map<ChannelMessageSink, Integer> protocolVersionPerOutgoingChannel;
	private final Map<Player, ServerConnection> previousServerConnectionPerPlayer;

	public VelocityCommandAPIMessenger(Object plugin, ProxyServer proxy) {
		// ...
		this.plugin = plugin;
		this.proxy = proxy;
		this.protocolVersionPerOutgoingChannel = new HashMap<>();
		this.previousServerConnectionPerPlayer = new HashMap<>();
		// ...

		this.proxy.getEventManager().register(this.plugin, this);
	}

	@Subscribe
	public void onServerConnected(ServerPostConnectEvent event) {
		Player player = event.getPlayer();

		this.protocolVersionPerOutgoingChannel.remove(this.previousServerConnectionPerPlayer.get(player));

		SetVersionPacket packet = SetVersionPacket.create(CommandAPIProtocol.PROTOCOL_VERSION);

		this.sendPacket(player, packet);

		ServerConnection newServer = player.getCurrentServer().get();
		this.sendPacket(newServer, packet);
		this.previousServerConnectionPerPlayer.put(player, newServer);
	}

	@Subscribe
	public void onPlayerLeave(DisconnectEvent event) {
		Player player = event.getPlayer();
		this.protocolVersionPerOutgoingChannel.remove(player);
		player.getCurrentServer().ifPresent(this.protocolVersionPerOutgoingChannel::remove);

		this.previousServerConnectionPerPlayer.remove(player);
	}

	public void setServerProtocolVersion(ServerConnection server, int protocolVersion) {
		this.protocolVersionPerOutgoingChannel.put(server, protocolVersion);
	}

	public void setPlayerProtocolVersion(Player player, int protocolVersion) {
		this.protocolVersionPerOutgoingChannel.put(player, protocolVersion);
	}

	@Override
	public int getConnectionProtocolVersion(ChannelMessageSink target) {
		return this.protocolVersionPerOutgoingChannel.getOrDefault(target, 0);
	}
}

For player join, we listen to ServerPostConnectEvent, which conveniently fires once the player is done connecting to the server and is in a state that can send plugin messages. The SetVersionPacket gets sent in both directions, to the player and the server they are connecting to.

The ServerPostConnectEvent also fires when a player switches servers. In that case, we want to forget about the protocol version for their previous connection. The Map<Player, ServerConnection> previousServerConnectionPerPlayer is used to hold the previous connection of each player for this purpose. The new server the player connects to is also updated by this event.

One thing to note is that ServerPostConnectEvent is annotated @Beta. Velocity is pretty vague as to what this means. @Beta's javadocs say the feature may be removed in the future. I hope that doesn't happen because this event is exactly what we need here. I guess we'll just have to see what happens.

Since Velocity could receive a SetVersionPacket from either a Player or ServerConnection, there are two methods for setting the protocol version, setServerProtocolVersion and setPlayerProtocolVersion. The received protocol version is put into the Map<ChannelMessageSink, Integer> protocolVersionPerOutgoingChannel for future reference.

If anyone needs to know the protocol version for a Player or ServerConnection, they can call the getConnectionProtocolVersion method. This retrieves the stored protocol version from the protocolVersionPerOutgoingChannel map. If the SetVersionPacket was never sent by the player or server, the default version is 0.

Finally, when the player disconnects from the proxy (DisconnectEvent), they get removed from the protocolVersionPerOutgoingChannel map. Their current ServerConnection is also removed and the previousServerConnectionPerPlayer map is cleared for the Player.

Sharing ProtocolVersionTooOldExceptions

Click to expand

The ClientToServerUpdateRequirements packet can now throw a ProtocolVersionTooOldException when the target connection is unable to receive plugin messages. In the future, new packets should use this when the target for the packet is too old to receive their data. In those cases, the target version could be capable of receiving an error message. That's what the ProtocolVersionTooOldPacket is for.

public class ProtocolVersionTooOldPacket implements CommandAPIPacket {
	public static ProtocolVersionTooOldPacket deserialize(FriendlyByteBuffer buffer) {
		int protocolVersion = buffer.readVarInt();
		String reason = buffer.readString();
		return new ProtocolVersionTooOldPacket(protocolVersion, reason);
	}

	public static ProtocolVersionTooOldPacket create(int protocolVersion, String reason) {
		return new ProtocolVersionTooOldPacket(protocolVersion, reason);
	}

	private final int protocolVersion;
	private final String reason;

	private ProtocolVersionTooOldPacket(int protocolVersion, String reason) {
		this.protocolVersion = protocolVersion;
		this.reason = reason;
	}

	public int getProtocolVersion() {
		return protocolVersion;
	}

	public String getReason() {
		return reason;
	}

	@Override
	public void write(FriendlyByteBuffer buffer, Object target, int protocolVersion) {
		buffer.writeVarInt(this.protocolVersion);
		buffer.writeString(this.reason);
	}
}

This packet holds an int for the protocol version of the sender and a String for the reason of the message. When this packet is received, this information is used to construct an appropriate ProtocolVersionTooOldException on the other side.

The ProtocolVersionTooOldPacket may be sent by CommandAPIMessenger#sendPacket

public abstract class CommandAPIMessenger<InputChannel, OutputChannel> {
	public void sendPacket(OutputChannel target, CommandAPIPacket packet) {
		// ...

		try {
			packet.write(output, target, this.getConnectionProtocolVersion(target));
		} catch (ProtocolVersionTooOldException exception) {
			this.sendPacket(target, ProtocolVersionTooOldPacket.create(CommandAPIProtocol.PROTOCOL_VERSION, exception.getReason()));
			throw exception;
		}

		// ...
	}
}

If a packet's write method throws a ProtocolVersionTooOldException, CommandAPIMessenger quickly catches the exception and creates an appropriate ProtocolVesionTooOldPacket. It sends that to the target instead of the original packet, then rethrows the ProtocolVersionTooOldException so it can continue bubbling up.

Sending packets on multiple channels (CommandAPIProtocol changes)

Click to expand

The SetVersionPacket and ProtocolVersionTooOldPacket are set up to be sent on the commandapi:handshake channel, while ClientToServerUpdateRequirementsPacket is now being sent on the commandapi:play channel. A lot of the changes in the commit add support for multiple channels like this.

CommandAPIProtocol has the most changes. It is still responsible for defining the configuration of the protocol. It looks like this now:

public enum CommandAPIProtocol {
	// Protocol states
	HANDSHAKE("commandapi:handshake", new PacketSetBuilder()
		.register(SetVersionPacket.class, SetVersionPacket::deserialize)
		.register(ProtocolVersionTooOldPacket.class, ProtocolVersionTooOldPacket::deserialize)
	),
	PLAY("commandapi:play", new PacketSetBuilder()
		.register(ClientToServerUpdateRequirementsPacket.class, ClientToServerUpdateRequirementsPacket::deserialize)
	);

	// Global channel variables
	public static final int PROTOCOL_VERSION = 1;
	private static final Map<String, CommandAPIProtocol> channelIdentifierToProtocol;
	private static final Map<Class<? extends CommandAPIPacket>, CommandAPIProtocol> packetToProtocol;

	// Initialize static variables
	static {
		channelIdentifierToProtocol = new HashMap<>();
		packetToProtocol = new HashMap<>();

		CommandAPIProtocol previousProtocol;
		for (CommandAPIProtocol protocol : values()) {
			previousProtocol = channelIdentifierToProtocol.put(protocol.getChannelIdentifier(), protocol);
			if (previousProtocol != null) {
				throw new IllegalStateException("Protocols " + protocol + " and " + previousProtocol + " cannot " +
					"share the same channel identifier: \"" + protocol.channelIdentifier + "\"");
			}

			for (Class<? extends CommandAPIPacket> packetType : protocol.getAllPacketTypes()) {
				previousProtocol = packetToProtocol.put(packetType, protocol);
				if (previousProtocol != null) {
					throw new IllegalStateException("Packet class \"" + packetType.getSimpleName() + " is already " +
						"assigned to protocol " + previousProtocol + "\n" +
						"It cannot also be assigned to protocol " + protocol);
				}
			}
		}
	}

	// Individual channel variables
	private final String channelIdentifier;
	private final List<Function<FriendlyByteBuffer, ? extends CommandAPIPacket>> idToPacket;
	private final Map<Class<? extends CommandAPIPacket>, Integer> packetToId;

	// Initialize instance variables
	CommandAPIProtocol(String channelIdentifier, PacketSetBuilder packetSet) {
		this.channelIdentifier = channelIdentifier;

		this.idToPacket = packetSet.idToPacket;
		this.packetToId = packetSet.packetToId;
	}

	private static final class PacketSetBuilder {
		final List<Function<FriendlyByteBuffer, ? extends CommandAPIPacket>> idToPacket = new ArrayList<>();
		final Map<Class<? extends CommandAPIPacket>, Integer> packetToId = new HashMap<>();

		<Packet extends CommandAPIPacket> PacketSetBuilder register(Class<Packet> clazz, Function<FriendlyByteBuffer, Packet> deserializer) {
			if(clazz == null) throw new IllegalStateException("Class cannot be null");
			if(deserializer == null) throw new IllegalStateException("Deserializer cannot be null");
			if (packetToId.containsKey(clazz)) {
				throw new IllegalStateException("Packet class \"" + clazz.getSimpleName() + "\" is already registered");
			}

			int id = idToPacket.size();
			idToPacket.add(deserializer);
			packetToId.put(clazz, id);

			return this;
		}
	}

	// Use static variables

	public static Set<String> getAllChannelIdentifiers() {
		return channelIdentifierToProtocol.keySet();
	}

	@Nullable
	public static CommandAPIProtocol getProtocolForPacket(Class<? extends CommandAPIPacket> clazz) {
		return packetToProtocol.get(clazz);
	}

	@Nullable
	public static CommandAPIProtocol getProtocolForChannel(String channelIdentifier) {
		return channelIdentifierToProtocol.get(channelIdentifier);
	}

	// Use instance variables

	public String getChannelIdentifier() {
		return channelIdentifier;
	}

	public Set<Class<? extends CommandAPIPacket>> getAllPacketTypes() {
		return packetToId.keySet();
	}

	@Nullable
	public CommandAPIPacket createPacket(int id, FriendlyByteBuffer buffer) {
		if (id < 0 || id >= idToPacket.size()) return null;
		return idToPacket.get(id).apply(buffer);
	}

	public int getId(Class<? extends CommandAPIPacket> clazz) {
		return packetToId.getOrDefault(clazz, -1);
	}
}

The different channels are represented by enum values, currently HANDSHAKE and PLAY. They are given a String for their channel identifier and a PacketSetBuilder. The PacketSetBuilder#register method is used to link packet classes to their deserialize method.

A static initializer block sets up two additional maps, Map<String, CommandAPIProtocol> channelIdentifierToProtocol and Map<Class<? extends CommandAPIPacket>, CommandAPIProtocol> packetToProtocol. These maps are used by the static getProtocolForPacket and getProtocolForChannel methods. These methods retrieve the appropriate CommandAPIProtocol object for handling a specified packet class or channel identifier.

The createPacket and getId methods are the same as before. They select and apply the appropriate deserializer and link a packet class to its id respectively. This only difference is that instead of being static, they are now instance methods. They use the List<Function<FriendlyByteBuffer, ? extends CommandAPIPacket>> idToPacket and Map<Class<? extends CommandAPIPacket>, Integer> packetToId objects set up when registering to each PacketBuilder.

Other methods provide basic information about the channels.

Sending packets on multiple channels (CommandAPIMessenger changes)

Click to expand

CommandAPIMessenger and its implementations have changed to listen and send on multiple channels. First, the initializing and closing code:

public abstract class CommandAPIMessenger<InputChannel, OutputChannel> {
	private final CommandAPIPacketHandlerProvider<InputChannel> packetHandlerProvider;

	protected CommandAPIMessenger(CommandAPIPacketHandlerProvider<InputChannel> packetHandlerProvider) {
		this.packetHandlerProvider = packetHandlerProvider;
	}

	public abstract void close();
}

public class BukkitCommandAPIMessenger extends CommandAPIMessenger<Player, Player> implements PluginMessageListener... {
	private final JavaPlugin plugin;

	public BukkitCommandAPIMessenger(JavaPlugin plugin) {
		super(new BukkitPacketHandlerProvider());
		this.plugin = plugin;
                // ...

		Messenger messenger = Bukkit.getServer().getMessenger();
		for (String channelIdentifier : CommandAPIProtocol.getAllChannelIdentifiers()) {
			messenger.registerIncomingPluginChannel(this.plugin, channelIdentifier, this);
			messenger.registerOutgoingPluginChannel(this.plugin, channelIdentifier);
		}

		// ...
	}

	@Override
	public void close() {
		Messenger messenger = Bukkit.getServer().getMessenger();
		for (String channelIdentifier : CommandAPIProtocol.getAllChannelIdentifiers()) {
			messenger.unregisterIncomingPluginChannel(this.plugin, channelIdentifier);
			messenger.unregisterOutgoingPluginChannel(this.plugin, channelIdentifier);
		}
		// ...
	}
}

public class VelocityCommandAPIMessenger extends CommandAPIMessenger<ChannelMessageSource, ChannelMessageSink> {
	private final Object plugin;
	private final ProxyServer proxy;

	public VelocityCommandAPIMessenger(Object plugin, ProxyServer proxy) {
		super(new VelocityPacketHandlerProvider());
		this.plugin = plugin;
		this.proxy = proxy;

		// ...

		ChannelRegistrar registrar = this.proxy.getChannelRegistrar();
		for (String channelIdentifier : CommandAPIProtocol.getAllChannelIdentifiers()) {
			registrar.register(MinecraftChannelIdentifier.from(channelIdentifier));
		}
		this.proxy.getEventManager().register(this.plugin, this);
	}

	@Override
	public void close() {
		ChannelRegistrar registrar = this.proxy.getChannelRegistrar();
		for (String channelIdentifier : CommandAPIProtocol.getAllChannelIdentifiers()) {
			registrar.unregister(MinecraftChannelIdentifier.from(channelIdentifier));
		}
		this.proxy.getEventManager().unregisterListener(this.plugin, this);
	}
}

Instead of providing an implementation of CommandAPIPacketHandler to CommandAPIMessenger, the subclasses now provide a CommandAPIPacketHandlerProvider for the platform (more on this in the next section). When initializing and closing, the platform implementations loop through CommandAPIProtocol.getAllChannelIdentifiers() to register and unregister all the channels defined by CommandAPIProtocol.

Receiving a packet now looks like this:

public class BukkitCommandAPIMessenger extends CommandAPIMessenger<Player, Player> ... {
	@Override
	public void onPluginMessageReceived(@NotNull String channel, @NotNull Player player, byte[] message) {
		CommandAPIProtocol protocol = CommandAPIProtocol.getProtocolForChannel(channel);
		if (protocol == null) return;

		messageReceived(protocol, player, message);
	}
}

public class VelocityCommandAPIMessenger extends CommandAPIMessenger<ChannelMessageSource, ChannelMessageSink> {
	@Subscribe
	public void onPluginMessageEvent(PluginMessageEvent event) {
		String channel = event.getIdentifier().getId();
		CommandAPIProtocol protocol = CommandAPIProtocol.getProtocolForChannel(channel);
		if (protocol == null) return;

		event.setResult(PluginMessageEvent.ForwardResult.handled());

		messageReceived(protocol, event.getSource(), event.getData());
	}
}

public abstract class CommandAPIMessenger<InputChannel, OutputChannel> {
	private final CommandAPIPacketHandlerProvider<InputChannel> packetHandlerProvider;

	protected void messageReceived(CommandAPIProtocol protocol, InputChannel sender, byte[] input) {
		// ...

		int id = buffer.readVarInt();
		CommandAPIPacket packet = protocol.createPacket(id, buffer);
		
		// ...

		packetHandlerProvider.getHandlerForProtocol(protocol).handlePacket(sender, packet);
	}
}

When the Bukkit or Velocity server receives a plugin message on any channel, they call our event listener. We get the configuration for the channel using CommandAPIProtocol#getProtocolForChannel. If this isn't a message for the CommandAPI, that method will return null, so we just return from the event. If a CommandAPIProtocol enum type was found, then it and the packet data get sent to CommandAPIMessenger#messageRecieved.

CommandAPIMessenger uses the instance method createPacket of the given CommandAPIProtocol to create its packet. This is necessary because the packet for an id depends on the channel it is sent on. For example, SetVersionPacket and ClientToServerUpdateRequirementsPacket both use id 0, just on commandapi:handshake and commandapi:play respectively. Once the packet has been read successfully, packetHandlerProvider is used to handle the packet (more on that in the next section).

Finally, sending a packet now looks like this:

public abstract class CommandAPIMessenger<InputChannel, OutputChannel> {
	public void sendPacket(OutputChannel target, CommandAPIPacket packet) {
		Class<? extends CommandAPIPacket> packetType = packet.getClass();

		CommandAPIProtocol protocol = CommandAPIProtocol.getProtocolForPacket(packetType);
		int id = protocol.getId(packetType);

		// ...

		sendRawBytes(protocol, target, output.toByteArray());
	}

	protected abstract void sendRawBytes(CommandAPIProtocol protocol, OutputChannel target, byte[] bytes);
}

public class BukkitCommandAPIMessenger extends CommandAPIMessenger<Player, Player> ... {
	private final JavaPlugin plugin;

	@Override
	public void sendRawBytes(CommandAPIProtocol protocol, Player target, byte[] bytes) {
		target.sendPluginMessage(this.plugin, protocol.getChannelIdentifier(), bytes);
	}
}

public class VelocityCommandAPIMessenger extends CommandAPIMessenger<ChannelMessageSource, ChannelMessageSink> {
	@Override
	public void sendRawBytes(CommandAPIProtocol protocol, ChannelMessageSink target, byte[] bytes) {
		target.sendPluginMessage(MinecraftChannelIdentifier.from(protocol.getChannelIdentifier()), bytes);
	}
}

When CommandAPIMessenger needs to send a packet, it uses CommandAPIProtocol#getProtocolForPacket to get the protocol that should be used for that packet. It then uses that protocol object to get the id for the packet.

After writing the packet's data, the protocol object is passed into sendRawBytes. The Bukkit and Velocity messengers use the channel identifier from this object to send the packets on the right channel.

Sending packets on multiple channels (packet handling changes)

Click to expand

CommandAPIMessenger now holds a CommandAPIPacketHandlerProvider object. That interface looks like this:

public interface CommandAPIPacketHandlerProvider<InputChannel> {
	default CommandAPIPacketHandler<InputChannel> getHandlerForProtocol(CommandAPIProtocol protocol) {
		return switch (protocol) {
			case HANDSHAKE -> getHandshakePacketHandler();
			case PLAY -> getPlayPacketHandler();
		};
	}

	HandshakePacketHandler<InputChannel> getHandshakePacketHandler();

	PlayPacketHandler<InputChannel> getPlayPacketHandler();
}

Each CommandAPIProtocol enum type gets its own CommandAPIPacketHandler, and CommandAPIPacketHandlerProvider provides those handlers for each protocol. Packet handling changes for each platform, so this interface is implemented on each platform, like so:

public class BukkitPacketHandlerProvider implements CommandAPIPacketHandlerProvider<Player> {
	private final BukkitHandshakePacketHandler handshakePacketHandler = new BukkitHandshakePacketHandler();

	@Override
	public BukkitHandshakePacketHandler getHandshakePacketHandler() {
		return handshakePacketHandler;
	}

	private final BukkitPlayPacketHandler playPacketHandler = new BukkitPlayPacketHandler();

	@Override
	public BukkitPlayPacketHandler getPlayPacketHandler() {
		return playPacketHandler;
	}
}

public class VelocityPacketHandlerProvider implements CommandAPIPacketHandlerProvider<ChannelMessageSource> {
	private final VelocityHandshakePacketHandler handshakePacketHandler = new VelocityHandshakePacketHandler();

	@Override
	public VelocityHandshakePacketHandler getHandshakePacketHandler() {
		return handshakePacketHandler;
	}

	private final VelocityPlayPacketHandler playPacketHandler = new VelocityPlayPacketHandler();

	@Override
	public VelocityPlayPacketHandler getPlayPacketHandler() {
		return playPacketHandler;
	}
}

These simply keep track of the platform-specific implementations of each packet handler and provides them when necessary.

The PacketHandler interfaces look like this:

public interface CommandAPIPacketHandler<InputChannel> {
	void handlePacket(InputChannel sender, CommandAPIPacket packet);
}

public interface HandshakePacketHandler<InputChannel> extends CommandAPIPacketHandler<InputChannel> {
	@Override
	default void handlePacket(InputChannel sender, CommandAPIPacket packet) {
		if (packet instanceof SetVersionPacket p) handleSetVersionPacket(sender, p);
		if (packet instanceof ProtocolVersionTooOldPacket p) handleProtocolVersionTooOldPacket(sender, p);
	}

	void handleSetVersionPacket(InputChannel sender, SetVersionPacket packet);

	default void handleProtocolVersionTooOldPacket(InputChannel sender, ProtocolVersionTooOldPacket packet) {
		throw ProtocolVersionTooOldException.received(sender, packet.getProtocolVersion(), packet.getReason());
	}
}

public interface PlayPacketHandler<InputChannel> extends CommandAPIPacketHandler<InputChannel> {
	@Override
	default void handlePacket(InputChannel sender, CommandAPIPacket packet) {
		if (packet instanceof ClientToServerUpdateRequirementsPacket p) handleUpdateRequirementsPacket(sender, p);
	}

	void handleUpdateRequirementsPacket(InputChannel sender, ClientToServerUpdateRequirementsPacket packet);
}

When HandshakePacketHandler or PlayPacketHandler need to handle a packet, they check for each packet class they are responsible for, and call an appropriate abstract method for handling that packet.

Note in the case of handling a ProtocolVersionTooOldPacket, the implementation does not depend on the platform, so it is handled directly in HandshakePacketHandler instead of a subclass.

Of course, HandshakePacketHandler and PlayPacketHandler are implemented on the platform level:

public class BukkitHandshakePacketHandler implements HandshakePacketHandler<Player> {
	@Override
	public void handleSetVersionPacket(Player sender, SetVersionPacket packet) {
		CommandAPIBukkit.get().getMessenger().setProtocolVersion(sender, packet.getProtocolVersion());
	}
}

public class BukkitPlayPacketHandler implements PlayPacketHandler<Player> {
	@Override
	public void handleUpdateRequirementsPacket(Player sender, ClientToServerUpdateRequirementsPacket packet) {
		CommandAPI.updateRequirements(sender);
	}
}

public class VelocityHandshakePacketHandler implements HandshakePacketHandler<ChannelMessageSource> {
	@Override
	public void handleSetVersionPacket(ChannelMessageSource sender, SetVersionPacket packet) {
		int protocolVersion = packet.getProtocolVersion();
		VelocityCommandAPIMessenger messenger = CommandAPIVelocity.get().getMessenger();

		if (sender instanceof ServerConnection server) messenger.setServerProtocolVersion(server, protocolVersion);
		if (sender instanceof Player player) messenger.setPlayerProtocolVersion(player, protocolVersion);
	}
}

public class VelocityPlayPacketHandler implements PlayPacketHandler<ChannelMessageSource> {
	@Override
	public void handleUpdateRequirementsPacket(ChannelMessageSource sender, ClientToServerUpdateRequirementsPacket packet) {
		if (sender instanceof ServerConnection) return;

		Player player = (Player) sender;
		CommandAPI.updateRequirements(player);
	}
}

These classes implement the appropriate platform-specific behavior for each of the packets.

FriendlyByteBuffer

Click to expand

This is a smaller, behind-the-scenes change, but byte array classes from the com.google.common.io package (ByteArrayDataInput, ByteArrayDataOutput, ByteStreams) are not being used anymore. I originally used these classes because the plugin messaging tutorials I followed used them (Bukkit Velocity), but there are various issues with these classes.

One problem, as google/guava#937 states, ByteArrayDataInput doesn't have a method that lets you know how many bytes are left to read. This created the following goofy code when making sure there wasn't any extra data after reading a packet:

boolean extraBytes;
try {
	buffer.readByte();
	extraBytes = true;
} catch (IllegalStateException ignored) {
	// Buffer is out of bytes, as expected
	// There isn't a method to check this for some reason?
	// https://github.com/google/guava/issues/937
	extraBytes = false;
}
if (extraBytes) {
	throw new IllegalStateException(
		"Packet was larger than expected! Extra bytes found after deserializing.\n" +
			"Given: " + Arrays.toString(input) + ", Read: " + packet
	);
}

With a countReadableBytes method, this code makes a lot more sense:

if (buffer.countReadableBytes() != 0) {
	// If the packet didn't read all the bytes it was given, we have a strange miscommunication
	throw new IllegalStateException(
		"Packet was larger than expected! " + buffer.countReadableBytes() + " extra byte(s) found after deserializing.\n" +
			"Given: " + Arrays.toString(input) + ", Read: " + packet
	);
}

One comment on google/guava#937 also says this:

At this point, I feel that ByteArrayData*put is more or less a failed experiment.

This implies they aren't really intended to be used. Various methods that were being used like ByteStreams#newDataInput are also marked as @Beta, implying an inherent instability.

Instead, inspired by net.minecraft.network.FriendlyByteBuf, I created my own class, FriendlyByteBuffer. FriendlyByteBuffer acts as both a byte input and output stream, so it is used for both packet writing and deserializing.

One benefit of having our own class is that we can add methods for special data types used in Minecraft's protocol. Currently, only the VarInt is implemented, but Minecraft has a few other special objects in its protocol that can be handled in the same way. I like this strategy better than something like the ProtocolUtils class Velocity uses internally.

Tests

Click to expand

Of course, the Bukkit tests have been updated to verify FriendlyByteBuffer, sending packets on multiple channels, and the new packets work. All tests for these network classes can be found in the dev.jorel.commandapi.test.network package.

Most important to note, there is now a NetworkTestBase class. It extends TestBase, but also provides extra methods and setup that help when testing packets. Each packet class gets its own test class that extends NetworkTestBase. For example, ProtocolVersionTooOldPacketTests:

class ProtocolVersionTooOldPacketTests  extends NetworkTestBase {

	/*********
	 * Setup *
	 *********/

	@BeforeEach
	public void setUp() {
		super.setUp();
	}

	@AfterEach
	public void tearDown() {
		super.tearDown();
	}

	/*********
	 * Tests *
	 *********/

	@Test
	void sendTestWithProtocolVersionTooOldPacket() {
		PlayerMock player = server.addPlayer();

		// Packet is encoded as id, VarInt protocol version, then String for the reason inside
		assertArrayEquals(
			new byte[]{1, 0, 9, 'M', 'e', 's', 's', 'a', 'g', 'e', ' ', '1'},
			getSentBytes(player, ProtocolVersionTooOldPacket.create(0, "Message 1"))
		);

		assertArrayEquals(
			new byte[]{1, (byte) 0x80, 0x01, 11, 'H', 'e', 'l', 'l', 'o', ' ', 'W', 'o', 'r', 'l', 'd'},
			getSentBytes(player, ProtocolVersionTooOldPacket.create(128, "Hello World"))
		);
	}

	@Test
	void receiveTestWithProtocolVersionTooOldPacket() {
		PlayerMock player = server.addPlayer();

		// When this packet is received, an appropriate ProtocolVersionTooOldException should be thrown
		assertThrowsWithMessage(
			ProtocolVersionTooOldException.class,
			player + " tried to send a packet here using protocol version 0. This system is using version 1. " +
				"This version is too old to receive the packet. Message 1",
			() -> getHandledPacket(player, "commandapi:handshake",
				new byte[]{1, 0, 9, 'M', 'e', 's', 's', 'a', 'g', 'e', ' ', '1'})
		);
		assertEquals(ProtocolVersionTooOldPacket.create(0, "Message 1"), packetCapture.getValue());
	}
}

You can see methods such as getSentBytes and getHandledPacket in use here.

---

And, I think that is all the changes that need to be mentioned. I'll be writing another comment soon that addresses how this impacts the questions in the original post. I'll probably also have some new questions to add there, so this PR still isn't ready for merging. Of course, when my explanation gives you more questions, ask in a comment here or on Discord

[willkroboth]

Here are the answers to the questions in the original post that I've landed on so far.

Is commandapi:plugin a good Plugin Message packet channel name? I'm not sure if there is a convention for channel names. Maybe commandapi and plugin are overly generic and could conflict with other channels.

commandapi is good. plugin is not very specific. Now, there are two channels named commandapi:handshake and commandapi:play. These names are inspired by Minecraft's protocol, and I think they make sense.

Is this a good encoding scheme? The packet id, data... system is used by Minecraft, but maybe there is a better way to encode the data.

Yeah, this seems to work. ClientToServerUpdateRequirementsPacket didn't have any data before, but now there is the SetVersionPacket and ProtocolVersionTooOldPacket. These packets actually have data, and it seems to work out.

Dose it make sense to hand out packet ids sequentially? Right now, the first CommandAPIPacket registered in CommandAPIProtocol is given id 0, then id 1, etc. Again, this is how Minecraft does it, but maybe it would make sense to hand out ids in a way that is independent of the order they are registered.

Sure, this works. Note that each channel handles its ids separately. Currently, commandapi:handshake has SetVersionPacket as id 0, and ProtocolVersionTooOldPacket as id 1. commandapi:play has ClientToServerUpdateRequirementsPacket as id 0. The specific ids don't really matter since they are abstracted away as the order they are registered to CommandAPIProtocol and handled internally.

What should happen when a new packet is sent to an old version of the plugin that doesn't know about that packet? Right now, an exception would be thrown since the packet id is invalid.

The newer version should take responsibility and never send data that would cause an old version to throw an exception. If a packet is incompatible with an old version, its write method can throw a ProtocolVersionTooOldPacket.

Should ClientToServerUpdateRequirementsPacket be given id 0? Perhaps there is a more important packet to put 'first'.

This is fine. The specific ids for each packet don't really matter.

What should happen if there is not a CommandAPI plugin on the backend server to receive the ClientToServerUpdateRequirementsPacket? Currently, nothing happens and the requirements are not updated.

If there is not a communicating instance of the CommandAPI installed on a target, it is treated as having protocol version 0. ClientToServerUpdateRequirementsPacket now throws a ProtocolVersionTooOldException that lets the user know that this isn't going to work as expected.

---

I think all of these questions about the public-facing protocol are well answered now. Of course, if there's anything that seems weird about the protocol, ask and we can figure it out.

The rest of the implementation details about how this works in the code should be flexible enough to change in the future if needed. So, the rest of my questions will be in code-review comments, which makes talking about the code easier.

[DerEchtePilz]

Really just two things, one just for me to understand something, the other one is the reason why this is requesting changes.

Anyway, this looks really good (although I might've missed a lot).
I also want to address another thing too: I think we shouldn't really release Velocity support in the next version since the snapshots have started in 9.0.4's snapshot "stage".

I would suggest waiting and releasing Velocity support by the time 9.1.0 is released.

[willkroboth]

I also want to address another thing too: I think we shouldn't really release Velocity support in the next version since the snapshots have started in 9.0.4's snapshot "stage".

I would suggest waiting and releasing Velocity support by the time 9.1.0 is released.

Yeah, Velocity should probably not be released in 9.0.4. Under semantic versioning rules, 9.1.0 is probably the lowest correct version since it's a major backward-compatible API change.

So, we'll definitely need to change the messages here:

CommandAPI/commandapi-core/src/main/java/dev/jorel/commandapi/network/packets/UpdateRequirementsPacket.java

Lines 50 to 59 in 3ec0874

	@Override
	public void write(FriendlyByteBuffer buffer, Object target, int protocolVersion) throws ProtocolVersionTooOldException {
	if (protocolVersion == 0) {
	throw ProtocolVersionTooOldException.whileSending(target, protocolVersion,
	// TODO: If the first released version of the CommandAPI that can receive messages is not 9.0.4, change this message
	"CommandAPI version 9.0.4 or greater is required to receive UpdateRequirementsPacket"
	);
	}
	// Nothing to write
	}

and here:

CommandAPI/commandapi-core/src/main/java/dev/jorel/commandapi/network/CommandAPIProtocol.java

Lines 35 to 47 in 3ec0874

	// TODO: If the first released version of the CommandAPI that can receive messages is not 9.0.4, change this description
	/**
	* The current version of the protocol. This should be incremented when the protocol is updated to indicate a large
	* change. A connection without a communicating instance of the CommandAPI (either no CommandAPI instance or a version
	* before 9.0.4) should be treated as protocol version 0.
	* <p>
	* This version number is used to communicate the capabilities of the CommandAPI instance on the other end of the
	* connection. The instance with the higher version should take responsibility to not send anything the lower version
	* cannot understand.
	* <p>
	* For example, say PV2 adds packet A, which PV1 doesn't know about. When PV2 communicates with PV1, it should not
	* send packet A, since PV1 would not know what to do with that packet.
	*/

as the TODOs mention.

We don't have to change these right now though. Perhaps Velocity will also be released in 10.0.0 along with annotations or whatever. We just need to make sure these messages agree with whatever version releases the networking system. Hopefully, the TODOs will serve as an effective reminder whenever that happens.

[willkroboth]

Just a reminder to address the TODO and change this JavaDoc before merging

[willkroboth]

Just a reminder to address the TODO and change this error message before merging

[willkroboth]

Just a reminder to change this error message before merging.

[willkroboth]

@Timongcraft suggested on Discord to create a networking-only Bukkit plugin so users who only wanted to use the CommandAPI on Velocity didn't need to add the whole CommandAPI plugin to each backend server.

I added a first draft of that with this commit: 3dee243. I haven't tested it yet, but the CommandAPINetworking plugin is 31 kB compared to 716 kB for the full CommandAPI plugin, or about 96% smaller.