From b3ac8604eb0c1799869d2aff4dabc55381f18174 Mon Sep 17 00:00:00 2001 From: CraftBukkit/Spigot Date: Tue, 3 Oct 2023 07:15:37 +1100 Subject: [PATCH] #1264: Load Bukkit class before creating Registry item This fixes a registry load order issue, which could cause that two different instances of the same registry item could exist, when the Bukkit class was not loaded before an item was queried. By: DerFrZocker --- .../org/bukkit/craftbukkit/CraftRegistry.java | 38 ++++- .../org/bukkit/RegistryLoadOrderTest.java | 145 ++++++++++++++++++ 2 files changed, 176 insertions(+), 7 deletions(-) create mode 100644 paper-server/src/test/java/org/bukkit/RegistryLoadOrderTest.java diff --git a/paper-server/src/main/java/org/bukkit/craftbukkit/CraftRegistry.java b/paper-server/src/main/java/org/bukkit/craftbukkit/CraftRegistry.java index b15f26bff2..98067400f0 100644 --- a/paper-server/src/main/java/org/bukkit/craftbukkit/CraftRegistry.java +++ b/paper-server/src/main/java/org/bukkit/craftbukkit/CraftRegistry.java @@ -46,32 +46,35 @@ public class CraftRegistry implements Registry { public static Registry createRegistry(Class bukkitClass, IRegistryCustom registryHolder) { if (bukkitClass == GameEvent.class) { - return new CraftRegistry<>(registryHolder.registryOrThrow(Registries.GAME_EVENT), CraftGameEvent::new); + return new CraftRegistry<>(GameEvent.class, registryHolder.registryOrThrow(Registries.GAME_EVENT), CraftGameEvent::new); } if (bukkitClass == MusicInstrument.class) { - return new CraftRegistry<>(registryHolder.registryOrThrow(Registries.INSTRUMENT), CraftMusicInstrument::new); + return new CraftRegistry<>(MusicInstrument.class, registryHolder.registryOrThrow(Registries.INSTRUMENT), CraftMusicInstrument::new); } if (bukkitClass == Structure.class) { - return new CraftRegistry<>(registryHolder.registryOrThrow(Registries.STRUCTURE), CraftStructure::new); + return new CraftRegistry<>(Structure.class, registryHolder.registryOrThrow(Registries.STRUCTURE), CraftStructure::new); } if (bukkitClass == StructureType.class) { - return new CraftRegistry<>(BuiltInRegistries.STRUCTURE_TYPE, CraftStructureType::new); + return new CraftRegistry<>(StructureType.class, BuiltInRegistries.STRUCTURE_TYPE, CraftStructureType::new); } if (bukkitClass == TrimMaterial.class) { - return new CraftRegistry<>(registryHolder.registryOrThrow(Registries.TRIM_MATERIAL), CraftTrimMaterial::new); + return new CraftRegistry<>(TrimMaterial.class, registryHolder.registryOrThrow(Registries.TRIM_MATERIAL), CraftTrimMaterial::new); } if (bukkitClass == TrimPattern.class) { - return new CraftRegistry<>(registryHolder.registryOrThrow(Registries.TRIM_PATTERN), CraftTrimPattern::new); + return new CraftRegistry<>(TrimPattern.class, registryHolder.registryOrThrow(Registries.TRIM_PATTERN), CraftTrimPattern::new); } return null; } + private final Class bukkitClass; private final Map cache = new HashMap<>(); private final IRegistry minecraftRegistry; private final BiFunction minecraftToBukkit; + private boolean init; - public CraftRegistry(IRegistry minecraftRegistry, BiFunction minecraftToBukkit) { + public CraftRegistry(Class bukkitClass, IRegistry minecraftRegistry, BiFunction minecraftToBukkit) { + this.bukkitClass = bukkitClass; this.minecraftRegistry = minecraftRegistry; this.minecraftToBukkit = minecraftToBukkit; } @@ -83,6 +86,27 @@ public class CraftRegistry implements Registry { return cached; } + // Make sure that the bukkit class is loaded before creating an instance. + // This ensures that only one instance with a given key is created. + // + // Without this code (when bukkit class is not loaded): + // Registry#get -> #createBukkit -> (load class -> create default) -> put in cache + // Result: Registry#get != . for possible one registry item + // + // With this code (when bukkit class is not loaded): + // Registry#get -> (load class -> create default) -> Registry#get -> get from cache + // Result: Registry#get == . + if (!init) { + init = true; + try { + Class.forName(bukkitClass.getName()); + } catch (ClassNotFoundException e) { + throw new RuntimeException("Could not load registry class " + bukkitClass, e); + } + + return get(namespacedKey); + } + B bukkit = createBukkit(namespacedKey, minecraftRegistry.getOptional(CraftNamespacedKey.toMinecraft(namespacedKey)).orElse(null)); if (bukkit == null) { return null; diff --git a/paper-server/src/test/java/org/bukkit/RegistryLoadOrderTest.java b/paper-server/src/test/java/org/bukkit/RegistryLoadOrderTest.java new file mode 100644 index 0000000000..5ae8ad5ce3 --- /dev/null +++ b/paper-server/src/test/java/org/bukkit/RegistryLoadOrderTest.java @@ -0,0 +1,145 @@ +package org.bukkit; + +import static org.junit.jupiter.api.Assertions.*; +import com.mojang.serialization.Lifecycle; +import java.util.function.BiFunction; +import java.util.function.Supplier; +import java.util.stream.Stream; +import net.minecraft.core.IRegistry; +import net.minecraft.core.RegistryMaterials; +import net.minecraft.resources.MinecraftKey; +import net.minecraft.resources.ResourceKey; +import org.bukkit.craftbukkit.CraftRegistry; +import org.bukkit.support.AbstractTestingBase; +import org.jetbrains.annotations.NotNull; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +public class RegistryLoadOrderTest extends AbstractTestingBase { + + private static boolean initInterface = false; + private static boolean initAbstract = false; + private static Registry registry; + + public static Stream data() { + return Stream.of( + Arguments.of( + (Supplier) () -> initInterface, + BukkitInterfaceTestType.class, + (BiFunction) CraftBukkitInterfaceTestType::new, + (Supplier) () -> BukkitInterfaceTestType.TEST_ONE, + (Supplier) () -> BukkitInterfaceTestType.TEST_TWO + ), + Arguments.of( + (Supplier) () -> initAbstract, + BukkitAbstractTestType.class, + (BiFunction) CraftBukkitAbstractTestType::new, + (Supplier) () -> BukkitAbstractTestType.TEST_ONE, + (Supplier) () -> BukkitAbstractTestType.TEST_TWO + ) + ); + } + + @ParameterizedTest + @MethodSource("data") + public void testRegistryLoadOrder(Supplier init, Class keyedClass, BiFunction minecraftToBukkit, Supplier first, Supplier second) { + testClassNotLoaded(init.get()); + + ResourceKey> resourceKey = ResourceKey.createRegistryKey(MinecraftKey.tryBuild("bukkit", "test-registry")); + RegistryMaterials minecraftRegistry = new RegistryMaterials<>(resourceKey, Lifecycle.experimental()); + + minecraftRegistry.register(ResourceKey.create(resourceKey, MinecraftKey.tryBuild("bukkit", "test-one")), new MinecraftTestType(), Lifecycle.experimental()); + minecraftRegistry.register(ResourceKey.create(resourceKey, MinecraftKey.tryBuild("bukkit", "test-two")), new MinecraftTestType(), Lifecycle.experimental()); + minecraftRegistry.freeze(); + + registry = new CraftRegistry<>(keyedClass, minecraftRegistry, minecraftToBukkit); + testClassNotLoaded(init.get()); + + Object testOne = registry.get(new NamespacedKey("bukkit", "test-one")); + Object otherTestOne = registry.get(new NamespacedKey("bukkit", "test-one")); + Object testTwo = registry.get(new NamespacedKey("bukkit", "test-two")); + + assertNotNull(testOne); + assertNotNull(otherTestOne); + assertNotNull(testTwo); + assertNotNull(first.get()); + assertNotNull(second.get()); + + assertSame(testOne, otherTestOne); + assertSame(testOne, first.get()); + assertSame(otherTestOne, first.get()); + assertSame(testTwo, second.get()); + + assertTrue(init.get()); + } + + private void testClassNotLoaded(boolean init) { + assertFalse(init, """ + TestType class is already loaded, this test however tests the behavior when the class is not loaded. + This should normally not happen with how classes should be loaded. + Something has changed how classes are loaded and a more manual deeper look is required. + """); + } + + public interface BukkitInterfaceTestType extends Keyed { + BukkitInterfaceTestType TEST_ONE = get("test-one"); + BukkitInterfaceTestType TEST_TWO = get("test-two"); + + private static BukkitInterfaceTestType get(String key) { + initInterface = true; + if (registry == null) { + return null; + } + + return (BukkitInterfaceTestType) registry.get(new NamespacedKey("bukkit", key)); + } + } + + public static class CraftBukkitInterfaceTestType implements BukkitInterfaceTestType { + + private final NamespacedKey key; + + public CraftBukkitInterfaceTestType(NamespacedKey key, MinecraftTestType minecraftTestType) { + this.key = key; + } + + @NotNull + @Override + public NamespacedKey getKey() { + return key; + } + } + + public abstract static class BukkitAbstractTestType implements Keyed { + public static final BukkitAbstractTestType TEST_ONE = get("test-one"); + public static final BukkitAbstractTestType TEST_TWO = get("test-two"); + + private static BukkitAbstractTestType get(String key) { + initAbstract = true; + if (registry == null) { + return null; + } + + return (BukkitAbstractTestType) registry.get(new NamespacedKey("bukkit", key)); + } + } + + public static class CraftBukkitAbstractTestType extends BukkitAbstractTestType { + + private final NamespacedKey key; + + public CraftBukkitAbstractTestType(NamespacedKey key, MinecraftTestType minecraftTestType) { + this.key = key; + } + + @NotNull + @Override + public NamespacedKey getKey() { + return key; + } + } + + public static class MinecraftTestType { + } +}