Use a concurrent cache in CraftRegistry#13973
Open
electronicboy wants to merge 1 commit into
Open
Conversation
7b8d3b3 to
6e4e6fe
Compare
CraftRegistry#get lazily populates its backing cache, reading and then writing a plain HashMap with no synchronization. That cache is reached off the main thread while building ItemMeta: CraftItemStack#getItemMeta resolves the item's CraftItemType via CraftRegistry#get to select the meta subclass, and CraftMetaItem resolves the item's enchantments through CraftRegistry#get as well. Plugins build ItemMeta from other threads, so this map is read and written concurrently, which is unsafe for a HashMap. Switch the cache to a ConcurrentHashMap and collapse the read-then-put into an atomic computeIfAbsent. This is safe against computeIfAbsent's recursive-update restriction: the wrapper constructors only store their holder and defer derived work lazily, so the mapping function never re-enters this cache. lockReferenceHolders is made volatile as it is now read from the concurrent path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
6e4e6fe to
61ed18e
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens CraftRegistry#get for off-main-thread access by making its lazy cache population thread-safe, addressing unsafe concurrent reads/writes to a plain HashMap when plugins build ItemMeta asynchronously.
Changes:
- Replace the backing cache with a
ConcurrentHashMap. - Use
computeIfAbsentto atomically load and cache registry entries. - Make
lockReferenceHoldersvolatileto ensure cross-thread visibility from theget()path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
CraftRegistry#getlazily populates its backingcache, reading and then writing a plainHashMapwith no synchronization.This cache is reached off the main thread while building
ItemMeta:CraftItemStack#getItemMetaresolves the item'sCraftItemTypeviaCraftRegistry#get(to select the meta subclass), andCraftMetaItemresolves the item's enchantments throughCraftRegistry#get.Plugins routinely build
ItemMetafrom other threads, so this map is read and written concurrently, which is unsafe for aHashMap.Change
cacheto aConcurrentHashMap.get()-then-put()sequence into a single atomiccomputeIfAbsent.lockReferenceHoldersvolatile, since it is now read from the concurrentget()path and flipped once during startup.Why this is safe for
computeIfAbsentConcurrentHashMap#computeIfAbsentforbids the mapping function from updating the same map (recursive update →IllegalStateException/bin live-lock). The registry wrapper constructors registered inPaperRegistriesuniformly just store theirHolderand defer derived work viaSuppliers.memoize(...); the mapping function only touches the NMS registry andHolder.Reference#createStandAlone, never this cache. So it never re-enters the map. Any cross-registry resolution a wrapper may perform later targets a differentCraftRegistryinstance (different map) and is lazy regardless.