From 9b349299a0787184744662e673d133434e8d98da Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Sat, 14 Dec 2013 04:05:12 +0100 Subject: [PATCH] Significantly reduce the possibility of a race condition. The vanilla server bootstrap (ServerConnectionChannel) is executed asynchronously when a new channel object has been registered in an event loop, much before it is ready to accept a new client connection. It is here the whole channel handler pipeline is set up, along with a NetworkManager responsible for reading and writing packets. The trouble starts when the bootstrap class adds the created NetworkManager to a list (f) of managers in ServerConnection. This list is regularly inspected by the main thread (in order to process packets on the main thread) and includes a clean up procedure (ServerConnection#61) in case it detects a disconnected network manager. Unfortunately, the network manager IS considered disconnected for a moment when its added to the list, so the main thread MAY end up getting to the network manager before Netty has connected the channel. This is still very rare under normal circumstances, but because ProtocolLib does a lot of initial processing in the channel handler, the asynchronous thread gets hold up for a long while the first time a player connects to the server, allowing the main thread sufficient time to catch up and evict the network manager. The partial solution here is to synchronize on the network manager list, stopping the main thread from processing network managers when we are preparing our new connection. But I suspect the best solution would be to correct the root of the problem, and correct the race condition in the server itself. This could be done by only adding the network manager when the channel is active (see ChannelInboundHandler.channelActive). --- .../injector/netty/BootstrapList.java | 4 +-- .../injector/netty/NettyProtocolInjector.java | 26 +++++++++++++++---- .../protocol/reflect/FuzzyReflection.java | 20 ++++++++++++++ .../protocol/reflect/accessors/Accessors.java | 11 ++++++++ 4 files changed, 53 insertions(+), 8 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/BootstrapList.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/BootstrapList.java index 747cb1bf..3e7643aa 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/BootstrapList.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/BootstrapList.java @@ -27,9 +27,7 @@ class BootstrapList implements List { // Process all existing bootstraps for (Object item : this) { - if (item instanceof ChannelFuture) { - processBootstrap((ChannelFuture) item); - } + processElement(item); } } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolInjector.java index 7ad0ee67..e198dd16 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolInjector.java @@ -35,6 +35,7 @@ import com.comphenix.protocol.injector.spigot.AbstractPacketInjector; import com.comphenix.protocol.injector.spigot.AbstractPlayerHandler; import com.comphenix.protocol.reflect.FuzzyReflection; import com.comphenix.protocol.reflect.VolatileField; +import com.comphenix.protocol.reflect.accessors.FieldAccessor; import com.comphenix.protocol.utility.MinecraftReflection; import com.google.common.collect.Lists; @@ -45,10 +46,12 @@ public class NettyProtocolInjector implements ChannelListener { // The temporary player factory private TemporaryPlayerFactory playerFactory = new TemporaryPlayerFactory(); private List bootstrapFields = Lists.newArrayList(); + private BootstrapList networkManagers; // Different sending filters private PacketTypeSet sendingFilters = new PacketTypeSet(); private PacketTypeSet reveivedFilters = new PacketTypeSet(); + // Packets that must be executed on the main thread private PacketTypeSet mainThreadFilters = new PacketTypeSet(); @@ -88,7 +91,11 @@ public class NettyProtocolInjector implements ChannelListener { if (closed) return; } - ChannelInjector.fromChannel(channel, NettyProtocolInjector.this, playerFactory).inject(); + + // This can take a while, so we need to stop the main thread from interfering + synchronized (networkManagers) { + ChannelInjector.fromChannel(channel, NettyProtocolInjector.this, playerFactory).inject(); + } } }; @@ -104,16 +111,25 @@ public class NettyProtocolInjector implements ChannelListener { } }; + // Get the current NetworkMananger list + Object networkManagerList = FuzzyReflection.fromObject(serverConnection, true). + invokeMethod(null, "getNetworkManagers", List.class, serverConnection); + // Insert ProtocolLib's connection interceptor bootstrapFields = getBootstrapFields(serverConnection); for (VolatileField field : bootstrapFields) { final List list = (List) field.getValue(); - + final BootstrapList bootstrap = new BootstrapList(list, connectionHandler); + // Synchronize with each list before we attempt to replace them. - field.setValue(new BootstrapList( - list, connectionHandler - )); + field.setValue(bootstrap); + + if (list == networkManagerList) { + // Save it for later + networkManagers = bootstrap; + continue; + } } injected = true; diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/FuzzyReflection.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/FuzzyReflection.java index 02c77e09..5ce44d14 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/FuzzyReflection.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/FuzzyReflection.java @@ -279,6 +279,26 @@ public class FuzzyReflection { throw new IllegalArgumentException("Unable to find " + name + " in " + source.getName()); } + /** + * Invoke a method by return type and parameters alone. + *

+ * The parameters must be non-null for this to work. + * @param target - the instance. + * @param name - the name of the method - for debugging. + * @param returnType - the expected return type. + * @param parameters - the parameters. + * @return The return value, or NULL. + */ + public Object invokeMethod(Object target, String name, Class returnType, Object... parameters) { + Class[] types = new Class[parameters.length]; + + for (int i = 0; i < types.length; i++) { + types[i] = parameters[i].getClass(); + } + return Accessors.getMethodAccessor(getMethodByParameters(name, returnType, types)). + invoke(target, parameters); + } + private boolean matchParameters(Pattern[] parameterMatchers, Class[] argTypes) { if (parameterMatchers.length != argTypes.length) throw new IllegalArgumentException("Arrays must have the same cardinality."); diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/accessors/Accessors.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/accessors/Accessors.java index bf6dc470..a0336d58 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/accessors/Accessors.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/accessors/Accessors.java @@ -136,6 +136,17 @@ public final class Accessors { * @return The method accessor. */ public static MethodAccessor getMethodAccessor(final Method method) { + return getMethodAccessor(method, true); + } + + /** + * Retrieve a method accessor for a particular method, avoding checked exceptions. + * @param method - the method to access. + * @param forceAccess - whether or not to skip Java access checking. + * @return The method accessor. + */ + public static MethodAccessor getMethodAccessor(final Method method, boolean forceAccess) { + method.setAccessible(forceAccess); return new DefaultMethodAccessor(method); }