ViaVersion Fix user connection race condition (PoC)

This is a PoC implementation of a fix for #2835

The idea is that on join event (on the main thread) we set the UserConnection in the ConnectionManager, this means any other plugin requesting player info (like the user's protocol) in PlayerJoinEvent (anything above lowest priority) will get correct info. For more info on the issue refer to #2835 .

This change makes onLoginSuccess be indepotent (assumed to be called twice for each player, once on login event from main thread, and once from network thread), and adds a listener for PlayerLoginEvent on lowest priority that finds the UserConnection from the Player -> EntityPlayer -> PlayerConnection -> NetworkManager -> Channel -> Viaversion encoder -> UserConnection

Edit: I have tested and validated that this works as intended on sportpaper 1.8.8 (fork of paper 1.8), i'd assume it should work on any 1.8 bukkit-based platform, if higher mc versions made changes it may not work on them, i did not test them.

Initially the PR had the logic on Login event, had to move it to Join event because bukkit does not create (and set) the PlayerConnection until after login event

Edit 2: Upgraded reflection to be more robust, works in spigot 1.8.8, paper 1.8.8, sportpaper 1.8.8, paper 1.12.2, paper 1.14.4, paper 1.17.1 and paper 1.18.1, in all of the cases connecting with a 1.18.1 client

Mar 18 '22 08:03 Pablete1234

I have synchronized onLoginSuccess, without making that method atomic, you could have the following order of events:

1 - network thread calls onLoginSuccess fully 2 - main thread calls onLoginSuccess, newlyAdded = false (it wasn't just added) 3 - the user disconnects, the listener calls onDisconnect 4 - the main thread continues the onLoginSuccess execution, re-adding to the map of uuid-connection, and without adding a listener to cleanup later

Mar 19 '22 12:03 Pablete1234

Looks better 👍 I'll properly review in a few days or so

Mar 19 '22 14:03 kennytv

Looking forward to this being further reviewed and/or merged; we've had this PR deployed in a production environment for a few months now with no issues. It has solved the relevant issue in our experience.

Jul 30 '22 06:07 calcastor

Sorry, this completely went under my radar. I'll make sure to review it again soon-ish

Jul 30 '22 07:07 kennytv

Rebased ontop of master & added support for mojmap names, as well as addressing the other concerns on the review

Aug 19 '22 06:08 Pablete1234

The issue is still there https://github.com/ViaVersion/ViaVersion/issues/3140

Oct 16 '22 22:10 Mercenarioo