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 3e7643aa..fedf47f1 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 @@ -4,10 +4,12 @@ import java.util.Collection; import java.util.Iterator; import java.util.List; import java.util.ListIterator; -import java.util.NoSuchElementException; +import java.util.concurrent.Callable; import com.google.common.collect.Lists; +import net.minecraft.util.io.netty.channel.Channel; + // Hopefully, CB won't version these as well import net.minecraft.util.io.netty.channel.ChannelFuture; import net.minecraft.util.io.netty.channel.ChannelHandler; @@ -94,11 +96,16 @@ class BootstrapList implements List { * @param future - the future. */ protected void unprocessBootstrap(ChannelFuture future) { - try { - future.channel().pipeline().remove(handler); - } catch (NoSuchElementException e) { - // Whatever - } + final Channel channel = future.channel(); + + // For thread safety - see ChannelInjector.close() + channel.eventLoop().submit(new Callable() { + @Override + public Object call() throws Exception { + channel.pipeline().remove(handler); + return null; + } + }); } /** diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelInjector.java index 5a5c4bab..3a0fd943 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelInjector.java @@ -6,8 +6,8 @@ import java.net.SocketAddress; import java.util.Collections; import java.util.List; import java.util.Map.Entry; -import java.util.NoSuchElementException; import java.util.Set; +import java.util.concurrent.Callable; import java.util.concurrent.ConcurrentMap; import net.minecraft.util.com.mojang.authlib.GameProfile; @@ -510,12 +510,31 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { if (injected) { channelField.revertValue(); - try { - originalChannel.pipeline().remove(this); - originalChannel.pipeline().remove(protocolEncoder); - } catch (NoSuchElementException e) { - // Ignore it - the player has logged out - } + // Calling remove() in the main thread will block the main thread, which may lead + // to a deadlock: + // http://pastebin.com/L3SBVKzp + // + // ProtocolLib executes this close() method through a PlayerQuitEvent in the main thread, + // which has implicitly aquired a lock on SimplePluginManager (see SimplePluginManager.callEvent(Event)). + // Unfortunately, the remove() method will schedule the removal on one of the Netty worker threads if + // it's called from a different thread, blocking until the removal has been confirmed. + // + // This is bad enough (Rule #1: Don't block the main thread), but the real trouble starts if the same + // worker thread happens to be handling a server ping connection when this removal task is scheduled. + // In that case, it may attempt to invoke an asynchronous ServerPingEvent (see PacketStatusListener) + // using SimplePluginManager.callEvent(). But, since this has already been locked by the main thread, + // we end up with a deadlock. The main thread is waiting for the worker thread to process the task, and + // the worker thread is waiting for the main thread to finish executing PlayerQuitEvent. + // + // TLDR: Concurrenty is hard. + originalChannel.eventLoop().submit(new Callable() { + @Override + public Object call() throws Exception { + originalChannel.pipeline().remove(ChannelInjector.this); + originalChannel.pipeline().remove(protocolEncoder); + return null; + } + }); // Clear cache factory.invalidate(player); }