[WIP] deprecate Task as values in config and getImmediately(Task) in favour of TaskFactory

api/src/main/java/org/apache/brooklyn/api/mgmt/ExecutionContext.java

@@ -97,8 +97,12 @@ public interface ExecutionContext extends Executor {
     @Beta
     <T> Maybe<T> getImmediately(Object callableOrSupplierOrTask);
     /** As {@link #getImmediately(Object)} but strongly typed for a task. */
+    // TODO deprecate
     @Beta
-    <T> Maybe<T> getImmediately(Task<T> callableOrSupplierOrTask);
+    <T> Maybe<T> getImmediately(TaskAdaptable<T> callableOrSupplierOrTask);
+    /** As {@link #getImmediately(Object)} but strongly typed for a task factory. */
+    @Beta
+    <T> Maybe<T> getImmediately(TaskFactory<Task<T>> callableOrSupplierOrTask);
 
     /**
      * Efficient implementation of common case when {@link #submit(TaskAdaptable)} is followed by an immediate {@link Task#get()}.

api/src/main/java/org/apache/brooklyn/api/objs/Configurable.java

@@ -20,10 +20,13 @@
 
 import java.util.Set;
 
+import org.apache.brooklyn.api.mgmt.ExecutionContext;
 import org.apache.brooklyn.api.mgmt.Task;
+import org.apache.brooklyn.api.mgmt.TaskAdaptable;
+import org.apache.brooklyn.api.mgmt.TaskFactory;
 import org.apache.brooklyn.config.ConfigKey;
-import org.apache.brooklyn.config.ConfigMap;
 import org.apache.brooklyn.config.ConfigKey.HasConfigKey;
+import org.apache.brooklyn.config.ConfigMap;
 
 import com.google.common.annotations.Beta;
 import com.google.common.base.Predicate;
@@ -85,19 +88,39 @@ public interface ConfigurationSupport {
         <T> T set(HasConfigKey<T> key, T val);
 
         /**
-         * Sets the config to the value returned by the task.
+         * Sets the config to return the value returned by the task this generates,
+         * on demand when invoked.
+         * <p>
+         * Returns immediately without blocking; subsequent calls to {@link #getConfig(ConfigKey)} 
+         * will create and execute an instance of the task, blocking unless otherwise configured.
          * 
+         * @see {@link #setConfig(ConfigKey, Object)}
+         */
+        <T> T set(ConfigKey<T> key, TaskFactory<? extends TaskAdaptable<T>> val);
+
+        /**
+         * Sets the config to the value returned by the task.
+         * <p>
+         * It is recommended to use {@link #set(ConfigKey, TaskFactory)} instead so that calls to validate 
+         * items via {@link ExecutionContext#getImmediately(Object)} do not interrupt and wreck the task set here. 
+         * <p>
          * Returns immediately without blocking; subsequent calls to {@link #getConfig(ConfigKey)} 
          * will execute the task, and block until the task completes.
          * 
          * @see {@link #setConfig(ConfigKey, Object)}
          */
-        <T> T set(ConfigKey<T> key, Task<T> val);
+        <T> T set(ConfigKey<T> key, TaskAdaptable<T> val);
+
+        /**
+         * @see {@link #setConfig(ConfigKey, TaskFactory)}
+         */
+        <T> T set(HasConfigKey<T> key, TaskFactory<? extends TaskAdaptable<T>> val);
 
         /**
          * @see {@link #setConfig(ConfigKey, Task)}
          */
-        <T> T set(HasConfigKey<T> key, Task<T> val);
+        // TODO deprecate
+        <T> T set(HasConfigKey<T> key, TaskAdaptable<T> val);
 
         /** @deprecated since 0.11.0 see {@link ConfigMap#findKeys(Predicate)} */
         @Deprecated

camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ObjectsYamlTest.java

@@ -27,7 +27,8 @@
 
 import org.apache.brooklyn.api.entity.Entity;
 import org.apache.brooklyn.api.mgmt.ManagementContext;
-import org.apache.brooklyn.api.mgmt.Task;
+import org.apache.brooklyn.api.mgmt.TaskAdaptable;
+import org.apache.brooklyn.api.mgmt.TaskFactory;
 import org.apache.brooklyn.api.objs.Configurable;
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.config.ConfigKey.HasConfigKey;
@@ -182,12 +183,22 @@ public <T> T set(HasConfigKey<T> key, T val) {
             }
 
             @Override
-            public <T> T set(ConfigKey<T> key, Task<T> val) {
+            public <T> T set(ConfigKey<T> key, TaskFactory<? extends TaskAdaptable<T>> val) {
                 throw new UnsupportedOperationException();
             }
 
             @Override
-            public <T> T set(HasConfigKey<T> key, Task<T> val) {
+            public <T> T set(HasConfigKey<T> key, TaskFactory<? extends TaskAdaptable<T>> val) {
+                return set(key.getConfigKey(), val);
+            }
+
+            @Override
+            public <T> T set(ConfigKey<T> key, TaskAdaptable<T> val) {
+                throw new UnsupportedOperationException();
+            }
+
+            @Override
+            public <T> T set(HasConfigKey<T> key, TaskAdaptable<T> val) {
                 return set(key.getConfigKey(), val);
             }
 

camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslTest.java

@@ -209,7 +209,7 @@ public void testConfigUsesParameterDefaultValue() throws Exception {
     public void testConfigImmediatelyDoesNotBlock() throws Exception {
         ConfigKey<String> configKey = ConfigKeys.newStringConfigKey("testConfig");
         BrooklynDslDeferredSupplier<?> attributeDsl = BrooklynDslCommon.attributeWhenReady(TestApplication.MY_ATTRIBUTE.getName());
-        app.config().set((ConfigKey)configKey, attributeDsl); // ugly cast because val is DSL, resolving to a string
+        app.config().set((ConfigKey)configKey, (Object)attributeDsl); // ugly cast because val is DSL, resolving to a string
         BrooklynDslDeferredSupplier<?> configDsl = BrooklynDslCommon.config(configKey.getName());
         Maybe<?> actualValue = execDslImmediately(configDsl, configKey.getType(), app, true);
         assertTrue(actualValue.isAbsent());

core/src/main/java/org/apache/brooklyn/core/effector/EffectorTasks.java

@@ -27,6 +27,7 @@
 import org.apache.brooklyn.api.entity.Entity;
 import org.apache.brooklyn.api.mgmt.Task;
 import org.apache.brooklyn.api.mgmt.TaskAdaptable;
+import org.apache.brooklyn.api.mgmt.TaskFactory;
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.core.config.ConfigKeys;
 import org.apache.brooklyn.core.location.Machines;
@@ -57,9 +58,14 @@ public class EffectorTasks {
     private static final Logger log = LoggerFactory.getLogger(EffectorTasks.class);
 
     public interface EffectorTaskFactory<T> {
+        // TODO deprecate
         public abstract TaskAdaptable<T> newTask(Entity entity, Effector<T> effector, ConfigBag parameters);
     }
 
+    public interface EffectorTaskFactoryFactory<T> {
+        public abstract TaskFactory<Task<T>> newTaskFactory(Entity entity, Effector<T> effector, ConfigBag parameters);
+    }
+
     /** wrapper for {@link EffectorBody} which simply runs that body on each invocation;
      * the body must be thread safe and ideally stateless */
     public static class EffectorBodyTaskFactory<T> implements EffectorTaskFactory<T> {
@@ -70,28 +76,52 @@ public EffectorBodyTaskFactory(EffectorBody<T> effectorBody) {
 
         @Override
         public Task<T> newTask(final Entity entity, final org.apache.brooklyn.api.effector.Effector<T> effector, final ConfigBag parameters) {
-            final AtomicReference<DynamicSequentialTask<T>> dst = new AtomicReference<DynamicSequentialTask<T>>();
-
-            dst.set(new DynamicSequentialTask<T>(
-                    getFlagsForTaskInvocationAt(entity, effector, parameters), 
-                    new Callable<T>() {
-                        @Override
-                        public T call() throws Exception {
-                            try {
-                                DynamicTasks.setTaskQueueingContext(dst.get());
-                                return effectorBody.call(parameters);
-                            } finally {
-                                DynamicTasks.removeTaskQueueingContext();
-                            }
-                        }
-                    }) {
-                        @Override
-                        public void handleException(Throwable throwable) throws Exception {
-                            throw EffectorUtils.handleEffectorException(entity, effector, throwable);
-                        }
-                    });
-            return dst.get();
+            return newTaskFactory(entity, effector, parameters).newTask();
         };
+
+        public TaskFactory<Task<T>> newTaskFactory(final Entity entity, final org.apache.brooklyn.api.effector.Effector<T> effector, final ConfigBag parameters) {
+            return new RealTaskFactory<T>(entity, effector, parameters, effectorBody, getFlagsForTaskInvocationAt(entity, effector, parameters));            
+        };
+
+        private static class RealTaskFactory<T> implements TaskFactory<Task<T>> {
+            final Entity entity;
+            final Effector<T> effector; 
+            final ConfigBag parameters;
+            final EffectorBody<T> effectorBody;
+            final Map<Object,Object> invocationFlags;
+            public RealTaskFactory(Entity entity, Effector<T> effector, ConfigBag parameters, EffectorBody<T> effectorBody, Map<Object,Object> invocationFlags) {
+                this.entity = entity;
+                this.effector = effector;
+                this.parameters = parameters;
+                this.effectorBody = effectorBody;
+                this.invocationFlags = invocationFlags;
+            }
+
+            @Override
+            public Task<T> newTask() {
+                final AtomicReference<DynamicSequentialTask<T>> dst = new AtomicReference<DynamicSequentialTask<T>>();
+
+                dst.set(new DynamicSequentialTask<T>(
+                        invocationFlags, 
+                        new Callable<T>() {
+                            @Override
+                            public T call() throws Exception {
+                                try {
+                                    DynamicTasks.setTaskQueueingContext(dst.get());
+                                    return effectorBody.call(parameters);
+                                } finally {
+                                    DynamicTasks.removeTaskQueueingContext();
+                                }
+                            }
+                        }) {
+                            @Override
+                            public void handleException(Throwable throwable) throws Exception {
+                                throw EffectorUtils.handleEffectorException(entity, effector, throwable);
+                            }
+                        });
+                return dst.get();
+            }
+        }
 
         /** @deprecated since 0.7.0 use {@link #getFlagsForTaskInvocationAt(Entity, Effector, ConfigBag)} */ @Deprecated
         protected final Map<Object,Object> getFlagsForTaskInvocationAt(Entity entity, Effector<?> effector) {

core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java

@@ -27,6 +27,8 @@
 
 import org.apache.brooklyn.api.mgmt.ExecutionContext;
 import org.apache.brooklyn.api.mgmt.Task;
+import org.apache.brooklyn.api.mgmt.TaskAdaptable;
+import org.apache.brooklyn.api.mgmt.TaskFactory;
 import org.apache.brooklyn.api.objs.BrooklynObject;
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.config.ConfigKey.HasConfigKey;
@@ -141,7 +143,12 @@ protected <T> Maybe<T> getNonBlockingResolvingSimple(ConfigKey<T> key) {
     }
 
     @Override
-    public <T> T set(HasConfigKey<T> key, Task<T> val) {
+    public <T> T set(HasConfigKey<T> key, TaskFactory<? extends TaskAdaptable<T>> val) {
+        return set(key.getConfigKey(), val);
+    }
+
+    @Override
+    public <T> T set(HasConfigKey<T> key, TaskAdaptable<T> val) {
         return set(key.getConfigKey(), val);
     }
 
@@ -176,10 +183,19 @@ public <T> T set(ConfigKey<T> key, T val) {
     }
 
     @Override
-    public <T> T set(ConfigKey<T> key, Task<T> val) {
+    public <T> T set(ConfigKey<T> key, TaskAdaptable<T> val) {
+        LOG.warn("Setting a task as a config value is deprecated; use a TaskFactory instead, "
+            + "when setting "+getContainer()+" "+key+" to "+val);
+        LOG.debug("Trace for setting "+getContainer()+" "+key+" to task "+val,
+            new Exception("Trace for setting "+getContainer()+" "+key+" to task "+val));
         return setConfigInternal(key, val);
     }
 
+    @Override
+    public <T> T set(ConfigKey<T> key, TaskFactory<? extends TaskAdaptable<T>> val) {
+        return setConfigInternal(key, val);
+    }
+
     @Override
     public ConfigBag getLocalBag() {
         return ConfigBag.newInstance(getConfigsInternal().getAllConfigLocalRaw());

core/src/main/java/org/apache/brooklyn/core/objs/BasicConfigurableObject.java

@@ -22,6 +22,8 @@
 
 import org.apache.brooklyn.api.mgmt.ManagementContext;
 import org.apache.brooklyn.api.mgmt.Task;
+import org.apache.brooklyn.api.mgmt.TaskAdaptable;
+import org.apache.brooklyn.api.mgmt.TaskFactory;
 import org.apache.brooklyn.api.objs.Configurable;
 import org.apache.brooklyn.api.objs.Identifiable;
 import org.apache.brooklyn.config.ConfigKey;
@@ -115,12 +117,22 @@ public <T> T set(HasConfigKey<T> key, T val) {
         }
 
         @Override
-        public <T> T set(ConfigKey<T> key, Task<T> val) {
-            throw new UnsupportedOperationException();
+        public <T> T set(ConfigKey<T> key, TaskFactory<? extends TaskAdaptable<T>> val) {
+            throw new UnsupportedOperationException("Setting tasks only supported on subtypes");
         }
 
         @Override
-        public <T> T set(HasConfigKey<T> key, Task<T> val) {
+        public <T> T set(HasConfigKey<T> key, TaskFactory<? extends TaskAdaptable<T>> val) {
+            return set(key.getConfigKey(), val);
+        }
+
+        @Override
+        public <T> T set(ConfigKey<T> key, TaskAdaptable<T> val) {
+            throw new UnsupportedOperationException("Setting tasks only supported on subtypes");
+        }
+
+        @Override
+        public <T> T set(HasConfigKey<T> key, TaskAdaptable<T> val) {
             return set(key.getConfigKey(), val);
         }
 

core/src/main/java/org/apache/brooklyn/core/sensor/DependentConfiguration.java

@@ -102,8 +102,15 @@
  * </pre>
  * <p>
  * Note that these methods return one-time tasks. The DslComponent methods return long-lasting pointers
- * and should now normally be used instead.
+ * and should now normally be used instead. If using these methods that return {@link Task} instances
+ * you should either ensure it is submitted or that validation and any other attempts to 
+ * {@link ExecutionContext#getImmediately(TaskAdaptable)} on this task is blocked;
+ * it is all to easy otherwise for an innocuous immediate-get to render this task interrupted
  */
+// possibly even the REST API just looking at config could cancel?
+// TODO should we deprecate these and provide variants that return TaskFactory instances?
+// maybe even ones that are nicely persistable? i (alex) think so, 2017-10.
+// see https://github.com/apache/brooklyn-server/pull/816#issuecomment-333858098
 public class DependentConfiguration {
 
     private static final Logger LOG = LoggerFactory.getLogger(DependentConfiguration.class);

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

@@ -40,6 +40,7 @@
 import org.apache.brooklyn.api.mgmt.HasTaskChildren;
 import org.apache.brooklyn.api.mgmt.Task;
 import org.apache.brooklyn.api.mgmt.TaskAdaptable;
+import org.apache.brooklyn.api.mgmt.TaskFactory;
 import org.apache.brooklyn.api.mgmt.entitlement.EntitlementContext;
 import org.apache.brooklyn.core.entity.EntityInternal;
 import org.apache.brooklyn.core.mgmt.BrooklynTaskTags;
@@ -244,20 +245,39 @@ private <T> Maybe<T> runInSameThread(final Task<T> task, Callable<Maybe<T>> job)
     }
 
     @Override
-    public <T> Maybe<T> getImmediately(Task<T> callableOrSupplier) {
-        return getImmediately((Object) callableOrSupplier);
+    public <T> Maybe<T> getImmediately(TaskFactory<Task<T>> callableOrSupplier) {
+        return getImmediatelyInternal(callableOrSupplier);
+    }
+
+    @Override
+    public <T> Maybe<T> getImmediately(TaskAdaptable<T> callableOrSupplier) {
+        if (!(callableOrSupplier instanceof TaskFactory)) {
+            Task<T> t = callableOrSupplier.asTask();
+            if (!t.isSubmitted()) {
+                log.warn("Deprecated call to getImmediately on unsubmitted task "+callableOrSupplier+"; "
+                    + "this will run the task in a crippled state. Pass a TaskFactory instead or submit the task beforehand.");
+                log.debug("Trace for deprecated call to getImmediately("+callableOrSupplier+")",
+                    new Exception("Trace for deprecated call to getImmediately("+callableOrSupplier+")"));
+            }
+        }
+        return getImmediatelyInternal(callableOrSupplier);
     }
 
     /** performs execution without spawning a new task thread, though it does temporarily set a fake task for the purpose of getting context;
-     * currently supports {@link Supplier}, {@link Callable}, {@link Runnable}, or {@link Task} instances; 
+     * currently supports {@link Supplier}, {@link Callable}, {@link Runnable}, 
+     * or {@link Task} (recommended to be {@link TaskFactory}) instances; 
      * with tasks if it is submitted or in progress,
      * it fails if not completed; with unsubmitted, unqueued tasks, it gets the {@link Callable} job and 
      * uses that; with such a job, or any other callable/supplier/runnable, it runs that
      * in an {@link InterruptingImmediateSupplier}, with as much metadata as possible (eg task name if
      * given a task) set <i>temporarily</i> in the current thread context */
-    @SuppressWarnings("unchecked")
     @Override
     public <T> Maybe<T> getImmediately(Object callableOrSupplier) {
+        return getImmediatelyInternal(callableOrSupplier);
+    }
+
+    @SuppressWarnings("unchecked")
+    private <T> Maybe<T> getImmediatelyInternal(Object callableOrSupplier) {
         BasicTask<T> fakeTaskForContext;
         if (callableOrSupplier instanceof BasicTask) {
             fakeTaskForContext = (BasicTask<T>)callableOrSupplier;
@@ -269,8 +289,12 @@ public <T> Maybe<T> getImmediately(Object callableOrSupplier) {
                 }
             }
             callableOrSupplier = fakeTaskForContext.getJob();
-        } else if (callableOrSupplier instanceof TaskAdaptable) {
-            return getImmediately( ((TaskAdaptable<T>)callableOrSupplier).asTask() );
+        } else if (callableOrSupplier instanceof TaskFactory) {
+            return getImmediatelyInternal( ((TaskFactory<Task<T>>)callableOrSupplier).newTask() );
+        } else if (callableOrSupplier instanceof TaskAdaptable && !(callableOrSupplier instanceof Task)) {
+            // if it's another form of task go into the next block where we will likely fail;
+            // not supported to get-immediate tasks that aren't basic tasks
+            return getImmediatelyInternal( ((TaskAdaptable<T>)callableOrSupplier).asTask() );
         } else {
             fakeTaskForContext = new BasicTask<T>(MutableMap.of("displayName", "Immediate evaluation"));
         }