Synchronize before attempting to modify thread-safe fields.

We also re-wrap proxied lists in Collections.synchronizedList(). May 
fix ticket #157 on BukkitDev.

ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelProxy.java

@@ -1,5 +1,6 @@
 package com.comphenix.protocol.injector.netty;
 
+import java.lang.reflect.Field;
 import java.net.SocketAddress;
 import java.util.Map;
 import java.util.concurrent.Callable;
@@ -25,6 +26,7 @@ abstract class ChannelProxy implements Channel {
 	private static final FieldAccessor MARK_NO_MESSAGE = new FieldAccessor() {
 		public void set(Object instance, Object value) { }
 		public Object get(Object instance) { return null; }
+		public Field getField() { return null; };
 	};
 	
 	// Looking up packets in inner classes

ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolInjector.java

@@ -5,6 +5,7 @@ import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.net.InetSocketAddress;
+import java.util.Collections;
 import java.util.List;
 import java.util.Set;
 import org.bukkit.Bukkit;
@@ -108,9 +109,13 @@ public class NettyProtocolInjector implements ChannelListener {
             bootstrapFields = getBootstrapFields(serverConnection);
             
             for (VolatileField field : bootstrapFields) {
-            	field.setValue(new BootstrapList(
-            		(List<Object>) field.getValue(), connectionHandler
-                ));
+            	final List<Object> list = (List<Object>) field.getValue();
+
+            	// Synchronize with each list before we attempt to replace them.
+            	// We also reapply the synchronized list wrapper.
+				field.setValue(Collections.synchronizedList(new BootstrapList(
+	            	list, connectionHandler
+	            )));
             }
 
             injected = true;
@@ -148,7 +153,8 @@ public class NettyProtocolInjector implements ChannelListener {
     	
     	// Find and (possibly) proxy every list
     	for (Field field : FuzzyReflection.fromObject(serverConnection, true).getFieldListByType(List.class)) {
-    		VolatileField volatileField = new VolatileField(field, serverConnection, true);
+    		VolatileField volatileField = new VolatileField(field, serverConnection, true).toSynchronized();
+    		
     		@SuppressWarnings("unchecked")
 			List<Object> list = (List<Object>) volatileField.getValue();
     		

ProtocolLib/src/main/java/com/comphenix/protocol/reflect/DefaultFieldAccessor.java

@@ -0,0 +1,62 @@
+package com.comphenix.protocol.reflect;
+
+import java.lang.reflect.Field;
+
+import com.comphenix.protocol.reflect.FuzzyReflection.FieldAccessor;
+
+final class DefaultFieldAccessor implements FieldAccessor {
+	private final Field field;
+	
+	public DefaultFieldAccessor(Field field) {
+		this.field = field;
+	}
+	
+	@Override
+	public Object get(Object instance) {
+		try {
+			return field.get(instance);
+		} catch (IllegalArgumentException e) {
+			throw new RuntimeException("Cannot read  " + field, e);
+		} catch (IllegalAccessException e) {
+			throw new IllegalStateException("Cannot use reflection.", e);
+		}
+	}
+	
+	@Override
+	public void set(Object instance, Object value) {
+		try {
+			field.set(instance, value);
+		} catch (IllegalArgumentException e) {
+			throw new RuntimeException("Cannot set field " + field + " to value " + value, e);
+		} catch (IllegalAccessException e) {
+			throw new IllegalStateException("Cannot use reflection.", e);
+		}
+	}
+	
+	@Override
+	public Field getField() {
+		return field;
+	}
+
+	@Override
+	public int hashCode() {
+		return field != null ? field.hashCode() : 0;
+	}
+
+	@Override
+	public boolean equals(Object obj) {
+		if (this == obj)
+			return true;
+		
+		if (obj instanceof DefaultFieldAccessor) {
+			DefaultFieldAccessor other = (DefaultFieldAccessor) obj;
+			return other.field == field;
+		}
+		return true;
+	}
+	
+	@Override
+	public String toString() {
+		return "DefaultFieldAccessor [field=" + field + "]";
+	}
+}

ProtocolLib/src/main/java/com/comphenix/protocol/reflect/DefaultMethodAccessor.java

@@ -0,0 +1,54 @@
+package com.comphenix.protocol.reflect;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+
+import com.comphenix.protocol.reflect.FuzzyReflection.MethodAccessor;
+
+final class DefaultMethodAccessor implements MethodAccessor {
+	private final Method method;
+	
+	public DefaultMethodAccessor(Method method) {
+		this.method = method;
+	}
+	
+	@Override
+	public Object invoke(Object target, Object... args) {
+		try {
+			return method.invoke(target, args);
+		} catch (IllegalAccessException e) {
+			throw new IllegalStateException("Cannot use reflection.", e);
+		} catch (InvocationTargetException e) {
+			throw new RuntimeException("An internal error occured.", e.getCause());
+		} catch (IllegalArgumentException e) {
+			throw e;
+		}
+	}
+	
+	@Override
+	public Method getMethod() {
+		return method;
+	}
+
+	@Override
+	public int hashCode() {
+		return method != null ? method.hashCode() : 0;
+	}
+
+	@Override
+	public boolean equals(Object obj) {
+		if (this == obj)
+			return true;
+		
+		if (obj instanceof DefaultMethodAccessor) {
+			DefaultMethodAccessor other = (DefaultMethodAccessor) obj;
+			return other.method == method;
+		}
+		return true;
+	}
+	
+	@Override
+	public String toString() {
+		return "DefaultMethodAccessor [method=" + method + "]";
+	}
+}

ProtocolLib/src/main/java/com/comphenix/protocol/reflect/FuzzyReflection.java

@@ -19,7 +19,6 @@ package com.comphenix.protocol.reflect;
 
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
-import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.util.ArrayList;
@@ -62,6 +61,12 @@ public class FuzzyReflection {
 		 * @param value - the new value of the field.
 		 */
 		public void set(Object instance, Object value);
+		
+		/**
+		 * Retrieve the underlying field.
+		 * @return The field.
+		 */
+		public Field getField();
 	}
 	
 	/**
@@ -76,6 +81,44 @@ public class FuzzyReflection {
 		 * @return The return value, or NULL for void methods.
 		 */
 		public Object invoke(Object target, Object... args);
+		
+		/**
+		 * Retrieve the underlying method.
+		 * @return The method.
+		 */
+		public Method getMethod();
+	}
+	
+	/**
+	 * Represents a field accessor that synchronizes access to the underlying field.
+	 * @author Kristian
+	 */
+	private static final class SynchronizedFieldAccessor implements FieldAccessor {
+		private final FieldAccessor accessor;
+		private SynchronizedFieldAccessor(FieldAccessor accessor) {
+			this.accessor = accessor;
+		}
+		
+		@Override
+		public void set(Object instance, Object value) {
+			Object lock = accessor.get(instance);
+			
+			if (lock != null) {
+				synchronized (lock) {
+					accessor.set(instance, value);
+				}
+			} else {
+				accessor.set(instance, value);
+			}
+		}
+		@Override
+		public Object get(Object instance) {
+			return accessor.get(instance);
+		}
+		@Override
+		public Field getField() {
+			return accessor.getField();
+		}
 	}
 	
 	// The class we're actually representing
@@ -170,26 +213,19 @@ public class FuzzyReflection {
 	 */
 	public static FieldAccessor getFieldAccessor(final Field field, boolean forceAccess) {
 		field.setAccessible(true);
-		
-		return new FieldAccessor() {
-			@Override
-			public Object get(Object instance) {
-				try {
-					return field.get(instance);
-				} catch (IllegalAccessException e) {
-					throw new IllegalStateException("Cannot use reflection.", e);
-				}
-			}
-			
-			@Override
-			public void set(Object instance, Object value) {
-				try {
-					field.set(instance, value);
-				} catch (IllegalAccessException e) {
-					throw new IllegalStateException("Cannot use reflection.", e);
-				}
-			}
-		};
+		return new DefaultFieldAccessor(field);
+	}
+	
+	/**
+	 * Retrieve a field accessor where the write operation is synchronized on the current field value.
+	 * @param accessor - the accessor.
+	 * @return The field accessor.
+	 */
+	public static FieldAccessor getSynchronized(final FieldAccessor accessor) {
+		// Only wrap once
+		if (accessor instanceof SynchronizedFieldAccessor)
+			return accessor;
+		return new SynchronizedFieldAccessor(accessor);
 	}
 	
 	/**
@@ -232,20 +268,7 @@ public class FuzzyReflection {
 	 * @return The method accessor.
 	 */
 	public static MethodAccessor getMethodAccessor(final Method method) {
-		return new MethodAccessor() {
-			@Override
-			public Object invoke(Object target, Object... args) {
-				try {
-					return method.invoke(target, args);
-				} catch (IllegalAccessException e) {
-					throw new IllegalStateException("Cannot use reflection.", e);
-				} catch (InvocationTargetException e) {
-					throw new RuntimeException("An internal error occured.", e.getCause());
-				} catch (IllegalArgumentException e) {
-					throw e;
-				}
-			}
-		};
+		return new DefaultMethodAccessor(method);
 	}
 	
 	/**

ProtocolLib/src/main/java/com/comphenix/protocol/reflect/VolatileField.java

@@ -19,6 +19,7 @@ package com.comphenix.protocol.reflect;
 
 import java.lang.reflect.Field;
 
+import com.comphenix.protocol.reflect.FuzzyReflection.FieldAccessor;
 import com.google.common.base.Objects;
 
 /**
@@ -27,8 +28,7 @@ import com.google.common.base.Objects;
  * @author Kristian
  */
 public class VolatileField {
-
-	private Field field;
+	private FieldAccessor accessor;
 	private Object container;
 	
 	// The current and previous values
@@ -48,7 +48,7 @@ public class VolatileField {
 	 * @param container - the object this field belongs to.
 	 */
 	public VolatileField(Field field, Object container) {
-		this.field = field;
+		this.accessor = FuzzyReflection.getFieldAccessor(field);
 		this.container = container;
 	}
 	
@@ -59,17 +59,27 @@ public class VolatileField {
 	 * @param forceAccess - whether or not to override any scope restrictions.
 	 */
 	public VolatileField(Field field, Object container, boolean forceAccess) {
-		this.field = field;
+		this.accessor = FuzzyReflection.getFieldAccessor(field, true);
 		this.container = container;
 		this.forceAccess = forceAccess;
 	}
 	
+	/**
+	 * Initializes a volatile field with the given accessor and associated object.
+	 * @param accessor - the field accessor.
+	 * @param container - the object this field belongs to.
+	 */
+	public VolatileField(FieldAccessor accessor, Object container) {
+		this.accessor = accessor;
+		this.container = container;
+	}
+	
 	/**
 	 * Retrieves the current field.
 	 * @return The stored field.
 	 */
 	public Field getField() {
-		return field;
+		return accessor.getField();
 	}
 	
 	/**
@@ -127,14 +137,9 @@ public class VolatileField {
 		// Remember to safe the previous value
 		ensureLoaded();
 		
-		try {
-			FieldUtils.writeField(field, container, newValue, forceAccess);
-			current = newValue;
-			currentSet = true;
-			
-		} catch (IllegalAccessException e) {
-			throw new RuntimeException("Unable to write field " + field.getName(), e);
-		}
+		writeFieldValue(newValue);
+		current = newValue;
+		currentSet = true;
 	}
 	
 	/**
@@ -178,11 +183,19 @@ public class VolatileField {
 			} else {
 				// This can be a bad sign
 				System.out.println(String.format("[ProtocolLib] Unable to switch %s to %s. Expected %s but got %s.",
-						field.toGenericString(), previous, current, getValue()));
+						getField().toGenericString(), previous, current, getValue()));
 			}
 		}
 	}
 	
+	/**
+	 * Retrieve a synchronized version of the current field.
+	 * @return A synchronized volatile field.
+	 */
+	public VolatileField toSynchronized() {
+		return new VolatileField(FuzzyReflection.getSynchronized(accessor), container);
+	}
+	
 	/**
 	 * Determine whether or not we'll need to revert the value.
 	 */
@@ -203,11 +216,7 @@ public class VolatileField {
 	 * @return The field value.
 	 */
 	private Object readFieldValue() {
-		try {
-			return FieldUtils.readField(field, container, forceAccess);
-		} catch (IllegalAccessException e) {
-			throw new RuntimeException("Unable to read field " + field.getName(), e);
-		}
+		return accessor.get(container);
 	}
 	
 	/**
@@ -215,11 +224,7 @@ public class VolatileField {
 	 * @param newValue - the new value.
 	 */
 	private void writeFieldValue(Object newValue) {
-		try {
-			FieldUtils.writeField(field, container, newValue, forceAccess);
-		} catch (IllegalAccessException e) {
-			throw new RuntimeException("Unable to write field " + field.getName(), e);
-		}
+		accessor.set(container, newValue);
 	}
 		
 	@Override
@@ -229,6 +234,8 @@ public class VolatileField {
 
 	@Override
 	public String toString() {
-		return "VolatileField [field=" + field + ", container=" + container + ", previous=" + previous + ", current=" + current + "]";
+		return "VolatileField [accessor=" + accessor + ", container=" + container + ", previous="
+				+ previous + ", current=" + current + ", previousLoaded=" + previousLoaded
+				+ ", currentSet=" + currentSet + ", forceAccess=" + forceAccess + "]";
 	}
 }