Archiviert
13
0

Prevented concurrency issues.

The SortedCopyOnWriteArray didn't wrap the iterator() in a
unmodifiable iterator. In addition, ConcurrentListenerMultimap
incorrectly used the iterator to remove objects from this Array. 

Added a "remove(T value)" method that is thread-safe.
Dieser Commit ist enthalten in:
Kristian S. Stangeland 2012-09-14 22:33:15 +02:00
Ursprung 88dcf0cb32
Commit 4505e31965
3 geänderte Dateien mit 73 neuen und 29 gelöschten Zeilen

Datei anzeigen

@ -1,7 +1,6 @@
package com.comphenix.protocol.injector;
import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
@ -13,6 +12,7 @@ import com.comphenix.protocol.events.ListeningWhitelist;
import com.comphenix.protocol.events.PacketAdapter;
import com.comphenix.protocol.events.PacketEvent;
import com.comphenix.protocol.events.PacketListener;
import com.google.common.base.Objects;
import com.google.common.primitives.Ints;
/**
@ -59,10 +59,8 @@ public class ConcurrentListenerMultimap {
}
}
// Careful when modifying the set
synchronized(list) {
list.insertSorted(listener);
}
// Thread safe
list.add(listener);
}
/**
@ -81,20 +79,14 @@ public class ConcurrentListenerMultimap {
// Remove any listeners
if (list != null) {
synchronized(list) {
// Don't remove from newly created lists
if (list.size() > 0) {
// Remove this listener
for (Iterator<PrioritizedListener> it = list.iterator(); it.hasNext(); ) {
if (it.next().getListener().equals(list)) {
it.remove();
}
}
if (list.size() == 0) {
listeners.remove(packetID);
removedPackets.add(packetID);
}
// Don't remove from newly created lists
if (list.size() > 0) {
// Remove this listener. Note that priority is generally ignored.
list.remove(new PrioritizedListener(listener, whitelist.getPriority()));
if (list.size() == 0) {
listeners.remove(packetID);
removedPackets.add(packetID);
}
}
}
@ -175,6 +167,19 @@ public class ConcurrentListenerMultimap {
other.getPriority().getSlot());
}
// Note that this equals() method is NOT consistent with compareTo().
// But, it's a private class so who cares.
@Override
public boolean equals(Object obj) {
// We only care about the listener - priority itself should not make a difference
if(obj instanceof PrioritizedListener){
final PrioritizedListener other = (PrioritizedListener) obj;
return Objects.equal(listener, other.listener);
} else {
return false;
}
}
public PacketListener getListener() {
return listener;
}

Datei anzeigen

@ -4,11 +4,12 @@ import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;
import com.google.common.base.Objects;
import com.google.common.collect.Iterables;
/**
* An implicitly sorted array list that preserves insertion order and maintains duplicates.
*
* Note that only the {@link insertSorted} method will update the list correctly,
* @param <T> - type of the sorted list.
* @param <T> - type of the elements in the list.
*/
class SortedCopyOnWriteArray<T> implements Iterable<T> {
// Prevent reordering
@ -27,7 +28,7 @@ class SortedCopyOnWriteArray<T> implements Iterable<T> {
* @param value - element to insert.
*/
@SuppressWarnings("unchecked")
public synchronized void insertSorted(T value) {
public synchronized void add(T value) {
// We use NULL as a special marker, so we don't allow it
if (value == null)
@ -52,6 +53,43 @@ class SortedCopyOnWriteArray<T> implements Iterable<T> {
list = copy;
}
/**
* Removes from the list by making a new list with every element except the one given.
* <p>
* Objects will be compared using the given objects equals() method.
* @param value - value to remove.
*/
public synchronized void remove(T value) {
List<T> copy = new ArrayList<T>();
// Copy every element except the one given to us
for (T element : list) {
if (value != null && !Objects.equal(value, element)) {
copy.add(element);
value = null;
}
}
list = copy;
}
/**
* Removes from the list by making a copy of every element except the one with the given index.
* @param index - index of the element to remove.
*/
public synchronized void remove(int index) {
List<T> copy = new ArrayList<T>(list);
copy.remove(index);
list = copy;
}
/**
* Retrieves an element by index.
* @param index - index of element to retrieve.
* @return The element at the given location.
*/
public T get(int index) {
return list.get(index);
}
@ -65,9 +103,10 @@ class SortedCopyOnWriteArray<T> implements Iterable<T> {
}
/**
* Retrieves an iterator over the elements in the given list.
* Retrieves an iterator over the elements in the given list.
* Warning: No not attempt to remove elements using the iterator.
*/
public Iterator<T> iterator() {
return list.iterator();
return Iterables.unmodifiableIterable(list).iterator();
}
}

Datei anzeigen

@ -32,7 +32,7 @@ public class SortedCopyOnWriteArrayTest {
// O(n^2) of course, so don't use too large numbers
for (int i = 0; i < MAX_NUMBER; i++) {
test.insertSorted(numbers.get(i));
test.add(numbers.get(i));
}
// Check that everything is correct
@ -48,9 +48,9 @@ public class SortedCopyOnWriteArrayTest {
PriorityStuff c = new PriorityStuff(ListenerPriority.NORMAL, 3);
SortedCopyOnWriteArray<PriorityStuff> test = new SortedCopyOnWriteArray<PriorityStuff>();
test.insertSorted(a);
test.insertSorted(b);
test.insertSorted(c);
test.add(a);
test.add(b);
test.add(c);
// Make sure the normal's are in the right order
assertEquals(2, test.get(0).id);