From 82be6bfecc05383e8816fee72603b5cfb842199a Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Sat, 17 May 2014 23:01:27 +0200 Subject: [PATCH] May solve a race condition in ticket 220 on BukkitDev. It is possible, though not confirmed, that ProtocolLib has not been fully cleaned up after a "reload" command and the next instance of ProtocolLib is loaded. In that case, it may be possible that a channel is injected in the main thread while its cleanup procedure is still running. This is an attempt to solve this problem. Though, it is not confirmed to work. --- .../injector/netty/ChannelInjector.java | 54 ++++++++++++++++--- .../injector/netty/NettyProtocolInjector.java | 2 +- 2 files changed, 48 insertions(+), 8 deletions(-) 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 388ab3cc..79bfeb3d 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 @@ -8,6 +8,7 @@ import java.util.Deque; import java.util.List; import java.util.ListIterator; import java.util.Map.Entry; +import java.util.NoSuchElementException; import java.util.concurrent.Callable; import java.util.concurrent.ConcurrentMap; @@ -34,6 +35,7 @@ import com.comphenix.protocol.PacketType.Protocol; import com.comphenix.protocol.ProtocolLibrary; import com.comphenix.protocol.error.Report; import com.comphenix.protocol.error.ReportType; +import com.comphenix.protocol.error.Report.ReportBuilder; import com.comphenix.protocol.events.ConnectionSide; import com.comphenix.protocol.events.NetworkMarker; import com.comphenix.protocol.events.PacketEvent; @@ -57,6 +59,7 @@ import com.google.common.collect.MapMaker; class ChannelInjector extends ByteToMessageDecoder implements Injector { public static final ReportType REPORT_CANNOT_INTERCEPT_SERVER_PACKET = new ReportType("Unable to intercept a written server packet."); public static final ReportType REPORT_CANNOT_INTERCEPT_CLIENT_PACKET = new ReportType("Unable to intercept a read client packet."); + public static final ReportType REPORT_CANNOT_EXECUTE_IN_CHANNEL_THREAD = new ReportType("Cannot execute code in channel thread."); /** * Indicates that a packet has bypassed packet listeners. @@ -166,7 +169,20 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { return false; if (!originalChannel.isActive()) return false; - + + // Main thread? We should synchronize with the channel thread, otherwise we might see a + // pipeline with only some of the handlers removed + if (Bukkit.isPrimaryThread()) { + // Just like in the close() method, we'll avoid blocking the main thread + executeInChannelThread(new Runnable() { + @Override + public void run() { + inject(); + } + }); + return false; // We don't know + } + // Don't inject the same channel twice if (findChannelHandler(originalChannel, ChannelInjector.class) != null) { return false; @@ -692,20 +708,44 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { // the worker thread is waiting for the main thread to finish executing PlayerQuitEvent. // // TLDR: Concurrenty is hard. - originalChannel.eventLoop().submit(new Callable() { + executeInChannelThread(new Runnable() { @Override - public Object call() throws Exception { - originalChannel.pipeline().remove(ChannelInjector.this); - originalChannel.pipeline().remove(finishHandler); - originalChannel.pipeline().remove(protocolEncoder); - return null; + public void run() { + for (ChannelHandler handler : new ChannelHandler[] { ChannelInjector.this, finishHandler, protocolEncoder }) { + try { + originalChannel.pipeline().remove(handler); + } catch (NoSuchElementException e) { + // Ignore + } + } } }); + // Clear cache factory.invalidate(player); } } } + + /** + * Execute a specific command in the channel thread. + *

+ * Exceptions are printed through the standard error reporter mechanism. + * @param command - the command to execute. + */ + private void executeInChannelThread(final Runnable command) { + originalChannel.eventLoop().execute(new Runnable() { + @Override + public void run() { + try { + command.run(); + } catch (Exception e) { + ProtocolLibrary.getErrorReporter().reportDetailed(ChannelInjector.this, + Report.newBuilder(REPORT_CANNOT_EXECUTE_IN_CHANNEL_THREAD).error(e).build()); + } + } + }); + } /** * Find the first channel handler that is assignable to a given type. 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 974394a1..b94dcd61 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 @@ -38,7 +38,7 @@ import com.google.common.collect.Lists; public class NettyProtocolInjector implements ChannelListener { - public static final ReportType REPORT_CANNOT_INJECT_INCOMING_CHANNEL = new ReportType("Unable to to inject incoming channel %s."); + public static final ReportType REPORT_CANNOT_INJECT_INCOMING_CHANNEL = new ReportType("Unable to inject incoming channel %s."); private volatile boolean injected; private volatile boolean closed;