Refactor package caching, account for some edge cases

modules/API/src/main/java/com/comphenix/protocol/utility/CachedPackage.java

@@ -1,4 +1,4 @@
-/*
+/**
  *  ProtocolLib - Bukkit server library that allows access to the Minecraft protocol.
  *  Copyright (C) 2012 Kristian S. Stangeland
  *
@@ -14,7 +14,6 @@
  *  if not, write to the Free Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 
  *  02111-1307 USA
  */
-
 package com.comphenix.protocol.utility;
 
 import java.util.Map;
@@ -51,7 +50,11 @@ class CachedPackage {
 	 * @param clazz - type of class.
 	 */
 	public void setPackageClass(String className, Class<?> clazz) {
-		cache.put(className, Optional.<Class<?>>fromNullable(clazz));
+		if (clazz != null) {
+			cache.put(className, Optional.<Class<?>> of(clazz));
+		} else {
+			cache.remove(className);
+		}
 	}
 
 	/**
@@ -61,28 +64,30 @@ class CachedPackage {
 	 * @throws RuntimeException If we are unable to find the given class.
 	 */
 	public Class<?> getPackageClass(String className) {
-		try {
+		Preconditions.checkNotNull(className, "className cannot be null!");
-			Optional<Class<?>> result = cache.get(Preconditions.checkNotNull(className, "className cannot be NULL"));
 
-			// Concurrency is not a problem - we don't care if we look up a class twice
+		// See if we've already looked it up
-			if (result == null) {
+		if (cache.containsKey(className)) {
-				// Look up the class dynamically
+			Optional<Class<?>> result = cache.get(className);
-				result = Optional.<Class<?>>fromNullable(source.loadClass(combine(packageName, className)));
-				if (!result.isPresent())
-					throw new IllegalArgumentException("Source " + source + " returned NULL for " + className);
-
-				cache.put(className, result);
-			}
-
-			// Class has been looked for and hasn't been found in the past
 			if (!result.isPresent()) {
-				throw new ClassNotFoundException();
+				throw new RuntimeException("Cannot find class " + className);
 			}
 
 			return result.get();
-		} catch (ClassNotFoundException e) {
+		}
-			setPackageClass(className, null);
+
-			throw new RuntimeException("Cannot find class " + combine(packageName, className), e);
+		try {
+			// Try looking it up
+			Class<?> clazz = source.loadClass(combine(packageName, className));
+			if (clazz == null) {
+				throw new IllegalArgumentException("Source " + source + " returned null for " + className);
+			}
+
+			cache.put(className, Optional.<Class<?>> of(clazz));
+			return clazz;
+		} catch (ClassNotFoundException ex) {
+			cache.put(className, Optional.<Class<?>> absent());
+			throw new RuntimeException("Cannot find class " + className, ex);
 		}
 	}
 

modules/API/src/main/java/com/comphenix/protocol/utility/MinecraftReflection.java

@@ -63,8 +63,8 @@ import com.comphenix.protocol.reflect.fuzzy.FuzzyClassContract;
 import com.comphenix.protocol.reflect.fuzzy.FuzzyFieldContract;
 import com.comphenix.protocol.reflect.fuzzy.FuzzyMatchers;
 import com.comphenix.protocol.reflect.fuzzy.FuzzyMethodContract;
-import com.comphenix.protocol.utility.RemappedClassSource.RemapperUnavaibleException;
+import com.comphenix.protocol.utility.RemappedClassSource.RemapperUnavailableException;
-import com.comphenix.protocol.utility.RemappedClassSource.RemapperUnavaibleException.Reason;
+import com.comphenix.protocol.utility.RemappedClassSource.RemapperUnavailableException.Reason;
 import com.comphenix.protocol.wrappers.nbt.NbtFactory;
 import com.comphenix.protocol.wrappers.nbt.NbtType;
 import com.google.common.base.Joiner;
@@ -934,6 +934,9 @@ public class MinecraftReflection {
 			return getMinecraftClass("MinecraftServer");
 		} catch (RuntimeException e) {
 			useFallbackServer();
+
+			// Reset cache and try again
+			setMinecraftClass("MinecraftServer", null);
 			return getMinecraftClass("MinecraftServer");
 		}
 	}
@@ -982,8 +985,10 @@ public class MinecraftReflection {
 		try {
 			return getMinecraftClass("ServerConfigurationManager", "PlayerList");
 		} catch (RuntimeException e) {
-			// Try again
 			useFallbackServer();
+
+			// Reset cache and try again
+			setMinecraftClass("ServerConfigurationManager", null);
 			return getMinecraftClass("ServerConfigurationManager");
 		}
 	}
@@ -1628,8 +1633,10 @@ public class MinecraftReflection {
 		try {
 			return getMinecraftClass("AttributeModifier");
 		} catch (RuntimeException e) {
-			// Initialize first
 			getAttributeSnapshotClass();
+
+			// Reset cache and try again
+			setMinecraftClass("AttributeModifier", null);
 			return getMinecraftClass("AttributeModifier");
 		}
 	}
@@ -2108,7 +2115,7 @@ public class MinecraftReflection {
 			// Attempt to use MCPC
 			try {
 				return classSource = new RemappedClassSource().initialize();
-			} catch (RemapperUnavaibleException e) {
+			} catch (RemapperUnavailableException e) {
 				if (e.getReason() != Reason.MCPC_NOT_PRESENT)
 					reporter.reportWarning(MinecraftReflection.class, Report.newBuilder(REPORT_CANNOT_FIND_MCPC_REMAPPER));
 			} catch (Exception e) {

modules/API/src/main/java/com/comphenix/protocol/utility/RemappedClassSource.java

@@ -28,7 +28,7 @@ import org.bukkit.Server;
 
 import com.comphenix.protocol.reflect.FieldUtils;
 import com.comphenix.protocol.reflect.MethodUtils;
-import com.comphenix.protocol.utility.RemappedClassSource.RemapperUnavaibleException.Reason;
+import com.comphenix.protocol.utility.RemappedClassSource.RemapperUnavailableException.Reason;
 
 class RemappedClassSource extends ClassSource {
 	private Object classRemapper;
@@ -53,7 +53,7 @@ class RemappedClassSource extends ClassSource {
 	/**
 	 * Attempt to load the MCPC remapper.
 	 * @return TRUE if we succeeded, FALSE otherwise.
-	 * @throws RemapperUnavaibleException If the remapper is not present.
+	 * @throws RemapperUnavailableException If the remapper is not present.
 	 */
 	public RemappedClassSource initialize() {
 		try {
@@ -64,14 +64,14 @@ class RemappedClassSource extends ClassSource {
 
 			String version = server.getVersion();
 			if (!version.contains("MCPC") && !version.contains("Cauldron")) {
-				throw new RemapperUnavaibleException(Reason.MCPC_NOT_PRESENT);
+				throw new RemapperUnavailableException(Reason.MCPC_NOT_PRESENT);
 			}
 
 			// Obtain the Class remapper used by MCPC+/Cauldron
 			this.classRemapper = FieldUtils.readField(getClass().getClassLoader(), "remapper", true);
 			
 			if (this.classRemapper == null) {
-				throw new RemapperUnavaibleException(Reason.REMAPPER_DISABLED);
+				throw new RemapperUnavailableException(Reason.REMAPPER_DISABLED);
 			}
 			
 			// Initialize some fields and methods used by the Jar Remapper
@@ -82,7 +82,7 @@ class RemappedClassSource extends ClassSource {
 			
 			return this;
 			
-		} catch (RemapperUnavaibleException e) {
+		} catch (RemapperUnavailableException e) {
 			throw e;
 		} catch (Exception e) {
 			// Damn it
@@ -115,7 +115,7 @@ class RemappedClassSource extends ClassSource {
 		}
 	}
 
-	public static class RemapperUnavaibleException extends RuntimeException {
+	public static class RemapperUnavailableException extends RuntimeException {
 		private static final long serialVersionUID = 1L;
 
 		public enum Reason {
@@ -139,7 +139,7 @@ class RemappedClassSource extends ClassSource {
 		
 		private final Reason reason;
 		
-		public RemapperUnavaibleException(Reason reason) {
+		public RemapperUnavailableException(Reason reason) {
 			super(reason.getMessage());
 			this.reason = reason;
 		}