Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/816#discussion_r141603195
--- Diff:
core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java ---
@@ -434,7 +434,11 @@ protected boolean isEvaluatingImmediately() {
// (should discourage this in favour of task
factories which can be transiently interrupted?)
BrooklynTaskTags.addTagDynamically(task,
BrooklynTaskTags.NON_TRANSIENT_TASK_TAG);
}
- exec.submit(task);
+ if (timer!=null ||
Thread.currentThread().isInterrupted()) {
+ exec.submit(task);
+ } else {
+ exec.get(task);
--- End diff --
I'm not sure how this relates to `isEvaluatingImmediately()`. For the
`instanceof Future` case, that checks explicitly for immediately. Here, it will
just call `exec.get()`. Previously, it would have done `submit` and then gone
into the next if block because Task extends Future. I think you should guard
this to only do `exec.submit` if also not `!isEvaluatingImmediately()`.
I'm guessing you might be relying on immediately having set the thread to
interrupted first? I'm uncomfortable about that because we call
`Tasks.resolving()...immediately(true)...getMaybe()`in a few places. That won't
have gone through the `executionContext.getImmediately` code.
It's really hard to reason about whether this code will work, and how it's
used, because it's all so weakly typed with `Object` and `instanceof` all over
the place. If I recall correctly, `immediately` with a `Task` never quite
worked properly but I can't find any appropriate tests in `ValueResolverTest`.
My general feeling is that we try to (accidentally?!) support way too many
things by taking `Object` and then adding a bunch of instanceof checks, and a
bunch of flags to customize the behaviour (which grow over time).
---
