sendServerPacket with filters=false is unreliable
bergerkiller opened this issue ยท 4 comments
Describe the bug
When sending a lot of packets in a burst specifying filters=false either of these issues may occur:
- The packet is sent to listeners anyway ( PaperMC/Paper#1934 is, I believe, related )
- A different packet that was sent is completely ignored by all listeners and monitors
To Reproduce
Since netty is full of multithreading nonsense, the bug occurs only 10% of the time and was affected by packet timings. Even whether or not another player was online influenced this.
I ran into this bug when debugging a strange issue in Traincarts/BKCommonLib. Sometimes when players joined, the listener that is notified of the entity spawn packets didnt receive a notification that the minecart/vehicle spawned. This caused further mysterious bugs in the plugin.
I can not confirm this, but I believe this only occurs right after a player joins the server.
Expected behavior
Only the packet sent using filters=false skips the listeners, not other packets, and listeners arent notified of the packet anyway.
Version Info
https://pastebin.com/KZH92ubH
Additional context
I walked through the source code of ProtocolLib to see what may have gone wrong, since the sendPacket(filters=true) definitely occurred and the client did see the entity spawn. After a long multi-hour search I found this line of code:
Which I then traced to:
https://github.com/dmulloy2/ProtocolLib/blob/master/src/main/java/com/comphenix/protocol/injector/netty/ChannelInjector.java#L663
Realizing I was sending a lot (dozens to hundreds) of packets silently in various plugins to avoid interfering with other plugins, I disabled the filters option to make it always true. Now the bug was completely gone! To doubly verify, I've compiled ProtocolLib myself and added some logging around that area, and indeed, the wrong packet skipped the listeners.
I'm noticing the field uses a ThreadLocal, is there any chance that where you're setting it and where it goes by that check occurs on different threads? Can it be that more than one packet is processed inside invokeSendPacket()?
I'm currently looking into a way to work around this issue in my own plugins. But I may decide to just stop using filters=false and stick to the silent packet queue fallback I wrote myself so at least it is filtered for my own plugins.
Suggestion
If you have any way or documentation that indicates how I should call sendServerPacket(filters=false) that avoids this bug, let me know. I've verified that I'm only calling it on the main thread currently.
I have found the exact problem.
This is a PaperSpigot only issue.
Paperspigot has a modified NetworkManager flushQueue function:
private boolean o() {
if (this.channel != null && this.channel.isOpen()) {
final Queue queue = this.packetQueue;
synchronized (this.packetQueue) {
while (!this.packetQueue.isEmpty()) {
final QueuedPacket networkmanager_queuedpacket = this.getPacketQueue().peek();
if (networkmanager_queuedpacket != null) {
if (networkmanager_queuedpacket.getPacket() instanceof PacketPlayOutMapChunk && !((PacketPlayOutMapChunk)networkmanager_queuedpacket.getPacket()).isReady()) {
return false;
}
this.getPacketQueue().poll();
this.dispatchPacket(networkmanager_queuedpacket.getPacket(), networkmanager_queuedpacket.getGenericFutureListener());
}
}
}
}
return true;
}
When there are map chunk packets in the queue that aren't "ready" yet (this directly ties into the ore obfuscator bug) the queue is never flushed. As a result, any next sendPacket that is executed is thrown into the queue (to preserve send order) rather than sending it immediately, bypassing ProtocolLibs function. As a result, the scoped 'process this packet?' field is useless, because the packet isn't send during invokeSendPacket.
I managed to eliminate this problem by doing the following (pseudocode, requires reflection):
NetworkManager manager = player.handle.playerConnection.networkManager;
synchronized (manager.packetQueue) {
while (!manager.o())
Thread.sleep(20L);
ProtocolLibrary.getProtocolManager().sendServerPacket(player, packet, null, false);
}
What this does, is wait for the queue to be fully empty before initiating the sendServerPacket, guaranteeing that the packet is sent during the invokeSendPacket() logic. We keep the packet queue locked during this to prevent anyone on another thread sneakily inserting another entry into the queue inbetween.
This also explains why it only seemed to occur on join - a lot of map packets are sent to the player during that time.
A way to fix this would be to keep an identity hashmap of packets to ignore (by reference) rather than using a ThreadLocal field for the current invokeSendPacket, which isn't reliable.
Very detailed, thank you. Could you make a pull request with that identity hashmap fix? I can do it if not, but it seems like you have a solid grasp on what's going on here.