This closes #59

* github/pr/59:
  Clocker-testing: fix where we delete the location
  Fix XML serialiser for SpecConverter
  LocationFactor’s “special constructor” for pre-existing loc
  Fix log.warn when calling Location.unmanage
  BROOKLYN-242: fix race in BasicTest.get()
  NotUpDiagnostics: don’t collect if starting && !up
  ServerPool: creates/registers single ServerPoolLocation
  Clocker pattern: change how LocationRegistry used
  Test the Clocker pattern (of LocationOwner and DynamicLocation)

core/src/main/java/org/apache/brooklyn/core/location/AbstractLocation.java

@@ -706,7 +706,9 @@ public boolean removeChild(Location child) {
             }
             child.setParent(null);
 
-            if (isManaged()) {
+            if (isManaged() && Locations.isManaged(child)) {
+                // This is called as part of child's LocalLocationManager.unmanage; don't try to 
+                // unmanage it yet again as then would get a log.warn!
                 getManagementContext().getLocationManager().unmanage(child);
             }
         }

core/src/main/java/org/apache/brooklyn/core/location/dynamic/DynamicLocation.java

@@ -20,6 +20,8 @@
 
 import org.apache.brooklyn.api.entity.Entity;
 import org.apache.brooklyn.api.location.Location;
+import org.apache.brooklyn.api.location.LocationDefinition;
+import org.apache.brooklyn.api.location.LocationRegistry;
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.core.config.ConfigKeys;
 import org.apache.brooklyn.util.core.flags.SetFromFlag;
@@ -47,4 +49,14 @@ public interface DynamicLocation<E extends Entity & LocationOwner<L, E>, L exten
 
     E getOwner();
 
+    /**
+     * An opportunity to register this location (e.g. with the {@link LocationRegistry} or the 
+     * catalog, so that it will be persisted).
+     */
+    LocationDefinition register();
+
+    /**
+     * The complement of {@link #register()}, to deregister this location.
+     */
+    void deregister();
 }

core/src/main/java/org/apache/brooklyn/core/location/dynamic/LocationOwner.java

@@ -22,7 +22,6 @@
 
 import org.apache.brooklyn.api.entity.Entity;
 import org.apache.brooklyn.api.location.Location;
-import org.apache.brooklyn.api.location.LocationDefinition;
 import org.apache.brooklyn.api.sensor.AttributeSensor;
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.core.config.ConfigKeys;
@@ -71,9 +70,6 @@ public interface LocationOwner<L extends Location & DynamicLocation<E, L>, E ext
     AttributeSensor<Boolean> DYNAMIC_LOCATION_STATUS = Sensors.newBooleanSensor(
             "entity.dynamicLocation.status", "The status of the location owned by this entity");
 
-    AttributeSensor<LocationDefinition> LOCATION_DEFINITION = Sensors.newSensor(
-        LocationDefinition.class, "entity.dynamicLocation.definition", "The location definition for the location owned by this entity");
-
     L getDynamicLocation();
 
     L createLocation(Map<String, ?> flags);

core/src/main/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializer.java

@@ -393,6 +393,12 @@ public Object unmarshal(HierarchicalStreamReader reader, UnmarshallingContext co
         }
     }
 
+    // Would prefer this as a field of SpecConverter, but can't do that because the class is not static.
+    // Must use thread-local storage because a single instance of the SpecConverter is used by this 
+    // XmlMementoSerializer. In BrooklynMementoPersisterToObjectStore.visitMemento, it uses a thread-pool
+    // for concurrently deserializing multiple objects.
+    private static final ThreadLocal<Object> SpecConverterLocalInstance = new ThreadLocal<Object>();
+
     /** When reading/writing specs, it checks whether there is a catalog item id set and uses it to load */
     public class SpecConverter extends ReflectionConverter {
         SpecConverter() {
@@ -454,25 +460,25 @@ public Object unmarshal(HierarchicalStreamReader reader, UnmarshallingContext co
                 result.catalogItemId(catalogItemId);
                 return result;
             } finally {
-                instance = null;
+                SpecConverterLocalInstance.remove();
                 if (customLoaderSet) {
                     popXstreamCustomClassLoader();
                 }
             }
         }
 
-        Object instance;
-
         @Override
         protected Object instantiateNewInstance(HierarchicalStreamReader reader, UnmarshallingContext context) {
             // the super calls getAttribute which requires that we have not yet done moveDown,
             // so we do this earlier and cache it for when we call super.unmarshal
+            Object instance = SpecConverterLocalInstance.get();
             if (instance==null)
                 throw new IllegalStateException("Instance should be created and cached");
             return instance;
         }
         protected void instantiateNewInstanceSettingCache(HierarchicalStreamReader reader, UnmarshallingContext context) {
-            instance = super.instantiateNewInstance(reader, context);
+            Object instance = super.instantiateNewInstance(reader, context);
+            SpecConverterLocalInstance.set(instance);
         }
     }
 

