Skip to content

Conversation

@Smart123s
Copy link
Contributor

@Smart123s Smart123s commented Jun 29, 2021

Summary of your change

Made FastLogin add Floodgate player name prefixes, if Floodgate fails to do it.
Added floodgatePrefixWorkaround to config.yml to enable or disable this feature.

Related issue

Proposes a workaround / fixes #493

Notes

This will leave Floodgate/SpigotDataHandler.java in a limbo state, so it'll run some code (mostly if checks to decide if it's receiving a login packet) but it didn't have a noticeable impact on performance and it also seemed to be stable.

@TuxCoding TuxCoding added the enhancement New feature or change request label Jun 29, 2021
@TuxCoding
Copy link
Owner

So the packet handler will run before or after Floodgate? I'm kind of afraid of integrating a implementation specific solution. This could break very fast.

What I also wondered about the cases where it tries to login while a player is online. What will happen if a player with the same name is already online and is either a floodgate player or not? This could potentially allow it skip authentication, doesn't it?

@TuxCoding TuxCoding self-requested a review June 30, 2021 14:19
@Smart123s
Copy link
Contributor Author

So the packet handler will run before or after Floodgate?

This is the Pipeline's order of the packet:

[13:33:52] [Netty Server IO #1/INFO]: FlushConsolidationHandler#0
[13:33:52] [Netty Server IO #1/INFO]: timeout
[13:33:52] [Netty Server IO #1/INFO]: legacy_query
[13:33:52] [Netty Server IO #1/INFO]: splitter
[13:33:52] [Netty Server IO #1/INFO]: protocol_lib_finish
[13:33:52] [Netty Server IO #1/INFO]: protocol_lib_decoder
[13:33:52] [Netty Server IO #1/INFO]: decoder
[13:33:52] [Netty Server IO #1/INFO]: prepender
[13:33:52] [Netty Server IO #1/INFO]: encoder
[13:33:52] [Netty Server IO #1/INFO]: protocol_lib_encoder
[13:33:52] [Netty Server IO #1/INFO]: floodgate_data_handler
[13:33:52] [Netty Server IO #1/INFO]: packet_handler
[13:33:52] [Netty Server IO #1/INFO]: floodgate_addon

So ProtocolLib's listeners run before Floodgate's.
Or I would better say: ProtocolLibs's listeners run, Floodgate's don't, if an async listener is registered via ProtocolLib.

This could break very fast.

This is supposed to be a temporary workaround until I (or someone) figure out the cause of the issue. It's not an issue with FastLogin's code. Simply registering an async START packet handler via ProtocolLib will break Floodgate's packet hooks.
And that's why it's an optional feature, which can be toggled in config.yml.

What I also wondered about the cases where it tries to login while a player is online. What will happen if a player with the same name is already online and is either a floodgate player or not? This could potentially allow it skip authentication, doesn't it?

I'm not 100% sure about this, so I'll test it when I'll have more time.

I'm kind of afraid of integrating a implementation specific solution.

This could be a separate module / JAR file. Or it could even be a separate Bukkit plugin. It wouldn't require any code changes to FastLogin. I might make a separate repository for this.

@TuxCoding
Copy link
Owner

This is the Pipeline's order of the packet:

This could be potentially problematic, because we want to depend on a status check that doesn't even ran yet. I think FastLogin's code should run after that from Floodgate for this. Therefore Floodgate is able to make its checks and change the name if required to. Maybe we need to depend on some status result checks from Floodgate if the plugin is available.

The following is outdated, because it assumes the existing order (FastLogin and then Floodgate):

Simply registering an async START packet handler via ProtocolLib will break Floodgate's packet hooks.
And that's why it's an optional feature, which can be toggled in config.yml.

Then maybe we could refactor the code to synchronous listening. I have two ideas:

  1. Listening to the packet synchronously, but then schedule an asynchronous Bukkit Task for blocking stuff like query database and Mojang checks. The AsyncMarker could be used to delay processing of it. However it could be that the AsyncMarker isn't available for this. It could be that there is a difference between async Minecraft processing and async ProtocolLib processing.

  2. Another idea is to always cancel the incoming packet, process the data asynchronously and then fake a new incoming login start packet similar to the existing premium login behavior. However Floodgate could not be compatible. It depends if Floodgate is able to see the fake incoming packet.

This can be used as a workaround for TuxCoding#493
This will leave
https://github.com/GeyserMC/Floodgate/blob/821be02bdb179f7d4ba7b0ec79bbc721c28105f5/spigot/src/main/java/org/geysermc/floodgate/addon/data/SpigotDataHandler.java
in a limbo state, but it shouldn't have a noticable impact on neither
performance nor stability.
This commit will try append prefixes to every player, even if it's not
needed of if Floodgate isn't installed.
Without this patch, Java players would also get a prefix.
@Smart123s
Copy link
Contributor Author

People started merging this branch to their fork, so I thought that at least I would resolve merge conflicts. That's why I've pushed some commits.

@TuxCoding
Copy link
Owner

Because this option is opt-in, I'll merge it. Maybe we need propose a specific API on the Floodgate project to fix this issue.

@TuxCoding TuxCoding merged commit 4a5516c into TuxCoding:main Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or change request Floodgate/Geyser

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Floodgate player name prefixes are not working with ProtocolLib

2 participants