From 73e71ff95466e693afffc867989f6ac6ae921fc8 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Sun, 6 Jan 2013 20:14:25 +0100 Subject: [PATCH] Don't load classes on secondary threads. ADDRESSES TICKET-27. We'll move the "load class" call to the main thread, hopefully fixing ticket 27. The secondary thread will still call the "define class" protected method, which may not be thread safe. --- .../reflect/compiler/BackgroundCompiler.java | 76 +++++++++++------- .../reflect/compiler/StructureCompiler.java | 79 ++++++++++++++----- 2 files changed, 107 insertions(+), 48 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/BackgroundCompiler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/BackgroundCompiler.java index 6f636c49..e8447303 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/BackgroundCompiler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/BackgroundCompiler.java @@ -150,41 +150,59 @@ public class BackgroundCompiler { if (executor == null || executor.isShutdown()) return; - try { - executor.submit(new Callable() { - @Override - public Object call() throws Exception { - StructureModifier modifier = uncompiled; - - // Do our compilation - try { - modifier = compiler.compile(modifier); - listener.onCompiled(modifier); + // Create the worker that will compile our modifier + Callable worker = new Callable() { + @Override + public Object call() throws Exception { + StructureModifier modifier = uncompiled; + + // Do our compilation + try { + modifier = compiler.compile(modifier); + listener.onCompiled(modifier); - } catch (Throwable e) { - // Disable future compilations! - setEnabled(false); - - // Inform about this error as best as we can - if (reporter != null) { - reporter.reportDetailed(BackgroundCompiler.this, - "Cannot compile structure. Disabing compiler.", e, uncompiled); - } else { - System.err.println("Exception occured in structure compiler: "); - e.printStackTrace(); - } + } catch (Throwable e) { + // Disable future compilations! + setEnabled(false); + + // Inform about this error as best as we can + if (reporter != null) { + reporter.reportDetailed(BackgroundCompiler.this, + "Cannot compile structure. Disabing compiler.", e, uncompiled); + } else { + System.err.println("Exception occured in structure compiler: "); + e.printStackTrace(); } - - // We'll also return the new structure modifier - return modifier; - } - }); + + // We'll also return the new structure modifier + return modifier; + + } + }; + + try { + // Lookup the previous class name on the main thread. + // This is necessary as the Bukkit class loaders are not thread safe + if (compiler.lookupClassLoader(uncompiled)) { + try { + worker.call(); + } catch (Exception e) { + // Impossible! + e.printStackTrace(); + } + + } else { + + // Perform the compilation on a seperate thread + executor.submit(worker); + } + } catch (RejectedExecutionException e) { // Occures when the underlying queue is overflowing. Since the compilation // is only an optmization and not really essential we'll just log this failure // and move on. - Logger.getLogger("Minecraft").log(Level.WARNING, "Unable to schedule compilation task.", e); + reporter.reportWarning(this, "Unable to schedule compilation task.", e); } } } @@ -209,7 +227,7 @@ public class BackgroundCompiler { try { executor.awaitTermination(timeout, unit); } catch (InterruptedException e) { - // Unlikely to ever occur. + // Unlikely to ever occur - it's the main thread e.printStackTrace(); } } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java index cf25ebe7..579c069a 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java @@ -21,9 +21,9 @@ import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.lang.reflect.Modifier; -import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import com.comphenix.protocol.reflect.StructureModifier; import com.google.common.base.Objects; @@ -98,6 +98,10 @@ public final class StructureCompiler { private Class targetType; private Class fieldType; + public StructureKey(StructureModifier source) { + this(source.getTargetType(), source.getFieldType()); + } + public StructureKey(Class targetType, Class fieldType) { this.targetType = targetType; this.fieldType = fieldType; @@ -123,7 +127,7 @@ public final class StructureCompiler { private volatile static Method defineMethod; @SuppressWarnings("rawtypes") - private Map compiledCache = new HashMap(); + private Map compiledCache = new ConcurrentHashMap(); // The class loader we'll store our classes private ClassLoader loader; @@ -142,6 +146,37 @@ public final class StructureCompiler { this.loader = loader; } + /** + * Lookup the current class loader for any previously generated classes before we attempt to generate something. + * @param source - the structure modifier to look up. + * @return TRUE if we successfully found a previously generated class, FALSE otherwise. + */ + public boolean lookupClassLoader(StructureModifier source) { + StructureKey key = new StructureKey(source); + + // See if there's a need to lookup the class name + if (compiledCache.containsKey(key)) { + return true; + } + + try { + String className = getCompiledName(source); + + // This class might have been generated before. Try to load it. + Class before = loader.loadClass(PACKAGE_NAME.replace('/', '.') + "." + className); + + if (before != null) { + compiledCache.put(key, before); + return true; + } + } catch (ClassNotFoundException e) { + // That's ok. + } + + // We need to compile the class + return false; + } + /** * Compiles the given structure modifier. *

@@ -158,7 +193,7 @@ public final class StructureCompiler { return source; } - StructureKey key = new StructureKey(source.getTargetType(), source.getFieldType()); + StructureKey key = new StructureKey(source); Class compiledClass = compiledCache.get(key); if (!compiledCache.containsKey(key)) { @@ -195,29 +230,35 @@ public final class StructureCompiler { return type.getCanonicalName().replace("[]", "Array").replace(".", "_"); } + /** + * Retrieve the compiled name of a given structure modifier. + * @param source - the structure modifier. + * @return The unique, compiled name of a compiled structure modifier. + */ + private String getCompiledName(StructureModifier source) { + Class targetType = source.getTargetType(); + + // Concat class and field type + return "CompiledStructure$" + + getSafeTypeName(targetType) + "$" + + getSafeTypeName(source.getFieldType()); + } + + /** + * Compile a structure modifier. + * @param source - structure modifier. + * @return The compiled structure modifier. + */ private Class generateClass(StructureModifier source) { ClassWriter cw = new ClassWriter(0); - - @SuppressWarnings("rawtypes") - Class targetType = source.getTargetType(); + Class targetType = source.getTargetType(); - String className = "CompiledStructure$" + - getSafeTypeName(targetType) + "$" + - getSafeTypeName(source.getFieldType()); + String className = getCompiledName(source); String targetSignature = Type.getDescriptor(targetType); String targetName = targetType.getName().replace('.', '/'); - try { - // This class might have been generated before. Try to load it. - Class before = loader.loadClass(PACKAGE_NAME.replace('/', '.') + "." + className); - - if (before != null) - return before; - } catch (ClassNotFoundException e) { - // That's ok. - } - + // Define class cw.visit(Opcodes.V1_6, Opcodes.ACC_PUBLIC + Opcodes.ACC_SUPER, PACKAGE_NAME + "/" + className, null, COMPILED_CLASS, null);