GeyserMC / Geyser

A bridge/proxy allowing you to connect to Minecraft: Java Edition servers with Minecraft: Bedrock Edition.
https://geysermc.org
MIT License
4.71k stars 674 forks source link

GeyserApi.api().connectionByUuid() is flaky #3456

Open tadhunt opened 1 year ago

tadhunt commented 1 year ago

Describe the bug

I'm creating a Velocity plugin dependent on Geyser (using the latest releases at the time of writing) for running some special business logic for choosing different servers for bedrock vs java players at connection time.

As per the Velocity docs, I'm receiving a PlayerChooseInitialServerEvent, and as per the Geyser wiki Using Geyser or Floodgate as a dependency, I'm attempting to determine if the player is a bedrock player connected through Geyser via:

GeyserConnection connection = GeyserApi.api().connectionByUuid(uuid);

However, this is at best flaky. Sometimes it finds the connection, but most of the time it doesn't. If use GeyserApi.api().onlineConnections() and iterate myself through the returned list, it consistently finds Bedrock connections accurately.

I can easily reproduce, but I'm not a Java expert, so getting am lost figuring out where the connectionsByUuid() function is failing.

For now, I'm use an onlineConnections() iterator comparing returned uuids instead of calling connectionByUuid() since that method seems to be reliable.

Code to reproduce is below.

To Reproduce

This is the core of the implementation of choosing a different server for bedrock vs java players, and it exhibits the issue. You can find the full plugin implementation at https://github.com/tadhunt/VelocityGeyserTransfer.

    @Subscribe
    public void onServerChooseEvent(PlayerChooseInitialServerEvent chooseServerEvent) {
        GeyserApi api = GeyserApi.api();

        if(api == null) {
            logger.info("GeyserApi not yet initialized (ignore)");
            return;
        }

        UUID uuid = chooseServerEvent.getPlayer().getUniqueId();
        logger.info(String.format("uuid: %s", uuid.toString()));

        List<? extends GeyserConnection> connections = GeyserApi.api().onlineConnections();
        GeyserConnection connection = null;
        for (GeyserConnection c : connections) {
            logger.info(String.format("java %s bedrock %s javauuid %s xuid %s", c.javaUsername(), c.bedrockUsername(), c.javaUuid().toString(), c.xuid()));

            UUID cUuid = c.javaUuid();
            if(uuid.equals(cUuid)) {
                connection = c;
            }
        }
        if(connection == null) {
            logger.info(String.format("uuid %s is not a bedrock player", uuid.toString()));
            return;
        }

        GeyserConnection c = api.connectionByUuid(uuid);
        if(c == null) {
            logger.info(String.format("bug: found by uuid compare, but not via connectionByUuid", uuid.toString()));
            return;
        }

        // about 9 out of 10 connections from the *same* bedrock player and *same* client, we reach this point,
        // but most of the time we exit via the "bug:" path above

        chooseServerEvent.setInitialServer(/*picks a server via my business logic*/)
}

Expected behaviour

My expectation is that when I have a UUID for a player, that

GeyserConnection connection = GeyserApi.api().connectionByUuid(uuid);

return a non-null GeyserConnection when the given uuid represents a Bedrock player connected to Velocity via Geyser.

Screenshots / Videos

PII has been replaced with NNNN, UUUU, and XXXX, otherwise this velocity console log is unchanged.

...
[11:40:58 INFO] [geyser]: Started Geyser on 0.0.0.0:10000
[11:40:58 INFO] [geyser]: Done (2.751s)! Run /geyser help for help!
[11:41:02 INFO] [geyser]: /N.N.N.N:NNNNNN tried to connect!
[11:41:03 INFO] [geyser]: Player connected with username UUUUUUUUUU
[11:41:03 INFO] [geyser]: UUUUUUUUUU (logged in as: UUUUUUUUUU) has connected to the Java server
[11:41:03 INFO] [floodgate]: Floodgate player who is logged in as *UUUUUUUUUU 00000000-0000-0000-000N-NNNNNNNNNNNN joined
[11:41:03 INFO]: [connected player] *UUUUUUUUUU (/N.N.N.N:0) has connected
[11:41:03 INFO] [velocitygeysertransfer]: uuid: 00000000-0000-0000-000N-NNNNNNNNNNNN
[11:41:03 INFO] [velocitygeysertransfer]: java UUUUUUUUUU bedrock UUUUUUUUUU javauuid 00000000-0000-0000-000N-NNNNNNNNNNNN xuid XXXXXXXXXXXXXXXXXXX
[11:41:03 INFO] [velocitygeysertransfer]: bug: found by uuid compare, but not via connectionByUuid

Server Version and Plugins

> velocity version
[12:21:24 INFO]: Velocity 3.1.2-SNAPSHOT (git-97770cd1-b196)
[12:21:24 INFO]: Copyright 2018-2021 Velocity Contributors. Velocity is licensed under the terms of the GNU General Public License v3.
[12:21:24 INFO]: velocitypowered.com - GitHub
> velocity plugins
[12:21:27 INFO]: Plugins: floodgate, geyser, velocitygeysertransfer
> 

Geyser Dump

No response

Geyser Version

[12:20:56 INFO]: This server is running Geyser version 2.1.0-SNAPSHOT (git-player-transfer-b27b1c8) (Java: 1.19.3, Bedrock: 1.19.20 - 1.19.50)

Minecraft: Bedrock Edition Device/Version

ipad 1.19.50

Additional Context

No response

Tim203 commented 1 year ago

Thank you for the detailed information! By just looking at the provided info and knowing how the API works internally, I expect this to the result of a race condition. Currently Geyser relies on a specific packet to know the uuid and username of the Bedrock player, and almost immediately after Velocity sent that packet PlayerChooseInitialServerEvent is fired.

Before Geyser receives that specific packet, Geyser sees that connection as a 'pending' connection and makes a guess at the uuid and username it expects depending on the selected auth-type. #onlineConnections() includes all players on Geyser (both 'pending' and 'connected') and the other methods including #connectionByUuid(UUID) only includes 'connected' players which would explain why #onlineConnections() is working all the time and #connectionByUuid(UUID) isn't.

This is only a theory, I haven't had the time to test it yet.

If this is the case, the following behavior could be observed: