From a40df621d0fd6b0554aab2ebbba63545b9214921 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Tue, 13 Nov 2012 18:31:06 +0100 Subject: [PATCH 01/31] Spelling error. --- .../protocol/wrappers/WrappedChunkCoordinate.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedChunkCoordinate.java b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedChunkCoordinate.java index 14a42b0f..8385770d 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedChunkCoordinate.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedChunkCoordinate.java @@ -61,7 +61,7 @@ public class WrappedChunkCoordinate implements Comparable Date: Fri, 16 Nov 2012 03:57:55 +0100 Subject: [PATCH 02/31] Fixed a bug that would clear the debug listeners on "add". This was caused by a bug in the abstract interval tree class that would clear the entire tree instead of the subtree whenever a new entry was added. Never roll your own custom collection implementation ... --- ProtocolLib/dependency-reduced-pom.xml | 2 +- .../protocol/concurrency/AbstractIntervalTree.java | 2 +- .../java/com/comphenix/protocol/metrics/Updater.java | 9 +++++++++ 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/ProtocolLib/dependency-reduced-pom.xml b/ProtocolLib/dependency-reduced-pom.xml index 52eae950..e3b1885e 100644 --- a/ProtocolLib/dependency-reduced-pom.xml +++ b/ProtocolLib/dependency-reduced-pom.xml @@ -4,7 +4,7 @@ com.comphenix.protocol ProtocolLib ProtocolLib - 1.6.0 + 1.6.1-SNAPSHOT Provides read/write access to the Minecraft protocol. http://dev.bukkit.org/server-mods/protocollib/ diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/concurrency/AbstractIntervalTree.java b/ProtocolLib/src/main/java/com/comphenix/protocol/concurrency/AbstractIntervalTree.java index e2748cbc..f36ddf0e 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/concurrency/AbstractIntervalTree.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/concurrency/AbstractIntervalTree.java @@ -257,7 +257,7 @@ public abstract class AbstractIntervalTree, TValue private void getEntries(Set destination, NavigableMap map) { Map.Entry last = null; - for (Map.Entry entry : bounds.entrySet()) { + for (Map.Entry entry : map.entrySet()) { switch (entry.getValue().state) { case BOTH: EndPoint point = entry.getValue(); diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/metrics/Updater.java b/ProtocolLib/src/main/java/com/comphenix/protocol/metrics/Updater.java index eaea4a7b..488b915d 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/metrics/Updater.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/metrics/Updater.java @@ -23,6 +23,8 @@ import javax.xml.stream.events.XMLEvent; import org.bukkit.configuration.file.YamlConfiguration; import org.bukkit.plugin.Plugin; +import com.google.common.base.Preconditions; + /** * Check dev.bukkit.org to find updates for a given plugin, and download the updates if needed. *

