Prevent classloader leak in metadata system. Fixes BUKKIT-3854

Metadata values keep strong reference to plugins and they are 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."

By: crast <contact@jamescrasta.com>
This commit is contained in:
Bukkit/Spigot 2013-03-21 18:13:20 -06:00
parent 1378ec9381
commit 2436d2b3f8
2 changed files with 26 additions and 33 deletions

View file

@ -6,7 +6,7 @@ import org.bukkit.plugin.Plugin;
import java.util.*; import java.util.*;
public abstract class MetadataStoreBase<T> { public abstract class MetadataStoreBase<T> {
private Map<String, List<MetadataValue>> metadataMap = new HashMap<String, List<MetadataValue>>(); private Map<String, Map<Plugin, MetadataValue>> metadataMap = new HashMap<String, Map<Plugin, MetadataValue>>();
/** /**
* Adds a metadata value to an object. Each metadata value is owned by a specific{@link Plugin}. * Adds a metadata value to an object. Each metadata value is owned by a specific{@link Plugin}.
@ -25,23 +25,16 @@ public abstract class MetadataStoreBase<T> {
* @throws IllegalArgumentException If value is null, or the owning plugin is null * @throws IllegalArgumentException If value is null, or the owning plugin is null
*/ */
public synchronized void setMetadata(T subject, String metadataKey, MetadataValue newMetadataValue) { public synchronized void setMetadata(T subject, String metadataKey, MetadataValue newMetadataValue) {
Plugin owningPlugin = newMetadataValue.getOwningPlugin();
Validate.notNull(newMetadataValue, "Value cannot be null"); Validate.notNull(newMetadataValue, "Value cannot be null");
Validate.notNull(newMetadataValue.getOwningPlugin(), "Plugin cannot be null"); Validate.notNull(owningPlugin, "Plugin cannot be null");
String key = disambiguate(subject, metadataKey); String key = disambiguate(subject, metadataKey);
if (!metadataMap.containsKey(key)) { Map<Plugin, MetadataValue> entry = metadataMap.get(key);
metadataMap.put(key, new ArrayList<MetadataValue>()); if (entry == null) {
entry = new WeakHashMap<Plugin, MetadataValue>(1);
metadataMap.put(key, entry);
} }
// we now have a list of subject's metadata for the given metadata key. If newMetadataValue's owningPlugin entry.put(owningPlugin, newMetadataValue);
// is found in this list, replace the value rather than add a new one.
List<MetadataValue> 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;
}
}
// we didn't find a duplicate...add the new metadata value
metadataList.add(newMetadataValue);
} }
/** /**
@ -56,7 +49,8 @@ public abstract class MetadataStoreBase<T> {
public synchronized List<MetadataValue> getMetadata(T subject, String metadataKey) { public synchronized List<MetadataValue> getMetadata(T subject, String metadataKey) {
String key = disambiguate(subject, metadataKey); String key = disambiguate(subject, metadataKey);
if (metadataMap.containsKey(key)) { if (metadataMap.containsKey(key)) {
return Collections.unmodifiableList(metadataMap.get(key)); Collection<MetadataValue> values = metadataMap.get(key).values();
return Collections.unmodifiableList(new ArrayList<MetadataValue>(values));
} else { } else {
return Collections.emptyList(); return Collections.emptyList();
} }
@ -86,15 +80,14 @@ public abstract class MetadataStoreBase<T> {
public synchronized void removeMetadata(T subject, String metadataKey, Plugin owningPlugin) { public synchronized void removeMetadata(T subject, String metadataKey, Plugin owningPlugin) {
Validate.notNull(owningPlugin, "Plugin cannot be null"); Validate.notNull(owningPlugin, "Plugin cannot be null");
String key = disambiguate(subject, metadataKey); String key = disambiguate(subject, metadataKey);
List<MetadataValue> metadataList = metadataMap.get(key); Map<Plugin, MetadataValue> entry = metadataMap.get(key);
if (metadataList == null) return; if (entry == null) {
for (int i = 0; i < metadataList.size(); i++) { return;
if (metadataList.get(i).getOwningPlugin().equals(owningPlugin)) { }
metadataList.remove(i);
if (metadataList.isEmpty()) { entry.remove(owningPlugin);
metadataMap.remove(key); if (entry.isEmpty()) {
} metadataMap.remove(key);
}
} }
} }
@ -108,11 +101,9 @@ public abstract class MetadataStoreBase<T> {
*/ */
public synchronized void invalidateAll(Plugin owningPlugin) { public synchronized void invalidateAll(Plugin owningPlugin) {
Validate.notNull(owningPlugin, "Plugin cannot be null"); Validate.notNull(owningPlugin, "Plugin cannot be null");
for (List<MetadataValue> values : metadataMap.values()) { for (Map<Plugin, MetadataValue> values : metadataMap.values()) {
for (MetadataValue value : values) { if (values.containsKey(owningPlugin)) {
if (value.getOwningPlugin().equals(owningPlugin)) { values.get(owningPlugin).invalidate();
value.invalidate();
}
} }
} }
} }

View file

@ -1,5 +1,7 @@
package org.bukkit.metadata; package org.bukkit.metadata;
import java.lang.ref.WeakReference;
import org.apache.commons.lang.Validate; import org.apache.commons.lang.Validate;
import org.bukkit.plugin.Plugin; import org.bukkit.plugin.Plugin;
import org.bukkit.util.NumberConversions; import org.bukkit.util.NumberConversions;
@ -13,15 +15,15 @@ import org.bukkit.util.NumberConversions;
* *
*/ */
public abstract class MetadataValueAdapter implements MetadataValue { public abstract class MetadataValueAdapter implements MetadataValue {
protected final Plugin owningPlugin; protected final WeakReference<Plugin> owningPlugin;
protected MetadataValueAdapter(Plugin owningPlugin) { protected MetadataValueAdapter(Plugin owningPlugin) {
Validate.notNull(owningPlugin, "owningPlugin cannot be null"); Validate.notNull(owningPlugin, "owningPlugin cannot be null");
this.owningPlugin = owningPlugin; this.owningPlugin = new WeakReference<Plugin>(owningPlugin);
} }
public Plugin getOwningPlugin() { public Plugin getOwningPlugin() {
return owningPlugin; return owningPlugin.get();
} }
public int asInt() { public int asInt() {