Prevent some rare cases of NPE and Deadlocks, better error handling on yaml load

8 files changed, 144 insertions, 104 deletions

diff --git a/Essentials/src/com/earth2me/essentials/storage/AbstractDelayedYamlFileReader.java b/Essentials/src/com/earth2me/essentials/storage/AbstractDelayedYamlFileReader.java
index 0bac39bdb..1f2887d88 100644
--- a/Essentials/src/com/earth2me/essentials/storage/AbstractDelayedYamlFileReader.java
+++ b/Essentials/src/com/earth2me/essentials/storage/AbstractDelayedYamlFileReader.java
@@ -8,6 +8,7 @@ import java.io.IOException;
 import java.util.logging.Level;
 import org.bukkit.Bukkit;
 import org.bukkit.plugin.Plugin;
+import org.yaml.snakeyaml.error.YAMLException;
 
 
 public abstract class AbstractDelayedYamlFileReader<T extends StorageObject> implements Runnable
@@ -33,9 +34,17 @@ public abstract class AbstractDelayedYamlFileReader<T extends StorageObject> imp
 		try
 		{
 			onStart();
-			reader = new FileReader(file);
-			final T object = new YamlStorageReader(reader, plugin).load(clazz);
-			onFinish(object);
+			try
+			{
+				reader = new FileReader(file);
+				T object = new YamlStorageReader(reader, plugin).load(clazz);
+				onSuccess(object);
+			}
+			catch (ObjectLoadException ex)
+			{
+				onException();
+				Bukkit.getLogger().log(Level.SEVERE, "File broken: " + file.toString(), ex.getCause());
+			}
 		}
 		catch (FileNotFoundException ex)
 		{
@@ -57,5 +66,7 @@ public abstract class AbstractDelayedYamlFileReader<T extends StorageObject> imp
 		}
 	}
 
-	public abstract void onFinish(T object);
+	public abstract void onSuccess(T object);
+
+	public abstract void onException();
 }
diff --git a/Essentials/src/com/earth2me/essentials/storage/IStorageReader.java b/Essentials/src/com/earth2me/essentials/storage/IStorageReader.java
index 5e259a322..d59adafe0 100644
--- a/Essentials/src/com/earth2me/essentials/storage/IStorageReader.java
+++ b/Essentials/src/com/earth2me/essentials/storage/IStorageReader.java
@@ -3,5 +3,5 @@ package com.earth2me.essentials.storage;
 
 public interface IStorageReader
 {
-	 <T extends StorageObject> T load(final Class<? extends T> clazz);
+	 <T extends StorageObject> T load(final Class<? extends T> clazz) throws ObjectLoadException;
 }
diff --git a/Essentials/src/com/earth2me/essentials/storage/ObjectLoadException.java b/Essentials/src/com/earth2me/essentials/storage/ObjectLoadException.java
new file mode 100644
index 000000000..8b804abc3
--- /dev/null
+++ b/Essentials/src/com/earth2me/essentials/storage/ObjectLoadException.java
@@ -0,0 +1,10 @@
+package com.earth2me.essentials.storage;
+
+
+public class ObjectLoadException extends Exception
+{
+	public ObjectLoadException(Throwable thrwbl)
+	{
+		super(thrwbl);
+	}
+}
diff --git a/Essentials/src/com/earth2me/essentials/storage/YamlStorageReader.java b/Essentials/src/com/earth2me/essentials/storage/YamlStorageReader.java
index 3aa483e5c..5d1ff668a 100644
--- a/Essentials/src/com/earth2me/essentials/storage/YamlStorageReader.java
+++ b/Essentials/src/com/earth2me/essentials/storage/YamlStorageReader.java
@@ -4,8 +4,6 @@ import java.io.Reader;
 import java.lang.reflect.Field;
 import java.util.*;
 import java.util.concurrent.locks.ReentrantLock;
-import java.util.logging.Level;
-import java.util.logging.Logger;
 import org.bukkit.plugin.Plugin;
 import org.yaml.snakeyaml.TypeDescription;
 import org.yaml.snakeyaml.Yaml;
