From c443fc3da62ddafceb28262c3bbf995bafc44ac1 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Sat, 2 Aug 2014 23:39:29 +0200 Subject: [PATCH] Execute onPacketSending() on the main thread for monitor listeners. A special-case occurs when a plugin sends a packet to a client with filters set to FALSE (that is, bypassing most packet listeners) - a new packet event is constructed solely for all MONITOR listeners, as they are informed regardless of the value of FILTER. Unfortunately, the sending method may be invoked on a thread other than the main thread, which will invoke onPacketSending() asynchronously. This violate the assumed thread affinity of onPacketSending(), so we will now schedule the packet sending on the main thread to correct this - but only if there are monitor listeners, and they have not specified ListenerOptions.ASYNC (which means onPacketSending() is thread safe). --- .../injector/PacketFilterManager.java | 40 +++++++++++++++---- .../injector/netty/NettyProtocolInjector.java | 5 +++ .../player/PlayerInjectionHandler.java | 9 +++++ .../player/ProxyPlayerInjectionHandler.java | 5 +++ .../injector/spigot/DummyPlayerHandler.java | 7 ++++ 5 files changed, 58 insertions(+), 8 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java index 4bc287b0..937e287d 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java @@ -23,6 +23,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Set; +import java.util.concurrent.Callable; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; @@ -33,6 +34,7 @@ import net.sf.cglib.proxy.Enhancer; import net.sf.cglib.proxy.MethodInterceptor; import net.sf.cglib.proxy.MethodProxy; +import org.bukkit.Bukkit; import org.bukkit.Location; import org.bukkit.Server; import org.bukkit.World; @@ -172,6 +174,9 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok // The current server private Server server; + // The current ProtocolLib library + private Plugin library; + // The async packet handler private AsyncFilterManager asyncFilterManager; @@ -292,6 +297,7 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok buildInjector(); } this.asyncFilterManager = builder.getAsyncManager(); + this.library = builder.getLibrary(); // Attempt to load the list of server and client packets try { @@ -771,26 +777,44 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok } @Override - public void sendServerPacket(Player receiver, PacketContainer packet, NetworkMarker marker, boolean filters) throws InvocationTargetException { + public void sendServerPacket(final Player receiver, final PacketContainer packet, NetworkMarker marker, final boolean filters) throws InvocationTargetException { if (receiver == null) throw new IllegalArgumentException("receiver cannot be NULL."); if (packet == null) throw new IllegalArgumentException("packet cannot be NULL."); + // We may have to enable player injection indefinitely after this if (packetCreation.compareAndSet(false, true)) incrementPhases(GamePhase.PLAYING); - - // Inform the MONITOR packets - if (!filters) { - PacketEvent event = PacketEvent.fromServer(this, packet, marker, receiver, false); + + if (!filters) { + // We may have to delay the packet due to non-asynchronous monitor listeners + if (!filters && !Bukkit.isPrimaryThread() && playerInjection.hasMainThreadListener(packet.getType())) { + final NetworkMarker copy = marker; + + server.getScheduler().scheduleSyncDelayedTask(library, new Runnable() { + @Override + public void run() { + try { + // Prevent infinite loops + if (!Bukkit.isPrimaryThread()) + throw new IllegalStateException("Scheduled task was not executed on the main thread!"); + sendServerPacket(receiver, packet, copy, filters); + } catch (Exception e) { + reporter.reportMinimal(library, "sendServerPacket-run()", e); + } + } + }); + return; + } - sendingListeners.invokePacketSending( - reporter, event, ListenerPriority.MONITOR); + PacketEvent event = PacketEvent.fromServer(this, packet, marker, receiver, false); + sendingListeners.invokePacketSending(reporter, event, ListenerPriority.MONITOR); marker = NetworkMarker.getNetworkMarker(event); } playerInjection.sendServerPacket(receiver, packet, marker, filters); } - + @Override public void recieveClientPacket(Player sender, PacketContainer packet) throws IllegalAccessException, InvocationTargetException { recieveClientPacket(sender, packet, null, true); 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 b94dcd61..ca169954 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 @@ -342,6 +342,11 @@ public class NettyProtocolInjector implements ChannelListener { sendServerPacket(packet.getHandle(), marker, filters); } + @Override + public boolean hasMainThreadListener(PacketType type) { + return mainThreadFilters.contains(type); + } + @Override public void recieveClientPacket(Player player, Object mcPacket) throws IllegalAccessException, InvocationTargetException { injectionFactory.fromPlayer(player, listener). diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/PlayerInjectionHandler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/PlayerInjectionHandler.java index 470db859..99acc7c2 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/PlayerInjectionHandler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/PlayerInjectionHandler.java @@ -181,4 +181,13 @@ public interface PlayerInjectionHandler { * Close any lingering proxy injections. */ public abstract void close(); + + /** + * Determine if we have packet listeners with the given type that must be executed on the main thread. + *

+ * This only applies for onPacketSending(), as it makes certain guarantees. + * @param type - the packet type. + * @return TRUE if we do, FALSE otherwise. + */ + public abstract boolean hasMainThreadListener(PacketType type); } \ No newline at end of file diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/ProxyPlayerInjectionHandler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/ProxyPlayerInjectionHandler.java index e62e19c6..d4389724 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/ProxyPlayerInjectionHandler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/ProxyPlayerInjectionHandler.java @@ -174,6 +174,11 @@ class ProxyPlayerInjectionHandler implements PlayerInjectionHandler { } } + @Override + public boolean hasMainThreadListener(PacketType type) { + return sendingFilters.contains(type.getLegacyId()); + } + /** * Sets how the server packets are read. * @param playerHook - the new injection method for reading server packets. diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/spigot/DummyPlayerHandler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/spigot/DummyPlayerHandler.java index 3f9878b7..a713d09e 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/spigot/DummyPlayerHandler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/spigot/DummyPlayerHandler.java @@ -3,8 +3,10 @@ package com.comphenix.protocol.injector.spigot; import java.io.InputStream; import java.lang.reflect.InvocationTargetException; import java.net.InetSocketAddress; + import org.bukkit.entity.Player; +import com.comphenix.protocol.PacketType; import com.comphenix.protocol.concurrency.PacketTypeSet; import com.comphenix.protocol.events.NetworkMarker; import com.comphenix.protocol.events.PacketContainer; @@ -50,6 +52,11 @@ class DummyPlayerHandler extends AbstractPlayerHandler { injector.injectPlayer(player); } + @Override + public boolean hasMainThreadListener(PacketType type) { + return sendingFilters.contains(type); + } + @Override public void handleDisconnect(Player player) { // Just ignore