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.
This commit is contained in:
Kristian S. Stangeland 2013-01-06 20:14:25 +01:00
parent 07ae10bcd6
commit 73e71ff954
2 changed files with 107 additions and 48 deletions

View File

@ -150,8 +150,8 @@ public class BackgroundCompiler {
if (executor == null || executor.isShutdown()) if (executor == null || executor.isShutdown())
return; return;
try { // Create the worker that will compile our modifier
executor.submit(new Callable<Object>() { Callable<?> worker = new Callable<Object>() {
@Override @Override
public Object call() throws Exception { public Object call() throws Exception {
StructureModifier<TKey> modifier = uncompiled; StructureModifier<TKey> modifier = uncompiled;
@ -179,12 +179,30 @@ public class BackgroundCompiler {
return 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) { } catch (RejectedExecutionException e) {
// Occures when the underlying queue is overflowing. Since the compilation // Occures when the underlying queue is overflowing. Since the compilation
// is only an optmization and not really essential we'll just log this failure // is only an optmization and not really essential we'll just log this failure
// and move on. // 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 { try {
executor.awaitTermination(timeout, unit); executor.awaitTermination(timeout, unit);
} catch (InterruptedException e) { } catch (InterruptedException e) {
// Unlikely to ever occur. // Unlikely to ever occur - it's the main thread
e.printStackTrace(); e.printStackTrace();
} }
} }

View File

@ -21,9 +21,9 @@ import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException; import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method; import java.lang.reflect.Method;
import java.lang.reflect.Modifier; import java.lang.reflect.Modifier;
import java.util.HashMap;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import com.comphenix.protocol.reflect.StructureModifier; import com.comphenix.protocol.reflect.StructureModifier;
import com.google.common.base.Objects; import com.google.common.base.Objects;
@ -98,6 +98,10 @@ public final class StructureCompiler {
private Class targetType; private Class targetType;
private Class fieldType; private Class fieldType;
public StructureKey(StructureModifier<?> source) {
this(source.getTargetType(), source.getFieldType());
}
public StructureKey(Class targetType, Class fieldType) { public StructureKey(Class targetType, Class fieldType) {
this.targetType = targetType; this.targetType = targetType;
this.fieldType = fieldType; this.fieldType = fieldType;
@ -123,7 +127,7 @@ public final class StructureCompiler {
private volatile static Method defineMethod; private volatile static Method defineMethod;
@SuppressWarnings("rawtypes") @SuppressWarnings("rawtypes")
private Map<StructureKey, Class> compiledCache = new HashMap<StructureKey, Class>(); private Map<StructureKey, Class> compiledCache = new ConcurrentHashMap<StructureKey, Class>();
// The class loader we'll store our classes // The class loader we'll store our classes
private ClassLoader loader; private ClassLoader loader;
@ -142,6 +146,37 @@ public final class StructureCompiler {
this.loader = loader; 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 <TField> boolean lookupClassLoader(StructureModifier<TField> 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. * Compiles the given structure modifier.
* <p> * <p>
@ -158,7 +193,7 @@ public final class StructureCompiler {
return source; return source;
} }
StructureKey key = new StructureKey(source.getTargetType(), source.getFieldType()); StructureKey key = new StructureKey(source);
Class<?> compiledClass = compiledCache.get(key); Class<?> compiledClass = compiledCache.get(key);
if (!compiledCache.containsKey(key)) { if (!compiledCache.containsKey(key)) {
@ -195,29 +230,35 @@ public final class StructureCompiler {
return type.getCanonicalName().replace("[]", "Array").replace(".", "_"); 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 <TField> Class<?> generateClass(StructureModifier<TField> source) { private <TField> Class<?> generateClass(StructureModifier<TField> source) {
ClassWriter cw = new ClassWriter(0); ClassWriter cw = new ClassWriter(0);
Class<?> targetType = source.getTargetType();
@SuppressWarnings("rawtypes") String className = getCompiledName(source);
Class targetType = source.getTargetType();
String className = "CompiledStructure$" +
getSafeTypeName(targetType) + "$" +
getSafeTypeName(source.getFieldType());
String targetSignature = Type.getDescriptor(targetType); String targetSignature = Type.getDescriptor(targetType);
String targetName = targetType.getName().replace('.', '/'); String targetName = targetType.getName().replace('.', '/');
try { // Define class
// 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.
}
cw.visit(Opcodes.V1_6, Opcodes.ACC_PUBLIC + Opcodes.ACC_SUPER, PACKAGE_NAME + "/" + className, cw.visit(Opcodes.V1_6, Opcodes.ACC_PUBLIC + Opcodes.ACC_SUPER, PACKAGE_NAME + "/" + className,
null, COMPILED_CLASS, null); null, COMPILED_CLASS, null);