From bdc41221dbb9feddeb2c44a7f37dd05f63d673db Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Mon, 29 Oct 2012 01:42:22 +0100 Subject: [PATCH] Fixed a serious misuse causing the creation of too many new classes. The different injectors using CGLib was using a custom CallbackFilter to optimize the resulting generated class. Unfortunately, the custom filter didn't properly implement equals() and hashCode(), and so every time a player logged, a new injector class had to be generated. This was fixed by making the injectors share a single CallbackFilter. In addition, I've begun adding cleanup code that will reset all static fields once the plugin has unloaded. --- ProtocolLib/dependency-reduced-pom.xml | 2 +- .../comphenix/protocol/ProtocolLibrary.java | 4 +++ .../protocol/async/AsyncListenerHandler.java | 2 +- .../protocol/injector/PacketConstructor.java | 2 +- .../player/NetworkObjectInjector.java | 26 ++++++++++------ .../player/NetworkServerInjector.java | 27 +++++++++++------ .../player/TemporaryPlayerFactory.java | 30 ++++++++++++------- 7 files changed, 61 insertions(+), 32 deletions(-) diff --git a/ProtocolLib/dependency-reduced-pom.xml b/ProtocolLib/dependency-reduced-pom.xml index 657d089f..7019dfad 100644 --- a/ProtocolLib/dependency-reduced-pom.xml +++ b/ProtocolLib/dependency-reduced-pom.xml @@ -4,7 +4,7 @@ com.comphenix.protocol ProtocolLib ProtocolLib - 1.4.3-SNAPSHOT + 1.4.4-SNAPSHOT Provides read/write access to the Minecraft protocol. http://dev.bukkit.org/server-mods/protocollib/ diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java index 9878ab66..04acbc26 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java @@ -190,6 +190,10 @@ public class ProtocolLibrary extends JavaPlugin { protocolManager.close(); protocolManager = null; statistisc = null; + + // Leaky ClassLoader begone! + CleanupStaticMembers cleanup = new CleanupStaticMembers(getClassLoader(), logger); + cleanup.resetAll(); } /** diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncListenerHandler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncListenerHandler.java index db01761b..9f77400f 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncListenerHandler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncListenerHandler.java @@ -54,7 +54,7 @@ public class AsyncListenerHandler { private static final AtomicInteger nextID = new AtomicInteger(); // Default queue capacity - private static int DEFAULT_CAPACITY = 1024; + private static final int DEFAULT_CAPACITY = 1024; // Cancel the async handler private volatile boolean cancelled; diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketConstructor.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketConstructor.java index 07b509f4..f8e0e654 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketConstructor.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketConstructor.java @@ -41,7 +41,7 @@ public class PacketConstructor { *