core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalLocationFactory.java

@@ -27,12 +27,15 @@
 import org.apache.brooklyn.api.mgmt.ManagementContext;
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.core.location.AbstractLocation;
+import org.apache.brooklyn.core.location.Locations;
 import org.apache.brooklyn.core.location.internal.LocationInternal;
 import org.apache.brooklyn.core.mgmt.internal.LocalLocationManager;
 import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal;
 import org.apache.brooklyn.util.core.config.ConfigBag;
 import org.apache.brooklyn.util.core.flags.FlagUtils;
 import org.apache.brooklyn.util.exceptions.Exceptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import com.google.common.collect.ImmutableMap;
 
@@ -46,6 +49,8 @@
  */
 public class InternalLocationFactory extends InternalFactory {
 
+    private static final Logger LOG = LoggerFactory.getLogger(InternalLocationFactory.class);
+
     /**
      * Returns true if this is a "new-style" location (i.e. where not expected to call the constructor to instantiate it).
      * 
@@ -96,9 +101,17 @@ public <T extends Location> T createLocation(LocationSpec<T> spec) {
 
             T loc = construct(clazz, spec, null);
 
+            if (Locations.isManaged(loc)) {
+                // Construct can return an existing instance, if using SpecialBrooklynObjectConstructor.Config.SPECIAL_CONSTRUCTOR.
+                // In which case, don't reconfigure it (don't change its parent, etc).
+                LOG.debug("Location-factory returning pre-existing location; skipping initialization of {}", loc);
+                return loc;
+            }
+
             if (spec.getId() != null) {
                 FlagUtils.setFieldsFromFlags(ImmutableMap.of("id", spec.getId()), loc);
             }
+
             managementContext.prePreManage(loc);
 
             if (spec.getDisplayName()!=null)

core/src/main/java/org/apache/brooklyn/util/core/task/BasicTask.java

@@ -451,7 +451,16 @@ public T get(Duration duration) throws InterruptedException, ExecutionException,
         }
         Long remaining = end==null ? null : end -  System.currentTimeMillis();
         if (isDone()) {
-            return internalFuture.get(1, TimeUnit.MILLISECONDS);
+            // Don't just call internalFuture.get(1ms) - see comment in isDone() about setting of endTimeUtc,
+            // and see BROOKLYN-242.
+            if (internalFuture == null) {
+                assert cancelled: "task="+this+"; endTimeUtc="+endTimeUtc+"; cancelled="+cancelled+"; isDone=true; null internal future";
+                throw new CancellationException();
+            } else if (remaining == null) {
+                return internalFuture.get();
+            } else {
+                return internalFuture.get(Math.max(remaining, 1000), TimeUnit.MILLISECONDS);
+            }
         } else if (remaining == null) {
             return internalFuture.get();
         } else if (remaining > 0) {

core/src/test/java/org/apache/brooklyn/core/effector/EffectorBasicTest.java

@@ -18,13 +18,16 @@
  */
 package org.apache.brooklyn.core.effector;
 
+import static org.testng.Assert.assertEquals;
+
 import java.util.List;
 import java.util.concurrent.Callable;
 
 import org.apache.brooklyn.api.entity.EntitySpec;
 import org.apache.brooklyn.api.mgmt.HasTaskChildren;
 import org.apache.brooklyn.api.mgmt.Task;
 import org.apache.brooklyn.core.entity.Entities;
+import org.apache.brooklyn.core.entity.StartableApplication;
 import org.apache.brooklyn.core.entity.trait.FailingEntity;
 import org.apache.brooklyn.core.entity.trait.Startable;
 import org.apache.brooklyn.core.location.SimulatedLocation;
@@ -36,11 +39,13 @@
 import org.apache.brooklyn.util.core.task.Tasks;
 import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.text.Strings;
+import org.apache.brooklyn.util.time.Duration;
 import org.testng.Assert;
 import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Lists;
 
 public class EffectorBasicTest extends BrooklynAppUnitTestSupport {
 
@@ -92,6 +97,30 @@ public void testInvokeEffectorTaskHasTag() {
         Assert.assertTrue(starting.getTags().contains(ManagementContextInternal.EFFECTOR_TAG));
     }
 
+    @Test
+    public void testInvokeEffectorListWithEmpty() throws Exception{
+        Entities.invokeEffectorList(app, ImmutableList.<StartableApplication>of(), Startable.STOP).get(Duration.THIRTY_SECONDS); 
+    }
+
+    @Test
+    public void testInvokeEffectorList() throws Exception{
+        List<TestEntity> entities = Lists.newArrayList();
+        for (int i = 0; i < 10; i++) {
+            entities.add(app.addChild(EntitySpec.create(TestEntity.class)));
+        }
+        Entities.invokeEffectorList(app, entities, Startable.STOP).get(Duration.THIRTY_SECONDS);
+        for (TestEntity entity : entities) {
+            assertEquals(entity.getCallHistory(), ImmutableList.of("stop"));
+        }
+    }
+
+    @Test(expectedExceptions=IllegalStateException.class, expectedExceptionsMessageRegExp=".*no longer managed.*")
+    public void testInvokeEffectorListWithEmptyUsingUnmanagedContext() throws Exception{
+        TestEntity entity = app.addChild(EntitySpec.create(TestEntity.class));
+        Entities.unmanage(entity);
+        Entities.invokeEffectorList(entity, ImmutableList.<StartableApplication>of(), Startable.STOP).get(Duration.THIRTY_SECONDS);
+    }
+
     // check various failure situations
 
     private FailingEntity createFailingEntity() {

software/base/src/main/java/org/apache/brooklyn/entity/machine/pool/ServerPool.java

@@ -21,8 +21,6 @@
 import java.util.Collection;
 import java.util.Map;
 
-import com.google.common.annotations.Beta;
-
 import org.apache.brooklyn.api.catalog.Catalog;
 import org.apache.brooklyn.api.entity.Entity;
 import org.apache.brooklyn.api.entity.EntitySpec;
@@ -41,6 +39,8 @@
 import org.apache.brooklyn.entity.group.DynamicCluster;
 import org.apache.brooklyn.entity.machine.MachineEntity;
 
+import com.google.common.annotations.Beta;
+
 /**
  * A preallocated server pool is an entity that other applications can deploy to.
  * Behaving as a cluster, the machines it creates for its members are reused.
@@ -70,6 +70,7 @@
 @ImplementedBy(ServerPoolImpl.class)
 @Beta
 public interface ServerPool extends DynamicCluster, LocationOwner<ServerPoolLocation, ServerPool> {
+    ConfigKey<String> LOCATION_NAME_PREFIX = ConfigKeys.newConfigKeyWithDefault(LocationOwner.LOCATION_NAME_PREFIX, "server-pool");
 
     ConfigKey<Integer> INITIAL_SIZE = ConfigKeys.newConfigKeyWithDefault(DynamicCluster.INITIAL_SIZE, 2);
 

software/base/src/main/java/org/apache/brooklyn/entity/machine/pool/ServerPoolImpl.java

@@ -25,6 +25,8 @@
 import org.apache.brooklyn.api.entity.Entity;
 import org.apache.brooklyn.api.location.Location;
 import org.apache.brooklyn.api.location.LocationDefinition;
+import org.apache.brooklyn.api.location.LocationRegistry;
+import org.apache.brooklyn.api.location.LocationSpec;
 import org.apache.brooklyn.api.location.MachineLocation;
 import org.apache.brooklyn.api.location.NoMachinesAvailableException;
 import org.apache.brooklyn.api.mgmt.LocationManager;
@@ -38,19 +40,19 @@
 import org.apache.brooklyn.core.entity.EntityInternal;
 import org.apache.brooklyn.core.entity.lifecycle.Lifecycle;
 import org.apache.brooklyn.core.entity.trait.Startable;
-import org.apache.brooklyn.core.location.BasicLocationDefinition;
 import org.apache.brooklyn.core.location.Machines;
 import org.apache.brooklyn.core.location.dynamic.DynamicLocation;
 import org.apache.brooklyn.core.location.internal.LocationInternal;
 import org.apache.brooklyn.core.mgmt.internal.LocalLocationManager;
+import org.apache.brooklyn.core.location.dynamic.LocationOwner;
 import org.apache.brooklyn.core.sensor.Sensors;
 import org.apache.brooklyn.entity.group.AbstractMembershipTrackingPolicy;
 import org.apache.brooklyn.entity.group.DynamicClusterImpl;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 import org.apache.brooklyn.util.collections.MutableMap;
 import org.apache.brooklyn.util.core.task.DynamicTasks;
 import org.apache.brooklyn.util.guava.Maybe;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import com.google.common.base.Function;
 import com.google.common.base.Joiner;
@@ -64,6 +66,19 @@
 import com.google.common.collect.Maps;
 import com.google.common.reflect.TypeToken;
 
+/**
+ * The normal usage pattern is to instantiate this entity to create your cluster, which will
+ * automatically create a {@link ServerPoolLocation}. The location can be looked up from the 
+ * {@link LocationRegistry} (e.g. {@code mgmt.getLocationRegistry().resolve("my-pool-name")}),
+ * which uses the {@link ServerPoolLocationResolver}.
+ * 
+ * Each location delegates to the associated {@link ServerPool} to claim machines (which are
+ * provisioned as required), and to release machines back to the pool.
+ * 
+ * This differs from Clocker's use of {@link DynamicLocation} in that the ServerPool has <em>multiple</em>
+ * locations. This makes the use of {@link LocationOwner} a little confusing 
+ * (e.g. {@link LocationOwner#DYNAMIC_LOCATION} is not set).
+ */
 public class ServerPoolImpl extends DynamicClusterImpl implements ServerPool {
 
     private static final Logger LOG = LoggerFactory.getLogger(ServerPoolImpl.class);
@@ -96,9 +111,6 @@ private static enum MachinePoolMemberStatus {
     public static final AttributeSensor<Map<MachineLocation, Entity>> MACHINE_ENTITY = Sensors.newSensor(new TypeToken<Map<MachineLocation, Entity>>() {},
             "pool.machineEntityMap", "A mapping of machine locations and their entities");
 
-    public static final AttributeSensor<LocationDefinition> DYNAMIC_LOCATION_DEFINITION = Sensors.newSensor(LocationDefinition.class,
-            "pool.locationDefinition", "The location definition used to create the pool's dynamic location");
-
     public static final ConfigKey<Boolean> REMOVABLE = ConfigKeys.newBooleanConfigKey(
             "pool.member.removable", "Whether a pool member is removable from the cluster. Used to denote additional " +
                     "existing machines that were manually added to the pool", true);
@@ -171,16 +183,18 @@ public ServerPoolLocation createLocation(Map<String, ?> flags) {
             locationName = Joiner.on("-").skipNulls().join(prefix, getId(), suffix);
         }
 
-        String locationSpec = String.format(ServerPoolLocationResolver.POOL_SPEC, getId()) + String.format(":(name=\"%s\")", locationName);
-        LocationDefinition definition = new BasicLocationDefinition(locationName, locationSpec, flags);
-        getManagementContext().getLocationRegistry().updateDefinedLocation(definition);
-        Location location = getManagementContext().getLocationManager().createLocation( getManagementContext().getLocationRegistry().getLocationSpec(definition).get() );
-        LOG.info("Resolved and registered dynamic location {}: {}", locationName, location);
+        ServerPoolLocation location = getManagementContext().getLocationManager().createLocation(LocationSpec.create(ServerPoolLocation.class)
+                .displayName("Server Pool(" + getId() + ")")
+                .configure(flags)
+                .configure("owner", getProxy())
+                .configure("locationName", locationName));
+
+        LocationDefinition definition = location.register();
+        LOG.info("Resolved and registered dynamic location {} for server pool {}: {}", new Object[] {locationName, this, location});
 
-        sensors().set(LOCATION_SPEC, locationSpec);
+        sensors().set(LOCATION_SPEC, definition.getSpec());
+        sensors().set(LOCATION_NAME, locationName);
         sensors().set(DYNAMIC_LOCATION, location);
-        sensors().set(LOCATION_NAME, location.getId());
-        sensors().set(DYNAMIC_LOCATION_DEFINITION, definition);
 
         return (ServerPoolLocation) location;
     }
@@ -189,20 +203,15 @@ public ServerPoolLocation createLocation(Map<String, ?> flags) {
     public void deleteLocation() {
         LocationManager mgr = getManagementContext().getLocationManager();
         ServerPoolLocation location = getDynamicLocation();
-        if (mgr.isManaged(location)) {
+        if (location != null && mgr.isManaged(location)) {
             LOG.debug("{} deleting and unmanaging location {}", this, location);
+            location.deregister();
             mgr.unmanage(location);
         }
-        // definition will only be null if deleteLocation has already been called, e.g. by two calls to stop().
-        LocationDefinition definition = getAttribute(DYNAMIC_LOCATION_DEFINITION);
-        if (definition != null) {
-            LOG.debug("{} unregistering dynamic location {}", this, definition);
-            getManagementContext().getLocationRegistry().removeDefinedLocation(definition.getId());
-        }
+
         sensors().set(LOCATION_SPEC, null);
         sensors().set(DYNAMIC_LOCATION, null);
         sensors().set(LOCATION_NAME, null);
-        sensors().set(DYNAMIC_LOCATION_DEFINITION, null);
     }
 
     @Override