From e56c0fec00e0cdebcf6202191a43fe897e78a8c8 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Tue, 24 Dec 2013 02:15:22 +0100 Subject: [PATCH] A possible fix for a rare but game-breaking deadlock. 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. --- .../injector/netty/BootstrapList.java | 19 +++++++---- .../injector/netty/ChannelInjector.java | 33 +++++++++++++++---- 2 files changed, 39 insertions(+), 13 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 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); }