From 245433b29ad47ad68dd9410ae72567345ae82bcf Mon Sep 17 00:00:00 2001 From: Dan Mulloy Date: Fri, 16 Oct 2015 22:36:01 -0400 Subject: [PATCH] Fix server pings and the infamous closed channel exception As a result, packet listeners for OUT_SERVER_INFO will be processed on the netty server io thread, so they will have to be thread safe if they aren't already. Fixes #119 --- .../com/comphenix/protocol/PacketType.java | 37 ++++++++++--------- .../independent/NettyChannelInjector.java | 32 +--------------- .../independent/NettyProtocolInjector.java | 2 +- .../netty/shaded/ShadedChannelInjector.java | 32 +--------------- .../netty/shaded/ShadedProtocolInjector.java | 2 +- 5 files changed, 23 insertions(+), 82 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/PacketType.java b/ProtocolLib/src/main/java/com/comphenix/protocol/PacketType.java index b0cbcdda..c7c20c8d 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/PacketType.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/PacketType.java @@ -269,7 +269,7 @@ public class PacketType implements Serializable, Comparable { public static class Server extends ObjectEnum { private final static Sender SENDER = Sender.SERVER; - public static final PacketType OUT_SERVER_INFO = new PacketType(PROTOCOL, SENDER, 0x00, 255); + public static final PacketType OUT_SERVER_INFO = new PacketType(PROTOCOL, SENDER, 0x00, 255).forceAsync(true); public static final PacketType OUT_PING = new PacketType(PROTOCOL, SENDER, 0x01, 230); private final static Server INSTANCE = new Server(); @@ -523,7 +523,8 @@ public class PacketType implements Serializable, Comparable { private final int currentId; private final int legacyId; private final MinecraftVersion version; - private final boolean dynamic; + private boolean forceAsync; + private boolean dynamic; /** * Retrieve the current packet/legacy lookup. @@ -679,7 +680,8 @@ public class PacketType implements Serializable, Comparable { PacketType type = getLookup().getFromCurrent(protocol, sender, packetId); if (type == null) { - type = new PacketType(protocol, sender, packetId, legacyId, PROTOCOL_VERSION, true); + type = new PacketType(protocol, sender, packetId, legacyId, PROTOCOL_VERSION); + type.dynamic = true; // Many may be scheduled, but only the first will be executed scheduleRegister(type, "Dynamic-" + UUID.randomUUID().toString()); @@ -798,25 +800,11 @@ public class PacketType implements Serializable, Comparable { * @param version - the version of the current ID. */ public PacketType(Protocol protocol, Sender sender, int currentId, int legacyId, MinecraftVersion version) { - this(protocol, sender, currentId, legacyId, version, false); - } - - /** - * Construct a new packet type. - * @param protocol - the current protocol. - * @param sender - client or server. - * @param currentId - the current packet ID. - * @param legacyId - the legacy packet ID. - * @param version - the version of the current ID. - * @param dynamic - if this type was created dynamically. - */ - public PacketType(Protocol protocol, Sender sender, int currentId, int legacyId, MinecraftVersion version, boolean dynamic) { this.protocol = Preconditions.checkNotNull(protocol, "protocol cannot be NULL"); this.sender = Preconditions.checkNotNull(sender, "sender cannot be NULL"); this.currentId = currentId; this.legacyId = legacyId; this.version = version; - this.dynamic = dynamic; } /** @@ -920,13 +908,26 @@ public class PacketType implements Serializable, Comparable { } /** - * Whether or not this packet was dynamically created (ie we don't have it registered) + * Whether or not this packet was dynamically created (i.e. we don't have it registered) * @return True if dnyamic, false if not. */ public boolean isDynamic() { return dynamic; } + private PacketType forceAsync(boolean forceAsync) { + this.forceAsync = forceAsync; + return this; + } + + /** + * Whether or not this packet must be processed asynchronously. + * @return True if it must be, false if not. + */ + public boolean forceAsync() { + return forceAsync; + } + @Override public int hashCode() { return Objects.hashCode(protocol, sender, currentId, legacyId); diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/compat/netty/independent/NettyChannelInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/compat/netty/independent/NettyChannelInjector.java index 59ec400f..3a1eb41b 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/compat/netty/independent/NettyChannelInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/compat/netty/independent/NettyChannelInjector.java @@ -19,7 +19,6 @@ package com.comphenix.protocol.compat.netty.independent; import io.netty.buffer.ByteBuf; import io.netty.channel.Channel; import io.netty.channel.ChannelHandler; -import io.netty.channel.ChannelHandlerAdapter; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelInboundHandlerAdapter; import io.netty.channel.ChannelPipeline; @@ -27,14 +26,12 @@ import io.netty.channel.ChannelPromise; import io.netty.channel.socket.SocketChannel; import io.netty.handler.codec.ByteToMessageDecoder; import io.netty.handler.codec.MessageToByteEncoder; -import io.netty.util.ReferenceCountUtil; import io.netty.util.concurrent.GenericFutureListener; import io.netty.util.internal.TypeParameterMatcher; import java.lang.reflect.InvocationTargetException; import java.net.Socket; import java.net.SocketAddress; -import java.nio.channels.ClosedChannelException; import java.util.ArrayDeque; import java.util.Deque; import java.util.List; @@ -283,37 +280,10 @@ public class NettyChannelInjector extends ByteToMessageDecoder implements Channe } }; - ChannelHandlerAdapter exceptionHandler = new ChannelHandlerAdapter() { - @Override - public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { - if (channelListener.isDebug()) { - // People were complaining about this on the forums, figure I might as well figure out the cause - System.out.println("------------ ProtocolLib Debug ------------"); - System.out.println("Caught an exception in " + playerName + "\'s channel pipeline."); - System.out.println("Context: " + ctx); - System.out.println("The exception was: " + cause); - System.out.println("Stack trace:"); - cause.printStackTrace(System.out); - System.out.println("Please create an issue on GitHub with the above message."); - System.out.println("https://github.com/dmulloy2/ProtocolLib/issues"); - System.out.println("-------------------------------------------"); - } - - if (cause instanceof ClosedChannelException) { - // This is what the DefaultChannelPipeline does - ReferenceCountUtil.release(cause); - } else { - // We only care about closed channel exceptions, pass everything else along - super.exceptionCaught(ctx, cause); - } - } - }; - // Insert our handlers - note that we effectively replace the vanilla encoder/decoder originalChannel.pipeline().addBefore("decoder", "protocol_lib_decoder", this); originalChannel.pipeline().addBefore("protocol_lib_decoder", "protocol_lib_finish", finishHandler); originalChannel.pipeline().addAfter("encoder", "protocol_lib_encoder", protocolEncoder); - originalChannel.pipeline().addLast("protocol_lib_exception_handler", exceptionHandler); // Intercept all write methods channelField.setValue(new NettyChannelProxy(originalChannel, MinecraftReflection.getPacketClass()) { @@ -840,7 +810,7 @@ public class NettyChannelInjector extends ByteToMessageDecoder implements Channe @Override public void run() { String[] handlers = new String[] { - "protocol_lib_decoder", "protocol_lib_finish", "protocol_lib_encoder", "protocol_lib_exception_handler" + "protocol_lib_decoder", "protocol_lib_finish", "protocol_lib_encoder" }; for (String handler : handlers) { diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/compat/netty/independent/NettyProtocolInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/compat/netty/independent/NettyProtocolInjector.java index 90a880e2..54972c7a 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/compat/netty/independent/NettyProtocolInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/compat/netty/independent/NettyProtocolInjector.java @@ -356,7 +356,7 @@ public class NettyProtocolInjector implements ProtocolInjector { @Override public void addPacketHandler(PacketType type, Set options) { - if (options != null && !options.contains(ListenerOptions.ASYNC)) + if (options != null && !type.forceAsync() && !options.contains(ListenerOptions.ASYNC)) mainThreadFilters.addType(type); super.addPacketHandler(type, options); } diff --git a/modules/v1_7_R4/src/main/java/com/comphenix/protocol/compat/netty/shaded/ShadedChannelInjector.java b/modules/v1_7_R4/src/main/java/com/comphenix/protocol/compat/netty/shaded/ShadedChannelInjector.java index ee7cb06e..17294860 100644 --- a/modules/v1_7_R4/src/main/java/com/comphenix/protocol/compat/netty/shaded/ShadedChannelInjector.java +++ b/modules/v1_7_R4/src/main/java/com/comphenix/protocol/compat/netty/shaded/ShadedChannelInjector.java @@ -19,7 +19,6 @@ package com.comphenix.protocol.compat.netty.shaded; import java.lang.reflect.InvocationTargetException; import java.net.Socket; import java.net.SocketAddress; -import java.nio.channels.ClosedChannelException; import java.util.ArrayDeque; import java.util.Deque; import java.util.List; @@ -32,7 +31,6 @@ import java.util.concurrent.ConcurrentMap; import net.minecraft.util.io.netty.buffer.ByteBuf; import net.minecraft.util.io.netty.channel.Channel; import net.minecraft.util.io.netty.channel.ChannelHandler; -import net.minecraft.util.io.netty.channel.ChannelHandlerAdapter; import net.minecraft.util.io.netty.channel.ChannelHandlerContext; import net.minecraft.util.io.netty.channel.ChannelInboundHandlerAdapter; import net.minecraft.util.io.netty.channel.ChannelPipeline; @@ -40,7 +38,6 @@ import net.minecraft.util.io.netty.channel.ChannelPromise; import net.minecraft.util.io.netty.channel.socket.SocketChannel; import net.minecraft.util.io.netty.handler.codec.ByteToMessageDecoder; import net.minecraft.util.io.netty.handler.codec.MessageToByteEncoder; -import net.minecraft.util.io.netty.util.ReferenceCountUtil; import net.minecraft.util.io.netty.util.concurrent.GenericFutureListener; import net.minecraft.util.io.netty.util.internal.TypeParameterMatcher; import net.sf.cglib.proxy.Factory; @@ -282,37 +279,10 @@ public class ShadedChannelInjector extends ByteToMessageDecoder implements Chann } }; - ChannelHandlerAdapter exceptionHandler = new ChannelHandlerAdapter() { - @Override - public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { - if (channelListener.isDebug()) { - // People were complaining about this on the forums, figure I might as well figure out the cause - System.out.println("------------ ProtocolLib Debug ------------"); - System.out.println("Caught an exception in " + playerName + "\'s channel pipeline."); - System.out.println("Context: " + ctx); - System.out.println("The exception was: " + cause); - System.out.println("Stack trace:"); - cause.printStackTrace(System.out); - System.out.println("Please create an issue on GitHub with the above message."); - System.out.println("https://github.com/dmulloy2/ProtocolLib/issues"); - System.out.println("-------------------------------------------"); - } - - if (cause instanceof ClosedChannelException) { - // This is what the DefaultChannelPipeline does - ReferenceCountUtil.release(cause); - } else { - // We only care about closed channel exceptions, pass everything else along - super.exceptionCaught(ctx, cause); - } - } - }; - // Insert our handlers - note that we effectively replace the vanilla encoder/decoder originalChannel.pipeline().addBefore("decoder", "protocol_lib_decoder", this); originalChannel.pipeline().addBefore("protocol_lib_decoder", "protocol_lib_finish", finishHandler); originalChannel.pipeline().addAfter("encoder", "protocol_lib_encoder", protocolEncoder); - originalChannel.pipeline().addLast("protocol_lib_exception_handler", exceptionHandler); // Intercept all write methods channelField.setValue(new ShadedChannelProxy(originalChannel, MinecraftReflection.getPacketClass()) { @@ -839,7 +809,7 @@ public class ShadedChannelInjector extends ByteToMessageDecoder implements Chann @Override public void run() { String[] handlers = new String[] { - "protocol_lib_decoder", "protocol_lib_finish", "protocol_lib_encoder", "protocol_lib_exception_handler" + "protocol_lib_decoder", "protocol_lib_finish", "protocol_lib_encoder" }; for (String handler : handlers) { diff --git a/modules/v1_7_R4/src/main/java/com/comphenix/protocol/compat/netty/shaded/ShadedProtocolInjector.java b/modules/v1_7_R4/src/main/java/com/comphenix/protocol/compat/netty/shaded/ShadedProtocolInjector.java index d7852d07..8a2b1a35 100644 --- a/modules/v1_7_R4/src/main/java/com/comphenix/protocol/compat/netty/shaded/ShadedProtocolInjector.java +++ b/modules/v1_7_R4/src/main/java/com/comphenix/protocol/compat/netty/shaded/ShadedProtocolInjector.java @@ -356,7 +356,7 @@ public class ShadedProtocolInjector implements ProtocolInjector { @Override public void addPacketHandler(PacketType type, Set options) { - if (options != null && !options.contains(ListenerOptions.ASYNC)) + if (options != null && !type.forceAsync() && !options.contains(ListenerOptions.ASYNC)) mainThreadFilters.addType(type); super.addPacketHandler(type, options); }