@@ -211,6 +213,13 @@ public class Updater */ public Updater(Plugin plugin, Logger logger, String slug, File file, String permission) { + // I hate NULL + Preconditions.checkNotNull(plugin, "plugin"); + Preconditions.checkNotNull(logger, "logger"); + Preconditions.checkNotNull(slug, "slug"); + Preconditions.checkNotNull(file, "file"); + Preconditions.checkNotNull(permission, "permission"); + this.plugin = plugin; this.file = file; this.slug = slug; From 4298ac609de4f7daf8ef1d1c45f3881b5fa66865 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Fri, 16 Nov 2012 04:10:59 +0100 Subject: [PATCH 03/31] Damn typos. --- .../java/com/comphenix/protocol/wrappers/ChunkPosition.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/ChunkPosition.java b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/ChunkPosition.java index e1066fbb..a00a139a 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/ChunkPosition.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/ChunkPosition.java @@ -130,7 +130,7 @@ public class ChunkPosition { return new EquivalentConverter() { @Override public Object getGeneric(Class genericType, ChunkPosition specific) { - return new net.minecraft.server.ChunkPosition(specific.x, specific.z, specific.z); + return new net.minecraft.server.ChunkPosition(specific.x, specific.y, specific.z); } @Override From 45f5d55b6a3c95c71a4291acf28231d6e0aee6a3 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Sat, 17 Nov 2012 08:47:25 +0100 Subject: [PATCH 04/31] Fixed the update feature (hopefully for the last time). The problem is that ProtocolLib is using a different naming convention where each release has a version suffix. So, while we can't use the update folder to replace the JAR file, it's also not needed since we can simply add it to the plugins directory directly and remove the old version on shutdown. --- .../comphenix/protocol/metrics/Updater.java | 150 ++++-------------- 1 file changed, 28 insertions(+), 122 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/metrics/Updater.java b/ProtocolLib/src/main/java/com/comphenix/protocol/metrics/Updater.java index 488b915d..50ea9583 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/metrics/Updater.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/metrics/Updater.java @@ -1,5 +1,8 @@ package com.comphenix.protocol.metrics; +// EXTENSIVELY MODIFIED BY AADNK/COMPHENIX +// CHECK GIT FOR DETAILS + /* * Updater for Bukkit. * @@ -9,12 +12,10 @@ import java.io.*; import java.net.MalformedURLException; import java.net.URL; import java.net.URLConnection; -import java.util.Enumeration; import java.util.HashMap; import java.util.Map; +import java.util.logging.Level; import java.util.logging.Logger; -import java.util.zip.ZipEntry; -import java.util.zip.ZipFile; import javax.xml.stream.XMLEventReader; import javax.xml.stream.XMLInputFactory; import javax.xml.stream.XMLStreamException; @@ -262,20 +263,30 @@ public class Updater String fileLink = getFile(versionLink); if(fileLink != null && type != UpdateType.NO_DOWNLOAD) { - String name = file.getName(); - // If it's a zip file, it shouldn't be downloaded as the plugin's name - if(fileLink.endsWith(".zip")) - { - String [] split = fileLink.split("/"); - name = split[split.length-1]; - } + String [] split = fileLink.split("/"); + String name = split[split.length-1]; + logger.info("Downloading " + fileLink); + // Never download the same file twice - if (!downloadedVersion.equalsIgnoreCase(versionLink)) { - saveFile(new File("plugins/" + updateFolder), name, fileLink); + if (downloadedVersion == null || !downloadedVersion.equalsIgnoreCase(versionLink)) { + File path = new File("plugins/"); + + // We can update the JAR in place as we're using different JAR file names + saveFile(path, name, fileLink); downloadedVersion = versionLink; result = UpdateResult.SUCCESS; + // ProtocolLib - try to remove the current version + try { + if (!file.delete()) { + File zeroCurrentJar = new File(path, updateFolder + "/" + file.getName()); + zeroCurrentJar.createNewFile(); + } + } catch (IOException e) { + logger.warning("Cannot delete old ProtocolLib version: " + file.getName()); + } + } else { result = UpdateResult.UPDATE_AVAILABLE; } @@ -347,26 +358,14 @@ public class Updater logger.info("Downloading update: " + percent + "% of " + fileLength + " bytes."); } } - //Just a quick check to make sure we didn't leave any files from last time... - for(File xFile : new File("plugins/" + updateFolder).listFiles()) - { - if(xFile.getName().endsWith(".zip")) - { - xFile.delete(); - } - } - // Check to see if it's a zip file, if it is, unzip it. - File dFile = new File(folder.getAbsolutePath() + "/" + file); - if(dFile.getName().endsWith(".zip")) - { - // Unzip - unzip(dFile.getCanonicalPath()); - } - if(announce) logger.info("Finished updating."); + + if(announce) + logger.info("Finished updating."); } catch (Exception ex) { - logger.warning("The auto-updater tried to download a new update, but was unsuccessful."); + logger.warning("The auto-updater tried to download a new update, but was unsuccessful."); + logger.log(Level.INFO, "Error message to submit as a ticket.", ex); result = Updater.UpdateResult.FAIL_DOWNLOAD; } finally @@ -388,99 +387,6 @@ public class Updater } } - /** - * Part of Zip-File-Extractor, modified by H31IX for use with Bukkit - */ - private void unzip(String file) - { - try - { - File fSourceZip = new File(file); - String zipPath = file.substring(0, file.length()-4); - ZipFile zipFile = new ZipFile(fSourceZip); - Enumeration e = zipFile.entries(); - while(e.hasMoreElements()) - { - ZipEntry entry = (ZipEntry)e.nextElement(); - File destinationFilePath = new File(zipPath,entry.getName()); - destinationFilePath.getParentFile().mkdirs(); - if(entry.isDirectory()) - { - continue; - } - else - { - BufferedInputStream bis = new BufferedInputStream(zipFile.getInputStream(entry)); - int b; - byte buffer[] = new byte[BYTE_SIZE]; - FileOutputStream fos = new FileOutputStream(destinationFilePath); - BufferedOutputStream bos = new BufferedOutputStream(fos, BYTE_SIZE); - while((b = bis.read(buffer, 0, BYTE_SIZE)) != -1) - { - bos.write(buffer, 0, b); - } - bos.flush(); - bos.close(); - bis.close(); - String name = destinationFilePath.getName(); - if(name.endsWith(".jar") && pluginFile(name)) - { - destinationFilePath.renameTo(new File("plugins/" + updateFolder + "/" + name)); - } - } - entry = null; - destinationFilePath = null; - } - e = null; - zipFile.close(); - zipFile = null; - // Move any plugin data folders that were included to the right place, Bukkit won't do this for us. - for(File dFile : new File(zipPath).listFiles()) - { - if(dFile.isDirectory()) - { - if(pluginFile(dFile.getName())) - { - File oFile = new File("plugins/" + dFile.getName()); // Get current dir - File [] contents = oFile.listFiles(); // List of existing files in the current dir - for(File cFile : dFile.listFiles()) // Loop through all the files in the new dir - { - boolean found = false; - for(File xFile : contents) // Loop through contents to see if it exists - { - if(xFile.getName().equals(cFile.getName())) - { - found = true; - break; - } - } - if(!found) - { - // Move the new file into the current dir - cFile.renameTo(new File(oFile.getCanonicalFile() + "/" + cFile.getName())); - } - else - { - // This file already exists, so we don't need it anymore. - cFile.delete(); - } - } - } - } - dFile.delete(); - } - new File(zipPath).delete(); - fSourceZip.delete(); - } - catch(IOException ex) - { - ex.printStackTrace(); - logger.warning("The auto-updater tried to unzip a new update file, but was unsuccessful."); - result = Updater.UpdateResult.FAIL_DOWNLOAD; - } - new File(file).delete(); - } - /** * Check if the name of a jar is one of the plugins currently installed, used for extracting the correct files out of a zip. */ From c2209138fee9dfc4365389398a1c3e0a2ed9d2ff Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Mon, 19 Nov 2012 20:43:13 +0100 Subject: [PATCH 05/31] Add the ability to clone a watchable object. --- .../wrappers/WrappedWatchableObject.java | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedWatchableObject.java b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedWatchableObject.java index 7323f273..d9862029 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedWatchableObject.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedWatchableObject.java @@ -1,8 +1,11 @@ package com.comphenix.protocol.wrappers; +import com.comphenix.protocol.reflect.EquivalentConverter; import com.comphenix.protocol.reflect.FieldAccessException; import com.comphenix.protocol.reflect.StructureModifier; +import com.comphenix.protocol.reflect.instances.DefaultInstances; +import net.minecraft.server.ItemStack; import net.minecraft.server.WatchableObject; /** @@ -157,4 +160,35 @@ public class WrappedWatchableObject { public boolean getDirtyState() throws FieldAccessException { return modifier.withType(boolean.class).read(0); } + + /** + * Clone the current wrapped watchable object, along with any contained objects. + * @return A deep clone of the current watchable object. + * @throws FieldAccessException If we're unable to use reflection. + */ + public WrappedWatchableObject deepClone() throws FieldAccessException { + WrappedWatchableObject clone = new WrappedWatchableObject(DefaultInstances.DEFAULT.getDefault(WatchableObject.class)); + + clone.setDirtyState(getDirtyState()); + clone.setIndex(getIndex()); + clone.setTypeID(getTypeID()); + clone.setValue(getClonedValue(), false); + return clone; + } + + // Helper + private Object getClonedValue() throws FieldAccessException { + Object value = getValue(); + + // Only a limited set of references types are supported + if (value instanceof net.minecraft.server.ChunkPosition) { + EquivalentConverter converter = ChunkPosition.getConverter(); + return converter.getGeneric(net.minecraft.server.ChunkPosition.class, converter.getSpecific(value)); + } else if (value instanceof ItemStack) { + return ((ItemStack) value).cloneItemStack(); + } else { + // A string or primitive wrapper, which are all immutable. + return value; + } + } } From cdc5740f85d362a194a135ae35945ba05fa06c0f Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Mon, 19 Nov 2012 23:27:04 +0100 Subject: [PATCH 06/31] Log the plugin version too. --- .../comphenix/protocol/ProtocolLibrary.java | 2 +- .../protocol/error/DetailedErrorReporter.java | 26 +++++++++++++++---- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java index b97b35f9..772066fb 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java @@ -90,7 +90,7 @@ public class ProtocolLibrary extends JavaPlugin { logger = getLoggerSafely(); // Add global parameters - DetailedErrorReporter reporter = new DetailedErrorReporter(); + DetailedErrorReporter reporter = new DetailedErrorReporter(this); updater = new Updater(this, logger, "protocollib", getFile(), "protocol.info"); try { diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/error/DetailedErrorReporter.java b/ProtocolLib/src/main/java/com/comphenix/protocol/error/DetailedErrorReporter.java index bceb7dca..7fd85123 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/error/DetailedErrorReporter.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/error/DetailedErrorReporter.java @@ -2,6 +2,7 @@ package com.comphenix.protocol.error; import java.io.PrintWriter; import java.io.StringWriter; +import java.lang.ref.WeakReference; import java.util.HashMap; import java.util.Map; import java.util.Set; @@ -40,6 +41,8 @@ public class DetailedErrorReporter implements ErrorReporter { protected int errorCount; protected int maxErrorCount; protected Logger logger; + + protected WeakReference pluginReference; // Whether or not Apache Commons is not present protected boolean apacheCommonsMissing; @@ -50,17 +53,18 @@ public class DetailedErrorReporter implements ErrorReporter { /** * Create a default error reporting system. */ - public DetailedErrorReporter() { - this(DEFAULT_PREFIX, DEFAULT_SUPPORT_URL); + public DetailedErrorReporter(Plugin plugin) { + this(plugin, DEFAULT_PREFIX, DEFAULT_SUPPORT_URL); } /** * Create a central error reporting system. + * @param plugin - the plugin owner. * @param prefix - default line prefix. * @param supportURL - URL to report the error. */ - public DetailedErrorReporter(String prefix, String supportURL) { - this(prefix, supportURL, DEFAULT_MAX_ERROR_COUNT, getBukkitLogger()); + public DetailedErrorReporter(Plugin plugin, String prefix, String supportURL) { + this(plugin, prefix, supportURL, DEFAULT_MAX_ERROR_COUNT, getBukkitLogger()); } // Attempt to get the logger. @@ -74,12 +78,17 @@ public class DetailedErrorReporter implements ErrorReporter { /** * Create a central error reporting system. + * @param plugin - the plugin owner. * @param prefix - default line prefix. * @param supportURL - URL to report the error. * @param maxErrorCount - number of errors to print before giving up. * @param logger - current logger. */ - public DetailedErrorReporter(String prefix, String supportURL, int maxErrorCount, Logger logger) { + public DetailedErrorReporter(Plugin plugin, String prefix, String supportURL, int maxErrorCount, Logger logger) { + if (plugin == null) + throw new IllegalArgumentException("Plugin cannot be NULL."); + + this.pluginReference = new WeakReference(plugin); this.prefix = prefix; this.supportURL = supportURL; this.maxErrorCount = maxErrorCount; @@ -157,6 +166,13 @@ public class DetailedErrorReporter implements ErrorReporter { writer.println("Sender:"); writer.println(addPrefix(getStringDescription(sender), SECOND_LEVEL_PREFIX)); + // And plugin + if (pluginReference.get() != null) { + Plugin plugin = pluginReference.get(); + writer.println("Version:"); + writer.println(addPrefix(plugin.toString() + "-" + plugin.getDescription().getVersion(), SECOND_LEVEL_PREFIX)); + } + // Add the server version too if (Bukkit.getServer() != null) { writer.println("Server:"); From d5aa1cde5169c2d9feff81d71c84e1b0d1a2c001 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Mon, 19 Nov 2012 23:44:13 +0100 Subject: [PATCH 07/31] Added the ability to enable or disable the background compiler. --- .../comphenix/protocol/ProtocolConfig.java | 21 +++++++++++++++++++ .../comphenix/protocol/ProtocolLibrary.java | 2 +- ProtocolLib/src/main/resources/config.yml | 5 ++++- 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolConfig.java b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolConfig.java index 078262c7..7e771e0b 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolConfig.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolConfig.java @@ -18,6 +18,8 @@ class ProtocolConfig { private static final String METRICS_ENABLED = "metrics"; + private static final String BACKGROUND_COMPILER_ENABLED = "background compiler"; + private static final String UPDATER_NOTIFY = "notify"; private static final String UPDATER_DOWNLAD = "download"; private static final String UPDATER_DELAY = "delay"; @@ -166,6 +168,25 @@ class ProtocolConfig { global.set(METRICS_ENABLED, enabled); } + /** + * Retrieve whether or not the background compiler for structure modifiers is enabled or not. + * @return TRUE if it is enabled, FALSE otherwise. + */ + public boolean isBackgroundCompilerEnabled() { + return global.getBoolean(BACKGROUND_COMPILER_ENABLED, true); + } + + /** + * Set whether or not the background compiler for structure modifiers is enabled or not. + *

+ * This setting will take effect next time ProtocolLib is started. + * + * @param enabled - TRUE if is enabled/running, FALSE otherwise. + */ + public void setBackgroundCompilerEnabled(boolean enabled) { + global.set(BACKGROUND_COMPILER_ENABLED, enabled); + } + /** * Set the last time we updated, in seconds since 1970.01.01 00:00. * @param lastTimeSeconds - new last update time. diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java index 772066fb..c62ae46b 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java @@ -166,7 +166,7 @@ public class ProtocolLibrary extends JavaPlugin { return; // Initialize background compiler - if (backgroundCompiler == null) { + if (backgroundCompiler == null && config.isBackgroundCompilerEnabled()) { backgroundCompiler = new BackgroundCompiler(getClassLoader()); BackgroundCompiler.setInstance(backgroundCompiler); } diff --git a/ProtocolLib/src/main/resources/config.yml b/ProtocolLib/src/main/resources/config.yml index 8bc3ca2d..15828ee1 100644 --- a/ProtocolLib/src/main/resources/config.yml +++ b/ProtocolLib/src/main/resources/config.yml @@ -9,4 +9,7 @@ global: # Last update time last: 0 - metrics: true \ No newline at end of file + metrics: true + + # Automatically compile structure modifiers + background compiler: true \ No newline at end of file From f8af92eb5b189eab579fdc149de9ef88e3d8969c Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Tue, 20 Nov 2012 00:26:16 +0100 Subject: [PATCH 08/31] Add the error reporter to the background compiler. --- .../comphenix/protocol/ProtocolLibrary.java | 21 ++++--- .../protocol/error/DetailedErrorReporter.java | 2 +- .../reflect/compiler/BackgroundCompiler.java | 56 ++++++++++++++++--- 3 files changed, 62 insertions(+), 17 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java index c62ae46b..2e1fe58c 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java @@ -90,13 +90,14 @@ public class ProtocolLibrary extends JavaPlugin { logger = getLoggerSafely(); // Add global parameters - DetailedErrorReporter reporter = new DetailedErrorReporter(this); + DetailedErrorReporter detailedReporter = new DetailedErrorReporter(this); updater = new Updater(this, logger, "protocollib", getFile(), "protocol.info"); + reporter = detailedReporter; try { config = new ProtocolConfig(this); } catch (Exception e) { - reporter.reportWarning(this, "Cannot load configuration", e); + detailedReporter.reportWarning(this, "Cannot load configuration", e); // Load it again deleteConfig(); @@ -105,18 +106,18 @@ public class ProtocolLibrary extends JavaPlugin { try { unhookTask = new DelayedSingleTask(this); - protocolManager = new PacketFilterManager(getClassLoader(), getServer(), unhookTask, reporter); - reporter.addGlobalParameter("manager", protocolManager); + protocolManager = new PacketFilterManager(getClassLoader(), getServer(), unhookTask, detailedReporter); + detailedReporter.addGlobalParameter("manager", protocolManager); // Initialize command handlers - commandProtocol = new CommandProtocol(reporter, this, updater, config); - commandPacket = new CommandPacket(reporter, this, logger, protocolManager); + commandProtocol = new CommandProtocol(detailedReporter, this, updater, config); + commandPacket = new CommandPacket(detailedReporter, this, logger, protocolManager); // Send logging information to player listeners too broadcastUsers(PERMISSION_INFO); } catch (Throwable e) { - reporter.reportDetailed(this, "Cannot load ProtocolLib.", e, protocolManager); + detailedReporter.reportDetailed(this, "Cannot load ProtocolLib.", e, protocolManager); disablePlugin(); } } @@ -167,8 +168,12 @@ public class ProtocolLibrary extends JavaPlugin { // Initialize background compiler if (backgroundCompiler == null && config.isBackgroundCompilerEnabled()) { - backgroundCompiler = new BackgroundCompiler(getClassLoader()); + backgroundCompiler = new BackgroundCompiler(getClassLoader(), reporter); BackgroundCompiler.setInstance(backgroundCompiler); + + logger.info("Started structure compiler thread."); + } else { + logger.info("Structure compiler thread has been disabled."); } // Set up command handlers diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/error/DetailedErrorReporter.java b/ProtocolLib/src/main/java/com/comphenix/protocol/error/DetailedErrorReporter.java index 7fd85123..3e9aafa6 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/error/DetailedErrorReporter.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/error/DetailedErrorReporter.java @@ -170,7 +170,7 @@ public class DetailedErrorReporter implements ErrorReporter { if (pluginReference.get() != null) { Plugin plugin = pluginReference.get(); writer.println("Version:"); - writer.println(addPrefix(plugin.toString() + "-" + plugin.getDescription().getVersion(), SECOND_LEVEL_PREFIX)); + writer.println(addPrefix(plugin.toString(), SECOND_LEVEL_PREFIX)); } // Add the server version too diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/BackgroundCompiler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/BackgroundCompiler.java index 647206a9..6f636c49 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/BackgroundCompiler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/BackgroundCompiler.java @@ -22,11 +22,16 @@ import java.util.concurrent.Callable; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.RejectedExecutionException; +import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; import java.util.logging.Level; import java.util.logging.Logger; +import javax.annotation.Nullable; + +import com.comphenix.protocol.error.ErrorReporter; import com.comphenix.protocol.reflect.StructureModifier; +import com.google.common.util.concurrent.ThreadFactoryBuilder; /** * Compiles structure modifiers on a background thread. @@ -37,6 +42,11 @@ import com.comphenix.protocol.reflect.StructureModifier; */ public class BackgroundCompiler { + /** + * The default format for the name of new worker threads. + */ + public static final String THREAD_FORMAT = "ProtocolLib-StructureCompiler %s"; + // How long to wait for a shutdown public static final int SHUTDOWN_DELAY_MS = 2000; @@ -48,6 +58,7 @@ public class BackgroundCompiler { private boolean shuttingDown; private ExecutorService executor; + private ErrorReporter reporter; /** * Retrieves the current background compiler. @@ -64,27 +75,41 @@ public class BackgroundCompiler { public static void setInstance(BackgroundCompiler backgroundCompiler) { BackgroundCompiler.backgroundCompiler = backgroundCompiler; } - + /** * Initialize a background compiler. + *

+ * Uses the default {@link #THREAD_FORMAT} to name worker threads. * @param loader - class loader from Bukkit. + * @param reporter - current error reporter. */ - public BackgroundCompiler(ClassLoader loader) { - this(loader, Executors.newSingleThreadExecutor()); + public BackgroundCompiler(ClassLoader loader, ErrorReporter reporter) { + ThreadFactory factory = new ThreadFactoryBuilder(). + setDaemon(true). + setNameFormat(THREAD_FORMAT). + build(); + initializeCompiler(loader, reporter, Executors.newSingleThreadExecutor(factory)); } - + /** * Initialize a background compiler utilizing the given thread pool. * @param loader - class loader from Bukkit. + * @param reporter - current error reporter. * @param executor - thread pool we'll use. */ - public BackgroundCompiler(ClassLoader loader, ExecutorService executor) { + public BackgroundCompiler(ClassLoader loader, ErrorReporter reporter, ExecutorService executor) { + initializeCompiler(loader, reporter, executor); + } + + // Avoid "Constructor call must be the first statement". + private void initializeCompiler(ClassLoader loader, @Nullable ErrorReporter reporter, ExecutorService executor) { if (loader == null) throw new IllegalArgumentException("loader cannot be NULL"); if (executor == null) throw new IllegalArgumentException("executor cannot be NULL"); this.compiler = new StructureCompiler(loader); + this.reporter = reporter; this.executor = executor; this.enabled = true; } @@ -129,15 +154,30 @@ public class BackgroundCompiler { executor.submit(new Callable() { @Override public Object call() throws Exception { - StructureModifier modifier = uncompiled; // Do our compilation - modifier = compiler.compile(modifier); - listener.onCompiled(modifier); + try { + modifier = compiler.compile(modifier); + listener.onCompiled(modifier); + + } catch (Throwable e) { + // Disable future compilations! + setEnabled(false); + + // Inform about this error as best as we can + if (reporter != null) { + reporter.reportDetailed(BackgroundCompiler.this, + "Cannot compile structure. Disabing compiler.", e, uncompiled); + } else { + System.err.println("Exception occured in structure compiler: "); + e.printStackTrace(); + } + } // We'll also return the new structure modifier return modifier; + } }); } catch (RejectedExecutionException e) { From 7b9d97123888b86c31ab135347ac8d4f0771c3ac Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Tue, 20 Nov 2012 03:23:43 +0100 Subject: [PATCH 09/31] Use a separate asynchronous sending queue for every online player. This ensures that packets intended for player A doesn't have to wait for the packets of player B to be finished processing. --- .../protocol/async/AsyncFilterManager.java | 105 ++++----- .../protocol/async/PacketProcessingQueue.java | 24 +- .../protocol/async/PacketSendingQueue.java | 2 +- .../protocol/async/PlayerSendingHandler.java | 217 ++++++++++++++++++ .../protocol/events/PacketEvent.java | 25 +- .../injector/PacketFilterManager.java | 119 ++++++---- 6 files changed, 376 insertions(+), 116 deletions(-) create mode 100644 ProtocolLib/src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java index 7dd7b504..44f49bba 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java @@ -23,6 +23,7 @@ import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicInteger; +import org.bukkit.entity.Player; import org.bukkit.plugin.Plugin; import org.bukkit.scheduler.BukkitScheduler; @@ -52,12 +53,12 @@ public class AsyncFilterManager implements AsynchronousManager { private Set timeoutListeners; private PacketProcessingQueue serverProcessingQueue; - private PacketSendingQueue serverQueue; - - private PacketProcessingQueue clientProcessingQueue; - private PacketSendingQueue clientQueue; + + // Sending queues + private PlayerSendingHandler playerSendingHandler; + // Report exceptions private ErrorReporter reporter; // The likely main thread @@ -72,38 +73,16 @@ public class AsyncFilterManager implements AsynchronousManager { // Current packet index private AtomicInteger currentSendingIndex = new AtomicInteger(); - // Whether or not we're currently cleaning up - private volatile boolean cleaningUp; - public AsyncFilterManager(ErrorReporter reporter, BukkitScheduler scheduler, ProtocolManager manager) { // Initialize timeout listeners serverTimeoutListeners = new SortedPacketListenerList(); clientTimeoutListeners = new SortedPacketListenerList(); timeoutListeners = Sets.newSetFromMap(new ConcurrentHashMap()); - - // Server packets are synchronized already - this.serverQueue = new PacketSendingQueue(false) { - @Override - protected void onPacketTimeout(PacketEvent event) { - if (!cleaningUp) { - serverTimeoutListeners.invokePacketSending(AsyncFilterManager.this.reporter, event); - } - } - }; - - // Client packets must be synchronized - this.clientQueue = new PacketSendingQueue(true) { - @Override - protected void onPacketTimeout(PacketEvent event) { - if (!cleaningUp) { - clientTimeoutListeners.invokePacketSending(AsyncFilterManager.this.reporter, event); - } - } - }; - - this.serverProcessingQueue = new PacketProcessingQueue(serverQueue); - this.clientProcessingQueue = new PacketProcessingQueue(clientQueue); + + this.playerSendingHandler = new PlayerSendingHandler(reporter, serverTimeoutListeners, clientTimeoutListeners); + this.serverProcessingQueue = new PacketProcessingQueue(playerSendingHandler); + this.clientProcessingQueue = new PacketProcessingQueue(playerSendingHandler); this.scheduler = scheduler; this.manager = manager; @@ -219,15 +198,12 @@ public class AsyncFilterManager implements AsynchronousManager { List removed = serverProcessingQueue.removeListener(handler, listener.getSendingWhitelist()); // We're already taking care of this, so don't do anything - if (!cleaningUp) - serverQueue.signalPacketUpdate(removed, synchronusOK); + playerSendingHandler.sendServerPackets(removed, synchronusOK); } if (hasValidWhitelist(listener.getReceivingWhitelist())) { List removed = clientProcessingQueue.removeListener(handler, listener.getReceivingWhitelist()); - - if (!cleaningUp) - clientQueue.signalPacketUpdate(removed, synchronusOK); + playerSendingHandler.sendClientPackets(removed, synchronusOK); } } @@ -337,15 +313,14 @@ public class AsyncFilterManager implements AsynchronousManager { @Override public void cleanupAll() { - cleaningUp = true; serverProcessingQueue.cleanupAll(); - serverQueue.cleanupAll(); - + playerSendingHandler.cleanupAll(); timeoutListeners.clear(); + serverTimeoutListeners = null; clientTimeoutListeners = null; } - + @Override public void signalPacketTransmission(PacketEvent packet) { signalPacketTransmission(packet, onMainThread()); @@ -366,8 +341,12 @@ public class AsyncFilterManager implements AsynchronousManager { "A packet must have been queued before it can be transmitted."); // Only send if the packet is ready - if (marker.decrementProcessingDelay() == 0) { - getSendingQueue(packet).signalPacketUpdate(packet, onMainThread); + if (marker.decrementProcessingDelay() == 0) { + PacketSendingQueue queue = getSendingQueue(packet, false); + + // No need to create a new queue if the player has logged out + if (queue != null) + queue.signalPacketUpdate(packet, onMainThread); } } @@ -376,8 +355,27 @@ public class AsyncFilterManager implements AsynchronousManager { * @param packet - the packet. * @return The server or client sending queue the packet belongs to. */ - private PacketSendingQueue getSendingQueue(PacketEvent packet) { - return packet.isServerPacket() ? serverQueue : clientQueue; + public PacketSendingQueue getSendingQueue(PacketEvent packet) { + return playerSendingHandler.getSendingQueue(packet); + } + + /** + * Retrieve the sending queue this packet belongs to. + * @param packet - the packet. + * @param createNew - if TRUE, create a new queue if it hasn't already been created. + * @return The server or client sending queue the packet belongs to. + */ + public PacketSendingQueue getSendingQueue(PacketEvent packet, boolean createNew) { + return playerSendingHandler.getSendingQueue(packet, createNew); + } + + /** + * Retrieve the processing queue this packet belongs to. + * @param packet - the packet. + * @return The server or client sending processing the packet belongs to. + */ + public PacketProcessingQueue getProcessingQueue(PacketEvent packet) { + return packet.isServerPacket() ? serverProcessingQueue : clientProcessingQueue; } /** @@ -388,24 +386,23 @@ public class AsyncFilterManager implements AsynchronousManager { getProcessingQueue(packet).signalProcessingDone(); } - /** - * Retrieve the processing queue this packet belongs to. - * @param packet - the packet. - * @return The server or client sending processing the packet belongs to. - */ - private PacketProcessingQueue getProcessingQueue(PacketEvent packet) { - return packet.isServerPacket() ? serverProcessingQueue : clientProcessingQueue; - } - /** * Send any due packets, or clean up packets that have expired. */ public void sendProcessedPackets(int tickCounter, boolean onMainThread) { // The server queue is unlikely to need checking that often if (tickCounter % 10 == 0) { - serverQueue.trySendPackets(onMainThread); + playerSendingHandler.trySendServerPackets(onMainThread); } + + playerSendingHandler.trySendClientPackets(onMainThread); + } - clientQueue.trySendPackets(onMainThread); + /** + * Clean up after a given player has logged out. + * @param player - the player that has just logged out. + */ + public void removePlayer(Player player) { + playerSendingHandler.removePlayer(player); } } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketProcessingQueue.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketProcessingQueue.java index 23defbf2..364b6c1f 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketProcessingQueue.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketProcessingQueue.java @@ -58,13 +58,13 @@ class PacketProcessingQueue extends AbstractConcurrentListenerMultimap processingQueue; // Packets for sending - private PacketSendingQueue sendingQueue; - - public PacketProcessingQueue(PacketSendingQueue sendingQueue) { - this(sendingQueue, INITIAL_CAPACITY, DEFAULT_QUEUE_LIMIT, DEFAULT_MAXIMUM_CONCURRENCY); + private PlayerSendingHandler sendingHandler; + + public PacketProcessingQueue(PlayerSendingHandler sendingHandler) { + this(sendingHandler, INITIAL_CAPACITY, DEFAULT_QUEUE_LIMIT, DEFAULT_MAXIMUM_CONCURRENCY); } - public PacketProcessingQueue(PacketSendingQueue sendingQueue, int initialSize, int maximumSize, int maximumConcurrency) { + public PacketProcessingQueue(PlayerSendingHandler sendingHandler, int initialSize, int maximumSize, int maximumConcurrency) { super(); this.processingQueue = Synchronization.queue(MinMaxPriorityQueue. @@ -74,7 +74,7 @@ class PacketProcessingQueue extends AbstractConcurrentListenerMultimap sendingQueue; diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java new file mode 100644 index 00000000..2854814a --- /dev/null +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java @@ -0,0 +1,217 @@ +package com.comphenix.protocol.async; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.ConcurrentHashMap; + +import org.bukkit.entity.Player; + +import com.comphenix.protocol.error.ErrorReporter; +import com.comphenix.protocol.events.PacketEvent; +import com.comphenix.protocol.injector.SortedPacketListenerList; + +/** + * Contains every sending queue for every player. + * + * @author Kristian + */ +class PlayerSendingHandler { + + private ErrorReporter reporter; + private ConcurrentHashMap playerSendingQueues; + + // Timeout listeners + private SortedPacketListenerList serverTimeoutListeners; + private SortedPacketListenerList clientTimeoutListeners; + + // Whether or not we're currently cleaning up + private volatile boolean cleaningUp; + + /** + * Sending queues for a given player. + * + * @author Kristian + */ + private class QueueContainer { + private PacketSendingQueue serverQueue; + private PacketSendingQueue clientQueue; + + public QueueContainer() { + // Server packets are synchronized already + serverQueue = new PacketSendingQueue(false) { + @Override + protected void onPacketTimeout(PacketEvent event) { + if (!cleaningUp) { + serverTimeoutListeners.invokePacketSending(reporter, event); + } + } + }; + + // Client packets must be synchronized + clientQueue = new PacketSendingQueue(true) { + @Override + protected void onPacketTimeout(PacketEvent event) { + if (!cleaningUp) { + clientTimeoutListeners.invokePacketSending(reporter, event); + } + } + }; + } + + public PacketSendingQueue getServerQueue() { + return serverQueue; + } + + public PacketSendingQueue getClientQueue() { + return clientQueue; + } + } + + public PlayerSendingHandler(ErrorReporter reporter, + SortedPacketListenerList serverTimeoutListeners, SortedPacketListenerList clientTimeoutListeners) { + + this.reporter = reporter; + this.serverTimeoutListeners = serverTimeoutListeners; + this.clientTimeoutListeners = clientTimeoutListeners; + + // Initialize storage of queues + playerSendingQueues = new ConcurrentHashMap(); + } + + /** + * Retrieve the sending queue this packet belongs to. + * @param packet - the packet. + * @return The server or client sending queue the packet belongs to. + */ + public PacketSendingQueue getSendingQueue(PacketEvent packet) { + return getSendingQueue(packet, true); + } + + /** + * Retrieve the sending queue this packet belongs to. + * @param packet - the packet. + * @param createNew - if TRUE, create a new queue if it hasn't already been created. + * @return The server or client sending queue the packet belongs to. + */ + public PacketSendingQueue getSendingQueue(PacketEvent packet, boolean createNew) { + String name = packet.getPlayer().getName(); + QueueContainer queues = playerSendingQueues.get(name); + + // Safe concurrent initialization + if (queues == null && createNew) { + QueueContainer previous = playerSendingQueues.putIfAbsent(name, new QueueContainer()); + + if (previous != null) + queues = previous; + } + + // Check for NULL again + if (queues != null) + return packet.isServerPacket() ? queues.getServerQueue() : queues.getClientQueue(); + else + return null; + } + + /** + * Send all pending packets. + */ + public void sendAllPackets() { + if (!cleaningUp) { + for (QueueContainer queues : playerSendingQueues.values()) { + queues.getClientQueue().cleanupAll(); + queues.getServerQueue().cleanupAll(); + } + } + } + + /** + * Immediately send every server packet with the given list of IDs. + * @param ids - ID of every packet to send immediately. + * @param synchronusOK - whether or not we're running on the main thread. + */ + public void sendServerPackets(List ids, boolean synchronusOK) { + if (!cleaningUp) { + for (QueueContainer queue : playerSendingQueues.values()) { + queue.getServerQueue().signalPacketUpdate(ids, synchronusOK); + } + } + } + + /** + * Immediately send every client packet with the given list of IDs. + * @param ids - ID of every packet to send immediately. + * @param synchronusOK - whether or not we're running on the main thread. + */ + public void sendClientPackets(List ids, boolean synchronusOK) { + if (!cleaningUp) { + for (QueueContainer queue : playerSendingQueues.values()) { + queue.getClientQueue().signalPacketUpdate(ids, synchronusOK); + } + } + } + + /** + * Send any outstanding server packets. + * @param onMainThread - whether or not this is occuring on the main thread. + */ + public void trySendServerPackets(boolean onMainThread) { + for (QueueContainer queue : playerSendingQueues.values()) { + queue.getServerQueue().trySendPackets(onMainThread); + } + } + + /** + * Send any outstanding server packets. + * @param onMainThread - whether or not this is occuring on the main thread. + */ + public void trySendClientPackets(boolean onMainThread) { + for (QueueContainer queue : playerSendingQueues.values()) { + queue.getClientQueue().trySendPackets(onMainThread); + } + } + + /** + * Retrieve every server packet queue for every player. + * @return Every sever packet queue. + */ + public List getServerQueues() { + List result = new ArrayList(); + + for (QueueContainer queue : playerSendingQueues.values()) + result.add(queue.getServerQueue()); + return result; + } + + /** + * Retrieve every client packet queue for every player. + * @return Every client packet queue. + */ + public List getClientQueues() { + List result = new ArrayList(); + + for (QueueContainer queue : playerSendingQueues.values()) + result.add(queue.getClientQueue()); + return result; + } + + /** + * Send all pending packets and clean up queues. + */ + public void cleanupAll() { + cleaningUp = true; + + sendAllPackets(); + playerSendingQueues.clear(); + } + + /** + * Invoked when a player has just logged out. + * @param player - the player that just logged out. + */ + public void removePlayer(Player player) { + String name = player.getName(); + + // Every packet will be dropped - there's nothing we can do + playerSendingQueues.remove(name); + } +} diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/events/PacketEvent.java b/ProtocolLib/src/main/java/com/comphenix/protocol/events/PacketEvent.java index c0580967..61062772 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/events/PacketEvent.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/events/PacketEvent.java @@ -20,6 +20,7 @@ package com.comphenix.protocol.events; import java.io.IOException; import java.io.ObjectInputStream; import java.io.ObjectOutputStream; +import java.lang.ref.WeakReference; import java.util.EventObject; import org.bukkit.entity.Player; @@ -32,8 +33,10 @@ public class PacketEvent extends EventObject implements Cancellable { * Automatically generated by Eclipse. */ private static final long serialVersionUID = -5360289379097430620L; - - private transient Player player; + + private transient WeakReference playerReference; + private transient Player offlinePlayer; + private PacketContainer packet; private boolean serverPacket; private boolean cancel; @@ -52,14 +55,14 @@ public class PacketEvent extends EventObject implements Cancellable { private PacketEvent(Object source, PacketContainer packet, Player player, boolean serverPacket) { super(source); this.packet = packet; - this.player = player; + this.playerReference = new WeakReference(player); this.serverPacket = serverPacket; } private PacketEvent(PacketEvent origial, AsyncMarker asyncMarker) { super(origial.source); this.packet = origial.packet; - this.player = origial.player; + this.playerReference = origial.playerReference; this.cancel = origial.cancel; this.serverPacket = origial.serverPacket; this.asyncMarker = asyncMarker; @@ -143,7 +146,7 @@ public class PacketEvent extends EventObject implements Cancellable { * @return The player associated with this event. */ public Player getPlayer() { - return player; + return playerReference.get(); } /** @@ -197,18 +200,20 @@ public class PacketEvent extends EventObject implements Cancellable { output.defaultWriteObject(); // Write the name of the player (or NULL if it's not set) - output.writeObject(player != null ? new SerializedOfflinePlayer(player) : null); + output.writeObject(playerReference.get() != null ? new SerializedOfflinePlayer(playerReference.get()) : null); } private void readObject(ObjectInputStream input) throws ClassNotFoundException, IOException { // Default deserialization input.defaultReadObject(); - final SerializedOfflinePlayer offlinePlayer = (SerializedOfflinePlayer) input.readObject(); + final SerializedOfflinePlayer serialized = (SerializedOfflinePlayer) input.readObject(); - if (offlinePlayer != null) { - // Better than nothing - player = offlinePlayer.getPlayer(); + // Better than nothing + if (serialized != null) { + // Store it, to prevent weak reference from cleaning up the reference + offlinePlayer = serialized.getPlayer(); + playerReference = new WeakReference(offlinePlayer); } } } 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 988e773b..a8706e90 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java @@ -598,46 +598,21 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok try { manager.registerEvents(new Listener() { - @EventHandler(priority = EventPriority.LOWEST, ignoreCancelled = true) + @EventHandler(priority = EventPriority.LOWEST) public void onPrePlayerJoin(PlayerJoinEvent event) { - try { - // Let's clean up the other injection first. - playerInjection.uninjectPlayer(event.getPlayer().getAddress()); - } catch (Exception e) { - reporter.reportDetailed(PacketFilterManager.this, "Unable to uninject net handler for player.", e, event); - } + PacketFilterManager.this.onPrePlayerJoin(event); } - - @EventHandler(priority = EventPriority.MONITOR, ignoreCancelled = true) + @EventHandler(priority = EventPriority.MONITOR) public void onPlayerJoin(PlayerJoinEvent event) { - try { - // This call will be ignored if no listeners are registered - playerInjection.injectPlayer(event.getPlayer()); - } catch (Exception e) { - reporter.reportDetailed(PacketFilterManager.this, "Unable to inject player.", e, event); - } + PacketFilterManager.this.onPlayerJoin(event); } - - @EventHandler(priority = EventPriority.MONITOR, ignoreCancelled = true) + @EventHandler(priority = EventPriority.MONITOR) public void onPlayerQuit(PlayerQuitEvent event) { - try { - playerInjection.handleDisconnect(event.getPlayer()); - playerInjection.uninjectPlayer(event.getPlayer()); - } catch (Exception e) { - reporter.reportDetailed(PacketFilterManager.this, "Unable to uninject logged off player.", e, event); - } + PacketFilterManager.this.onPlayerQuit(event); } - - @EventHandler(priority = EventPriority.MONITOR, ignoreCancelled = true) + @EventHandler(priority = EventPriority.MONITOR) public void onPluginDisabled(PluginDisableEvent event) { - try { - // Clean up in case the plugin forgets - if (event.getPlugin() != plugin) { - removePacketListeners(event.getPlugin()); - } - } catch (Exception e) { - reporter.reportDetailed(PacketFilterManager.this, "Unable handle disabled plugin.", e, event); - } + PacketFilterManager.this.onPluginDisabled(event, plugin); } }, plugin); @@ -648,6 +623,47 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok } } + private void onPrePlayerJoin(PlayerJoinEvent event) { + try { + // Let's clean up the other injection first. + playerInjection.uninjectPlayer(event.getPlayer().getAddress()); + } catch (Exception e) { + reporter.reportDetailed(PacketFilterManager.this, "Unable to uninject net handler for player.", e, event); + } + } + + private void onPlayerJoin(PlayerJoinEvent event) { + try { + // This call will be ignored if no listeners are registered + playerInjection.injectPlayer(event.getPlayer()); + } catch (Exception e) { + reporter.reportDetailed(PacketFilterManager.this, "Unable to inject player.", e, event); + } + } + + private void onPlayerQuit(PlayerQuitEvent event) { + try { + Player player = event.getPlayer(); + + asyncFilterManager.removePlayer(player); + playerInjection.handleDisconnect(player); + playerInjection.uninjectPlayer(player); + } catch (Exception e) { + reporter.reportDetailed(PacketFilterManager.this, "Unable to uninject logged off player.", e, event); + } + } + + private void onPluginDisabled(PluginDisableEvent event, Plugin protocolLibrary) { + try { + // Clean up in case the plugin forgets + if (event.getPlugin() != protocolLibrary) { + removePacketListeners(event.getPlugin()); + } + } catch (Exception e) { + reporter.reportDetailed(PacketFilterManager.this, "Unable handle disabled plugin.", e, event); + } + } + /** * Retrieve the number of listeners that expect packets during playing. * @return Number of listeners. @@ -689,7 +705,7 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok // Yes, this is crazy. @SuppressWarnings({ "unchecked", "rawtypes" }) - private void registerOld(PluginManager manager, Plugin plugin) { + private void registerOld(PluginManager manager, final Plugin plugin) { try { ClassLoader loader = manager.getClass().getClassLoader(); @@ -699,6 +715,7 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok Class eventPriority = loader.loadClass("org.bukkit.event.Event$Priority"); // Get the priority + Object priorityLowest = Enum.valueOf(eventPriority, "Lowest"); Object priorityMonitor = Enum.valueOf(eventPriority, "Monitor"); // Get event types @@ -714,26 +731,40 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok Method registerEvent = FuzzyReflection.fromObject(manager).getMethodByParameters("registerEvent", eventTypes, Listener.class, eventPriority, Plugin.class); + Enhancer playerLow = new Enhancer(); Enhancer playerEx = new Enhancer(); Enhancer serverEx = new Enhancer(); - playerEx.setSuperclass(playerListener); - playerEx.setClassLoader(classLoader); - playerEx.setCallback(new MethodInterceptor() { + playerLow.setSuperclass(playerListener); + playerLow.setClassLoader(classLoader); + playerLow.setCallback(new MethodInterceptor() { @Override public Object intercept(Object obj, Method method, Object[] args, MethodProxy proxy) throws Throwable { // Must have a parameter + if (args.length == 1) { + Object event = args[0]; + + if (event instanceof PlayerJoinEvent) { + onPrePlayerJoin((PlayerJoinEvent) event); + } + } + return null; + } + }); + + playerEx.setSuperclass(playerListener); + playerEx.setClassLoader(classLoader); + playerEx.setCallback(new MethodInterceptor() { + @Override + public Object intercept(Object obj, Method method, Object[] args, MethodProxy proxy) throws Throwable { if (args.length == 1) { Object event = args[0]; // Check for the correct event if (event instanceof PlayerJoinEvent) { - Player player = ((PlayerJoinEvent) event).getPlayer(); - playerInjection.injectPlayer(player); + onPlayerJoin((PlayerJoinEvent) event); } else if (event instanceof PlayerQuitEvent) { - Player player = ((PlayerQuitEvent) event).getPlayer(); - playerInjection.handleDisconnect(player); - playerInjection.uninjectPlayer(player); + onPlayerQuit((PlayerQuitEvent) event); } } return null; @@ -751,16 +782,18 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok Object event = args[0]; if (event instanceof PluginDisableEvent) - removePacketListeners(((PluginDisableEvent) event).getPlugin()); + onPluginDisabled((PluginDisableEvent) event, plugin); } return null; } }); // Create our listener + Object playerProxyLow = playerLow.create(); Object playerProxy = playerEx.create(); Object serverProxy = serverEx.create(); + registerEvent.invoke(manager, playerJoinType, playerProxyLow, priorityLowest, plugin); registerEvent.invoke(manager, playerJoinType, playerProxy, priorityMonitor, plugin); registerEvent.invoke(manager, playerQuitType, playerProxy, priorityMonitor, plugin); registerEvent.invoke(manager, pluginDisabledType, serverProxy, priorityMonitor, plugin); From 9170e48992b592a61c992623347e77377cf7d4eb Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Tue, 20 Nov 2012 03:32:44 +0100 Subject: [PATCH 10/31] Correct the concurrent initialization pattern. --- .../comphenix/protocol/async/PlayerSendingHandler.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java index 2854814a..cb234d88 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java @@ -99,10 +99,14 @@ class PlayerSendingHandler { // Safe concurrent initialization if (queues == null && createNew) { - QueueContainer previous = playerSendingQueues.putIfAbsent(name, new QueueContainer()); + final QueueContainer newContainer = new QueueContainer(); + + // Attempt to map the queue + queues = playerSendingQueues.putIfAbsent(name, newContainer); - if (previous != null) - queues = previous; + if (queues == null) { + queues = newContainer; + } } // Check for NULL again From 456764468add43c298a66ec8fdc12603f4dfeebd Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Tue, 20 Nov 2012 03:50:44 +0100 Subject: [PATCH 11/31] Always generate classes with legal identifiers. Certain types, such as ItemStack[], would cause the StructureCompiler to generate classes with the name CompiledStructure@ParentItemStack[], which are not legal names. Instead, we'll replace the brackets with the word Array. In addition, to accomodate classes with identical names, we'll use the following naming convention instead: CompiledStructure$[Canonical name of target]$Canonical name of type], where the canonical name (net.minecraft.server.ItemStack[]) is transformed to a legal name by replacing "." to "_" and "[]" to array. In our example, that would result in the following class name: net_minecraft_server_ItemStackArray --- .../reflect/compiler/StructureCompiler.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java index 266bb45e..87175da6 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java @@ -186,6 +186,15 @@ public final class StructureCompiler { } } + /** + * Retrieve a variable identifier that can uniquely represent the given type. + * @param type - a type. + * @return A unique and legal identifier for the given type. + */ + private String getSafeTypeName(Class type) { + return type.getCanonicalName().replace("[]", "Array").replace(".", "_"); + } + private Class generateClass(StructureModifier source) { ClassWriter cw = new ClassWriter(0); @@ -193,7 +202,9 @@ public final class StructureCompiler { @SuppressWarnings("rawtypes") Class targetType = source.getTargetType(); - String className = "CompiledStructure$" + targetType.getSimpleName() + source.getFieldType().getSimpleName(); + String className = "CompiledStructure$" + + getSafeTypeName(targetType) + "$" + + getSafeTypeName(source.getFieldType()); String targetSignature = Type.getDescriptor(targetType); String targetName = targetType.getName().replace('.', '/'); From ac993896cc84952c81897358287fd56e30454944 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Tue, 20 Nov 2012 06:17:52 +0100 Subject: [PATCH 12/31] Fixed writing private fields with a compiled structure modifier. Incredibly hard to track down. Lucked out by randomly removing a semicolon. --- .../compiler/CompiledStructureModifier.java | 25 ++++++------- .../reflect/compiler/StructureCompiler.java | 35 +++++++++++-------- 2 files changed, 30 insertions(+), 30 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/CompiledStructureModifier.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/CompiledStructureModifier.java index 45bca0f9..0698892c 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/CompiledStructureModifier.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/CompiledStructureModifier.java @@ -30,9 +30,8 @@ import com.google.common.collect.Sets; * Represents a compiled structure modifier. * * @author Kristian - * @param Field type. */ -public abstract class CompiledStructureModifier extends StructureModifier { +public abstract class CompiledStructureModifier extends StructureModifier { // Used to compile instances of structure modifiers protected StructureCompiler compiler; @@ -64,9 +63,8 @@ public abstract class CompiledStructureModifier extends StructureModifie } // Speed up the default writer - @SuppressWarnings("unchecked") @Override - public StructureModifier writeDefaults() throws FieldAccessException { + public StructureModifier writeDefaults() throws FieldAccessException { DefaultInstances generator = DefaultInstances.DEFAULT; @@ -75,21 +73,20 @@ public abstract class CompiledStructureModifier extends StructureModifie Integer index = entry.getValue(); Field field = entry.getKey(); - write(index, (TField) generator.getDefault(field.getType())); + write(index, (Object) generator.getDefault(field.getType())); } return this; } - @SuppressWarnings("unchecked") @Override - public final TField read(int fieldIndex) throws FieldAccessException { + public final Object read(int fieldIndex) throws FieldAccessException { Object result = readGenerated(fieldIndex); if (converter != null) return converter.getSpecific(result); else - return (TField) result; + return result; } /** @@ -104,11 +101,10 @@ public abstract class CompiledStructureModifier extends StructureModifie protected abstract Object readGenerated(int fieldIndex) throws FieldAccessException; - @SuppressWarnings("unchecked") @Override - public StructureModifier write(int index, Object value) throws FieldAccessException { + public StructureModifier write(int index, Object value) throws FieldAccessException { if (converter != null) - value = converter.getGeneric(getFieldType(index), (TField) value); + value = converter.getGeneric(getFieldType(index), value); return writeGenerated(index, value); } @@ -118,15 +114,14 @@ public abstract class CompiledStructureModifier extends StructureModifie * @param value - new value. * @throws FieldAccessException The field doesn't exist, or it cannot be accessed under the current security contraints. */ - @SuppressWarnings("unchecked") protected void writeReflected(int index, Object value) throws FieldAccessException { - super.write(index, (TField) value); + super.write(index, value); } - protected abstract StructureModifier writeGenerated(int index, Object value) throws FieldAccessException; + protected abstract StructureModifier writeGenerated(int index, Object value) throws FieldAccessException; @Override - public StructureModifier withTarget(Object target) { + public StructureModifier withTarget(Object target) { if (compiler != null) return compiler.compile(super.withTarget(target)); else diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java index 87175da6..ac49f45b 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java @@ -138,7 +138,7 @@ public final class StructureCompiler { * Construct a structure compiler. * @param loader - main class loader. */ - StructureCompiler(ClassLoader loader) { + public StructureCompiler(ClassLoader loader) { this.loader = loader; } @@ -219,15 +219,14 @@ public final class StructureCompiler { } cw.visit(Opcodes.V1_6, Opcodes.ACC_PUBLIC + Opcodes.ACC_SUPER, PACKAGE_NAME + "/" + className, - "L" + COMPILED_CLASS + ";", - COMPILED_CLASS, null); + null, COMPILED_CLASS, null); createFields(cw, targetSignature); createConstructor(cw, className, targetSignature, targetName); createReadMethod(cw, className, source.getFields(), targetSignature, targetName); createWriteMethod(cw, className, source.getFields(), targetSignature, targetName); cw.visitEnd(); - + byte[] data = cw.toByteArray(); // Call the define method @@ -295,14 +294,16 @@ public final class StructureCompiler { private void createWriteMethod(ClassWriter cw, String className, List fields, String targetSignature, String targetName) { String methodDescriptor = "(ILjava/lang/Object;)L" + SUPER_CLASS + ";"; - String methodSignature = "(ITTField;)L" + SUPER_CLASS + ";"; + String methodSignature = "(ILjava/lang/Object;)L" + SUPER_CLASS + ";"; MethodVisitor mv = cw.visitMethod(Opcodes.ACC_PROTECTED, "writeGenerated", methodDescriptor, methodSignature, new String[] { FIELD_EXCEPTION_CLASS }); BoxingHelper boxingHelper = new BoxingHelper(mv); + String generatedClassName = PACKAGE_NAME + "/" + className; + mv.visitCode(); mv.visitVarInsn(Opcodes.ALOAD, 0); - mv.visitFieldInsn(Opcodes.GETFIELD, PACKAGE_NAME + "/" + className, "typedTarget", targetSignature); + mv.visitFieldInsn(Opcodes.GETFIELD, generatedClassName, "typedTarget", targetSignature); mv.visitVarInsn(Opcodes.ASTORE, 3); mv.visitVarInsn(Opcodes.ILOAD, 1); @@ -351,7 +352,7 @@ public final class StructureCompiler { mv.visitVarInsn(Opcodes.ALOAD, 0); mv.visitVarInsn(Opcodes.ILOAD, 1); mv.visitVarInsn(Opcodes.ALOAD, 2); - mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL, COMPILED_CLASS, "writeReflected", "(ILjava/lang/Object;)V;"); + mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL, generatedClassName, "writeReflected", "(ILjava/lang/Object;)V"); } mv.visitJumpInsn(Opcodes.GOTO, returnLabel); @@ -384,9 +385,11 @@ public final class StructureCompiler { new String[] { "com/comphenix/protocol/reflect/FieldAccessException" }); BoxingHelper boxingHelper = new BoxingHelper(mv); + String generatedClassName = PACKAGE_NAME + "/" + className; + mv.visitCode(); mv.visitVarInsn(Opcodes.ALOAD, 0); - mv.visitFieldInsn(Opcodes.GETFIELD, PACKAGE_NAME + "/" + className, "typedTarget", targetSignature); + mv.visitFieldInsn(Opcodes.GETFIELD, generatedClassName, "typedTarget", targetSignature); mv.visitVarInsn(Opcodes.ASTORE, 2); mv.visitVarInsn(Opcodes.ILOAD, 1); @@ -425,7 +428,7 @@ public final class StructureCompiler { // We have to use reflection for private and protected fields. mv.visitVarInsn(Opcodes.ALOAD, 0); mv.visitVarInsn(Opcodes.ILOAD, 1); - mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL, COMPILED_CLASS, "readReflected", "(I)Ljava/lang/Object;"); + mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL, generatedClassName, "readReflected", "(I)Ljava/lang/Object;"); } mv.visitInsn(Opcodes.ARETURN); @@ -451,25 +454,27 @@ public final class StructureCompiler { private void createConstructor(ClassWriter cw, String className, String targetSignature, String targetName) { MethodVisitor mv = cw.visitMethod(Opcodes.ACC_PUBLIC, "", "(L" + SUPER_CLASS + ";L" + PACKAGE_NAME + "/StructureCompiler;)V", - "(L" + SUPER_CLASS + ";L" + SUPER_CLASS + ";)V", null); + "(L" + SUPER_CLASS + ";L" + PACKAGE_NAME + "/StructureCompiler;)V", null); + String fullClassName = PACKAGE_NAME + "/" + className; + mv.visitCode(); mv.visitVarInsn(Opcodes.ALOAD, 0); mv.visitMethodInsn(Opcodes.INVOKESPECIAL, COMPILED_CLASS, "", "()V"); mv.visitVarInsn(Opcodes.ALOAD, 0); mv.visitVarInsn(Opcodes.ALOAD, 1); - mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL, PACKAGE_NAME + "/" + className, "initialize", "(L" + SUPER_CLASS + ";)V"); + mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL, fullClassName, "initialize", "(L" + SUPER_CLASS + ";)V"); mv.visitVarInsn(Opcodes.ALOAD, 0); mv.visitVarInsn(Opcodes.ALOAD, 1); mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL, SUPER_CLASS, "getTarget", "()Ljava/lang/Object;"); - mv.visitFieldInsn(Opcodes.PUTFIELD, PACKAGE_NAME + "/" + className, "target", "Ljava/lang/Object;"); + mv.visitFieldInsn(Opcodes.PUTFIELD, fullClassName, "target", "Ljava/lang/Object;"); mv.visitVarInsn(Opcodes.ALOAD, 0); mv.visitVarInsn(Opcodes.ALOAD, 0); - mv.visitFieldInsn(Opcodes.GETFIELD, PACKAGE_NAME + "/" + className, "target", "Ljava/lang/Object;"); + mv.visitFieldInsn(Opcodes.GETFIELD, fullClassName, "target", "Ljava/lang/Object;"); mv.visitTypeInsn(Opcodes.CHECKCAST, targetName); - mv.visitFieldInsn(Opcodes.PUTFIELD, PACKAGE_NAME + "/" + className, "typedTarget", targetSignature); + mv.visitFieldInsn(Opcodes.PUTFIELD, fullClassName, "typedTarget", targetSignature); mv.visitVarInsn(Opcodes.ALOAD, 0); mv.visitVarInsn(Opcodes.ALOAD, 2); - mv.visitFieldInsn(Opcodes.PUTFIELD, PACKAGE_NAME + "/" + className, "compiler", "L" + PACKAGE_NAME + "/StructureCompiler;"); + mv.visitFieldInsn(Opcodes.PUTFIELD, fullClassName, "compiler", "L" + PACKAGE_NAME + "/StructureCompiler;"); mv.visitInsn(Opcodes.RETURN); mv.visitMaxs(2, 3); mv.visitEnd(); From 023c3908ae2d11fb879bd4dd56eb695d68001487 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Tue, 20 Nov 2012 06:33:54 +0100 Subject: [PATCH 13/31] Fixed error reporter not being assigned properly. --- .../src/main/java/com/comphenix/protocol/CommandBase.java | 3 ++- .../src/main/java/com/comphenix/protocol/CommandPacket.java | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/CommandBase.java b/ProtocolLib/src/main/java/com/comphenix/protocol/CommandBase.java index 3128e6c5..7cda5346 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/CommandBase.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/CommandBase.java @@ -19,7 +19,8 @@ abstract class CommandBase implements CommandExecutor { private String permission; private String name; private int minimumArgumentCount; - private ErrorReporter reporter; + + protected ErrorReporter reporter; public CommandBase(ErrorReporter reporter, String permission, String name) { this(reporter, permission, name, 0); diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/CommandPacket.java b/ProtocolLib/src/main/java/com/comphenix/protocol/CommandPacket.java index dbe85114..d609bf4e 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/CommandPacket.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/CommandPacket.java @@ -65,7 +65,6 @@ class CommandPacket extends CommandBase { private Plugin plugin; private Logger logger; - private ErrorReporter reporter; private ProtocolManager manager; private ChatExtensions chatter; From 95dbddf9bb2208ecff2c6b0689902b0d12172f94 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Tue, 20 Nov 2012 06:46:21 +0100 Subject: [PATCH 14/31] Fixed a number of minor bugs. --- .../comphenix/protocol/async/AsyncMarker.java | 4 +-- .../protocol/error/DetailedErrorReporter.java | 5 ++-- .../player/NetworkServerInjector.java | 4 +-- .../player/PlayerInjectionHandler.java | 5 ++-- .../protocol/reflect/StructureModifier.java | 4 +-- .../protocol/wrappers/BukkitConverters.java | 5 ++-- .../protocol/wrappers/WrappedDataWatcher.java | 27 ++++++++++--------- 7 files changed, 30 insertions(+), 24 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java index febbbbcf..694f55a8 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java @@ -99,8 +99,8 @@ public class AsyncMarker implements Serializable, Comparable { private transient int workerID; // Determine if Minecraft processes this packet asynchronously - private static Method isMinecraftAsync; - private static boolean alwaysSync; + private volatile static Method isMinecraftAsync; + private volatile static boolean alwaysSync; /** * Create a container for asyncronous packets. diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/error/DetailedErrorReporter.java b/ProtocolLib/src/main/java/com/comphenix/protocol/error/DetailedErrorReporter.java index 3e9aafa6..2257cbca 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/error/DetailedErrorReporter.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/error/DetailedErrorReporter.java @@ -121,6 +121,8 @@ public class DetailedErrorReporter implements ErrorReporter { @Override public void reportDetailed(Object sender, String message, Throwable error, Object... parameters) { + final Plugin plugin = pluginReference.get(); + // Do not overtly spam the server! if (++errorCount > maxErrorCount) { String maxReached = String.format("Reached maxmimum error count. Cannot pass error %s from %s.", error, sender); @@ -167,8 +169,7 @@ public class DetailedErrorReporter implements ErrorReporter { writer.println(addPrefix(getStringDescription(sender), SECOND_LEVEL_PREFIX)); // And plugin - if (pluginReference.get() != null) { - Plugin plugin = pluginReference.get(); + if (plugin != null) { writer.println("Version:"); writer.println(addPrefix(plugin.toString(), SECOND_LEVEL_PREFIX)); } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkServerInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkServerInjector.java index c97d9f50..3a346dfd 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkServerInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkServerInjector.java @@ -53,8 +53,8 @@ public class NetworkServerInjector extends PlayerInjector { private volatile static CallbackFilter callbackFilter; - private static Field disconnectField; - private static Method sendPacketMethod; + private volatile static Field disconnectField; + private volatile static Method sendPacketMethod; private InjectedServerConnection serverInjection; // Determine if we're listening 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 373df511..fb2136b3 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 @@ -604,10 +604,11 @@ public class PlayerInjectionHandler { */ public void scheduleDataInputRefresh(Player player) { final PlayerInjector injector = getInjector(player); - final DataInputStream old = injector.getInputStream(true); - + // Update the DataInputStream if (injector != null) { + final DataInputStream old = injector.getInputStream(true); + injector.scheduleAction(new Runnable() { @Override public void run() { diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/StructureModifier.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/StructureModifier.java index 9323065d..81599064 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/StructureModifier.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/StructureModifier.java @@ -202,7 +202,7 @@ public class StructureModifier { */ public boolean isReadOnly(int fieldIndex) { if (fieldIndex < 0 || fieldIndex >= data.size()) - new IllegalArgumentException("Index parameter is not within [0 - " + data.size() + ")"); + throw new IllegalArgumentException("Index parameter is not within [0 - " + data.size() + ")"); return Modifier.isFinal(data.get(fieldIndex).getModifiers()); } @@ -219,7 +219,7 @@ public class StructureModifier { */ public void setReadOnly(int fieldIndex, boolean value) throws FieldAccessException { if (fieldIndex < 0 || fieldIndex >= data.size()) - new IllegalArgumentException("Index parameter is not within [0 - " + data.size() + ")"); + throw new IllegalArgumentException("Index parameter is not within [0 - " + data.size() + ")"); try { StructureModifier.setFinalState(data.get(fieldIndex), value); diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/BukkitConverters.java b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/BukkitConverters.java index c8bc5935..ce2af3e8 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/BukkitConverters.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/BukkitConverters.java @@ -193,10 +193,11 @@ public class BukkitConverters { public Entity getSpecific(Object generic) { try { Integer id = (Integer) generic; + ProtocolManager manager = managerRef.get(); // Use the - if (id != null && managerRef.get() != null) { - return managerRef.get().getEntityFromID(container, id); + if (id != null && manager != null) { + return manager.getEntityFromID(container, id); } else { return null; } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedDataWatcher.java b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedDataWatcher.java index 7290b036..5f39621e 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedDataWatcher.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedDataWatcher.java @@ -8,6 +8,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; @@ -46,7 +47,7 @@ public class WrappedDataWatcher { private static Method getKeyValueMethod; // Entity methods - private static Field entityDataField; + private volatile static Field entityDataField; /** * Whether or not this class has already been initialized. @@ -275,12 +276,13 @@ public class WrappedDataWatcher { * @throws FieldAccessException If we're unable to read the underlying object. */ public Set indexSet() throws FieldAccessException { + Lock readLock = getReadWriteLock().readLock(); + readLock.lock(); + try { - getReadWriteLock().readLock().lock(); return new HashSet(getWatchableObjectMap().keySet()); - } finally { - getReadWriteLock().readLock().unlock(); + readLock.unlock(); } } @@ -290,12 +292,13 @@ public class WrappedDataWatcher { * @throws FieldAccessException If we're unable to read the underlying object. */ public int size() throws FieldAccessException { + Lock readLock = getReadWriteLock().readLock(); + readLock.lock(); + try { - getReadWriteLock().readLock().lock(); return getWatchableObjectMap().size(); - } finally { - getReadWriteLock().readLock().unlock(); + readLock.unlock(); } } @@ -337,18 +340,18 @@ public class WrappedDataWatcher { * @throws FieldAccessException Cannot read underlying field. */ private void setObjectRaw(int index, Object newValue, boolean update) throws FieldAccessException { - WatchableObject watchable; + // Aquire write lock + Lock writeLock = getReadWriteLock().writeLock(); + writeLock.lock(); try { - // Aquire write lock - getReadWriteLock().writeLock().lock(); - watchable = getWatchedObject(index); + WatchableObject watchable = getWatchedObject(index); if (watchable != null) { new WrappedWatchableObject(watchable).setValue(newValue, update); } } finally { - getReadWriteLock().writeLock().unlock(); + writeLock.unlock(); } } From dc186df695f36d0b8290a0a68855c1e51d6c3eda Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Tue, 20 Nov 2012 06:48:05 +0100 Subject: [PATCH 15/31] Small fix for OpenJDK (FindBugs). --- .../comphenix/protocol/ProtocolLibrary.java | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java index 2e1fe58c..65a85100 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java @@ -79,6 +79,7 @@ public class ProtocolLibrary extends JavaPlugin { // Logger private Logger logger; + private Handler redirectHandler; // Commands private CommandProtocol commandProtocol; @@ -137,8 +138,12 @@ public class ProtocolLibrary extends JavaPlugin { } private void broadcastUsers(final String permission) { - // Broadcast information to every user too - logger.addHandler(new Handler() { + // Guard against multiple calls + if (redirectHandler != null) + return; + + // Broadcast information to every user too + redirectHandler = new Handler() { @Override public void publish(LogRecord record) { commandPacket.broadcastMessageSilently(record.getMessage(), permission); @@ -153,7 +158,9 @@ public class ProtocolLibrary extends JavaPlugin { public void close() throws SecurityException { // Do nothing. } - }); + }; + + logger.addHandler(redirectHandler); } @Override @@ -293,6 +300,11 @@ public class ProtocolLibrary extends JavaPlugin { asyncPacketTask = -1; } + // And redirect handler too + if (redirectHandler != null) { + logger.removeHandler(redirectHandler); + } + unhookTask.close(); protocolManager.close(); protocolManager = null; From d4d763af94821906eee22ebc5a367b2951e3018b Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Tue, 20 Nov 2012 06:53:39 +0100 Subject: [PATCH 16/31] Add a warning message when trying to correct invalid configuration. --- .../java/com/comphenix/protocol/ProtocolLibrary.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java index 65a85100..ffe6b34d 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java @@ -101,8 +101,11 @@ public class ProtocolLibrary extends JavaPlugin { detailedReporter.reportWarning(this, "Cannot load configuration", e); // Load it again - deleteConfig(); - config = new ProtocolConfig(this); + if (deleteConfig()) { + config = new ProtocolConfig(this); + } else { + reporter.reportWarning(this, "Cannot delete old ProtocolLib configuration."); + } } try { @@ -123,8 +126,8 @@ public class ProtocolLibrary extends JavaPlugin { } } - private void deleteConfig() { - config.getFile().delete(); + private boolean deleteConfig() { + return config.getFile().delete(); } @Override From a849c38ce6c734facf18ffc7bc31a18315463f78 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Tue, 20 Nov 2012 06:59:56 +0100 Subject: [PATCH 17/31] Implement equals() and hashCode(), since we've implemented compareTo. --- .../comphenix/protocol/async/AsyncMarker.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java index 694f55a8..b1a02e4d 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java @@ -32,6 +32,7 @@ import com.comphenix.protocol.events.PacketEvent; import com.comphenix.protocol.injector.PrioritizedListener; import com.comphenix.protocol.reflect.FieldAccessException; import com.comphenix.protocol.reflect.FuzzyReflection; +import com.google.common.base.Objects; import com.google.common.primitives.Longs; /** @@ -447,4 +448,22 @@ public class AsyncMarker implements Serializable, Comparable { else return Longs.compare(getNewSendingIndex(), o.getNewSendingIndex()); } + + @Override + public boolean equals(Object other) { + // Standard equals + if (other == this) + return true; + if (other == null) + return false; + if (other instanceof AsyncMarker) + return Objects.equal(getNewSendingIndex(), ((AsyncMarker) other).getNewSendingIndex()); + + return false; + } + + @Override + public int hashCode() { + return Longs.hashCode(getNewSendingIndex()); + } } From 53fe3e5b616814b99bfb9c65bbfcbda3f2cd64b2 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Tue, 20 Nov 2012 07:10:46 +0100 Subject: [PATCH 18/31] Fixed a range of smaller bugs discovered by FindBugs. --- .../comphenix/protocol/async/AsyncMarker.java | 9 +++------ .../protocol/async/PacketEventHolder.java | 17 +++++++++++++++++ .../injector/player/InjectedArrayList.java | 6 +++--- .../protocol/reflect/StructureModifier.java | 2 +- .../reflect/compiler/StructureCompiler.java | 2 +- .../reflect/instances/CollectionGenerator.java | 2 +- .../reflect/instances/ExistingGenerator.java | 1 - .../reflect/instances/PrimitiveGenerator.java | 5 +++-- .../injector/SortedCopyOnWriteArrayTest.java | 2 +- 9 files changed, 30 insertions(+), 16 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java index b1a02e4d..6cf5147c 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java @@ -32,7 +32,6 @@ import com.comphenix.protocol.events.PacketEvent; import com.comphenix.protocol.injector.PrioritizedListener; import com.comphenix.protocol.reflect.FieldAccessException; import com.comphenix.protocol.reflect.FuzzyReflection; -import com.google.common.base.Objects; import com.google.common.primitives.Longs; /** @@ -454,12 +453,10 @@ public class AsyncMarker implements Serializable, Comparable { // Standard equals if (other == this) return true; - if (other == null) - return false; if (other instanceof AsyncMarker) - return Objects.equal(getNewSendingIndex(), ((AsyncMarker) other).getNewSendingIndex()); - - return false; + return getNewSendingIndex() == ((AsyncMarker) other).getNewSendingIndex(); + else + return false; } @Override diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketEventHolder.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketEventHolder.java index da0ed7e8..5dce81a9 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketEventHolder.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketEventHolder.java @@ -20,6 +20,7 @@ package com.comphenix.protocol.async; import com.comphenix.protocol.events.PacketEvent; import com.google.common.base.Preconditions; import com.google.common.collect.ComparisonChain; +import com.google.common.primitives.Longs; /** * Provides a comparable to a packet event. @@ -56,4 +57,20 @@ class PacketEventHolder implements Comparable { compare(sendingIndex, other.sendingIndex). result(); } + + @Override + public boolean equals(Object other) { + // Standard equals + if (other == this) + return true; + if (other instanceof PacketEventHolder) + return sendingIndex == ((PacketEventHolder) other).sendingIndex; + else + return false; + } + + @Override + public int hashCode() { + return Longs.hashCode(sendingIndex); + } } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/InjectedArrayList.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/InjectedArrayList.java index aa50212f..fd5a0b80 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/InjectedArrayList.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/InjectedArrayList.java @@ -42,9 +42,9 @@ class InjectedArrayList extends ArrayList { */ private static final long serialVersionUID = -1173865905404280990L; - private PlayerInjector injector; - private Set ignoredPackets; - private ClassLoader classLoader; + private transient PlayerInjector injector; + private transient Set ignoredPackets; + private transient ClassLoader classLoader; public InjectedArrayList(ClassLoader classLoader, PlayerInjector injector, Set ignoredPackets) { this.classLoader = classLoader; diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/StructureModifier.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/StructureModifier.java index 81599064..87a309fa 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/StructureModifier.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/StructureModifier.java @@ -400,7 +400,7 @@ public class StructureModifier { if (a == null) return b == null; else if (b == null) - return a == null; + return false; else return a.getSpecificType().equals(b.getSpecificType()); } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java index ac49f45b..0c33b847 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java @@ -94,7 +94,7 @@ public final class StructureCompiler { // Used to store generated classes of different types @SuppressWarnings("rawtypes") - private class StructureKey { + private static class StructureKey { private Class targetType; private Class fieldType; diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/CollectionGenerator.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/CollectionGenerator.java index 674565dc..1e350ad7 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/CollectionGenerator.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/CollectionGenerator.java @@ -47,7 +47,7 @@ public class CollectionGenerator implements InstanceProvider { @Override public Object create(@Nullable Class type) { // Standard collection types - if (type.isInterface()) { + if (type != null && type.isInterface()) { if (type.equals(Collection.class) || type.equals(List.class)) return new ArrayList(); else if (type.equals(Set.class)) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/ExistingGenerator.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/ExistingGenerator.java index 956ee71f..43527d17 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/ExistingGenerator.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/ExistingGenerator.java @@ -119,7 +119,6 @@ public class ExistingGenerator implements InstanceProvider { @Override public Object create(@Nullable Class type) { - Object value = existingValues.get(type.getName()); // NULL values indicate that the generator failed diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/PrimitiveGenerator.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/PrimitiveGenerator.java index 5563acc6..aaaf19ca 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/PrimitiveGenerator.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/PrimitiveGenerator.java @@ -57,8 +57,9 @@ public class PrimitiveGenerator implements InstanceProvider { @Override public Object create(@Nullable Class type) { - - if (type.isPrimitive()) { + if (type == null) { + return null; + } else if (type.isPrimitive()) { return Defaults.defaultValue(type); } else if (Primitives.isWrapperType(type)) { return Defaults.defaultValue(Primitives.unwrap(type)); diff --git a/ProtocolLib/src/test/java/com/comphenix/protocol/injector/SortedCopyOnWriteArrayTest.java b/ProtocolLib/src/test/java/com/comphenix/protocol/injector/SortedCopyOnWriteArrayTest.java index 85821ffa..775a197e 100644 --- a/ProtocolLib/src/test/java/com/comphenix/protocol/injector/SortedCopyOnWriteArrayTest.java +++ b/ProtocolLib/src/test/java/com/comphenix/protocol/injector/SortedCopyOnWriteArrayTest.java @@ -58,7 +58,7 @@ public class SortedCopyOnWriteArrayTest { assertEquals(3, test.get(1).id); } - private class PriorityStuff implements Comparable { + private static class PriorityStuff implements Comparable { public ListenerPriority priority; public int id; From dd9cb30d2546e6903540caaf90cbac4c03bec66a Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Tue, 20 Nov 2012 18:47:22 +0100 Subject: [PATCH 19/31] Structure compiler shouldn't be public. --- .../protocol/reflect/compiler/StructureCompiler.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java index 0c33b847..cf25ebe7 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/compiler/StructureCompiler.java @@ -52,8 +52,8 @@ import net.sf.cglib.asm.*; // case 0: return (Object) target.a; // case 1: return (Object) target.b; // case 2: return (Object) target.c; -// case 3: return super.read(fieldIndex); -// case 4: return super.read(fieldIndex); +// case 3: return super.readReflected(fieldIndex); +// case 4: return super.readReflected(fieldIndex); // case 5: return (Object) target.f; // case 6: return (Object) target.g; // case 7: return (Object) target.h; @@ -72,8 +72,8 @@ import net.sf.cglib.asm.*; // case 1: target.b = (String) value; break; // case 2: target.c = (Integer) value; break; // case 3: target.d = (Integer) value; break; -// case 4: super.write(index, value); break; -// case 5: super.write(index, value); break; +// case 4: super.writeReflected(index, value); break; +// case 5: super.writeReflected(index, value); break; // case 6: target.g = (Byte) value; break; // case 7: target.h = (Integer) value; break; // default: @@ -138,7 +138,7 @@ public final class StructureCompiler { * Construct a structure compiler. * @param loader - main class loader. */ - public StructureCompiler(ClassLoader loader) { + StructureCompiler(ClassLoader loader) { this.loader = loader; } From 36f867cafaf33a022e1d6780b1468682947a135c Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Wed, 21 Nov 2012 00:15:53 +0100 Subject: [PATCH 20/31] Added synchronous packet processing. Client packets are typically processed asynchronously (in a client's reader thread), and should never access the Bukkit API directly, with a few exceptions. This is problematic if you need to cancel a packet as a response to the Bukkit API, such as the permission system. Currently, you will have to either cancel the packet - which is discuraged - sync with the main thread and then re-transmit it outside the filters, or use an asynchronous thread with callSyncMethod and wait on the returned future. A better method is needed. Synchronous processing allows you to run light-weight packet listeners on the main thread without having to deal with synchronization, concurrency or the overhead of an additional thread. It can also process multiple packets per tick with a configurable timeout. This, along with 7b9d97123888b86c31ab135347ac8d4f0771c3ac, makes it easy to delay light-weight packets to be synchronously processed. --- .../protocol/async/AsyncFilterManager.java | 36 +-- .../protocol/async/AsyncListenerHandler.java | 226 ++++++++++++++---- 2 files changed, 202 insertions(+), 60 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java index 44f49bba..5fe61857 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java @@ -56,29 +56,36 @@ public class AsyncFilterManager implements AsynchronousManager { private PacketProcessingQueue clientProcessingQueue; // Sending queues - private PlayerSendingHandler playerSendingHandler; + private final PlayerSendingHandler playerSendingHandler; // Report exceptions - private ErrorReporter reporter; + private final ErrorReporter reporter; // The likely main thread - private Thread mainThread; + private final Thread mainThread; // Default scheduler - private BukkitScheduler scheduler; + private final BukkitScheduler scheduler; // Our protocol manager - private ProtocolManager manager; + private final ProtocolManager manager; // Current packet index - private AtomicInteger currentSendingIndex = new AtomicInteger(); + private final AtomicInteger currentSendingIndex = new AtomicInteger(); + /** + * Initialize a asynchronous filter manager. + *

+ * Internal method. Retrieve the global asynchronous manager from the protocol manager instead. + * @param reporter - desired error reporter. + * @param scheduler - task scheduler. + * @param manager - protocol manager. + */ public AsyncFilterManager(ErrorReporter reporter, BukkitScheduler scheduler, ProtocolManager manager) { - // Initialize timeout listeners - serverTimeoutListeners = new SortedPacketListenerList(); - clientTimeoutListeners = new SortedPacketListenerList(); - timeoutListeners = Sets.newSetFromMap(new ConcurrentHashMap()); + this.serverTimeoutListeners = new SortedPacketListenerList(); + this.clientTimeoutListeners = new SortedPacketListenerList(); + this.timeoutListeners = Sets.newSetFromMap(new ConcurrentHashMap()); this.playerSendingHandler = new PlayerSendingHandler(reporter, serverTimeoutListeners, clientTimeoutListeners); this.serverProcessingQueue = new PacketProcessingQueue(playerSendingHandler); @@ -263,12 +270,11 @@ public class AsyncFilterManager implements AsynchronousManager { } /** - * Used to create a default asynchronous task. - * @param plugin - the calling plugin. - * @param runnable - the runnable. + * Retrieve the current task scheduler. + * @return Current task scheduler. */ - public void scheduleAsyncTask(Plugin plugin, Runnable runnable) { - scheduler.scheduleAsyncDelayedTask(plugin, runnable); + public BukkitScheduler getScheduler() { + return scheduler; } @Override diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncListenerHandler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncListenerHandler.java index 0b1ef3ba..0ccb719c 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncListenerHandler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncListenerHandler.java @@ -20,6 +20,7 @@ package com.comphenix.protocol.async; import java.util.HashSet; import java.util.Set; import java.util.concurrent.ArrayBlockingQueue; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; @@ -75,10 +76,19 @@ public class AsyncListenerHandler { private final Set stoppedTasks = new HashSet(); private final Object stopLock = new Object(); + // Processing task on the main thread + private int syncTask = -1; + // Minecraft main thread private Thread mainThread; - public AsyncListenerHandler(Thread mainThread, AsyncFilterManager filterManager, PacketListener listener) { + /** + * Construct a manager for an asynchronous packet handler. + * @param mainThread - the main game thread. + * @param filterManager - the parent filter manager. + * @param listener - the current packet listener. + */ + AsyncListenerHandler(Thread mainThread, AsyncFilterManager filterManager, PacketListener listener) { if (filterManager == null) throw new IllegalArgumentException("filterManager cannot be NULL"); if (listener == null) @@ -89,10 +99,18 @@ public class AsyncListenerHandler { this.listener = listener; } + /** + * Determine whether or not this asynchronous handler has been cancelled. + * @return TRUE if it has been cancelled/stopped, FALSE otherwise. + */ public boolean isCancelled() { return cancelled; } + /** + * Retrieve the current asynchronous packet listener. + * @return Current packet listener. + */ public PacketListener getAsyncListener() { return listener; } @@ -223,7 +241,7 @@ public class AsyncListenerHandler { final AsyncRunnable listenerLoop = getListenerLoop(); - filterManager.scheduleAsyncTask(listener.getPlugin(), new Runnable() { + filterManager.getScheduler().scheduleAsyncDelayedTask(listener.getPlugin(), new Runnable() { @Override public void run() { Thread thread = Thread.currentThread(); @@ -271,7 +289,7 @@ public class AsyncListenerHandler { final AsyncRunnable listenerLoop = getListenerLoop(); final Function delegateCopy = executor; - filterManager.scheduleAsyncTask(listener.getPlugin(), new Runnable() { + filterManager.getScheduler().scheduleAsyncDelayedTask(listener.getPlugin(), new Runnable() { @Override public void run() { delegateCopy.apply(listenerLoop); @@ -308,6 +326,104 @@ public class AsyncListenerHandler { return Joiner.on(", ").join(whitelist.getWhitelist()); } + /** + * Start processing packets on the main thread. + *

+ * This is useful if you need to synchronize with the main thread in your packet listener, but + * you're not performing any expensive processing. + *

+ * Note: Use a asynchronous worker if the packet listener may use more than 0.5 ms + * of processing time on a single packet. Do as much as possible on the worker thread, and schedule synchronous tasks + * to use the Bukkit API instead. + * @return TRUE if the synchronized processing was successfully started, FALSE if it's already running. + * @throws IllegalStateException If we couldn't start the underlying task. + */ + public synchronized boolean syncStart() { + return syncStart(500, TimeUnit.MICROSECONDS); + } + + /** + * Start processing packets on the main thread. + *

+ * This is useful if you need to synchronize with the main thread in your packet listener, but + * you're not performing any expensive processing. + *

+ * The processing time parameter gives the upper bound for the amount of time spent processing pending packets. + * It should be set to a fairly low number, such as 0.5 ms or 1% of a game tick - to reduce the impact + * on the main thread. Never go beyond 50 milliseconds. + *

+ * Note: Use a asynchronous worker if the packet listener may exceed the ideal processing time + * on a single packet. Do as much as possible on the worker thread, and schedule synchronous tasks + * to use the Bukkit API instead. + * + * @param time - the amount of processing time alloted per game tick (20 ticks per second). + * @param unit - the unit of the processingTime argument. + * @return TRUE if the synchronized processing was successfully started, FALSE if it's already running. + * @throws IllegalStateException If we couldn't start the underlying task. + */ + public synchronized boolean syncStart(final long time, final TimeUnit unit) { + if (time <= 0) + throw new IllegalArgumentException("Time must be greater than zero."); + if (unit == null) + throw new IllegalArgumentException("TimeUnit cannot be NULL."); + + final long tickDelay = 1; + final int workerID = nextID.incrementAndGet(); + + if (syncTask < 0) { + syncTask = filterManager.getScheduler().scheduleSyncRepeatingTask(getPlugin(), new Runnable() { + @Override + public void run() { + long stopTime = System.nanoTime() + unit.convert(time, TimeUnit.NANOSECONDS); + + while (!cancelled) { + PacketEvent packet = queuedPackets.poll(); + + if (packet == INTERUPT_PACKET || packet == WAKEUP_PACKET) { + // Sorry, asynchronous threads! + queuedPackets.add(packet); + + // Try again next tick + break; + } else if (packet != null && packet.getAsyncMarker() != null) { + processPacket(workerID, packet, "onSyncPacket()"); + } else { + // No more packets left - wait a tick + break; + } + + // Check time here, ensuring that we at least process one packet + if (System.nanoTime() < stopTime) + break; + } + } + }, tickDelay, tickDelay); + + // This is very bad - force the caller to handle it + if (syncTask < 0) + throw new IllegalStateException("Cannot start synchronous task."); + else + return true; + } else { + return false; + } + } + + /** + * Stop processing packets on the main thread. + * @return TRUE if we stopped any processing tasks, FALSE if it has already been stopped. + */ + public synchronized boolean syncStop() { + if (syncTask > 0) { + filterManager.getScheduler().cancelTask(syncTask); + + syncTask = -1; + return true; + } else { + return false; + } + } + /** * Start multiple worker threads for this listener. * @param count - number of worker threads to start. @@ -386,9 +502,13 @@ public class AsyncListenerHandler { } } - // DO NOT call this method from the main thread + /** + * The main processing loop of asynchronous threads. + *

+ * Note: DO NOT call this method from the main thread + * @param workerID - the current worker ID. + */ private void listenerLoop(int workerID) { - // Danger, danger! if (Thread.currentThread().getId() == mainThread.getId()) throw new IllegalStateException("Do not call this method from the main thread."); @@ -403,16 +523,11 @@ public class AsyncListenerHandler { // Proceed started.incrementAndGet(); - mainLoop: while (!cancelled) { PacketEvent packet = queuedPackets.take(); - AsyncMarker marker = packet.getAsyncMarker(); // Handle cancel requests - if (packet == null || marker == null || packet == INTERUPT_PACKET) { - return; - - } else if (packet == WAKEUP_PACKET) { + if (packet == WAKEUP_PACKET) { // This is a bit slow, but it should be safe synchronized (stopLock) { // Are we the one who is supposed to stop? @@ -421,42 +536,13 @@ public class AsyncListenerHandler { if (waitForStops()) return; } + } else if (packet == INTERUPT_PACKET) { + return; } - // Here's the core of the asynchronous processing - try { - marker.setListenerHandler(this); - marker.setWorkerID(workerID); - - synchronized (marker.getProcessingLock()) { - if (packet.isServerPacket()) - listener.onPacketSending(packet); - else - listener.onPacketReceiving(packet); - } - - } catch (Throwable e) { - // Minecraft doesn't want your Exception. - filterManager.getErrorReporter().reportMinimal(listener.getPlugin(), "onAsyncPacket()", e); + if (packet != null && packet.getAsyncMarker() == null) { + processPacket(workerID, packet, "onAsyncPacket()"); } - - // Now, get the next non-cancelled listener - if (!marker.hasExpired()) { - for (; marker.getListenerTraversal().hasNext(); ) { - AsyncListenerHandler handler = marker.getListenerTraversal().next().getListener(); - - if (!handler.isCancelled()) { - handler.enqueuePacket(packet); - continue mainLoop; - } - } - } - - // There are no more listeners - queue the packet for transmission - filterManager.signalFreeProcessingSlot(packet); - - // Note that listeners can opt to delay the packet transmission - filterManager.signalPacketTransmission(packet); } } catch (InterruptedException e) { @@ -464,16 +550,66 @@ public class AsyncListenerHandler { } finally { // Clean up started.decrementAndGet(); - close(); } } + /** + * Called when a packet is scheduled for processing. + * @param workerID - the current worker ID. + * @param packet - the current packet. + * @param methodName - name of the method. + */ + private void processPacket(int workerID, PacketEvent packet, String methodName) { + AsyncMarker marker = packet.getAsyncMarker(); + + // Here's the core of the asynchronous processing + try { + synchronized (marker.getProcessingLock()) { + marker.setListenerHandler(this); + marker.setWorkerID(workerID); + + if (packet.isServerPacket()) + listener.onPacketSending(packet); + else + listener.onPacketReceiving(packet); + } + + } catch (Throwable e) { + // Minecraft doesn't want your Exception. + filterManager.getErrorReporter().reportMinimal(listener.getPlugin(), methodName, e); + } + + // Now, get the next non-cancelled listener + if (!marker.hasExpired()) { + for (; marker.getListenerTraversal().hasNext(); ) { + AsyncListenerHandler handler = marker.getListenerTraversal().next().getListener(); + + if (!handler.isCancelled()) { + handler.enqueuePacket(packet); + return; + } + } + } + + // There are no more listeners - queue the packet for transmission + filterManager.signalFreeProcessingSlot(packet); + + // Note that listeners can opt to delay the packet transmission + filterManager.signalPacketTransmission(packet); + } + + /** + * Close all worker threads and the handler itself. + */ private synchronized void close() { // Remove the listener itself if (!cancelled) { filterManager.unregisterAsyncHandlerInternal(this); cancelled = true; + // Close processing tasks + syncStop(); + // Tell every uncancelled thread to end stopThreads(); } From 9482818751f5167ecfe7f08820d0ef6f149b5043 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Wed, 21 Nov 2012 00:24:19 +0100 Subject: [PATCH 21/31] Fixed an incorrect null check. --- .../java/com/comphenix/protocol/async/AsyncListenerHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncListenerHandler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncListenerHandler.java index 0ccb719c..65e4871a 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncListenerHandler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncListenerHandler.java @@ -540,7 +540,7 @@ public class AsyncListenerHandler { return; } - if (packet != null && packet.getAsyncMarker() == null) { + if (packet != null && packet.getAsyncMarker() != null) { processPacket(workerID, packet, "onAsyncPacket()"); } } From 0b200472f187a7bb296f194f341f9037e111037f Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Wed, 21 Nov 2012 01:39:43 +0100 Subject: [PATCH 22/31] Process asynchronous packets on an async thread. Bukkit complains if we try to send an async packet on the main thread, so we will have to add a new background thread that can transmit packets processed by light-weight packet listeners. In addition, fixed a bug causing the "uncancel" method in PacketInjector from not working properly. That bug is as persistent as a zombie. --- .../protocol/async/AsyncFilterManager.java | 1 + .../protocol/async/PacketSendingQueue.java | 159 ++++++++++++------ .../protocol/async/PlayerSendingHandler.java | 42 ++++- .../protocol/injector/PacketInjector.java | 4 +- .../protocol/injector/ReadPacketModifier.java | 9 + 5 files changed, 155 insertions(+), 60 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java index 5fe61857..92de92a1 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java @@ -90,6 +90,7 @@ public class AsyncFilterManager implements AsynchronousManager { this.playerSendingHandler = new PlayerSendingHandler(reporter, serverTimeoutListeners, clientTimeoutListeners); this.serverProcessingQueue = new PacketProcessingQueue(playerSendingHandler); this.clientProcessingQueue = new PacketProcessingQueue(playerSendingHandler); + this.playerSendingHandler.initializeScheduler(); this.scheduler = scheduler; this.manager = manager; diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketSendingQueue.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketSendingQueue.java index d55aea30..bc1bcaf5 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketSendingQueue.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketSendingQueue.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.concurrent.Executor; import java.util.concurrent.PriorityBlockingQueue; import org.bukkit.entity.Player; @@ -39,8 +40,24 @@ abstract class PacketSendingQueue { private PriorityBlockingQueue sendingQueue; + // Asynchronous packet sending + private Executor asynchronousSender; + // Whether or not packet transmission can only occur on the main thread private final boolean synchronizeMain; + + // Whether or not we've run the cleanup procedure + private boolean cleanedUp = false; + + /** + * Create a packet sending queue. + * @param synchronizeMain - whether or not to synchronize with the main thread. + */ + public PacketSendingQueue(boolean synchronizeMain, Executor asynchronousSender) { + this.sendingQueue = new PriorityBlockingQueue(INITIAL_CAPACITY); + this.synchronizeMain = synchronizeMain; + this.asynchronousSender = asynchronousSender; + } /** * Number of packet events in the queue. @@ -50,15 +67,6 @@ abstract class PacketSendingQueue { return sendingQueue.size(); } - /** - * Create a packet sending queue. - * @param synchronizeMain - whether or not to synchronize with the main thread. - */ - public PacketSendingQueue(boolean synchronizeMain) { - this.sendingQueue = new PriorityBlockingQueue(INITIAL_CAPACITY); - this.synchronizeMain = synchronizeMain; - } - /** * Enqueue a packet for sending. * @param packet - packet to queue. @@ -119,55 +127,99 @@ abstract class PacketSendingQueue { * @param onMainThread - whether or not this is occuring on the main thread. */ public void trySendPackets(boolean onMainThread) { - + // Whether or not to continue sending packets + boolean sending = true; + // Transmit as many packets as we can - while (true) { - PacketEventHolder holder = sendingQueue.peek(); - + while (sending) { + PacketEventHolder holder = sendingQueue.poll(); + if (holder != null) { - PacketEvent current = holder.getEvent(); - AsyncMarker marker = current.getAsyncMarker(); - boolean hasExpired = marker.hasExpired(); - - // Abort if we're not on the main thread - if (synchronizeMain) { - try { - boolean wantAsync = marker.isMinecraftAsync(current); - boolean wantSync = !wantAsync; - - // Quit if we haven't fulfilled our promise - if ((onMainThread && wantAsync) || (!onMainThread && wantSync)) - return; - - } catch (FieldAccessException e) { - e.printStackTrace(); - return; - } + sending = processPacketHolder(onMainThread, holder); + + if (!sending) { + // Add it back again + sendingQueue.add(holder); } - if (marker.isProcessed() || hasExpired) { - if (hasExpired) { - // Notify timeout listeners - onPacketTimeout(current); - - // Recompute - marker = current.getAsyncMarker(); - hasExpired = marker.hasExpired(); - } - if (marker.isProcessed() && !current.isCancelled() && !hasExpired) { - // Silently skip players that have logged out - if (isOnline(current.getPlayer())) { - sendPacket(current); - } + } else { + // No more packets to send + sending = false; + } + } + } + + /** + * Invoked when a packet might be ready for transmission. + * @param onMainThread - TRUE if we're on the main thread, FALSE otherwise. + * @param holder - packet container. + * @return TRUE to continue sending packets, FALSE otherwise. + */ + private boolean processPacketHolder(boolean onMainThread, final PacketEventHolder holder) { + PacketEvent current = holder.getEvent(); + AsyncMarker marker = current.getAsyncMarker(); + boolean hasExpired = marker.hasExpired(); + + // Guard in cause the queue is closed + if (cleanedUp) { + return true; + } + + // End condition? + if (marker.isProcessed() || hasExpired) { + if (hasExpired) { + // Notify timeout listeners + onPacketTimeout(current); + + // Recompute + marker = current.getAsyncMarker(); + hasExpired = marker.hasExpired(); + } + + // Abort if we're not on the main thread + if (synchronizeMain && !hasExpired) { + try { + boolean wantAsync = marker.isMinecraftAsync(current); + boolean wantSync = !wantAsync; + + // Wait for the next main thread heartbeat if we haven't fulfilled our promise + if (!onMainThread && wantSync) { + return false; } - sendingQueue.poll(); - continue; + // Let's give it what it wants, then + if (onMainThread && wantAsync) { + asynchronousSender.execute(new Runnable() { + @Override + public void run() { + // We know this isn't on the main thread + processPacketHolder(false, holder); + } + }); + + // The executor will take it from here + return true; + } + + } catch (FieldAccessException e) { + e.printStackTrace(); + // Skip this packet + return true; } } - // Only repeat when packets are removed - break; + if (marker.isProcessed() && !current.isCancelled() && !hasExpired) { + // Silently skip players that have logged out + if (isOnline(current.getPlayer())) { + sendPacket(current); + } + } + + return true; + + } else { + // Add it back and stop sending + return false; } } @@ -234,7 +286,12 @@ abstract class PacketSendingQueue { * Automatically transmits every delayed packet. */ public void cleanupAll() { - // Note that the cleanup itself will always occur on the main thread - forceSend(); + if (!cleanedUp) { + // Note that the cleanup itself will always occur on the main thread + forceSend(); + + // And we're done + cleanedUp = true; + } } } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java index cb234d88..59bf3a2e 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java @@ -3,12 +3,16 @@ package com.comphenix.protocol.async; import java.util.ArrayList; import java.util.List; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executor; +import java.util.concurrent.Executors; +import java.util.concurrent.ThreadFactory; import org.bukkit.entity.Player; import com.comphenix.protocol.error.ErrorReporter; import com.comphenix.protocol.events.PacketEvent; import com.comphenix.protocol.injector.SortedPacketListenerList; +import com.google.common.util.concurrent.ThreadFactoryBuilder; /** * Contains every sending queue for every player. @@ -24,6 +28,9 @@ class PlayerSendingHandler { private SortedPacketListenerList serverTimeoutListeners; private SortedPacketListenerList clientTimeoutListeners; + // Asynchronous packet sending + private Executor asynchronousSender; + // Whether or not we're currently cleaning up private volatile boolean cleaningUp; @@ -38,7 +45,7 @@ class PlayerSendingHandler { public QueueContainer() { // Server packets are synchronized already - serverQueue = new PacketSendingQueue(false) { + serverQueue = new PacketSendingQueue(false, asynchronousSender) { @Override protected void onPacketTimeout(PacketEvent event) { if (!cleaningUp) { @@ -48,7 +55,7 @@ class PlayerSendingHandler { }; // Client packets must be synchronized - clientQueue = new PacketSendingQueue(true) { + clientQueue = new PacketSendingQueue(true, asynchronousSender) { @Override protected void onPacketTimeout(PacketEvent event) { if (!cleaningUp) { @@ -67,6 +74,12 @@ class PlayerSendingHandler { } } + /** + * Initialize a packet sending handler. + * @param reporter - error reporter. + * @param serverTimeoutListeners - set of server timeout listeners. + * @param clientTimeoutListeners - set of client timeout listeners. + */ public PlayerSendingHandler(ErrorReporter reporter, SortedPacketListenerList serverTimeoutListeners, SortedPacketListenerList clientTimeoutListeners) { @@ -75,7 +88,20 @@ class PlayerSendingHandler { this.clientTimeoutListeners = clientTimeoutListeners; // Initialize storage of queues - playerSendingQueues = new ConcurrentHashMap(); + this.playerSendingQueues = new ConcurrentHashMap(); + } + + /** + * Start the asynchronous packet sender. + */ + public synchronized void initializeScheduler() { + if (asynchronousSender == null) { + ThreadFactory factory = new ThreadFactoryBuilder(). + setDaemon(true). + setNameFormat("ProtocolLib-AsyncSender %s"). + build(); + asynchronousSender = Executors.newSingleThreadExecutor(factory); + } } /** @@ -202,10 +228,12 @@ class PlayerSendingHandler { * Send all pending packets and clean up queues. */ public void cleanupAll() { - cleaningUp = true; - - sendAllPackets(); - playerSendingQueues.clear(); + if (!cleaningUp) { + cleaningUp = true; + + sendAllPackets(); + playerSendingQueues.clear(); + } } /** diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketInjector.java index 951e4d79..7630c360 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketInjector.java @@ -83,8 +83,8 @@ class PacketInjector { public void undoCancel(Integer id, Packet packet) { ReadPacketModifier modifier = readModifier.get(id); - // Cancelled packets are represented with NULL - if (modifier != null && modifier.getOverride(packet) == null) { + // See if this packet has been cancelled before + if (modifier != null && modifier.hasCancelled(packet)) { modifier.removeOverride(packet); } } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/ReadPacketModifier.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/ReadPacketModifier.java index 6e1967e1..ee32ddc2 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/ReadPacketModifier.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/ReadPacketModifier.java @@ -74,6 +74,15 @@ class ReadPacketModifier implements MethodInterceptor { return override.get(packet); } + /** + * Determine if the given packet has been cancelled before. + * @param packet - the packet to check. + * @return TRUE if it has been cancelled, FALSE otherwise. + */ + public boolean hasCancelled(Packet packet) { + return getOverride(packet) == CANCEL_MARKER; + } + @Override public Object intercept(Object thisObj, Method method, Object[] args, MethodProxy proxy) throws Throwable { From 858f9ee02d8d8bac5a35360c50d0f79d724ab160 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Wed, 21 Nov 2012 01:42:59 +0100 Subject: [PATCH 23/31] Renamed 'synchronizeMain' to 'notThreadSafe'. --- .../protocol/async/PacketSendingQueue.java | 14 +++++++------- .../protocol/async/PlayerSendingHandler.java | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketSendingQueue.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketSendingQueue.java index bc1bcaf5..dd4c9f70 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketSendingQueue.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketSendingQueue.java @@ -43,19 +43,19 @@ abstract class PacketSendingQueue { // Asynchronous packet sending private Executor asynchronousSender; - // Whether or not packet transmission can only occur on the main thread - private final boolean synchronizeMain; + // Whether or not packet transmission must occur on a specific thread + private final boolean notThreadSafe; // Whether or not we've run the cleanup procedure private boolean cleanedUp = false; /** * Create a packet sending queue. - * @param synchronizeMain - whether or not to synchronize with the main thread. + * @param notThreadSafe - whether or not to synchronize with the main thread or a background thread. */ - public PacketSendingQueue(boolean synchronizeMain, Executor asynchronousSender) { + public PacketSendingQueue(boolean notThreadSafe, Executor asynchronousSender) { this.sendingQueue = new PriorityBlockingQueue(INITIAL_CAPACITY); - this.synchronizeMain = synchronizeMain; + this.notThreadSafe = notThreadSafe; this.asynchronousSender = asynchronousSender; } @@ -177,7 +177,7 @@ abstract class PacketSendingQueue { } // Abort if we're not on the main thread - if (synchronizeMain && !hasExpired) { + if (notThreadSafe && !hasExpired) { try { boolean wantAsync = marker.isMinecraftAsync(current); boolean wantSync = !wantAsync; @@ -253,7 +253,7 @@ abstract class PacketSendingQueue { * @return TRUE if it must, FALSE otherwise. */ public boolean isSynchronizeMain() { - return synchronizeMain; + return notThreadSafe; } /** diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java index 59bf3a2e..8dc4d0be 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java @@ -44,7 +44,7 @@ class PlayerSendingHandler { private PacketSendingQueue clientQueue; public QueueContainer() { - // Server packets are synchronized already + // Server packets can be sent concurrently serverQueue = new PacketSendingQueue(false, asynchronousSender) { @Override protected void onPacketTimeout(PacketEvent event) { From 524ef2e6c953970bbc8d0a805b187e7269d688ca Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Wed, 21 Nov 2012 02:18:56 +0100 Subject: [PATCH 24/31] Fix the packet sending procedure. --- .../protocol/async/PacketSendingQueue.java | 81 ++++++++++--------- 1 file changed, 44 insertions(+), 37 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketSendingQueue.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketSendingQueue.java index dd4c9f70..d7f13233 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketSendingQueue.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketSendingQueue.java @@ -174,53 +174,60 @@ abstract class PacketSendingQueue { // Recompute marker = current.getAsyncMarker(); hasExpired = marker.hasExpired(); - } - - // Abort if we're not on the main thread - if (notThreadSafe && !hasExpired) { - try { - boolean wantAsync = marker.isMinecraftAsync(current); - boolean wantSync = !wantAsync; - - // Wait for the next main thread heartbeat if we haven't fulfilled our promise - if (!onMainThread && wantSync) { - return false; - } - - // Let's give it what it wants, then - if (onMainThread && wantAsync) { - asynchronousSender.execute(new Runnable() { - @Override - public void run() { - // We know this isn't on the main thread - processPacketHolder(false, holder); - } - }); - - // The executor will take it from here - return true; - } - - } catch (FieldAccessException e) { - e.printStackTrace(); - // Skip this packet - return true; + + // Could happen due to the timeout listeners + if (!marker.isProcessed()) { + return false; } } - if (marker.isProcessed() && !current.isCancelled() && !hasExpired) { + // Is it okay to send the packet? + if (!current.isCancelled() && !hasExpired) { + // Make sure we're on the main thread + if (notThreadSafe) { + try { + boolean wantAsync = marker.isMinecraftAsync(current); + boolean wantSync = !wantAsync; + + // Wait for the next main thread heartbeat if we haven't fulfilled our promise + if (!onMainThread && wantSync) { + return false; + } + + // Let's give it what it wants + if (onMainThread && wantAsync) { + asynchronousSender.execute(new Runnable() { + @Override + public void run() { + // We know this isn't on the main thread + processPacketHolder(false, holder); + } + }); + + // Scheduler will do the rest + return true; + } + + } catch (FieldAccessException e) { + e.printStackTrace(); + + // Just drop the packet + return true; + } + } + // Silently skip players that have logged out if (isOnline(current.getPlayer())) { sendPacket(current); } - } + } + // Drop the packet return true; - - } else { - // Add it back and stop sending - return false; } + + // Add it back and stop sending + return false; } /** From 8c4b4fcaa41a7b59e24378bc21fd01d950bcaf22 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Wed, 21 Nov 2012 02:42:18 +0100 Subject: [PATCH 25/31] Advertise the incrementProcessingDelay() function. Discurage plugins of re-sending cancelled packets, as it makes it impossible for other plugins to take part in the processing. Assume plugin A delays transmission of packet X by cancelling the event, and then retransmitting X outside the filters. It is then impossible for another plugin B to extend the delay without fighting plugin A for control over the packet, for instance by decreasing the listener priority and cancelling first. It is much better for plugin A to call incrementProcessingDelay() in an asynchronous listener. Then plugin B can do the same, and the packet will be sent after both plugins has called signalProcessingDone(). --- .../com/comphenix/protocol/AsynchronousManager.java | 3 +++ .../comphenix/protocol/async/AsyncFilterManager.java | 6 +++++- .../protocol/async/AsyncListenerHandler.java | 3 ++- .../com/comphenix/protocol/async/AsyncMarker.java | 6 ++---- .../com/comphenix/protocol/events/PacketEvent.java | 11 ++++++++++- 5 files changed, 22 insertions(+), 7 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/AsynchronousManager.java b/ProtocolLib/src/main/java/com/comphenix/protocol/AsynchronousManager.java index 4d1d8f35..4a7d76d1 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/AsynchronousManager.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/AsynchronousManager.java @@ -22,6 +22,7 @@ import java.util.Set; import org.bukkit.plugin.Plugin; import com.comphenix.protocol.async.AsyncListenerHandler; +import com.comphenix.protocol.async.AsyncMarker; import com.comphenix.protocol.error.ErrorReporter; import com.comphenix.protocol.events.PacketEvent; import com.comphenix.protocol.events.PacketListener; @@ -35,6 +36,8 @@ public interface AsynchronousManager { /** * Registers an asynchronous packet handler. *

+ * Use {@link AsyncMarker#incrementProcessingDelay()} to delay a packet until its ready to be transmitted. + *

* To start listening asynchronously, pass the getListenerLoop() runnable to a different thread. * @param listener - the packet listener that will recieve these asynchronous events. * @return An asynchrouns handler. diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java index 92de92a1..1b3bd8cc 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java @@ -43,7 +43,9 @@ import com.google.common.collect.Sets; /** * Represents a filter manager for asynchronous packets. - * + *

+ * By using {@link AsyncMarker#incrementProcessingDelay()}, a packet can be delayed without having to block the + * processing thread. * @author Kristian */ public class AsyncFilterManager implements AsynchronousManager { @@ -128,6 +130,8 @@ public class AsyncFilterManager implements AsynchronousManager { /** * Registers an asynchronous packet handler. *

+ * Use {@link AsyncMarker#incrementProcessingDelay()} to delay a packet until its ready to be transmitted. + *

* To start listening asynchronously, pass the getListenerLoop() runnable to a different thread. *

* Asynchronous events will only be executed if a synchronous listener with the same packets is registered. diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncListenerHandler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncListenerHandler.java index 65e4871a..f37511f8 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncListenerHandler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncListenerHandler.java @@ -35,7 +35,8 @@ import com.google.common.base.Joiner; /** * Represents a handler for an asynchronous event. - * + *

+ * Use {@link AsyncMarker#incrementProcessingDelay()} to delay a packet until a certain condition has been met. * @author Kristian */ public class AsyncListenerHandler { diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java index 6cf5147c..7b6b038b 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java @@ -206,7 +206,7 @@ public class AsyncMarker implements Serializable, Comparable { } /** - * Increment the number of times this packet must be signalled as done before its transmitted. + * Increment the number of times the current packet must be signalled as done before its transmitted. *

* This is useful if an asynchronous listener is waiting for further information before the * packet can be sent to the user. A packet listener MUST eventually call @@ -215,9 +215,7 @@ public class AsyncMarker implements Serializable, Comparable { *

* It is recommended that processing outside a packet listener is wrapped in a synchronized block * using the {@link #getProcessingLock()} method. - *

- * To decrement the processing delay, call signalPacketUpdate. A thread that calls this method - * multiple times must call signalPacketUpdate at least that many times. + * * @return The new processing delay. */ public int incrementProcessingDelay() { diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/events/PacketEvent.java b/ProtocolLib/src/main/java/com/comphenix/protocol/events/PacketEvent.java index 61062772..c35330cb 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/events/PacketEvent.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/events/PacketEvent.java @@ -134,7 +134,16 @@ public class PacketEvent extends EventObject implements Cancellable { } /** - * Sets whether or not the packet should be cancelled. + * Sets whether or not the packet should be cancelled. Uncancelling is possible. + *

+ * Warning: A cancelled packet should never be re-transmitted. Use the asynchronous + * packet manager if you need to perform extensive processing. It should also be used + * if you need to synchronize with the main thread. + *

+ * This ensures that other plugins can work with the same packet. + *

+ * An asynchronous listener can also delay a packet indefinitely without having to block its thread. + * * @param cancel - TRUE if it should be cancelled, FALSE otherwise. */ public void setCancelled(boolean cancel) { From 95fe40aef38c414109b85c6470335f9e0bd7e4b7 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Wed, 21 Nov 2012 02:48:14 +0100 Subject: [PATCH 26/31] Discurage sending of cancelled packets in ProtocolManager too. --- .../comphenix/protocol/ProtocolManager.java | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolManager.java b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolManager.java index cbdc93f1..3e319249 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolManager.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolManager.java @@ -17,6 +17,7 @@ package com.comphenix.protocol; +import java.lang.reflect.InvocationTargetException; import java.util.List; import java.util.Set; @@ -25,6 +26,7 @@ import org.bukkit.entity.Entity; import org.bukkit.entity.Player; import org.bukkit.plugin.Plugin; +import com.comphenix.protocol.async.AsyncMarker; import com.comphenix.protocol.events.PacketContainer; import com.comphenix.protocol.events.PacketListener; import com.comphenix.protocol.injector.PacketConstructor; @@ -37,6 +39,37 @@ import com.google.common.collect.ImmutableSet; */ public interface ProtocolManager extends PacketStream { + /** + * Send a packet to the given player. + *

+ * Re-sending a previously cancelled packet is discuraged. Use {@link AsyncMarker#incrementProcessingDelay()} + * to delay a packet until a certain condition has been met. + * + * @param reciever - the reciever. + * @param packet - packet to send. + * @param filters - whether or not to invoke any packet filters. + * @throws InvocationTargetException - if an error occured when sending the packet. + */ + @Override + public void sendServerPacket(Player reciever, PacketContainer packet, boolean filters) + throws InvocationTargetException; + + /** + * Simulate recieving a certain packet from a given player. + *

+ * Receiving a previously cancelled packet is discuraged. Use {@link AsyncMarker#incrementProcessingDelay()} + * to delay a packet until a certain condition has been met. + * + * @param sender - the sender. + * @param packet - the packet that was sent. + * @param filters - whether or not to invoke any packet filters. + * @throws InvocationTargetException If the reflection machinery failed. + * @throws IllegalAccessException If the underlying method caused an error. + */ + @Override + public void recieveClientPacket(Player sender, PacketContainer packet, boolean filters) + throws IllegalAccessException, InvocationTargetException; + /** * Retrieves a list of every registered packet listener. * @return Every registered packet listener. From 4b19f8498b6ffce8dde9d94393b62119dec0aa7a Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Wed, 21 Nov 2012 03:40:31 +0100 Subject: [PATCH 27/31] Allow timeout listeners to cancel expiration. --- .../java/com/comphenix/protocol/async/PacketSendingQueue.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketSendingQueue.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketSendingQueue.java index d7f13233..b9d5b7b0 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketSendingQueue.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/PacketSendingQueue.java @@ -176,7 +176,7 @@ abstract class PacketSendingQueue { hasExpired = marker.hasExpired(); // Could happen due to the timeout listeners - if (!marker.isProcessed()) { + if (!marker.isProcessed() && !hasExpired) { return false; } } From 4b2f69c3c8c0d173dda6f8a6662d43fdbb55277f Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Wed, 21 Nov 2012 05:50:23 +0100 Subject: [PATCH 28/31] Fix removing intervals in the interval tree. --- .../concurrency/AbstractIntervalTree.java | 183 ++++++++++++++---- 1 file changed, 142 insertions(+), 41 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/concurrency/AbstractIntervalTree.java b/ProtocolLib/src/main/java/com/comphenix/protocol/concurrency/AbstractIntervalTree.java index f36ddf0e..571083d5 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/concurrency/AbstractIntervalTree.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/concurrency/AbstractIntervalTree.java @@ -6,6 +6,7 @@ import java.util.NavigableMap; import java.util.Set; import java.util.TreeMap; +import com.google.common.base.Objects; import com.google.common.collect.Range; import com.google.common.collect.Ranges; @@ -32,24 +33,25 @@ public abstract class AbstractIntervalTree, TValue * Represents a range and a value in this interval tree. */ public class Entry implements Map.Entry, TValue> { - private final Range key; private EndPoint left; private EndPoint right; - Entry(Range key, EndPoint left, EndPoint right) { + Entry(EndPoint left, EndPoint right) { if (left == null) throw new IllegalAccessError("left cannot be NUll"); if (right == null) throw new IllegalAccessError("right cannot be NUll"); + if (left.key.compareTo(right.key) > 0) + throw new IllegalArgumentException( + "Left key (" + left.key + ") cannot be greater than the right key (" + right.key + ")"); - this.key = key; this.left = left; this.right = right; } @Override public Range getKey() { - return key; + return Ranges.closed(left.key, right.key); } @Override @@ -66,6 +68,31 @@ public abstract class AbstractIntervalTree, TValue right.value = value; return old; } + + @SuppressWarnings("rawtypes") + @Override + public boolean equals(Object obj) { + // Quick equality check + if (obj == this) { + return true; + } else if (obj instanceof AbstractIntervalTree.Entry) { + return Objects.equal(left.key, ((AbstractIntervalTree.Entry) obj).left.key) && + Objects.equal(right.key, ((AbstractIntervalTree.Entry) obj).right.key) && + Objects.equal(left.value, ((AbstractIntervalTree.Entry) obj).left.value); + } else { + return false; + } + } + + @Override + public int hashCode() { + return Objects.hashCode(left.key, right.key, left.value); + } + + @Override + public String toString() { + return String.format("Value %s at [%s, %s]", left.value, left.key, right.key); + } } /** @@ -78,9 +105,13 @@ public abstract class AbstractIntervalTree, TValue // The value this range contains public TValue value; + + // The key of this end point + public TKey key; - public EndPoint(State state, TValue value) { + public EndPoint(State state, TKey key, TValue value) { this.state = state; + this.key = key; this.value = value; } } @@ -96,7 +127,7 @@ public abstract class AbstractIntervalTree, TValue public Set remove(TKey lowerBound, TKey upperBound) { return remove(lowerBound, upperBound, false); } - + /** * Removes every interval that intersects with the given range. * @param lowerBound - lowest value to remove. @@ -107,31 +138,46 @@ public abstract class AbstractIntervalTree, TValue checkBounds(lowerBound, upperBound); NavigableMap range = bounds.subMap(lowerBound, true, upperBound, true); - boolean emptyRange = range.isEmpty(); - TKey first = !emptyRange ? range.firstKey() : null; - TKey last = !emptyRange ? range.lastKey() : null; - + EndPoint first = getNextEndPoint(lowerBound, true); + EndPoint last = getPreviousEndPoint(upperBound, true); + + // Used while resizing intervals + EndPoint previous = null; + EndPoint next = null; + Set resized = new HashSet(); Set removed = new HashSet(); // Remove the previous element too. A close end-point must be preceded by an OPEN end-point. - if (first != null && range.get(first).state == State.CLOSE) { - TKey key = bounds.floorKey(first); - EndPoint removedPoint = removeIfNonNull(key); + if (first != null && first.state == State.CLOSE) { + previous = getPreviousEndPoint(first.key, false); // Add the interval back - if (removedPoint != null && preserveDifference) { - resized.add(putUnsafe(key, decrementKey(lowerBound), removedPoint.value)); + if (previous != null) { + removed.add(getEntry(previous, first)); } } // Get the closing element too. - if (last != null && range.get(last).state == State.OPEN) { - TKey key = bounds.ceilingKey(last); - EndPoint removedPoint = removeIfNonNull(key); + if (last != null && last.state == State.OPEN) { + next = getNextEndPoint(last.key, false); - if (removedPoint != null && preserveDifference) { - resized.add(putUnsafe(incrementKey(upperBound), key, removedPoint.value)); + if (next != null) { + removed.add(getEntry(last, next)); + } + } + + // Now remove both ranges + removeEntrySafely(previous, first); + removeEntrySafely(last, next); + + // Add new resized intervals + if (preserveDifference) { + if (previous != null) { + resized.add(putUnsafe(previous.key, decrementKey(lowerBound), previous.value)); + } + if (next != null) { + resized.add(putUnsafe(incrementKey(upperBound), next.key, next.value)); } } @@ -140,7 +186,6 @@ public abstract class AbstractIntervalTree, TValue invokeEntryRemoved(removed); if (preserveDifference) { - invokeEntryRemoved(resized); invokeEntryAdded(resized); } @@ -149,12 +194,30 @@ public abstract class AbstractIntervalTree, TValue return removed; } - // Helper - private EndPoint removeIfNonNull(TKey key) { - if (key != null) { - return bounds.remove(key); + /** + * Retrieve the entry from a given set of end points. + * @param left - leftmost end point. + * @param right - rightmost end point. + * @return The associated entry. + */ + protected Entry getEntry(EndPoint left, EndPoint right) { + if (left == null) + throw new IllegalArgumentException("left endpoint cannot be NULL."); + if (right == null) + throw new IllegalArgumentException("right endpoint cannot be NULL."); + + // Make sure the order is correct + if (right.key.compareTo(left.key) < 0) { + return getEntry(right, left); } else { - return null; + return new Entry(left, right); + } + } + + private void removeEntrySafely(EndPoint left, EndPoint right) { + if (left != null && right != null) { + bounds.remove(left.key); + bounds.remove(right.key); } } @@ -165,7 +228,7 @@ public abstract class AbstractIntervalTree, TValue if (endPoint != null) { endPoint.state = State.BOTH; } else { - endPoint = new EndPoint(state, value); + endPoint = new EndPoint(state, key, value); bounds.put(key, endPoint); } return endPoint; @@ -198,9 +261,8 @@ public abstract class AbstractIntervalTree, TValue if (value != null) { EndPoint left = addEndPoint(lowerBound, value, State.OPEN); EndPoint right = addEndPoint(upperBound, value, State.CLOSE); - - Range range = Ranges.closed(lowerBound, upperBound); - return new Entry(range, left, right); + + return new Entry(left, right); } else { return null; } @@ -261,11 +323,12 @@ public abstract class AbstractIntervalTree, TValue switch (entry.getValue().state) { case BOTH: EndPoint point = entry.getValue(); - destination.add(new Entry(Ranges.singleton(entry.getKey()), point, point)); + destination.add(new Entry(point, point)); break; case CLOSE: - Range range = Ranges.closed(last.getKey(), entry.getKey()); - destination.add(new Entry(range, last.getValue(), entry.getValue())); + if (last != null) { + destination.add(new Entry(last.getValue(), entry.getValue())); + } break; case OPEN: // We don't know the full range yet @@ -284,7 +347,7 @@ public abstract class AbstractIntervalTree, TValue public void putAll(AbstractIntervalTree other) { // Naively copy every range. for (Entry entry : other.entrySet()) { - put(entry.key.lowerEndpoint(), entry.key.upperEndpoint(), entry.getValue()); + put(entry.left.key, entry.right.key, entry.getValue()); } } @@ -303,7 +366,7 @@ public abstract class AbstractIntervalTree, TValue } /** - * Get the end-point composite associated with this key. + * Get the left-most end-point associated with this key. * @param key - key to search for. * @return The end point found, or NULL. */ @@ -311,22 +374,60 @@ public abstract class AbstractIntervalTree, TValue EndPoint ends = bounds.get(key); if (ends != null) { - // This is a piece of cake - return ends; - } else { + // Always return the end point to the left + if (ends.state == State.CLOSE) { + Map.Entry left = bounds.floorEntry(decrementKey(key)); + return left != null ? left.getValue() : null; + + } else { + return ends; + } + } else { // We need to determine if the point intersects with a range - TKey left = bounds.floorKey(key); + Map.Entry left = bounds.floorEntry(key); // We only need to check to the left - if (left != null && bounds.get(left).state == State.OPEN) { - return bounds.get(left); + if (left != null && left.getValue().state == State.OPEN) { + return left.getValue(); } else { return null; } } } + /** + * Get the previous end point of a given key. + * @param point - the point to search with. + * @param inclusive - whether or not to include the current point in the search. + * @return The previous end point of a given given key, or NULL if not found. + */ + protected EndPoint getPreviousEndPoint(TKey point, boolean inclusive) { + if (point != null) { + Map.Entry previous = bounds.floorEntry(inclusive ? point : decrementKey(point)); + + if (previous != null) + return previous.getValue(); + } + return null; + } + + /** + * Get the next end point of a given key. + * @param point - the point to search with. + * @param inclusive - whether or not to include the current point in the search. + * @return The next end point of a given given key, or NULL if not found. + */ + protected EndPoint getNextEndPoint(TKey point, boolean inclusive) { + if (point != null) { + Map.Entry next = bounds.ceilingEntry(inclusive ? point : incrementKey(point)); + + if (next != null) + return next.getValue(); + } + return null; + } + private void invokeEntryAdded(Entry added) { if (added != null) { onEntryAdded(added); From 21a1dfcbac75ce820ec10f6651db03afe65e621a Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Wed, 21 Nov 2012 05:53:26 +0100 Subject: [PATCH 29/31] Preserve intervals outside the given range to remove. --- .../src/main/java/com/comphenix/protocol/CommandPacket.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/CommandPacket.java b/ProtocolLib/src/main/java/com/comphenix/protocol/CommandPacket.java index d609bf4e..3b8245c9 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/CommandPacket.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/CommandPacket.java @@ -459,9 +459,9 @@ class CommandPacket extends CommandBase { // The interval tree will automatically remove the listeners for us if (side.isForClient()) - result.addAll(clientListeners.remove(idStart, idStop)); + result.addAll(clientListeners.remove(idStart, idStop, true)); if (side.isForServer()) - result.addAll(serverListeners.remove(idStart, idStop)); + result.addAll(serverListeners.remove(idStart, idStop, true)); return result; } From 78387a033b8f01ab498b12b9c08c01138b72b781 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Wed, 21 Nov 2012 05:57:07 +0100 Subject: [PATCH 30/31] Bumping to version 1.7.0 --- ProtocolLib/pom.xml | 2 +- ProtocolLib/src/main/resources/plugin.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ProtocolLib/pom.xml b/ProtocolLib/pom.xml index 8c71c890..5897f7c6 100644 --- a/ProtocolLib/pom.xml +++ b/ProtocolLib/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.comphenix.protocol ProtocolLib - 1.6.1-SNAPSHOT + 1.7.0 jar Provides read/write access to the Minecraft protocol. diff --git a/ProtocolLib/src/main/resources/plugin.yml b/ProtocolLib/src/main/resources/plugin.yml index eabb68c0..d632e814 100644 --- a/ProtocolLib/src/main/resources/plugin.yml +++ b/ProtocolLib/src/main/resources/plugin.yml @@ -1,5 +1,5 @@ name: ProtocolLib -version: 1.6.1-SNAPSHOT +version: 1.7.0 description: Provides read/write access to the Minecraft protocol. author: Comphenix website: http://www.comphenix.net/ProtocolLib From a4f79ccb3f31fbac1e05ecc2ce5137dae33e7f91 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Wed, 21 Nov 2012 06:27:18 +0100 Subject: [PATCH 31/31] Maven crap. --- ProtocolLib/dependency-reduced-pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ProtocolLib/dependency-reduced-pom.xml b/ProtocolLib/dependency-reduced-pom.xml index e3b1885e..a83cd055 100644 --- a/ProtocolLib/dependency-reduced-pom.xml +++ b/ProtocolLib/dependency-reduced-pom.xml @@ -4,7 +4,7 @@ com.comphenix.protocol ProtocolLib ProtocolLib - 1.6.1-SNAPSHOT + 1.7.0 Provides read/write access to the Minecraft protocol. http://dev.bukkit.org/server-mods/protocollib/