@@ -14,8 +12,8 @@ import org.yaml.snakeyaml.constructor.Constructor;
 
 public class YamlStorageReader implements IStorageReader
 {
-	private transient static Map<Class, Yaml> preparedYamls = Collections.synchronizedMap(new HashMap<Class, Yaml>());
-	private transient static Map<Class, ReentrantLock> locks = new HashMap<Class, ReentrantLock>();
+	private transient static final Map<Class, Yaml> PREPARED_YAMLS = Collections.synchronizedMap(new HashMap<Class, Yaml>());
+	private transient static final Map<Class, ReentrantLock> LOCKS = new HashMap<Class, ReentrantLock>();
 	private transient final Reader reader;
 	private transient final Plugin plugin;
 
@@ -26,49 +24,40 @@ public class YamlStorageReader implements IStorageReader
 	}
 
 	@Override
-	public <T extends StorageObject> T load(final Class<? extends T> clazz)
+	public <T extends StorageObject> T load(final Class<? extends T> clazz) throws ObjectLoadException
 	{
-		Yaml yaml = preparedYamls.get(clazz);
+		Yaml yaml = PREPARED_YAMLS.get(clazz);
 		if (yaml == null)
 		{
 			yaml = new Yaml(prepareConstructor(clazz));
-			preparedYamls.put(clazz, yaml);
+			PREPARED_YAMLS.put(clazz, yaml);
 		}
 		ReentrantLock lock;
-		synchronized (locks)
+		synchronized (LOCKS)
 		{
-			lock = locks.get(clazz);
+			lock = LOCKS.get(clazz);
 			if (lock == null)
 			{
 				lock = new ReentrantLock();
 			}
 		}
-		T ret;
 		lock.lock();
 		try
 		{
-			ret = (T)yaml.load(reader);
+			T object = (T)yaml.load(reader);
+			if (object == null) {
+				object = clazz.newInstance();
+			}
+			return object;
 		}
-		finally
+		catch (Exception ex)
 		{
-			lock.unlock();
+			throw new ObjectLoadException(ex);
 		}
-		if (ret == null)
+		finally
 		{
-			try
-			{
-				ret = (T)clazz.newInstance();
-			}
-			catch (InstantiationException ex)
-			{
-				Logger.getLogger(StorageObject.class.getName()).log(Level.SEVERE, null, ex);
-			}
-			catch (IllegalAccessException ex)
-			{
-				Logger.getLogger(StorageObject.class.getName()).log(Level.SEVERE, null, ex);
-			}
+			lock.unlock();
 		}
-		return ret;
 	}
 
 	private Constructor prepareConstructor(final Class<?> clazz)
diff --git a/Essentials/src/com/earth2me/essentials/user/User.java b/Essentials/src/com/earth2me/essentials/user/User.java
index 70f2c7f3c..55740134f 100644
--- a/Essentials/src/com/earth2me/essentials/user/User.java
+++ b/Essentials/src/com/earth2me/essentials/user/User.java
@@ -81,7 +81,6 @@ public class User extends UserBase implements IOfflineUser
 		new UserDataWriter();
 	}
 
