You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@brooklyn.apache.org by he...@apache.org on 2017/10/03 14:24:04 UTC
[27/35] brooklyn-server git commit: improve description of
`getImmediately` and tidy a few usages
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 <al...@cloudsoftcorp.com>
Authored: Thu Sep 28 16:14:17 2017 +0100
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
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");