Internal client-bound packet listener API at the player Connection class level

[stonar96]

Is your feature request related to a problem?

Hi, I'd like to discuss an idea or suggestion regarding an internal client-bound packet listener API at the player Connection class level. First, I will start with the problem(s) that I'm actually trying to solve.

The traditional way is to inject a listener into the Netty channel pipeline. In my opinion, this hook is already very late in the process for some use cases. Running listener logic on the main thread, for example, involves multiple thread or tick context jumps between originally dispatching the packet (e.g., on the main thread) and handling it in the listener.

If the listener logic depends on packet data and server state, this inherent delay could lead to unexpected problems. (Just to give an example: Imagine handling a block update packet using the coordinates from the packet, but the server player changed worlds in the meantime.)

The ProtocolLib approach, as shown in the flowchart, is also interesting because it avoids thread jumps by proxying the channel and its event loop completely. However, this approach is very "invasive" and impractical for most simple applications. Moreover, this approach can only be used by a single plugin at the same time, otherwise, the (un)injection logic will conflict with each other.

Describe the solution you'd like.

My suggestion to solve these problems, and to generally make it much easier for plugins to hook into the packet-sending process, is to add an internal API to the Connection class. This API should be kept simple and stupid to avoid performance overhead (i.e., no allocation of new event objects, no processing, no validation, etc.). This shouldn't be a problem because it should be an internal API anyway.

I've implemented a proof of concept to show how such an internal API could work (see diff below). Plugins can implement a CustomPacketListener interface and add (or remove) instances to (from) a CopyOnWriteArrayList in the Connection class. There are three different stages where listeners may hook:

