Fabric API

Fabric API

106M Downloads

Expose NetworkHandler or ClientConnection to clientside network events, fix player retrieval

sfPlayer1 opened this issue ยท 4 comments

commented

MC creates and sets the player on the client thread by synchronizing the join packet, which happens after receiving other packets.

In the meanwhile PacketContext.getPlayer() returns null and the connection is not easily unobtainable for an immediate reply (only reflectively from MinecraftClient.clientConnection, ConnectScreen.connection or RealmsConnect.connection depending on the connection origin).

The potential null result and thread unsafety[1] of PacketContext.getPlayer() is problematic, maybe it's better to block its use on the network threads. Most of those uses are likely a bug, with the most significant exception being the serverside NetworkThreadUtils.forceMainThread(..., player.getServerWorld()), which could be replaced by exposing the server conveniently instead.

[1] There are no guarantees on the chronologically "correct" value propagating without proper synchronization. It may be significantly delayed or even sporadically revert to an outdated value between consecutive field reads (e.g. thread moved between cores and new core's cache has older state).

commented

MC creates and sets the player on the client thread by synchronizing the join packet, which happens after receiving other packets.

Hmm, is there any evidence that the client play network handler receives any packet before the game join one after it is initialized after receiving the login success packet?

The networking v1 should resolve the connection retrieval difficulty though.

For calling getPlayer in netty's event loop, preventing it is a good idea, but how? Should we emit a warning message or block the event loop until the game engine has finished its scheduled tasks (often the next tick)?

commented

IIRC this is a nice location for mods to do global data syncing. It ensures the world is still pending loading, vanilla didn't transfer any data yet.

Wrt getPlayer I was considering to ban its use via a thread check.

commented

IIRC this is a nice location for mods to do global data syncing.

Unfortunately it is not... In this stage, the Minecraft server may either be in server login network handler post-login cleanup (delayed accept of player) or just had the server play network handler setup.

These two protocols don't have shared packets, and if the client sends a packet during this time, it may be received by either handler, so packet sending between client reception of login success and game join is very non-feasible due to vanilla restrictions.

For data syncing, I have proposed to use login handshake system instead (though vanilla imposes a 600-tick timeout for login, that should be sufficient and can be modified by mods)

For getPlayer thread-check throwing, I guess we can handle it the same way like unregistered channel send warning, in which we have a default-on flag that enables thread check and throwing (if off, a log will be emitted instead)

commented

@sfPlayer1 Does this look feasible for player retrieval? FabLabsMC/networking-api-v1-draft@3e38612