* Remember to call withPacket(). */ - public static final PacketConstructor DEFAULT = new PacketConstructor(null); + public static PacketConstructor DEFAULT = new PacketConstructor(null); // The constructor method that's actually responsible for creating the packet private Constructor constructorMethod; diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkObjectInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkObjectInjector.java index e0b77d46..4f98d3cd 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkObjectInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkObjectInjector.java @@ -50,6 +50,9 @@ class NetworkObjectInjector extends PlayerInjector { // Used to construct proxy objects private ClassLoader classLoader; + + // Shared callback filter - avoid creating a new class every time + private static CallbackFilter callbackFilter; public NetworkObjectInjector(ClassLoader classLoader, Logger logger, Player player, ListenerInvoker invoker, IntegerSet sendingFilters) throws IllegalAccessException { @@ -131,20 +134,25 @@ class NetworkObjectInjector extends PlayerInjector { } }; + // Share callback filter - that way, we avoid generating a new class every time. + if (callbackFilter == null) { + callbackFilter = new CallbackFilter() { + @Override + public int accept(Method method) { + if (method.equals(queueMethod)) + return 0; + else + return 1; + } + }; + } + // Create our proxy object Enhancer ex = new Enhancer(); ex.setClassLoader(classLoader); ex.setSuperclass(networkInterface); ex.setCallbacks(new Callback[] { queueFilter, dispatch }); - ex.setCallbackFilter(new CallbackFilter() { - @Override - public int accept(Method method) { - if (method.equals(queueMethod)) - return 0; - else - return 1; - } - }); + ex.setCallbackFilter(callbackFilter); // Inject it, if we can. networkManagerRef.setValue(ex.create()); diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkServerInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkServerInjector.java index 1fcb28ed..9ad93166 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkServerInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkServerInjector.java @@ -52,6 +52,8 @@ import com.comphenix.protocol.reflect.instances.ExistingGenerator; */ public class NetworkServerInjector extends PlayerInjector { + private volatile static CallbackFilter callbackFilter; + private static Field disconnectField; private static Method sendPacketMethod; private InjectedServerConnection serverInjection; @@ -170,18 +172,24 @@ public class NetworkServerInjector extends PlayerInjector { }; Callback noOpCallback = NoOp.INSTANCE; + // Share callback filter - that way, we avoid generating a new class for + // every logged in player. + if (callbackFilter == null) { + callbackFilter = new CallbackFilter() { + @Override + public int accept(Method method) { + if (method.equals(sendPacketMethod)) + return 0; + else + return 1; + } + }; + } + ex.setClassLoader(classLoader); ex.setSuperclass(serverClass); ex.setCallbacks(new Callback[] { sendPacketCallback, noOpCallback }); - ex.setCallbackFilter(new CallbackFilter() { - @Override - public int accept(Method method) { - if (method.equals(sendPacketMethod)) - return 0; - else - return 1; - } - }); + ex.setCallbackFilter(callbackFilter); // Find the Minecraft NetServerHandler superclass Class minecraftSuperClass = getFirstMinecraftSuperClass(serverHandler.getClass()); @@ -208,6 +216,7 @@ public class NetworkServerInjector extends PlayerInjector { if (proxyObject != null) { // This will be done by InjectedServerConnection instead //copyTo(serverHandler, proxyObject); + serverInjection.replaceServerHandler(serverHandler, proxyObject); serverHandlerRef.setValue(proxyObject); return true; diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/TemporaryPlayerFactory.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/TemporaryPlayerFactory.java index b6afb6a8..891edc7c 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/TemporaryPlayerFactory.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/TemporaryPlayerFactory.java @@ -42,6 +42,9 @@ class TemporaryPlayerFactory { // Helpful constructors private final PacketConstructor chatPacket; + // Prevent too many class creations + private static CallbackFilter callbackFilter; + public TemporaryPlayerFactory() { chatPacket = PacketConstructor.DEFAULT.withPacket(3, new Object[] { "DEMO" }); } @@ -120,22 +123,27 @@ class TemporaryPlayerFactory { } }; + // Shared callback filter + if (callbackFilter == null) { + callbackFilter = new CallbackFilter() { + @Override + public int accept(Method method) { + // Do not override the object method or the superclass methods + if (method.getDeclaringClass().equals(Object.class) || + method.getDeclaringClass().equals(InjectContainer.class)) + return 0; + else + return 1; + } + }; + } + // CGLib is amazing Enhancer ex = new Enhancer(); ex.setSuperclass(InjectContainer.class); ex.setInterfaces(new Class[] { Player.class }); ex.setCallbacks(new Callback[] { NoOp.INSTANCE, implementation }); - ex.setCallbackFilter(new CallbackFilter() { - @Override - public int accept(Method method) { - // Do not override the object method or the superclass methods - if (method.getDeclaringClass().equals(Object.class) || - method.getDeclaringClass().equals(InjectContainer.class)) - return 0; - else - return 1; - } - }); + ex.setCallbackFilter(callbackFilter); return (Player) ex.create(); }