1. onPacketDispatch: This is executed immediately and guaranteed in the same thread and tick context in which the packet was originally sent.
2. onPacketReadyToSend: In stage 1, packets may not be ready (mainly due to Anti-Xray packet processing and delaying subsequent packets to preserve the packet order). Stage 2 is executed when the packet is ready to be sent but before switching to the Netty event loop thread. This is either the same thread and tick context as in stage 1 (usually the case if the packet hasn't been queued), or later, likely due to Connection#tick on the server main thread. This is similar to the ProtocolLib approach, but without Netty proxying.
3. onPacketSend: This is executed right before Channel#writeAndFlush on the Netty event loop thread. This is similar to the traditional Netty pipeline injection approach, but occurs just before the Netty pipeline.

diff --git a/net/minecraft/network/Connection.java b/net/minecraft/network/Connection.java
index 19ec939..c102f61 100644
--- a/net/minecraft/network/Connection.java
+++ b/net/minecraft/network/Connection.java
@@ -25,8 +25,10 @@ import io.netty.handler.timeout.TimeoutException;
 import java.net.InetSocketAddress;
 import java.net.SocketAddress;
 import java.nio.channels.ClosedChannelException;
+import java.util.List;
 import java.util.Objects;
 import java.util.Queue;
+import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.RejectedExecutionException;
 import java.util.function.Consumer;
 import javax.crypto.Cipher;
@@ -63,6 +65,7 @@ public class Connection extends SimpleChannelInboundHandler<Packet<?>> {
     public static final Marker PACKET_RECEIVED_MARKER = Util.make(MarkerFactory.getMarker("PACKET_RECEIVED"), marker -> marker.add(PACKET_MARKER));
     public static final Marker PACKET_SENT_MARKER = Util.make(MarkerFactory.getMarker("PACKET_SENT"), marker -> marker.add(PACKET_MARKER));
     private static final ProtocolInfo<ServerHandshakePacketListener> INITIAL_PROTOCOL = HandshakeProtocols.SERVERBOUND;
+    public static final List<CustomPacketListener> customPacketListeners = new CopyOnWriteArrayList<>();
     private final PacketFlow receiving;
     private volatile boolean sendLoginDisconnect = true;
     private final Queue<WrappedConsumer> pendingActions = Queues.newConcurrentLinkedQueue(); // Paper - Optimize network
@@ -111,6 +114,7 @@ public class Connection extends SimpleChannelInboundHandler<Packet<?>> {
 
     private boolean stopReadingPackets;
     private void killForPacketSpam() {
+        // TODO: Call onPacketDispatch here because Connection#send is skipped.
         this.sendPacket(new ClientboundDisconnectPacket(io.papermc.paper.adventure.PaperAdventure.asVanilla(io.papermc.paper.configuration.GlobalConfiguration.get().packetLimiter.kickMessage)), PacketSendListener.thenRun(() -> {
             this.disconnect(io.papermc.paper.adventure.PaperAdventure.asVanilla(io.papermc.paper.configuration.GlobalConfiguration.get().packetLimiter.kickMessage));
         }), true);
@@ -384,6 +388,7 @@ public class Connection extends SimpleChannelInboundHandler<Packet<?>> {
             this.disconnectListener = packetListener;
             this.runOnceConnected(connection -> {
                 this.setupInboundProtocol(clientboundProtocol, packetListener);
+                // TODO: Call onPacketDispatch here because Connection#send is skipped.
                 connection.sendPacket(new ClientIntentionPacket(SharedConstants.getCurrentVersion().protocolVersion(), hostName, port, intention), null, true);
                 this.setupOutboundProtocol(serverboundProtocol);
             });
@@ -405,6 +410,14 @@ public class Connection extends SimpleChannelInboundHandler<Packet<?>> {
             return;
         }
 
+        for (CustomPacketListener customPacketListener : customPacketListeners) {
+            packet = customPacketListener.onPacketDispatch(this, packet); // TODO: Error handling?
+
+            if (packet == null) {
+                return;
+            }
+        }
+
         packet.onPacketDispatch(this.getPlayer());
         if (connected && (InnerUtil.canSendImmediate(this, packet)
             || (io.papermc.paper.util.MCUtil.isMainThread() && packet.isReady() && this.pendingActions.isEmpty()
@@ -444,11 +457,20 @@ public class Connection extends SimpleChannelInboundHandler<Packet<?>> {
     }
 
     private void sendPacket(Packet<?> packet, @Nullable ChannelFutureListener sendListener, boolean flush) {
+        for (CustomPacketListener customPacketListener : customPacketListeners) {
+            packet = customPacketListener.onPacketReadyToSend(this, packet); // TODO: Error handling?
+
+            if (packet == null) {
+                return;
+            }
+        }
+
         this.sentPackets++;
         if (this.channel.eventLoop().inEventLoop()) {
             this.doSendPacket(packet, sendListener, flush);
         } else {
-            this.channel.eventLoop().execute(() -> this.doSendPacket(packet, sendListener, flush));
+            Packet<?> finalPacket = packet;
+            this.channel.eventLoop().execute(() -> this.doSendPacket(finalPacket, sendListener, flush));
         }
     }
 
@@ -459,6 +481,15 @@ public class Connection extends SimpleChannelInboundHandler<Packet<?>> {
             packet.onPacketDispatchFinish(player, null);
             return;
         }
+
+        for (CustomPacketListener customPacketListener : customPacketListeners) {
+            packet = customPacketListener.onPacketSend(this, packet); // TODO: Error handling?
+
+            if (packet == null) {
+                return;
+            }
+        }
+
         try {
         final ChannelFuture channelFuture;
         // Paper end - Optimize network
@@ -473,7 +504,8 @@ public class Connection extends SimpleChannelInboundHandler<Packet<?>> {
 
         // Paper start - Optimize network
         if (packet.hasFinishListener()) {
-            channelFuture.addListener((ChannelFutureListener) future -> packet.onPacketDispatchFinish(player, future));
+            Packet<?> finalPacket = packet;
+            channelFuture.addListener((ChannelFutureListener) future -> finalPacket.onPacketDispatchFinish(player, future));
         }
         } catch (final Exception e) {
             LOGGER.error("NetworkException: {}", player, e);
@@ -946,4 +978,18 @@ public class Connection extends SimpleChannelInboundHandler<Packet<?>> {
         }
     }
     // Paper end - Optimize network
+
+    public interface CustomPacketListener { // TODO: Maybe create an interface and a listener list per method?
+        default @Nullable Packet<?> onPacketDispatch(Connection connection, Packet<?> packet) {
+            return packet;
+        }
+
+        default @Nullable Packet<?> onPacketReadyToSend(Connection connection, Packet<?> packet) {
+            return packet;
+        }
+
+        default @Nullable Packet<?> onPacketSend(Connection connection, Packet<?> packet) {
+            return packet;
+        }
+    }
 }

Please let me know what you think about this suggestion. Thanks!

Describe alternatives you've considered.

I've already mentioned some alternatives above. Especially ProtocolLib avoids the mentioned problems with thread and context jumps. However, this suggestion is about making it easier to implement an own packet listener.

Other

No response

[kennytv]

I can definitely see some value in this in getting to the immediacy problem, but unless you pick the netty option (which sounds like isn't meant to be the interesting/core part of the API) you a) don't know what order a packet is actually sent it, b) could be on any thread, and c) don't get to see what skips the server's internal packet logic.

... point b here probably being the worst offender

What are specific use cases you have in mind for this?

[stonar96]

Thanks for your response!

a) don't know what order a packet is actually sent it

I'm not entirely sure what you mean by that. The packet order (for packets that are sent with a happens-before relationship, e.g. consecutively sent packets on the same thread) should always be preserved, at least by the logic in the Connection class, hence the packet queue. (Of course this doesn't apply to packet types that bypass the packet queue intentionally, however, those bypass the packet queue precisely because the order isn't important.)

Or doesn't Netty itself preserve the write order? If so, the order is meaningless anyway.

In case you are talking about packets from different non-synchronized threads, then again, this order is meaningless too.

b) could be on any thread

Yes that's true, but on the other hand this is in my opinion one of the main points that would make this API powerful. It also depends on which listener we are talking about:

1. onPacketDispatch: This is executed immediately and guaranteed in the same thread and tick context in which the packet was originally sent.
2. onPacketReadyToSend: Here it's not so straightforward. This is executed

• on the same thread as stage 1 if the packet can be sent immediately (if the packet is not queued). (This is usually the case if Anti-Xray is disabled afaik.)
• on the main or tick thread if the packet has been queued and is polled by Connection#flushQueue.

3. onPacketSend: This is always executed on the Netty event loop thread.

Probably the most packet types are exclusively sent by a defined thread (let's assume the main thread or a region thread on folia). If I follow the "traditional" Netty channel pipeline injection approach, I will end up in a Netty thread, no matter what. Then, if I need to execute some logic on the main or region thread within the listener, I will have to schedule a task and end up with even more thread or context jumps. On the other hand, the proposed listeners onPacketDispatch and onPacketReadyToSend will either be called immediately or via the queue later on the main or region thread. I can then check whether I'm already on the expected thread or schedule a task only if needed.

Even if there are some rare conditions where the thread may be undefined (due to plugins sending packets, or special treatment during the login phase, or Minecraft logic in general), I can avoid (additional) thread jumps in the vast majority of cases by checking the current thread (in case I even care).

c) don't get to see what skips the server's internal packet logic

That's true. However, do such cases that skip the internal packet logic already exist in the server code? (Edit: I've checked it and that's not the case.) Or are you talking about plugins? If so, whether this is an issue of course depends on the intentions of my plugin and the other plugin.

What are specific use cases you have in mind for this?

I think I can't describe any specific use cases that would add any further value to this discussion. But in general I would like to use this as a replacement for ProtocolLib in my plugins, where I have packet listeners, which require main or region thread handling. As I said above, ProtocolLib solves these problems by proxying Netty (and scheduling tasks if the thread isn't already the main thread). However, there's no easy way to do this and it would also conflict with ProtocolLib. For my personal use cases the onPacketReadyToSend listener would be the most useful one since I'm also dealing with chunk packets.

A concern on my side: Since the server sends a large number of packets, do you think the listener implementation would introduce significant overhead, even if it's not actively used?

[kennytv]

If you slap in an empty check before iterating it's free, and actual use will generally still be much better than other injection points, so that's alright. I'm just worried that it'll lead to problems with wrong thread expectations or stalling whatever thread any other plugin could be sending a packet from.

Separate lists would be good, and I would add a generic or at least a check so you have to return the same type of packet you are given as input

[stonar96]

Good points. I agree with the empty check and separate lists.

I wouldn't add generics, though, because that would force the packet listener implementation to use ugly casts and ignore warnings even in perfectly valid cases. Since the actual type is unknown, you could only return null or the input instance itself without casts and warnings, but no new or other instances, even if the types match. See the following example:

public <T extends Object> T transform(T t) {
    if (t instanceof String) {
        // return null; // Okay.
        // return "test"; // error: incompatible types: String cannot be converted to T
        return (T) "test"; // warning: [unchecked] unchecked cast
    }

    return t; // Okay.
}

I also wouldn't add type checks. Exchanging packets with different types might even be a (weird but valid) use case that we would unnecessarily restrict. Note that canceling a packet by returning null and resending a new one isn't always an option because one may want to preserve the packet order.

I'm just worried that it'll lead to problems with wrong thread expectations or stalling whatever thread any other plugin could be sending a packet from.

Yes, I fully understand your concerns here and I don't have a good solution for this. I was thinking about adding Async or something like that to the packet listener method names, however, that would be missleading too. I think the best we can do is add some (doc) comments to the packet listener methods with some descriptive warnings. Since it's an internal API, I assume people will look at the server code anyway when using it.

I would be happy to implement the whole thing. Do you think I can already start working on a PR for this, or does the fundamental question of whether we want to add something like this require further discussion with additional parties?

[kennytv]

Returning a packet of a different type sounds muddy. If you want to replace a packet with something completely different (maybe even multiple packets?) that should be an explicit, separate part of it. Though I don't have any immediate ideas on how exactly that should look

If we assume more than just one or two plugins might be using this at a time, it might also be better to already handle calls depending on the PacketType ourselves instead of every single plugin having their own map lookup or chained instanceof checks. This would also solve the generics problem, leave us to make it more performant if needed, and move even further away from what we really didn't want to add before, which is just getting a raw Object and pushing the actual design off

[kennytv]

... but with those two things in mind and generally having everything well documented, I'd say you can go ahead and draft a PR