Fabric API

Fabric API

106M Downloads

Syncing for TrackedDataHandlerRegistry

Gaming32 opened this issue ยท 15 comments

commented

Currently, TrackedDataHandlerRegistry does not use a registry, which means that when TrackedDataHandlers are registered, their numeric ID is simply incremented. As such, if the mod loading order differs between client and server, or if their mod lists don't match exactly, there is a very real possibility that these IDs will mismatch, corrupting any EntityTrackerUpdateS2CPacket that involve custom TrackedDataHandlers.

This issue proposes that some system be added to sync these IDs. This could be resolved by either migrating the class to a registry, or by setting up some kind of sync to synchronize these IDs on join. I personally believe using a registry would be the ideal approach here since registry syncing already exists.

commented

What is the use case for making custom TDHs?

commented

Terraform uses it for its boats here. While the order issue shouldn't occur since Terraform only uses the tracked data handler for its own entity class, is this not a valid use of tracked data?

commented

The order issue can still occur. Say the mod A on the server has a handler with ID 28 and mod B has a handler with ID 29. Then, for whatever reason, they're swapped on the client. Any EntityTrackerUpdateS2CPacket with either of these handlers will be corrupt.

commented

can be easily replaced with an optional int handler that already exists. its just passing the raw registry id as an optional

commented

Right, since the ID of the tracked data handler isn't dependent on the entity unlike the tracked data itself.

What do you mean with the optional integer handler? The ID read in the DataTracker$SerializedEntry.fromBuf method that is used to look up a tracked data handler isn't tied to a registry.

commented

The terraform boat handler is just a registry id write/read handler, which is a lookup from the registry to write the raw id.

That said, they also do it correctly and initialize it from their mod initializer so there should be no issues with their mod as long as other mods also do this

commented

Isn't registering from the mod initializer the issue, though? The TrackedDataHandlerRegistry.register method just adds to a list:

private static final Int2ObjectBiMap<TrackedDataHandler<?>> DATA_HANDLERS = Int2ObjectBiMap.create(16);

public static void register(TrackedDataHandler<?> handler) {
	DATA_HANDLERS.add(handler);
}

The order in which a mod initializer called the TrackedDataHandlerRegistry.register method isn't guaranteed to be stable, so the order of tracked data handlers isn't guaranteed to be stable, which is dangerous when a mismatch could definitely occur between the server and the client.

commented

the handler registration only matters that it happens before world load. handled data is serialized to the nbt at save. mod load order should be reliable from system to system using the same (handler adding) modset. The larger issue is when mods put them inside classes, for example a new handler inside a custom entity, which may be loaded at any time during play

If truly random mod load order can occur on the same mod files, then arguably every registry (thats lowercase "registry" not "Registry") should be synced or wrapped in a synced registry

commented

Relying on mod load order is generally pretty brittle. Loader even shuffles mods in dev to discourage relying on load order. It's also generally assumed that clients with more content mods can join servers with less. If the order is mismatched, the packet is silently corrupted, causing very difficult to diagnose errors.

commented

generally assumed that clients with more content mods can join servers with less

...I have never heard this. this can screw up block display among other things and is always a bad idea

commented

I don't think we expect mods to register things we don't offer registrars for, but maybe I was wrong.

The likely solution is not to add a syncing, but to offer a wrapped handler taking Identifier as the ID.

commented

What is the use case for making custom TDHs?

Anytime you have data that cannot be represented by existing data types for TrackedData such as a custom enum state for what state your entity is in like this:
https://github.com/TelepathicGrunt/Bumblezone/blob/1a3d3be95332c521770095fb0bb0ed5181b64d0f/common/src/main/java/com/telepathicgrunt/the_bumblezone/entities/mobs/BeeQueenEntity.java#L111

On Forge/NeoForge, my mod and Ars Nouveau had a verified race condition by using the vanilla way to register our custom TrackedDataHandlers because those modloaders were parallelizing mod loading. So client and server sometimes did not register our two mod's TrackedDataHandlers in the same order which then caused client disconnects that were very hard to track down.

There is a second way to cause disconnects with TrackedData and that is people mixin injecting DataTracker.registerData into the same entity classes as well. Two mods mixin adding that to same class could cause a disconnect if the mixins apply out of order on client vs server because DataTracker.TRACKED_ENTITIES is also order dependent. I have found many mods mixin injecting DataTracker.registerData as well so that is also a concern.

The entirety of TrackData's workflow is highly dependent on same order on client vs servers that it's surprisingly fragile. Yet we had gone so long without reports of issues on Fabric. That or player just don't know how to diagnose the issue if they get it which would be incredibly difficult for them to figure out. Wouldn't hurt to try and make the system more robust or less likely to have a desync between client and server.

commented

Isnt enumstate also just an INT behind the scenes? While I agree that we should have an id based handler/serializer system, between INT, Optional INT, and NBT, pretty much every use-case of new handlers is already covered.

If the same modset and versions are used client and serverside, the order is deterministic on fabric and is not threaded. mixin application among other things should allow for tracked data itself to be used without problems as long as the modsets are the same. different versions and different content mods would have caused problems but thats always been the case.

That said, since deterministic mod load order is not something that wants to be accepted, and thus randomized mod load order is perfectly within scope, there is a need for a completely separate system utilizing ids or a synced registry

commented

The API proposed in #1049 only applies to tracked data; this issue should remain open as tracked data handler support can be improved without defining an API for tracked data.

commented

Duplicate of #1049