PaperMC / Velocity

The modern, next-generation Minecraft server proxy.
https://papermc.io/software/velocity
GNU General Public License v3.0
1.68k stars 583 forks source link

Fast server switch causes "race condition" in client #1334

Open valaphee opened 1 month ago

valaphee commented 1 month ago

Due to chat signing, after a server switch / join packet the client spins up a future which fetches the player session and sends it to the server.

Sometimes server do multiple switches closely together (but still after the first one in Velocities view is finished), and if the player has a slower/unstable connection, the player session packet is sent during the config phase which causes an exception and results in a disconnect.

Contents of the log image

Culprit in the client

if (this.connection.isEncrypted()) {
    this.client.getProfileKeys().fetchKeyPair().thenAcceptAsync(keyPair -> keyPair.ifPresent(this::updateKeyPair), (Executor)((Object)this.client));
}

To circumvent this it might be necessary to wait for the packet to arrive with a timeout (3 seconds should be enough)

electronicboy commented 1 month ago

I'm not fond of the notion of solving these sort of issues on our side because it starts to introduce a whole bunch of extra state that we need to manage which gets fun because of how many broken clients exist, especially around this system...

Though, I guess that that would be a fair hackaround for this solution to try to minimise what is happening. I really do want to rewrite a whole bunch of the server transfer stuff, but, it's a fair while aways before I can really commit to taking on such a project

I want to say that there is already a mojira ticket for this issue?

valaphee commented 1 month ago

Fair, for completeness https://bugs.mojang.com/browse/MC-272506, but it's not that common that they fix 3rd party issues even though they introduced a lot of stuff for 3rd partys recently.

And this wouldn't help for versions prior the version where it might get fixed.

electronicboy commented 1 month ago

This is a race condition regarding a system they implemented, it's likely not a priority for them, but, it is on their side; We'd need to see if we can mitigate it reasonably for affected versions, but reducing the amount of hackarounds in effect for any version is always nice