[Bleeding] Fixes deadlock issue and generic usage. Fixes BUKKIT-844 and fixes BUKKIT-854

3 files changed, 60 insertions, 37 deletions

diff --git a/src/main/java/org/bukkit/event/server/ServiceRegisterEvent.java b/src/main/java/org/bukkit/event/server/ServiceRegisterEvent.java
index 89c852d5..628110aa 100644
--- a/src/main/java/org/bukkit/event/server/ServiceRegisterEvent.java
+++ b/src/main/java/org/bukkit/event/server/ServiceRegisterEvent.java
@@ -4,7 +4,9 @@ import org.bukkit.event.HandlerList;
 import org.bukkit.plugin.RegisteredServiceProvider;
 
 /**
- * This event is called when a service is registered.
+ * This event is called when a service is registered.<br>
+ * <b>Warning:</b> The order in which register and unregister
+ * events are called should not be relied upon.
  */
 public class ServiceRegisterEvent extends ServiceEvent {
     private static final HandlerList handlers = new HandlerList();
@@ -13,6 +15,7 @@ public class ServiceRegisterEvent extends ServiceEvent {
         super(registeredProvider);
     }
 
+    @Override
     public HandlerList getHandlers() {
         return handlers;
     }
diff --git a/src/main/java/org/bukkit/event/server/ServiceUnregisterEvent.java b/src/main/java/org/bukkit/event/server/ServiceUnregisterEvent.java
index 608f5576..e018f8e6 100644
--- a/src/main/java/org/bukkit/event/server/ServiceUnregisterEvent.java
+++ b/src/main/java/org/bukkit/event/server/ServiceUnregisterEvent.java
@@ -4,7 +4,9 @@ import org.bukkit.event.HandlerList;
 import org.bukkit.plugin.RegisteredServiceProvider;
 
 /**
- * This event is called when a service is unregistered.
+ * This event is called when a service is unregistered.<br>
+ * <b>Warning:</b> The order in which register and unregister
+ * events are called should not be relied upon.
  */
 public class ServiceUnregisterEvent extends ServiceEvent {
     private static final HandlerList handlers = new HandlerList();
@@ -13,6 +15,7 @@ public class ServiceUnregisterEvent extends ServiceEvent {
         super(serviceProvider);
     }
 
+    @Override
     public HandlerList getHandlers() {
         return handlers;
     }
diff --git a/src/main/java/org/bukkit/plugin/SimpleServicesManager.java b/src/main/java/org/bukkit/plugin/SimpleServicesManager.java
index 94600518..4e177112 100644
--- a/src/main/java/org/bukkit/plugin/SimpleServicesManager.java
+++ b/src/main/java/org/bukkit/plugin/SimpleServicesManager.java
@@ -4,14 +4,17 @@ import org.bukkit.Bukkit;
 import org.bukkit.event.server.ServiceRegisterEvent;
 import org.bukkit.event.server.ServiceUnregisterEvent;
 
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
 import java.util.List;
 import java.util.NoSuchElementException;
+import java.util.Set;
 
 /**
  * A simple services manager.
@@ -33,24 +36,26 @@ public class SimpleServicesManager implements ServicesManager {
      * @param priority priority of the provider
      */
     public <T> void register(Class<T> service, T provider, Plugin plugin, ServicePriority priority) {
-
+        RegisteredServiceProvider<T> registeredProvider = null;
         synchronized (providers) {
             List<RegisteredServiceProvider<?>> registered = providers.get(service);
-
             if (registered == null) {
                 registered = new ArrayList<RegisteredServiceProvider<?>>();
                 providers.put(service, registered);
             }
-            RegisteredServiceProvider<T> registeredProvider = new RegisteredServiceProvider<T>(service, provider, priority, plugin);
 
-            registered.add(registeredProvider);
+            registeredProvider = new RegisteredServiceProvider<T>(service, provider, priority, plugin);
 
-            // Make sure that providers are in the right order in order
-            // for priorities to work correctly
-            Collections.sort(registered);
+            // Insert the provider into the collection, much more efficient big O than sort
+            int position = Collections.binarySearch(registered, registeredProvider);
+            if (position < 0) {
+                registered.add(-(position + 1), registeredProvider);
+            } else {
+                registered.add(position, registeredProvider);
+            }
 
-            Bukkit.getServer().getPluginManager().callEvent(new ServiceRegisterEvent(registeredProvider));
         }
+        Bukkit.getServer().getPluginManager().callEvent(new ServiceRegisterEvent(registeredProvider));
     }
 
     /**
@@ -59,6 +64,7 @@ public class SimpleServicesManager implements ServicesManager {
      * @param plugin The plugin
      */
     public void unregisterAll(Plugin plugin) {
+        ArrayList<ServiceUnregisterEvent> unregisteredEvents = new ArrayList<ServiceUnregisterEvent>();
         synchronized (providers) {
             Iterator<Map.Entry<Class<?>, List<RegisteredServiceProvider<?>>>> it = providers.entrySet().iterator();
 
@@ -73,9 +79,9 @@ public class SimpleServicesManager implements ServicesManager {
                         while (it2.hasNext()) {
                             RegisteredServiceProvider<?> registered = it2.next();
 
-                            if (registered.getPlugin() == plugin) {
+                            if (registered.getPlugin().equals(plugin)) {
                                 it2.remove();
-                                Bukkit.getServer().getPluginManager().callEvent(new ServiceUnregisterEvent(registered));
+                                unregisteredEvents.add(new ServiceUnregisterEvent(registered));
                             }
                         }
                     } catch (NoSuchElementException e) { // Why does Java suck
@@ -88,6 +94,9 @@ public class SimpleServicesManager implements ServicesManager {
                 }
             } catch (NoSuchElementException e) {}
         }
+        for (ServiceUnregisterEvent event : unregisteredEvents) {
+            Bukkit.getServer().getPluginManager().callEvent(event);
+        }
     }
 
     /**
@@ -97,6 +106,7 @@ public class SimpleServicesManager implements ServicesManager {
      * @param provider The service provider implementation
      */
     public void unregister(Class<?> service, Object provider) {
+        ArrayList<ServiceUnregisterEvent> unregisteredEvents = new ArrayList<ServiceUnregisterEvent>();
         synchronized (providers) {
             Iterator<Map.Entry<Class<?>, List<RegisteredServiceProvider<?>>>> it = providers.entrySet().iterator();
 
@@ -119,7 +129,7 @@ public class SimpleServicesManager implements ServicesManager {
 
                             if (registered.getProvider() == provider) {
                                 it2.remove();
-                                Bukkit.getServer().getPluginManager().callEvent(new ServiceUnregisterEvent(registered));
+                                unregisteredEvents.add(new ServiceUnregisterEvent(registered));
                             }
                         }
                     } catch (NoSuchElementException e) { // Why does Java suck
@@ -132,6 +142,9 @@ public class SimpleServicesManager implements ServicesManager {
                 }
             } catch (NoSuchElementException e) {}
         }
+        for (ServiceUnregisterEvent event : unregisteredEvents) {
+            Bukkit.getServer().getPluginManager().callEvent(event);
+        }
     }
 
     /**
@@ -140,6 +153,7 @@ public class SimpleServicesManager implements ServicesManager {
      * @param provider The service provider implementation
      */
     public void unregister(Object provider) {
+        ArrayList<ServiceUnregisterEvent> unregisteredEvents = new ArrayList<ServiceUnregisterEvent>();
         synchronized (providers) {
             Iterator<Map.Entry<Class<?>, List<RegisteredServiceProvider<?>>>> it = providers.entrySet().iterator();
 
@@ -154,9 +168,9 @@ public class SimpleServicesManager implements ServicesManager {
                         while (it2.hasNext()) {
                             RegisteredServiceProvider<?> registered = it2.next();
 
-                            if (registered.getProvider() == provider) {
+                            if (registered.getProvider().equals(provider)) {
                                 it2.remove();
-                                Bukkit.getServer().getPluginManager().callEvent(new ServiceUnregisterEvent(registered));
+                                unregisteredEvents.add(new ServiceUnregisterEvent(registered));
                             }
                         }
                     } catch (NoSuchElementException e) { // Why does Java suck
@@ -169,6 +183,9 @@ public class SimpleServicesManager implements ServicesManager {
                 }
             } catch (NoSuchElementException e) {}
         }
+        for (ServiceUnregisterEvent event : unregisteredEvents) {
+            Bukkit.getServer().getPluginManager().callEvent(event);
+        }
     }
 
     /**
@@ -179,7 +196,6 @@ public class SimpleServicesManager implements ServicesManager {
      * @param service The service interface
      * @return provider or null
      */
-    @SuppressWarnings("unchecked")
     public <T> T load(Class<T> service) {
         synchronized (providers) {
             List<RegisteredServiceProvider<?>> registered = providers.get(service);
@@ -189,7 +205,7 @@ public class SimpleServicesManager implements ServicesManager {
             }
 
             // This should not be null!
-            return (T) registered.get(0).getProvider();
+            return service.cast(registered.get(0).getProvider());
         }
     }
 
@@ -222,68 +238,69 @@ public class SimpleServicesManager implements ServicesManager {
      * @return provider registration or null
      */
     public List<RegisteredServiceProvider<?>> getRegistrations(Plugin plugin) {
+        ImmutableList.Builder<RegisteredServiceProvider<?>> ret = ImmutableList.<RegisteredServiceProvider<?>>builder();
         synchronized (providers) {
-            List<RegisteredServiceProvider<?>> ret = new ArrayList<RegisteredServiceProvider<?>>();
-
             for (List<RegisteredServiceProvider<?>> registered : providers.values()) {
                 for (RegisteredServiceProvider<?> provider : registered) {
-                    if (provider.getPlugin() == plugin) {
+                    if (provider.getPlugin().equals(plugin)) {
                         ret.add(provider);
                     }
                 }
             }
-
-            return ret;
         }
+        return ret.build();
     }
 
     /**
      * Get registrations of providers for a service. The returned list is
-     * unmodifiable.
+     * an unmodifiable copy.
      *
      * @param <T> The service interface
      * @param service The service interface
-     * @return list of registrations
+     * @return a copy of the list of registrations
      */
     @SuppressWarnings("unchecked")
-    public <T> Collection<RegisteredServiceProvider<T>> getRegistrations(Class<T> service) {
+    public <T> List<RegisteredServiceProvider<T>> getRegistrations(Class<T> service) {
+        ImmutableList.Builder<RegisteredServiceProvider<T>> ret;
         synchronized (providers) {
             List<RegisteredServiceProvider<?>> registered = providers.get(service);
 
             if (registered == null) {
-                return Collections.unmodifiableList(new ArrayList<RegisteredServiceProvider<T>>());
+                return ImmutableList.<RegisteredServiceProvider<T>>of();
             }
 
-            List<RegisteredServiceProvider<T>> ret = new ArrayList<RegisteredServiceProvider<T>>();
+            ret = ImmutableList.<RegisteredServiceProvider<T>>builder();
 
             for (RegisteredServiceProvider<?> provider : registered) {
                 ret.add((RegisteredServiceProvider<T>) provider);
             }
 
-            return Collections.unmodifiableList(ret);
         }
+        return ret.build();
     }
 
     /**
      * Get a list of known services. A service is known if it has registered
      * providers for it.
      *
-     * @return list of known services
+     * @return a copy of the set of known services
      */
-    public Collection<Class<?>> getKnownServices() {
-        return Collections.unmodifiableSet(providers.keySet());
+    public Set<Class<?>> getKnownServices() {
+        synchronized (providers) {
+            return ImmutableSet.<Class<?>>copyOf(providers.keySet());
+        }
     }
 
     /**
-     * Returns whether a provider has been registered for a service. Do not
-     * check this first only to call <code>load(service)</code> later, as that
-     * would be a non-thread safe situation.
+     * Returns whether a provider has been registered for a service.
      *
      * @param <T> service
      * @param service service to check
-     * @return whether there has been a registered provider
+     * @return true if and only if there are registered providers
      */
     public <T> boolean isProvidedFor(Class<T> service) {
-        return getRegistration(service) != null;
+        synchronized (providers) {
+            return providers.containsKey(service);
+        }
     }
 }