improve description of `getImmediately` and tidy a few usages all usages seem consistent with the current cancellation semantics, they just weren't very clear
Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/afc17ee0 Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/afc17ee0 Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/afc17ee0 Branch: refs/heads/master Commit: afc17ee0025235e061cfb29a44863ad5fab9fa9d Parents: ed4eeba Author: Alex Heneveld <[email protected]> Authored: Thu Sep 28 16:14:17 2017 +0100 Committer: Alex Heneveld <[email protected]> Committed: Thu Sep 28 17:28:29 2017 +0100 ---------------------------------------------------------------------- .../org/apache/brooklyn/api/mgmt/ExecutionContext.java | 8 +++++++- .../brooklyn/util/core/task/BasicExecutionContext.java | 12 ++++++++---- .../apache/brooklyn/util/core/task/ValueResolver.java | 9 +++++---- .../brooklyn/core/effector/EffectorSayHiTest.java | 2 +- 4 files changed, 21 insertions(+), 10 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/afc17ee0/api/src/main/java/org/apache/brooklyn/api/mgmt/ExecutionContext.java ---------------------------------------------------------------------- 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 142e664..2ad9482 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 @@ -75,7 +75,13 @@ 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> - * Also supports {@link Callable}, {@link Runnable}, and {@link Supplier} argument types. + * Supports {@link Callable}, {@link Runnable}, and {@link Supplier} argument types as well as {@link Task}. + * <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). */ // TODO reference ImmediateSupplier when that class is moved to utils project @Beta http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/afc17ee0/core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java ---------------------------------------------------------------------- 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 5e645a9..435e50e 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 @@ -281,10 +281,14 @@ public class BasicExecutionContext extends AbstractExecutionContext { try { return runInSameThread(fakeTaskForContext, new Callable<Maybe<T>>() { public Maybe<T> call() { - // TODO could try to make this work for more types of tasks by not cancelling, it just interrupting; - // however the biggest place "getImmediate" fails is with DSTs where interrupting is sufficient to abort them - // unnecessarily, as queue.andWait attempts to block (again, unnecessarily, but not a straightforward fix). - // limited success of getImmediately is okay -- but no harm in expanding coverage by resolving that and removing cancel. + // could try to make this work for more types of tasks by not cancelling, just interrupting; + // however there is a danger that immediate-submission tasks are leaked if we don't cancel. + // for instance with DSTs the thread interrupt may apply only to the main job queue.andWait blocking, + // leaving other tasks leaked. + // + // this method is best-effort so fine if it doesn't succeed. good if we can expand + // coverage but NOT at the expense of major leaks of course! + // // see WIP test in EffectorSayHiTest fakeTaskForContext.cancel(); http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/afc17ee0/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java b/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java index 0e75d16..5f7621b 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java @@ -434,11 +434,12 @@ public class ValueResolver<T> implements DeferredSupplier<T>, Iterable<Maybe<Obj // (should discourage this in favour of task factories which can be transiently interrupted?) BrooklynTaskTags.addTagDynamically(task, BrooklynTaskTags.NON_TRANSIENT_TASK_TAG); } - // FIXME - if (timer!=null || Thread.currentThread().isInterrupted() || isEvaluatingImmediately()) { - exec.submit(task); - } else { + if (timer==null && !Thread.currentThread().isInterrupted() && !isEvaluatingImmediately()) { + // if all conditions above: no timeout, not immediate, and not interrupted, + // then we can run in this thread exec.get(task); + } else { + exec.submit(task); } } } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/afc17ee0/core/src/test/java/org/apache/brooklyn/core/effector/EffectorSayHiTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/brooklyn/core/effector/EffectorSayHiTest.java b/core/src/test/java/org/apache/brooklyn/core/effector/EffectorSayHiTest.java index 5110315..bc41d45 100644 --- a/core/src/test/java/org/apache/brooklyn/core/effector/EffectorSayHiTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/effector/EffectorSayHiTest.java @@ -110,7 +110,7 @@ public class EffectorSayHiTest extends BrooklynAppUnitTestSupport { } @Test(groups="WIP") // see comments at BasicExecutionContext.getImmediately - // FIXME + // TODO this will be fixed soon by #835 public void testInvocationGetImmediately() throws Exception { assertEquals(((EntityInternal)e).getExecutionContext() .getImmediately( Effectors.invocation(e, MyEntity.SAY_HI_1, ImmutableMap.of("name", "Bob", "greeting", "hi")) ), "hi Bob");
