Floodgate player name prefixes break after registering an Async Start Listener using ProtocolLib

Smart123s commented 3 years ago

Summary

After registering an Asynchronous Packet Handler for the START type with ProtocolLib, Floodgate name prefixes won't be present. If I make the handler Synchronous or register another type of async Packet Handler (ex. ENCRYPTION_BEGIN) then the prefixes will work fine.

Server Details

Java Version: OpenJDK 15.0.2 OS: Arch Linux (up to date packages as of 2021-05-08 10:00AM CEST) Minecraft Server: Paper 1.16.5 #650 Plguins:

Floodgate-spigot (branch: dev/2.0; master also affected) #36
Geyser-Spigot (branch: floodgate-2.0) #13
ProtocolLib v4.6.0
The example plugin from below

Example Code

A ready to compile version of the code with plugin.yml and everything can be found at https://github.com/Smart123s/ProtocolLib-Dummy-Listener

Alternatively you can download a precompiled JAR file from the Releases section to test it on a server.

EmptyListener.java:

public class EmptyListener extends PacketAdapter {

    public EmptyListener(Plugin plugin) {
        super(params().plugin(plugin).types(START).optionAsync());
    }

    public static void register(Plugin plugin) {
        // Register async Listener
        ProtocolLibrary.getProtocolManager().getAsynchronousManager().registerAsyncHandler(new EmptyListener(plugin))
                .start();
    }

    @Override
    public void onPacketReceiving(PacketEvent packetEvent) {
        // It deosn't matter if you do anything here or not.
        Bukkit.getLogger().info("[ProtocolLib-Dummy-Listener] Caught START PacketEvent from ProtocolLib");
        Bukkit.getLogger().info("[ProtocolLib-Dummy-Listener] Floodgate name prefixes should be broken now");
    }

}

Main Class:

public void onEnable() {
    if (Bukkit.getPluginManager().isPluginEnabled("ProtocolLib")) {
        EmptyListener.register(this);
    }
}

Logs

*IPs, Ports, and UUIDs have been removed

With async handler registered:

[09:57:13 INFO]: [Geyser-Spigot] /192.168.0.0:0000 tried to connect!
[09:57:14 INFO]: [Geyser-Spigot] Player connected with username Smart123s
[09:57:24 INFO]: [Geyser-Spigot] Attempting to login using floodgate mode... authentication will be encrypted.
[09:57:24 INFO]: [Geyser-Spigot] Smart123s (logged in as: Smart123s) has connected to remote java server on address 192.168.0.0
[09:57:24 INFO]: [ProtocolLib-Dummy-Listener] Caught START PacketEvent from ProtocolLib
[09:57:24 INFO]: [ProtocolLib-Dummy-Listener] Floodgate name prefixes should be broken now
[09:57:24 INFO]: UUID of player Smart123s is 00000000-0000-0000-000*-************
[09:57:24 INFO]: [floodgate] Floodgate player logged in as .Smart123s joined (UUID: 00000000-0000-0000-000*-************)
[09:57:24 INFO]: [Geyser-Spigot] Registering bedrock skin for Smart123s (00000000-0000-0000-000*-************)
[09:57:24 INFO]: [Geyser-Spigot] Unable to load bedrock skin for 'Smart123s' as they are likely using a customised skin
[09:57:24 INFO]: Smart123s joined the game
[09:57:24 INFO]: Smart123s[/192.168.0.0:0000] logged in with entity id 436 at ([world]-265.5, 66.0, -62.5)

Without async handler:

[10:02:21 INFO]: [Geyser-Spigot] /192.168.0.0:0000 tried to connect!
[10:02:22 INFO]: [Geyser-Spigot] Player connected with username Smart123s
[10:02:31 INFO]: [Geyser-Spigot] Attempting to login using floodgate mode... authentication will be encrypted.
[10:02:31 INFO]: [Geyser-Spigot] Smart123s (logged in as: Smart123s) has connected to remote java server on address 192.168.1.210
[10:02:32 INFO]: UUID of player .Smart123s is 00000000-0000-0000-000*-************
[10:02:32 INFO]: [floodgate] Floodgate player logged in as .Smart123s joined (UUID: 00000000-0000-0000-000*-************)
[10:02:32 INFO]: [Geyser-Spigot] Registering bedrock skin for .Smart123s (00000000-0000-0000-000*-************)
[10:02:32 INFO]: [Geyser-Spigot] Unable to load bedrock skin for '.Smart123s' as they are likely using a customised skin
[10:02:32 INFO]: .Smart123s joined the game
[10:02:32 INFO]: .Smart123s[/192.168.0.0:0000] logged in with entity id 236 at ([world]-265.5, 66.0, -62.5)

SalomonPicos commented 3 years ago

The problem is that protocollib is broken with floodgate prefix, so we should wait for update from protocollib. The only solution is to install another plugin for api called protocolsupport. This plugin won't work with the most of the plugin so i think it's not a solution. FastLogin loads the player before floodgate and doesn't let it loads the prefix.

Smart123s commented 2 years ago

In ProtocolLib, if an async listener is registered to a PacketEvent, then then original event (which Floodgate inject into) will get canceled, and ProtocolLib will create a new event if the asynchronous processing has finished.

https://github.com/dmulloy2/ProtocolLib/blob/96155b10651df1c8c37431774cd5b02b252150cc/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java#L552-L559=

And canceled events aren't fired down the pipeline, so the code injected by Floodgate will never get executed.

https://github.com/dmulloy2/ProtocolLib/blob/073bfa2b86048aade0a0f32638f0255fdbd3f282/src/main/java/com/comphenix/protocol/injector/netty/channel/InboundPacketInterceptor.java#L37-L39=

Tim203 commented 1 day ago

Is this still an issue with the latest ProtocolLib version?

Smart123s commented 20 hours ago

Yes, the issue still persists.

I've checked today with floodgate v2.2.3-SNAPSHOT (b113-06f2ed9) and ProtocolLib 5.3.0. If you install my sample ProtocolLib-Dummy-Listener plugin from the original issue, username prefixes will break.

Basically, what happens, is that Floodgate's channelRead() method never gets executed if an async login event listnerer is registered with ProtocollLib. That's because after plugins complte all their tasks, ProtcolLib calls Minecraft's channelRead() method directly, instead of calling ctx.fireChannelRead();.

Note, that the issue does not happen, if a sync (not async) event handler is registered. Note, that this issue is only related to Bukkit & ProtocolLib (I saw your comment the other day on FastLogin Velocity, but that crappy implementation doesn't have to do anything with this issue. That's completely on me.).

FastLogin's current workaround to this issue was to reimplement all of Floodgate's tasks from the channelRead() method, and the we run them ourselves. See here.

Also, ProtocolLib doesn't expose the Channel, so we have to extract that with Reflections to get the floodgate-player from the Channel attribute. yey :)

One possible fix would be to ensure that your channelRead executes before ProtocolLib's does, if that's possible. Another fix would be to have a native ProtocolLib implementation for packet handlin in Floodgate (similar to ProtocolSupport), but since it wouldn't fix any other issues, I don't think it'd be worth the hassle. I don't know of anyone else (other than FastLogin) that registers async login event handlers. Why would you even do that?

To be honest, I wouldn't mind, if this would be one of the "won't fix" issues from your side. If you could somehow create a method that FastLogin could call to execute Floodgate's channelRead() tasks (when ProtocolLib screws up), it would be awesome. Exposing that to the API would be confusing in my opinion, since, again, only FastLogin needs that. So if it's easily callable with a reflection, that's also fine with me. Tho having a static executeChannelReadTasks(FloodgatePlayer player) would also look bad in your code from an obejct oriented standpoint.

I have no ide what would be a good approach to this issue.

GeyserMC / Floodgate