-
 	private class UserDataWriter extends AbstractDelayedYamlFileWriter
 	{
 		public UserDataWriter()
diff --git a/Essentials/test/com/earth2me/essentials/StorageTest.java b/Essentials/test/com/earth2me/essentials/StorageTest.java
index d4dc1be70..b7fe23433 100644
--- a/Essentials/test/com/earth2me/essentials/StorageTest.java
+++ b/Essentials/test/com/earth2me/essentials/StorageTest.java
@@ -1,6 +1,7 @@
 package com.earth2me.essentials;
 
 import com.earth2me.essentials.settings.Settings;
+import com.earth2me.essentials.storage.ObjectLoadException;
 import com.earth2me.essentials.storage.StorageObject;
 import com.earth2me.essentials.storage.YamlStorageReader;
 import com.earth2me.essentials.storage.YamlStorageWriter;
@@ -11,7 +12,6 @@ import org.bukkit.World;
 import org.bukkit.World.Environment;
 import org.bukkit.plugin.InvalidDescriptionException;
 import org.junit.Test;
-import org.yaml.snakeyaml.Yaml;
 
 
 public class StorageTest extends TestCase
@@ -42,82 +42,94 @@ public class StorageTest extends TestCase
 	@Test
 	public void testSettings()
 	{
-		assertTrue(StorageObject.class.isAssignableFrom(Settings.class));
-		ExecuteTimer ext = new ExecuteTimer();
-		ext.start();
-		final ByteArrayInputStream bais = new ByteArrayInputStream(new byte[0]);
-		final Reader reader = new InputStreamReader(bais);
-		final Settings settings = new YamlStorageReader(reader, null).load(Settings.class);
-		ext.mark("load empty settings");
-		final ByteArrayInputStream bais3 = new ByteArrayInputStream(new byte[0]);
-		final Reader reader3 = new InputStreamReader(bais3);
-		final Settings settings3 = new YamlStorageReader(reader3, null).load(Settings.class);
-		ext.mark("load empty settings (class cached)");
-		final ByteArrayOutputStream baos = new ByteArrayOutputStream();
-		final PrintWriter writer = new PrintWriter(baos);
-		new YamlStorageWriter(writer).save(settings);
-		writer.close();
-		ext.mark("write settings");
-		byte[] written = baos.toByteArray();
-		System.out.println(new String(written));
-		final ByteArrayInputStream bais2 = new ByteArrayInputStream(written);
-		final Reader reader2 = new InputStreamReader(bais2);
-		final Settings settings2 = new YamlStorageReader(reader2, null).load(Settings.class);
-		System.out.println(settings.toString());
-		System.out.println(settings2.toString());
-		ext.mark("reload settings");
-		System.out.println(ext.end());
-		//assertEquals("Default and rewritten config should be equal", settings, settings2);
-		//that assertion fails, because empty list and maps return as null
+		try
+		{
+			assertTrue(StorageObject.class.isAssignableFrom(Settings.class));
+			ExecuteTimer ext = new ExecuteTimer();
+			ext.start();
+			final ByteArrayInputStream bais = new ByteArrayInputStream(new byte[0]);
+			final Reader reader = new InputStreamReader(bais);
+			final Settings settings = new YamlStorageReader(reader, null).load(Settings.class);
+			ext.mark("load empty settings");
+			final ByteArrayInputStream bais3 = new ByteArrayInputStream(new byte[0]);
+			final Reader reader3 = new InputStreamReader(bais3);
+			final Settings settings3 = new YamlStorageReader(reader3, null).load(Settings.class);
+			ext.mark("load empty settings (class cached)");
+			final ByteArrayOutputStream baos = new ByteArrayOutputStream();
+			final PrintWriter writer = new PrintWriter(baos);
+			new YamlStorageWriter(writer).save(settings);
+			writer.close();
+			ext.mark("write settings");
+			byte[] written = baos.toByteArray();
+			System.out.println(new String(written));
+			final ByteArrayInputStream bais2 = new ByteArrayInputStream(written);
+			final Reader reader2 = new InputStreamReader(bais2);
+			final Settings settings2 = new YamlStorageReader(reader2, null).load(Settings.class);
+			System.out.println(settings.toString());
+			System.out.println(settings2.toString());
+			ext.mark("reload settings");
+			System.out.println(ext.end());
+			//assertEquals("Default and rewritten config should be equal", settings, settings2);
+			//that assertion fails, because empty list and maps return as null
+		}
+		catch (ObjectLoadException ex)
+		{
+			fail(ex.getMessage());
+		}
 	}
 
 	@Test
 	public void testUserdata()
 	{
-		FakeServer server = new FakeServer();
-		World world = server.createWorld("testWorld", Environment.NORMAL);
-		ExecuteTimer ext = new ExecuteTimer();
-		ext.start();
-		final ByteArrayInputStream bais = new ByteArrayInputStream(new byte[0]);
-		final Reader reader = new InputStreamReader(bais);
-		final com.earth2me.essentials.user.UserData userdata = new YamlStorageReader(reader, null).load(com.earth2me.essentials.user.UserData.class);
-		ext.mark("load empty user");
-		final ByteArrayInputStream bais3 = new ByteArrayInputStream(new byte[0]);
-		final Reader reader3 = new InputStreamReader(bais3);
-		final com.earth2me.essentials.user.UserData userdata3 = new YamlStorageReader(reader3, null).load(com.earth2me.essentials.user.UserData.class);
-		ext.mark("load empty user (class cached)");
+		try
+		{
+			FakeServer server = new FakeServer();
+			World world = server.createWorld("testWorld", Environment.NORMAL);
+			ExecuteTimer ext = new ExecuteTimer();
+			ext.start();
+			final ByteArrayInputStream bais = new ByteArrayInputStream(new byte[0]);
+			final Reader reader = new InputStreamReader(bais);
+			final com.earth2me.essentials.user.UserData userdata = new YamlStorageReader(reader, null).load(com.earth2me.essentials.user.UserData.class);
+			ext.mark("load empty user");
+			final ByteArrayInputStream bais3 = new ByteArrayInputStream(new byte[0]);
+			final Reader reader3 = new InputStreamReader(bais3);
+			final com.earth2me.essentials.user.UserData userdata3 = new YamlStorageReader(reader3, null).load(com.earth2me.essentials.user.UserData.class);
+			ext.mark("load empty user (class cached)");
 
-		for (int j = 0; j < 10000; j++)
+			for (int j = 0; j < 10000; j++)
+			{
+				userdata.getHomes().put("home", new Location(world, j, j, j));
+			}
+			ext.mark("change home 10000 times");
+			final ByteArrayOutputStream baos = new ByteArrayOutputStream();
+			final PrintWriter writer = new PrintWriter(baos);
+			new YamlStorageWriter(writer).save(userdata);
+			writer.close();
+			ext.mark("write user");
+			final ByteArrayOutputStream baos2 = new ByteArrayOutputStream();
+			final PrintWriter writer2 = new PrintWriter(baos2);
+			new YamlStorageWriter(writer2).save(userdata);
+			writer2.close();
+			ext.mark("write user (cached)");
+			byte[] written = baos.toByteArray();
+			System.out.println(new String(written));
+			ext.mark("debug output");
+			final ByteArrayInputStream bais2 = new ByteArrayInputStream(written);
+			final Reader reader2 = new InputStreamReader(bais2);
+			final com.earth2me.essentials.user.UserData userdata2 = new YamlStorageReader(reader2, null).load(com.earth2me.essentials.user.UserData.class);
+			ext.mark("reload file");
+			final ByteArrayInputStream bais4 = new ByteArrayInputStream(written);
+			final Reader reader4 = new InputStreamReader(bais4);
+			final com.earth2me.essentials.user.UserData userdata4 = new YamlStorageReader(reader4, null).load(com.earth2me.essentials.user.UserData.class);
+			ext.mark("reload file (cached)");
+			System.out.println(userdata.toString());
+			System.out.println(userdata2.toString());
+			System.out.println(ext.end());
+		}
+		catch (ObjectLoadException ex)
 		{
-			userdata.getHomes().put("home", new Location(world, j, j, j));
+			fail(ex.getMessage());
 		}
-		ext.mark("change home 10000 times");
-		final ByteArrayOutputStream baos = new ByteArrayOutputStream();
-		final PrintWriter writer = new PrintWriter(baos);
-		new YamlStorageWriter(writer).save(userdata);
-		writer.close();
-		ext.mark("write user");
-		final ByteArrayOutputStream baos2 = new ByteArrayOutputStream();
-		final PrintWriter writer2 = new PrintWriter(baos2);
-		new YamlStorageWriter(writer2).save(userdata);
-		writer2.close();
-		ext.mark("write user (cached)");
-		byte[] written = baos.toByteArray();
-		System.out.println(new String(written));
-		ext.mark("debug output");
-		final ByteArrayInputStream bais2 = new ByteArrayInputStream(written);
-		final Reader reader2 = new InputStreamReader(bais2);
-		final com.earth2me.essentials.user.UserData userdata2 = new YamlStorageReader(reader2, null).load(com.earth2me.essentials.user.UserData.class);
-		ext.mark("reload file");
-		final ByteArrayInputStream bais4 = new ByteArrayInputStream(written);
-		final Reader reader4 = new InputStreamReader(bais4);
-		final com.earth2me.essentials.user.UserData userdata4 = new YamlStorageReader(reader4, null).load(com.earth2me.essentials.user.UserData.class);
-		ext.mark("reload file (cached)");
-		System.out.println(userdata.toString());
-		System.out.println(userdata2.toString());
-		System.out.println(ext.end());
-		com.earth2me.essentials.user.User test = new com.earth2me.essentials.user.User(null, ess);
-		test.example();
 
 	}
 
diff --git a/EssentialsSpawn/src/com/earth2me/essentials/spawn/EssentialsSpawn.java b/EssentialsSpawn/src/com/earth2me/essentials/spawn/EssentialsSpawn.java
index 6f6244f25..ad42f9dee 100644
--- a/EssentialsSpawn/src/com/earth2me/essentials/spawn/EssentialsSpawn.java
+++ b/EssentialsSpawn/src/com/earth2me/essentials/spawn/EssentialsSpawn.java
@@ -35,6 +35,7 @@ public class EssentialsSpawn extends JavaPlugin
 		}
 
 		spawns = new SpawnStorage(ess);
