From 9b54794f6b012cc1e84d9875ea07d097761af796 Mon Sep 17 00:00:00 2001 From: Dan Mulloy Date: Tue, 15 Jun 2021 23:58:40 -0400 Subject: [PATCH] Fix cloning in 1.17 Addresses #1222 --- .travis.yml | 4 +- .../protocol/events/PacketContainer.java | 25 ++++++--- .../protocol/injector/StructureCache.java | 55 +++++++++---------- .../protocol/reflect/ObjectWriter.java | 11 ++-- .../reflect/cloning/AggregateCloner.java | 10 ++-- .../protocol/events/PacketContainerTest.java | 48 +++++++++------- 6 files changed, 84 insertions(+), 69 deletions(-) diff --git a/.travis.yml b/.travis.yml index 5678ed91..9484886d 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,6 +1,8 @@ language: java +os: linux +dist: focal jdk: - - openjdk8 + - openjdk16 script: mvn clean test install: true notifications: diff --git a/src/main/java/com/comphenix/protocol/events/PacketContainer.java b/src/main/java/com/comphenix/protocol/events/PacketContainer.java index 73a3b025..1843df8a 100644 --- a/src/main/java/com/comphenix/protocol/events/PacketContainer.java +++ b/src/main/java/com/comphenix/protocol/events/PacketContainer.java @@ -35,6 +35,7 @@ import com.comphenix.protocol.reflect.cloning.*; import com.comphenix.protocol.reflect.cloning.AggregateCloner.BuilderParameters; import com.comphenix.protocol.reflect.fuzzy.FuzzyMethodContract; import com.comphenix.protocol.reflect.instances.DefaultInstances; +import com.comphenix.protocol.reflect.instances.InstanceProvider; import com.comphenix.protocol.utility.MinecraftMethods; import com.comphenix.protocol.utility.MinecraftReflection; import com.comphenix.protocol.utility.MinecraftVersion; @@ -82,11 +83,11 @@ public class PacketContainer implements Serializable { // Support for serialization private static ConcurrentMap, Method> writeMethods = Maps.newConcurrentMap(); private static ConcurrentMap, Method> readMethods = Maps.newConcurrentMap(); - + // Used to clone packets private static final AggregateCloner DEEP_CLONER = AggregateCloner .newBuilder() - .instanceProvider(DefaultInstances.DEFAULT) + .instanceProvider(StructureCache::newPacket) .andThen(BukkitCloner.class) .andThen(ImmutableDetector.class) .andThen(JavaOptionalCloner.class) @@ -97,7 +98,7 @@ public class PacketContainer implements Serializable { private static final AggregateCloner SHALLOW_CLONER = AggregateCloner .newBuilder() - .instanceProvider(DefaultInstances.DEFAULT) + .instanceProvider(StructureCache::newPacket) .andThen(param -> { if (param == null) throw new IllegalArgumentException("Cannot be NULL."); @@ -110,8 +111,12 @@ public class PacketContainer implements Serializable { .build(); // Packets that cannot be cloned by our default deep cloner - private static final Set CLONING_UNSUPPORTED = Sets.newHashSet( - PacketType.Play.Server.UPDATE_ATTRIBUTES, PacketType.Status.Server.SERVER_INFO); + private static final Set FAST_CLONE_UNSUPPORTED = Sets.newHashSet( + PacketType.Play.Server.BOSS, + PacketType.Play.Server.ADVANCEMENTS, + PacketType.Play.Client.USE_ENTITY, + PacketType.Status.Server.SERVER_INFO + ); /** * Creates a packet container for a new packet. @@ -1155,14 +1160,16 @@ public class PacketContainer implements Serializable { */ public PacketContainer deepClone() { Object clonedPacket = null; - - // Fall back on the alternative (but slower) method of reading and writing back the packet - if (!CLONING_UNSUPPORTED.contains(type)) { + + if (!FAST_CLONE_UNSUPPORTED.contains(type)) { try { clonedPacket = DEEP_CLONER.clone(getHandle()); - } catch (Exception ignored) {} + } catch (Exception ex) { + FAST_CLONE_UNSUPPORTED.add(type); + } } + // Fall back on the slower alternative method of reading and writing back the packet if (clonedPacket == null) { clonedPacket = SerializableCloner.clone(this).getHandle(); } diff --git a/src/main/java/com/comphenix/protocol/injector/StructureCache.java b/src/main/java/com/comphenix/protocol/injector/StructureCache.java index f4ff5be4..002bda06 100644 --- a/src/main/java/com/comphenix/protocol/injector/StructureCache.java +++ b/src/main/java/com/comphenix/protocol/injector/StructureCache.java @@ -26,7 +26,6 @@ import com.comphenix.protocol.PacketType; import com.comphenix.protocol.injector.packet.PacketRegistry; import com.comphenix.protocol.reflect.StructureModifier; import com.comphenix.protocol.reflect.compiler.BackgroundCompiler; -import com.comphenix.protocol.reflect.compiler.CompileListener; import com.comphenix.protocol.reflect.compiler.CompiledStructureModifier; import com.comphenix.protocol.reflect.instances.DefaultInstances; import com.comphenix.protocol.utility.MinecraftReflection; @@ -41,9 +40,28 @@ import net.minecraft.network.PacketDataSerializer; */ public class StructureCache { // Structure modifiers - private static ConcurrentMap> structureModifiers = new ConcurrentHashMap<>(); + private static final ConcurrentMap> structureModifiers = new ConcurrentHashMap<>(); - private static Set compiling = new HashSet<>(); + private static final Set compiling = new HashSet<>(); + + public static Object newPacket(Class clazz) { + Object result = DefaultInstances.DEFAULT.create(clazz); + + // TODO make these generic + if (result == null) { + try { + return clazz.getConstructor(PacketDataSerializer.class).newInstance(new PacketDataSerializer(new ZeroBuffer())); + } catch (ReflectiveOperationException ex) { + try { + return clazz.getConstructor(PacketDataSerializer.class).newInstance(new ZeroPacketDataSerializer()); + } catch (ReflectiveOperationException ex1) { + throw new IllegalArgumentException("Failed to create packet: " + clazz, ex); + } + } + } + + return result; + } /** * Creates an empty Minecraft packet of the given type. @@ -52,25 +70,8 @@ public class StructureCache { */ public static Object newPacket(PacketType type) { Class clazz = PacketRegistry.getPacketClassFromType(type, true); - - // Check the return value if (clazz != null) { - // TODO: Optimize DefaultInstances - Object result = DefaultInstances.DEFAULT.create(clazz); - - if (result == null) { - try { - return clazz.getConstructor(PacketDataSerializer.class).newInstance(new PacketDataSerializer(new ZeroBuffer())); - } catch (ReflectiveOperationException ex) { - try { - return clazz.getConstructor(PacketDataSerializer.class).newInstance(new ZeroPacketDataSerializer()); - } catch (ReflectiveOperationException ex1) { - throw new IllegalArgumentException("Failed to create packet for type: " + type, ex); - } - } - } - - return result; + return newPacket(clazz); } throw new IllegalArgumentException("Cannot find associated packet class: " + type); } @@ -115,13 +116,13 @@ public class StructureCache { * @return A structure modifier. */ public static StructureModifier getStructure(final PacketType type, boolean compile) { - Preconditions.checkNotNull(type); + Preconditions.checkNotNull(type, "type cannot be null"); StructureModifier result = structureModifiers.get(type); // We don't want to create this for every lookup if (result == null) { // Use the vanilla class definition - final StructureModifier value = new StructureModifier( + final StructureModifier value = new StructureModifier<>( PacketRegistry.getPacketClassFromType(type, true), MinecraftReflection.getPacketClass(), true); result = structureModifiers.putIfAbsent(type, value); @@ -139,12 +140,8 @@ public class StructureCache { final BackgroundCompiler compiler = BackgroundCompiler.getInstance(); if (!compiling.contains(type) && compiler != null) { - compiler.scheduleCompilation(result, new CompileListener() { - @Override - public void onCompiled(StructureModifier compiledModifier) { - structureModifiers.put(type, compiledModifier); - } - }); + compiler.scheduleCompilation(result, + compiledModifier -> structureModifiers.put(type, compiledModifier)); compiling.add(type); } } diff --git a/src/main/java/com/comphenix/protocol/reflect/ObjectWriter.java b/src/main/java/com/comphenix/protocol/reflect/ObjectWriter.java index 91e3a14b..f0c963a3 100644 --- a/src/main/java/com/comphenix/protocol/reflect/ObjectWriter.java +++ b/src/main/java/com/comphenix/protocol/reflect/ObjectWriter.java @@ -117,12 +117,11 @@ public class ObjectWriter { } // Copy private fields underneath - Class superclass = commonType.getSuperclass(); - - if (superclass != null && !superclass.equals(Object.class)) { - copyToInternal(source, destination, superclass, false); - } - +// Class superclass = commonType.getSuperclass(); +// +// if (superclass != null && !superclass.equals(Object.class)) { +// copyToInternal(source, destination, superclass, false); +// } } catch (FieldAccessException e) { throw new RuntimeException("Unable to copy fields from " + commonType.getName(), e); } diff --git a/src/main/java/com/comphenix/protocol/reflect/cloning/AggregateCloner.java b/src/main/java/com/comphenix/protocol/reflect/cloning/AggregateCloner.java index 9be0c581..a80daa99 100644 --- a/src/main/java/com/comphenix/protocol/reflect/cloning/AggregateCloner.java +++ b/src/main/java/com/comphenix/protocol/reflect/cloning/AggregateCloner.java @@ -256,11 +256,11 @@ public class AggregateCloner implements Cloner { if (index < cloners.size()) { Cloner cloner = cloners.get(index); - try { - return cloner.clone(source); - } catch (Exception ex) { - throw new RuntimeException("Failed to clone " + source + " (" + source.getClass() + ") with " + cloner, ex); - } +// try { + return cloner.clone(source); +// } catch (Exception ex) { +// throw new RuntimeException("Failed to clone " + source + " (" + source.getClass() + ") with " + cloner, ex); +// } } // Damn - failure diff --git a/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java b/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java index 8a511923..31a1d133 100644 --- a/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java +++ b/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java @@ -413,7 +413,7 @@ public class PacketContainerTest { } field.setAccessible(true); - assertEquals(field.get(snapshot), field.get(clonedSnapshot)); + testEquality(field.get(snapshot), field.get(clonedSnapshot)); } catch (AssertionError e) { throw e; } catch (Throwable ex) { @@ -611,8 +611,27 @@ public class PacketContainerTest { assertArrayEquals(components, back); } + private void assertPacketsEqual(PacketContainer constructed, PacketContainer cloned) { + StructureModifier firstMod = constructed.getModifier(), secondMod = cloned.getModifier(); + assertEquals(firstMod.size(), secondMod.size()); + + if (PacketType.Status.Server.SERVER_INFO.equals(constructed.getType())) { + assertArrayEquals(SerializationUtils.serialize(constructed), SerializationUtils.serialize(cloned)); + } else { + // Make sure all the fields are equivalent + for (int i = 0; i < firstMod.size(); i++) { + if (firstMod.getField(i).getType().isArray()) + assertArrayEquals(getArray(firstMod.read(i)), getArray(secondMod.read(i))); + else + testEquality(firstMod.read(i), secondMod.read(i)); + } + } + } + @Test - public void testDeepClone() { + public void testCloning() { + boolean failed = false; + // Try constructing all the packets for (PacketType type : PacketType.values()) { if (type.isDeprecated() || type.name().contains("CUSTOM_PAYLOAD") || type.name().contains("TAGS") || !type.isSupported() @@ -644,28 +663,19 @@ public class PacketContainerTest { //constructed.getModifier().write(1, TEST_COMPONENT); } - // Clone the packet - PacketContainer cloned = constructed.deepClone(); + // Clone the packet both ways + PacketContainer shallowCloned = constructed.shallowClone(); + assertPacketsEqual(constructed, shallowCloned); - // Make sure they're equivalent - StructureModifier firstMod = constructed.getModifier(), secondMod = cloned.getModifier(); - assertEquals(firstMod.size(), secondMod.size()); - - if (PacketType.Status.Server.SERVER_INFO.equals(type)) { - assertArrayEquals(SerializationUtils.serialize(constructed), SerializationUtils.serialize(cloned)); - } else { - // Make sure all the fields are equivalent - for (int i = 0; i < firstMod.size(); i++) { - if (firstMod.getField(i).getType().isArray()) - assertArrayEquals(getArray(firstMod.read(i)), getArray(secondMod.read(i))); - else - testEquality(firstMod.read(i), secondMod.read(i)); - } - } + PacketContainer deepCloned = constructed.deepClone(); + assertPacketsEqual(constructed, deepCloned); } catch (Exception ex) { ex.printStackTrace(); + failed = true; } } + + assertFalse("Packet(s) failed to clone", failed); } // Convert to objects that support equals()