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

Assign ServerConnection to Player before processing post-login packets #1309

Merged
merged 8 commits into from
Jul 4, 2024

Conversation

wallenjos01
Copy link
Contributor

Currently, when transitioning to a new server, the proxy begins auto-reading (and therefore processing) packets from the pending server connection before assigning that connection to the player via ConnectedPlayer::setConnectedServer. This causes a race condition if the backend server sends certain packets (particularly plugin messages) right after sending a JoinGamePacket. For example, the following has inconsistent behavior depending on when a plugin message is sent:

@Subscribe
public void onPluginMessage(PluginMessageEvent event) {
    if(event.getSource() instanceof ServerConnection sc) {

        // Will work consistently
        sc.sendPluginMessage(MinecraftChannelIdentifier.create("test", "message"), "Hello, World".getBytes()); 
        
        // Will often fail if the backend server sends a plugin message immediately after join.
        sc.getPlayer()
            .getCurrentServer()
            .orElseThrow()
            .sendPluginMessage(MinecraftChannelIdentifier.create("test", "message"), "Hello, World".getBytes());
    }
}

This PR reverses the order of two lines in TransitionSessionHandler, making the proxy wait to process packets sent by a backend server until after assigning the player's connection.

// Now set the connected server.
serverConn.getPlayer().setConnectedServer(serverConn);

// Clean up disabling auto-read while the connected event was being processed.
smc.setAutoReading(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind expanding the comment above this line to include your description (or similar)?

@Gerrygames
Copy link
Contributor

Pretty sure that this does not make a difference. The code in question runs on the event loop of the server connection, so no packets from that connection can be handled during its execution.

@electronicboy
Copy link
Member

Has this been tested? Because I'd imagine that the more bigger issue here is going to be that disabling auto-read doesn't promise to disable reading everything, and so there can still be stuff which ends up being processed, at least that is my understanding; The auto-reading re-enable + field update both occur within the same area.

@wallenjos01
Copy link
Contributor Author

Has this been tested?

I created this PR because I ran into errors caused by this bug while working on a plugin for my private server, but just to be sure, I spent some time writing a plugin to test it. I tested with both Velocity build 385, downloaded from the website, and my own fork. I tested both Spigot 1.20.5 and Fabric 1.20.5 backends. I got about a 40% error rate when using build 385, and a 0% error rate using my fork. I have attached the relevant logs and code.

pmtest.zip

Copy link
Member

@kennytv kennytv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a quick look, this seems to be caused by the manual channel.read call in MinecraftConnection#setAutoReading, so the PR should be good

@kennytv kennytv merged commit aa4e878 into PaperMC:dev/3.0.0 Jul 4, 2024
1 check passed
pull bot pushed a commit to WiIIiam278/Velocity that referenced this pull request Jul 5, 2024
qyl27 pushed a commit to MeowCraftMC/Velocity that referenced this pull request Jul 6, 2024
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.

7 participants