This closes #883

Author: ahgittin

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

@@ -85,20 +85,26 @@ public interface ExecutionContext extends Executor {
      * Implementations will typically act like {@link #get(TaskAdaptable)} with additional
      * tricks to attempt to be non-blocking, such as recognizing some "immediate" markers.  
      * <p>
-     * Supports {@link Callable}, {@link Runnable}, and {@link Supplier} argument types as well as {@link Task}.
+     * Supports {@link Callable}, {@link Runnable}, {@link Supplier}, and {@link TaskFactory} argument types.
      * <p>
-     * This executes the given code, and in the case of {@link Task} it may cancel it, 
-     * so the caller should not use this if the argument is going to be used later and
-     * is expected to be pristine.  Supply a {@link TaskFactory} if this method's {@link Task#cancel(boolean)}
-     * is problematic, or consider other utilities (such as ValueResolver with immediate(true)
-     * in a downstream project).
+     * Passing in {@link Task} is deprecated and discouraged - see {@link #getImmediately(Task)}.
      */
     // TODO reference ImmediateSupplier when that class is moved to utils project
     @Beta
-    <T> Maybe<T> getImmediately(Object callableOrSupplierOrTask);
-    /** As {@link #getImmediately(Object)} but strongly typed for a task. */
+    <T> Maybe<T> getImmediately(Object callableOrSupplierOrTaskFactory);
+    
+    /**
+     * As {@link #getImmediately(Object)} but strongly typed for a task.
+     * 
+     * @deprecated since 1.0.0; this can cause the task to be interrupted/cancelled, such that subsequent  
+     *             use of {@code task.get()} will fail (if the value was not resolved immediately previously).
+     *             It is only safe to call this if the the given task is for a one-off usage (not expected
+     *             to be used again later). Consider supplying a {@link TaskFactory} if this tasks's 
+     *             {@link Task#cancel(boolean)} is problematic, or consider other utilities 
+     *             (such as ValueResolver with immediate(true) in the brooklyn-core project).
+     */
     @Beta
-    <T> Maybe<T> getImmediately(Task<T> callableOrSupplierOrTask);
+    <T> Maybe<T> getImmediately(Task<T> task);
 
     /**
      * 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

@@ -63,7 +63,7 @@ public interface ConfigurationSupport {
         <T> T get(ConfigKey<T> key);
 
         /**
-         * @see {@link #getConfig(ConfigKey)}
+         * @see {@link #get(ConfigKey)}
          */
         <T> T get(HasConfigKey<T> key);
 
@@ -73,7 +73,7 @@ public interface ConfigurationSupport {
         <T> T set(ConfigKey<T> key, T val); 
 
         /**
-         * @see {@link #setConfig(HasConfigKey, Object)}
+         * @see {@link #set(ConfigKey, Object)}
          */
         <T> T set(HasConfigKey<T> key, T val);
 
@@ -83,12 +83,16 @@ public interface ConfigurationSupport {
          * 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)}
+         * @deprecated since 1.0.0; do not use task because can be evaluated only once, and if 
+         *             cancelled will affect all subsequent lookups of the config value.
+         *             Consider using a {@link org.apache.brooklyn.api.mgmt.TaskFactory}.
          */
         <T> T set(ConfigKey<T> key, Task<T> val);
 
         /**
-         * @see {@link #setConfig(ConfigKey, Task)}
+         * @see {@link #set(ConfigKey, Task)}
+         * 
+         * @deprecated since 1.0.0 (see {@link #set(ConfigKey, Task)}
          */
         <T> T set(HasConfigKey<T> key, Task<T> val);
 

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

@@ -270,7 +270,21 @@ public <T> Maybe<T> getImmediately(Object callableOrSupplier) {
             }
             callableOrSupplier = fakeTaskForContext.getJob();
         } else if (callableOrSupplier instanceof TaskAdaptable) {
-            return getImmediately( ((TaskAdaptable<T>)callableOrSupplier).asTask() );
+            Task<T> task = ((TaskAdaptable<T>)callableOrSupplier).asTask();
+            if (task == callableOrSupplier) {
+                // Our TaskAdaptable was a task, but not a BasicTask.
+                // Avoid infinite recursion (don't just call ourselves again!).
+                if (task.isDone()) {
+                    return Maybe.of(task.getUnchecked());
+                } else if (task.isSubmitted() || task.isBegun()) {
+                    throw new ImmediateUnsupportedException("Task is in progress and incomplete: "+task);
+                } else {
+                    throw new ImmediateUnsupportedException("Task not a 'BasicTask', so cannot extract job to get immediately: "+task);
+                }
+            } else {
+                // recurse - try again with the task we've just generated
+                return getImmediately(task);
+            }
         } else {
             fakeTaskForContext = new BasicTask<T>(MutableMap.of("displayName", "Immediate evaluation"));
         }

core/src/test/java/org/apache/brooklyn/util/core/task/ValueResolverTest.java

@@ -35,6 +35,7 @@
 import org.apache.brooklyn.core.mgmt.BrooklynTaskTags;
 import org.apache.brooklyn.core.test.BrooklynAppUnitTestSupport;
 import org.apache.brooklyn.test.Asserts;
+import org.apache.brooklyn.util.core.task.ImmediateSupplier.ImmediateUnsupportedException;
 import org.apache.brooklyn.util.core.task.ImmediateSupplier.ImmediateValueNotAvailableException;
 import org.apache.brooklyn.util.guava.Maybe;
 import org.apache.brooklyn.util.time.Duration;
@@ -188,6 +189,50 @@ public void testUnsubmittedTaskWithExecutionContextExecutesAndTimesOutImmediate(
         Asserts.assertThat(t, (tt) -> !tt.isDone());
     }
 
+    public void testExecutionContextGetImmediatelyBasicTaskTooSlow() throws Exception {
+        final Task<String> t = newSleepTask(Duration.ONE_MINUTE, "foo");
+        
+        // Extracts job from task, tries to run it, and sees it tries to block so aborts.
+        // It will also render the task unusable (cancelled); not bothering to assert that!
+        Maybe<String> result = app.getExecutionContext().getImmediately(t);
+        Assert.assertFalse(result.isPresent(), "result="+result);
+    }
+
+    public void testExecutionContextGetImmediatelyBasicTaskSucceeds() throws Exception {
+        final Task<String> t = newSleepTask(Duration.ZERO, "foo");
+        
+        // Extracts job from task, tries to run it; because it doesn't block we'll get the result.
+        // It will also render the task unusable (cancelled); calling `t.get()` will throw CancellationException.
+        Maybe<String> result = app.getExecutionContext().getImmediately(t);
+        Assert.assertTrue(result.isPresent(), "result="+result);
+        Assert.assertEquals(result.get(), "foo", "result="+result);
+    }
+
+    public void testExecutionContextGetImmediatelyTaskNotBasicFails() throws Exception {
+        final TaskInternal<String> t = (TaskInternal<String>) newSleepTask(Duration.ZERO, "foo");
+        final Task<String> t2 = new ForwardingTask<String>() {
+            @Override
+            protected TaskInternal<String> delegate() {
+                return t;
+            }
+            @Override public boolean cancel(TaskCancellationMode mode) {
+                return delegate().cancel();
+            }
+            public Task<String> asTask() {
+                return this;
+            }
+        };
+        
+        // Does not handle non-basic tasks; an acceptable limitation.
+        // Previously it threw StackOverflowError; now says unsupported.
+        try {
+            Maybe<String> result = app.getExecutionContext().getImmediately(t2);
+            Asserts.shouldHaveFailedPreviously("result="+result);
+        } catch (ImmediateUnsupportedException e) {
+            Asserts.expectedFailureContains(e, "cannot extract job");
+        }
+    }
+
     public void testSwallowError() {
         ValueResolver<String> result = Tasks.resolving(newThrowTask(Duration.ZERO)).as(String.class).context(app).swallowExceptions();
         assertMaybeIsAbsent(result);