From f9285f05593663ec59cd8df16e499e2eaa58010e Mon Sep 17 00:00:00 2001 From: crast Date: Thu, 21 Mar 2013 18:13:20 -0600 Subject: [PATCH] Prevent classloader leakage in metadata system. Fixes Bukkit-3854 Metadata values keep strong reference to plugins, and they're not cleared out when plugins are unloaded. This system adds weak reference logic to allow these values to fall out of scope. In addition we get some operations turning to O(1) "for free." --- .../org/bukkit/metadata/MetadataStoreBase.java | 48 +++++++------------ .../org/bukkit/metadata/MetadataValueAdapter.java | 8 ++- 2 files changed, 23 insertions(+), 33 deletions(-) diff --git a/src/main/java/org/bukkit/metadata/MetadataStoreBase.java b/src/main/java/org/bukkit/metadata/MetadataStoreBase.java index 7febbd4..8b7aabc 100644 --- a/src/main/java/org/bukkit/metadata/MetadataStoreBase.java +++ b/src/main/java/org/bukkit/metadata/MetadataStoreBase.java @@ -6,7 +6,7 @@ import org.bukkit.plugin.Plugin; import java.util.*; public abstract class MetadataStoreBase { - private Map> metadataMap = new HashMap>(); + private Map> metadataMap = new HashMap>(); private WeakHashMap> disambiguationCache = new WeakHashMap>(); /** @@ -26,23 +26,16 @@ public abstract class MetadataStoreBase { * @throws IllegalArgumentException If value is null, or the owning plugin is null */ public synchronized void setMetadata(T subject, String metadataKey, MetadataValue newMetadataValue) { + Plugin owningPlugin = newMetadataValue.getOwningPlugin(); Validate.notNull(newMetadataValue, "Value cannot be null"); - Validate.notNull(newMetadataValue.getOwningPlugin(), "Plugin cannot be null"); + Validate.notNull(owningPlugin, "Plugin cannot be null"); String key = cachedDisambiguate(subject, metadataKey); - if (!metadataMap.containsKey(key)) { - metadataMap.put(key, new ArrayList()); - } - // we now have a list of subject's metadata for the given metadata key. If newMetadataValue's owningPlugin - // is found in this list, replace the value rather than add a new one. - List metadataList = metadataMap.get(key); - for (int i = 0; i < metadataList.size(); i++) { - if (metadataList.get(i).getOwningPlugin().equals(newMetadataValue.getOwningPlugin())) { - metadataList.set(i, newMetadataValue); - return; - } + Map entry = metadataMap.get(key); + if (entry == null) { + entry = new WeakHashMap(1); + metadataMap.put(key, entry); } - // we didn't find a duplicate...add the new metadata value - metadataList.add(newMetadataValue); + entry.put(owningPlugin, newMetadataValue); } /** @@ -57,7 +50,8 @@ public abstract class MetadataStoreBase { public synchronized List getMetadata(T subject, String metadataKey) { String key = cachedDisambiguate(subject, metadataKey); if (metadataMap.containsKey(key)) { - return Collections.unmodifiableList(metadataMap.get(key)); + Collection values = metadataMap.get(key).values(); + return Collections.unmodifiableList(new ArrayList(values)); } else { return Collections.emptyList(); } @@ -87,15 +81,11 @@ public abstract class MetadataStoreBase { public synchronized void removeMetadata(T subject, String metadataKey, Plugin owningPlugin) { Validate.notNull(owningPlugin, "Plugin cannot be null"); String key = cachedDisambiguate(subject, metadataKey); - List metadataList = metadataMap.get(key); - if (metadataList == null) return; - for (int i = 0; i < metadataList.size(); i++) { - if (metadataList.get(i).getOwningPlugin().equals(owningPlugin)) { - metadataList.remove(i); - if (metadataList.isEmpty()) { - metadataMap.remove(key); - } - } + Map entry = metadataMap.get(key); + if (entry == null) return; + entry.remove(owningPlugin); + if (entry.isEmpty()) { + metadataMap.remove(key); } } @@ -109,11 +99,9 @@ public abstract class MetadataStoreBase { */ public synchronized void invalidateAll(Plugin owningPlugin) { Validate.notNull(owningPlugin, "Plugin cannot be null"); - for (List values : metadataMap.values()) { - for (MetadataValue value : values) { - if (value.getOwningPlugin().equals(owningPlugin)) { - value.invalidate(); - } + for (Map values : metadataMap.values()) { + if (values.containsKey(owningPlugin)) { + values.get(owningPlugin).invalidate(); } } } diff --git a/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java b/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java index c4b8b39..3b83380 100644 --- a/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java +++ b/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java @@ -1,5 +1,7 @@ package org.bukkit.metadata; +import java.lang.ref.WeakReference; + import org.apache.commons.lang.Validate; import org.bukkit.plugin.Plugin; import org.bukkit.util.NumberConversions; @@ -13,15 +15,15 @@ import org.bukkit.util.NumberConversions; * */ public abstract class MetadataValueAdapter implements MetadataValue { - protected final Plugin owningPlugin; + protected final WeakReference owningPlugin; protected MetadataValueAdapter(Plugin owningPlugin) { Validate.notNull(owningPlugin, "owningPlugin cannot be null"); - this.owningPlugin = owningPlugin; + this.owningPlugin = new WeakReference(owningPlugin); } public Plugin getOwningPlugin() { - return owningPlugin; + return owningPlugin.get(); } public int asInt() { -- 1.7.0.4