Fix NPE from map corruption when sync handlers register entries during init

[Luca-Guettinger]

PanelSyncManager.initialize() iterates over syncHandlers (an Object2ReferenceLinkedOpenHashMap) via forEach while calling init() on each handler. If a handler registers additional sync handlers during init() (e.g., GT5's GhostCircuitSyncHandler registering an IntSyncValue), the map is structurally modified during iteration. Unlike standard Java collections, fastutil's open-addressing maps don't throw ConcurrentModificationException, they fail silently, producing phantom null entries that cause an NPE. The fix snapshots the map's keys before iterating and looks up each value from the live map. Keys are immutable Strings, so the snapshot is always valid, and get() works correctly after any rehash. Entry objects can't be snapshotted because fastutil's MapEntry stores an internal array index that becomes stale after rehash.

[YannickMG]

I would rather have us catch people trying to register Sync Handlers during sync handler initialization and throw at that time, so the contents of syncHandlers can be relied upon. There isn't actually a scenario where we should be doing this, so we shouldn't stop this from crashing.

If we do end up deciding this is a good idea anyway, at least consider adding some loud logging to the branch where there was a null in the list.

[brachy84]

I agree with Yannick. We should rather lock before init all sycn handlers.

[brachy84]

Also this pr doesnt register the sync handlers that were just registered in other sync handlers init. It only registers the current sync handlers. So you would need put this in a while loop and check when the size no longer changes.

[YannickMG]

As per discussion, we shouldn't allow the anti pattern of registering sync handlers during sync handler initialisation.

I still suggest that instead we make it crash earlier and/or with a clear debug message.

[Luca-Guettinger]

Also this pr doesnt register the sync handlers that were just registered in other sync handlers init. It only registers the current sync handlers. So you would need put this in a while loop and check when the size no longer changes.

it does, putSyncValue already handles this recursively.

I do get that we dont wannt to discourage this, but i think just adding a log message might be better then restricting it completly, or why would it be bad if we sometimes allow that?

[brachy84]

it does, putSyncValue already handles this recursively.

I do get that we dont wannt to discourage this, but i think just adding a log message might be better then restricting it completly, or why would it be bad if we sometimes allow that?

I dont see where putSyncValue does anything recursively.
Registering sync handlers in init of other sync handlers is bad practice. This will not be further discussed.