From 2e4fc3e205df4f9da01751fc2aba4ea004ef96bf Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Tue, 7 Nov 2017 16:51:54 +0000 Subject: [PATCH] Deprecates use of Task in ConfigKey Also avoid StackOverflowError in ExecutionContext.getImmediately(task) When a non-BasicTask is passed in. --- .../brooklyn/api/mgmt/ExecutionContext.java | 24 ++++++---- .../brooklyn/api/objs/Configurable.java | 12 +++-- .../util/core/task/BasicExecutionContext.java | 16 ++++++- .../util/core/task/ValueResolverTest.java | 45 +++++++++++++++++++ 4 files changed, 83 insertions(+), 14 deletions(-) diff --git a/api/src/main/java/org/apache/brooklyn/api/mgmt/ExecutionContext.java b/api/src/main/java/org/apache/brooklyn/api/mgmt/ExecutionContext.java index bb95ea9eed..d0fead1ac0 100644 --- a/api/src/main/java/org/apache/brooklyn/api/mgmt/ExecutionContext.java +++ b/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. *

- * 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. *

- * 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 - Maybe getImmediately(Object callableOrSupplierOrTask); - /** As {@link #getImmediately(Object)} but strongly typed for a task. */ + Maybe 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 - Maybe getImmediately(Task callableOrSupplierOrTask); + Maybe getImmediately(Task task); /** * Efficient implementation of common case when {@link #submit(TaskAdaptable)} is followed by an immediate {@link Task#get()}. diff --git a/api/src/main/java/org/apache/brooklyn/api/objs/Configurable.java b/api/src/main/java/org/apache/brooklyn/api/objs/Configurable.java index 40de655527..5187d00fe9 100644 --- a/api/src/main/java/org/apache/brooklyn/api/objs/Configurable.java +++ b/api/src/main/java/org/apache/brooklyn/api/objs/Configurable.java @@ -63,7 +63,7 @@ public interface ConfigurationSupport { T get(ConfigKey key); /** - * @see {@link #getConfig(ConfigKey)} + * @see {@link #get(ConfigKey)} */ T get(HasConfigKey key); @@ -73,7 +73,7 @@ public interface ConfigurationSupport { T set(ConfigKey key, T val); /** - * @see {@link #setConfig(HasConfigKey, Object)} + * @see {@link #set(ConfigKey, Object)} */ T set(HasConfigKey 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 set(ConfigKey key, Task val); /** - * @see {@link #setConfig(ConfigKey, Task)} + * @see {@link #set(ConfigKey, Task)} + * + * @deprecated since 1.0.0 (see {@link #set(ConfigKey, Task)} */ T set(HasConfigKey key, Task val); diff --git a/core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java b/core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java index 8b37e2f3c0..32ca5b7cc9 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java @@ -270,7 +270,21 @@ public Maybe getImmediately(Object callableOrSupplier) { } callableOrSupplier = fakeTaskForContext.getJob(); } else if (callableOrSupplier instanceof TaskAdaptable) { - return getImmediately( ((TaskAdaptable)callableOrSupplier).asTask() ); + Task task = ((TaskAdaptable)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(MutableMap.of("displayName", "Immediate evaluation")); } diff --git a/core/src/test/java/org/apache/brooklyn/util/core/task/ValueResolverTest.java b/core/src/test/java/org/apache/brooklyn/util/core/task/ValueResolverTest.java index 455f8ad919..a9e2f3d017 100644 --- a/core/src/test/java/org/apache/brooklyn/util/core/task/ValueResolverTest.java +++ b/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 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 result = app.getExecutionContext().getImmediately(t); + Assert.assertFalse(result.isPresent(), "result="+result); + } + + public void testExecutionContextGetImmediatelyBasicTaskSucceeds() throws Exception { + final Task 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 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 t = (TaskInternal) newSleepTask(Duration.ZERO, "foo"); + final Task t2 = new ForwardingTask() { + @Override + protected TaskInternal delegate() { + return t; + } + @Override public boolean cancel(TaskCancellationMode mode) { + return delegate().cancel(); + } + public Task asTask() { + return this; + } + }; + + // Does not handle non-basic tasks; an acceptable limitation. + // Previously it threw StackOverflowError; now says unsupported. + try { + Maybe result = app.getExecutionContext().getImmediately(t2); + Asserts.shouldHaveFailedPreviously("result="+result); + } catch (ImmediateUnsupportedException e) { + Asserts.expectedFailureContains(e, "cannot extract job"); + } + } + public void testSwallowError() { ValueResolver result = Tasks.resolving(newThrowTask(Duration.ZERO)).as(String.class).context(app).swallowExceptions(); assertMaybeIsAbsent(result);