Fix chat race condition

[EpicPlayerA10]

The issue

Chat system in velocity has a race condition. When player is sending messages very fast, then a backend server will kick him for out-of-order chat.

You can test this by installing litematica mod and use the feature called Paste schematic into world. Then litematica will send commands very fast and then you get kicked from the proxy. This issue only happens when player is connected to the proxy. When you are connected to the minecraft server directly then the issue disappears.

The race condition looks like this:

Flowchart before

You can see here that velocity doesn't wait for packet to be sent. The chat gets out of sync and thats why players are getting kicked from backend server.

Fix

This pull request fixes it by waiting for packets to be sent. Now the flowchart looks like this:

Flowchart after

Related issues

• Player get kicked due to out-of-order chat when sending messages rapidly #909

[electronicboy]

Don't have hardware to look into this properly, but on the surface, this looks much closer to how I said this stuff should probably work in the first place

[intersecato]

Hi @EpicPlayerA10 @electronicboy,
we were able to test the fix in production with various users. The problem seems to persists but more rarely.
We will try to investigate the problem further.

[EpicPlayerA10]

The problem seems to persists but more rarely.

Okay, now the latest commit will ensure that chat packets are fully sent. This should get rid of this problem.

[intersecato]

Okay, now the latest commit will ensure that chat packets are fully sent. This should get rid of this problem.

The problem is still there. A player reported me about a kick again today.

[EpicPlayerA10]

The problem is still there. A player reported me about a kick again today.

Hmm. I don't know why this is still happening. I have no idea. I think that i did everything correct.

[CatTeaA]

Is there any progress? Looking forward to it

[electronicboy]

I recall looking over this a few times and it generally looked fine, finally have hardware to test, just time; if this can be rebased, I'll try and get to this on the weekend or sometime next week

[EpicPlayerA10]

Rebased

[demxn013]

this issue still exists on 1.20 paper backend server on client version 1.21. Im using the most recent version of velocity.
It only happens with commands though.

[7wOv6ySCjo]

The bug is still occurring in version 26.1.2
We get the error: ‘keepalive: time out (out of order!)’
Several disconnections a day for various players.

I'm not entirely sure whether it's an ‘out of order’ error with keepalive packets or with the chat specifically. The strange thing is that you don't even need to type commands or send messages in the chat; just exploring the world for a few minutes is enough to trigger this ‘timeout’ error.

[EpicPlayerA10]

The bug is still occurring in version 26.1.2 We get the error: ‘keepalive: time out (out of order!)’ Several disconnections a day for various players.

I'm not entirely sure whether it's an ‘out of order’ error with keepalive packets or with the chat specifically. The strange thing is that you don't even need to type commands or send messages in the chat; just exploring the world for a few minutes is enough to trigger this ‘timeout’ error.

Can you send a screenshot of this error?

[WouterGritter]

@7wOv6ySCjo could you check and confirm whether #1782 solves this issue for you or not?

[7wOv6ySCjo]

The bug is still occurring in version 26.1.2 We get the error: ‘keepalive: time out (out of order!)’ Several disconnections a day for various players.
I'm not entirely sure whether it's an ‘out of order’ error with keepalive packets or with the chat specifically. The strange thing is that you don't even need to type commands or send messages in the chat; just exploring the world for a few minutes is enough to trigger this ‘timeout’ error.

Can you send a screenshot of this error?

This usually happens with entity ID number 1, as it doesn’t happen as often with subsequent entities ID, but it still disconnects the user.

I investigated the code and the line causing the disconnection is in: net.minecraft.server.network.ServerCommonPacketListenerImpl

            LOGGER.info("Disconnecting {} for sending keepalive response ({}) out-of-order!", this.playerProfile().name(), packet.getId());
            //this.disconnectAsync(TIMEOUT_DISCONNECTION_MESSAGE, io.papermc.paper.connection.DisconnectionReason.TIMEOUT);
            
            and
            
    LOGGER.info("Disconnecting {} for sending keepalive response ({}) without matching challenge!", this.playerProfile().name(), packet.getId());
    //this.disconnectAsync(TIMEOUT_DISCONNECTION_MESSAGE, io.papermc.paper.connection.DisconnectionReason.TIMEOUT);

I simply commented out those two disconnection lines and that was it; I no longer lose users due to arbitrary disconnections, although I don’t know what the consequences of preventing this disconnection might be.