+		ess.addReloadListener(spawns);
 
 		final EssentialsSpawnPlayerListener playerListener = new EssentialsSpawnPlayerListener(ess, spawns);
 		pluginManager.registerEvent(Type.PLAYER_RESPAWN, playerListener, Priority.Low, this);
diff --git a/EssentialsSpawn/src/com/earth2me/essentials/spawn/SpawnStorage.java b/EssentialsSpawn/src/com/earth2me/essentials/spawn/SpawnStorage.java
index ac98b5921..02fb26c9b 100644
--- a/EssentialsSpawn/src/com/earth2me/essentials/spawn/SpawnStorage.java
+++ b/EssentialsSpawn/src/com/earth2me/essentials/spawn/SpawnStorage.java
@@ -6,12 +6,14 @@ import com.earth2me.essentials.IEssentialsModule;
 import com.earth2me.essentials.settings.Spawns;
 import com.earth2me.essentials.storage.AbstractDelayedYamlFileReader;
 import com.earth2me.essentials.storage.AbstractDelayedYamlFileWriter;
+import com.earth2me.essentials.storage.ObjectLoadException;
 import com.earth2me.essentials.storage.StorageObject;
 import java.io.File;
 import java.util.HashMap;
 import java.util.Locale;
 import java.util.Map;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.World;
 
@@ -35,6 +37,10 @@ public class SpawnStorage implements IConf, IEssentialsModule
 		rwl.writeLock().lock();
 		try
 		{
+			if (spawns == null)
+			{
+				spawns = new Spawns();
+			}
 			if (spawns.getSpawns() == null)
 			{
 				spawns.setSpawns(new HashMap<String, Location>());
@@ -136,9 +142,21 @@ public class SpawnStorage implements IConf, IEssentialsModule
 		}
 
 		@Override
-		public void onFinish(final Spawns object)
+		public void onSuccess(final Spawns object)
+		{
+			if (object != null)
+			{
+				spawns = object;
+			}
+			rwl.writeLock().unlock();
+		}
+
+		@Override
+		public void onException()
 		{
-			spawns = object;
+			if (spawns == null) {
+				spawns = new Spawns();
+			}
 			rwl.writeLock().unlock();
 		}
 	}