Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve the pipeline order when async receiving packets #3351

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Tim203
Copy link

@Tim203 Tim203 commented Jan 30, 2025

This fixes one of the oldest open issues in Floodgate GeyserMC/Floodgate#143.
ProtocolLib their async packet handlers currently don't respect the pipeline order and instead always let the Minecraft Connection class handle everything.

The Floodgate issue describes the following scenario:
A packet handler is created for PacketType.Login.Client.START, and when it comes time to handle that packet the async handler cancels the packet and keeps a copy for later use. If the packet is not async then ctx#fireChannelRead is executed. When the plugin is done handling that packet readServerboundPacket sends it to the Connection class without considering that there might be other channel handlers between ProtocolLib and Minecraft's packet_handler.

This PR calls ctx#fireChannelRead, fixing the order that was broken.

@Tim203 Tim203 changed the title Preserve the pipeline order when async sending/receiving packets Preserve the pipeline order when async receiving packets Jan 30, 2025
@Ingrim4
Copy link
Collaborator

Ingrim4 commented Jan 31, 2025

This will also cause plugin generated packets to be send through the partial netty pipeline I don't know if this could cause problems as some plugins might expect the minecraft packet listener to directly receive the packet without further modification. Other than that lgtm.

@Tim203
Copy link
Author

Tim203 commented Feb 1, 2025

If a packet fails to pass from its own ctx to minecraft's packet handler, then that's an issue in that other plugin not protocollib.
If a packet is edited in a way after protocollib that is incompatible for the plugin, then we have a plugin-level incompatibility and the affected plugins will need to find a solution.
But at the moment you already have incompatibilities due to the way that this skips parts of the pipeline. In FastLogin their case they resorted to copying the Floodgate code and run it themselves, which is also far from an ideal situation.

@Ingrim4
Copy link
Collaborator

Ingrim4 commented Feb 1, 2025

Fair, was just pointing out possible problems.

@Tim203
Copy link
Author

Tim203 commented Feb 2, 2025

Ah ok. Good you're pointing it out yes.

@dmulloy2
Copy link
Owner

dmulloy2 commented Feb 4, 2025

thanks for the PR! which minecraft version(s) did you test it with? wondering if there might be issues on older versions here

@Tim203
Copy link
Author

Tim203 commented Feb 4, 2025

Good question, I believe I tested with 1.20.4.
Which version range do you target for ProtocolLib? Especially since you are planning to require Java 17 on the next release.
With Geyser we had to bump up our minimum supported version because some dependencies of Spigot doesn't support those newer Java versions.
And so I know which minimum version to check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants