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

Open
wants to merge 8 commits into
base: dev/3.0.0
Choose a base branch
from

Conversation

wallenjos01
Copy link

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
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

@@ -137,12 +137,14 @@ public boolean handle(JoinGamePacket packet) {
smc.setActiveSessionHandler(StateRegistry.PLAY,
new BackendPlaySessionHandler(server, serverConn));

// Now set the connected server.
Copy link
Contributor

Choose a reason for hiding this comment

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

Trim whitespace after the period (line 140)

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.

None yet

6 participants