Skip binding references on SimpleRegistry#freeze #4123
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Registry Order Fix
Description
These changes fix a rare bug where overwritten registry entries get bound to the wrong value.
Explanation
This problem occurs on
SimpleRegistry#freeze
atvalueToEntry.forEach
whenever a new value has been assigned to an existing key. The issue is thatSimpleRegistry#valueToEntry
is an instance ofIdentityHashMap
, which does not guarantee that itsforEach
method will run in order of insertion. Thus, occasionally, registry entries can get bound in an unpredictable order. The actual order is based on object identity; however, this order is also not guaranteed.Design Considerations
One downside to this solution is that any existing mixins wrapping this operation may not get invoked. Thus, there is some potential for breakage with existing mods. However, I am not aware of any such mods and believe this change to be largely safe.
Alternate Strategies
In fixing this issue, I had also considered switching the implementation of
valueToEntry
to be aLinkedHashMap
, which would guarantee order by insertion; however, such a change would alter fundamental systems involving registry operations. In my opinion, the scope of this change is greater than simply skipping that binding in the first place.Automated Testing
This change is already covered by an existing test in
RegistrySyncTest
atregisterBlocks
, which confirms that this bind operation was unnecessary.