[GitHub] brooklyn-server pull request #480: Config self reference fix

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/brooklyn-server/pull/480


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r103505398
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java ---
@@ -512,6 +559,24 @@ public Object call() throws Exception {
 }
 }
 
+protected Maybe execImmediate(ExecutionContext exec, Object 
immediateSupplierOrImmediateTask) {
+Maybe result;
+try {
+result = exec.getImmediately(immediateSupplierOrImmediateTask);
+} catch (ImmediateSupplier.ImmediateUnsupportedException e) {
+return null;
--- End diff --

+1


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r103504542
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java ---
@@ -352,33 +357,73 @@ public T get() {
 
 //if the expected type is a closure or map and that's what we 
have, we're done (or if it's null);
 //but not allowed to return a future or DeferredSupplier as the 
resolved value
-if (v==null || (!forceDeep && type.isInstance(v) && 
!Future.class.isInstance(v) && !DeferredSupplier.class.isInstance(v)))
+if (v==null || (!forceDeep && type.isInstance(v) && 
!Future.class.isInstance(v) && !DeferredSupplier.class.isInstance(v) && 
!TaskFactory.class.isInstance(v)))
 return Maybe.of((T) v);
 
 try {
-if (immediately && v instanceof ImmediateSupplier) {
-final ImmediateSupplier supplier = 
(ImmediateSupplier) v;
+boolean allowImmediateExecution = false;
+boolean bailOutAfterImmediateExecution = false;
+
+if (v instanceof ImmediateSupplier) {
+allowImmediateExecution = true;
+
+} else {
+if ((v instanceof TaskFactory) && !(v instanceof 
DeferredSupplier)) {
+v = ((TaskFactory)v).newTask();
+allowImmediateExecution = true;
+bailOutAfterImmediateExecution = true;
+
BrooklynTaskTags.setTransient(((TaskAdaptable)v).asTask());
+if (isEvaluatingImmediately()) {
+// not needed if executing immediately
+BrooklynTaskTags.addTagDynamically( 
((TaskAdaptable)v).asTask(), BrooklynTaskTags.IMMEDIATE_TASK_TAG );
+}
+}
+
+//if it's a task or a future, we wait for the task to 
complete
+if (v instanceof TaskAdaptable) {
+v = ((TaskAdaptable) v).asTask();
+}
+}
+
+if (allowImmediateExecution && isEvaluatingImmediately()) {
+// TODO could allow for everything, when evaluating 
immediately -- but if the target isn't safe to run again
+// then we have to fail if immediate didn't work; to avoid 
breaking semantics we only do that for a few cases;
+// might be nice to get to the point where we can break 
those semantics however, 
+// ie weakening what getImmediate supports and making it 
be non-blocking, so that bailOut=true is the default.
+// if: v instanceof TaskFactory -- it is safe, it's a new 
API (but it is currently the only one supported);
+// more than safe, we have to do it -- or add code 
here to cancel tasks -- because it spawns new tasks
+// (other objects passed through here don't get 
cancelled, because other things might try again later;
+// ie a task or future passed in here might naturally 
be long-running so cancelling is wrong,
+// but with a task factory generated task it would 
leak if we submitted and didn't cancel!)
--- End diff --

aha, `DST` which we mostly use is fine, but its `doCancel` method was 
missing from the lighter-weight `CompoundTask`; have added it there and test 
passes


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r103500728
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java ---
@@ -352,33 +357,73 @@ public T get() {
 
 //if the expected type is a closure or map and that's what we 
have, we're done (or if it's null);
 //but not allowed to return a future or DeferredSupplier as the 
resolved value
-if (v==null || (!forceDeep && type.isInstance(v) && 
!Future.class.isInstance(v) && !DeferredSupplier.class.isInstance(v)))
+if (v==null || (!forceDeep && type.isInstance(v) && 
!Future.class.isInstance(v) && !DeferredSupplier.class.isInstance(v) && 
!TaskFactory.class.isInstance(v)))
 return Maybe.of((T) v);
 
 try {
-if (immediately && v instanceof ImmediateSupplier) {
-final ImmediateSupplier supplier = 
(ImmediateSupplier) v;
+boolean allowImmediateExecution = false;
+boolean bailOutAfterImmediateExecution = false;
+
+if (v instanceof ImmediateSupplier) {
+allowImmediateExecution = true;
+
+} else {
+if ((v instanceof TaskFactory) && !(v instanceof 
DeferredSupplier)) {
+v = ((TaskFactory)v).newTask();
+allowImmediateExecution = true;
+bailOutAfterImmediateExecution = true;
+
BrooklynTaskTags.setTransient(((TaskAdaptable)v).asTask());
+if (isEvaluatingImmediately()) {
+// not needed if executing immediately
+BrooklynTaskTags.addTagDynamically( 
((TaskAdaptable)v).asTask(), BrooklynTaskTags.IMMEDIATE_TASK_TAG );
+}
+}
+
+//if it's a task or a future, we wait for the task to 
complete
+if (v instanceof TaskAdaptable) {
+v = ((TaskAdaptable) v).asTask();
--- End diff --

as above, definitely not


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r103500687
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java
 ---
@@ -96,7 +98,50 @@ public ExecutionManager getExecutionManager() {
 /** returns tasks started by this context (or tasks which have all the 
tags on this object) */
 @Override
 public Set getTasks() { return 
executionManager.getTasksWithAllTags(tags); }
- 
+
+/** performs execution without spawning a new task thread, though it 
does temporarily set a fake task for the purpose of getting context;
+ * currently supports suppliers or callables  */
+@SuppressWarnings("unchecked")
+@Override
+public  Maybe getImmediately(Object callableOrSupplier) {
+BasicTask fakeTaskForContext;
+if (callableOrSupplier instanceof BasicTask) {
+fakeTaskForContext = (BasicTask)callableOrSupplier;
+if (fakeTaskForContext.isQueuedOrSubmitted()) {
+if (fakeTaskForContext.isDone()) {
+return Maybe.of((T)fakeTaskForContext.getUnchecked());
+} else {
+throw new ImmediateUnsupportedException("Task is in 
progress and incomplete: "+fakeTaskForContext);
+}
+}
+callableOrSupplier = fakeTaskForContext.getJob();
--- End diff --

as above


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r103498263
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java
 ---
@@ -96,7 +98,50 @@ public ExecutionManager getExecutionManager() {
 /** returns tasks started by this context (or tasks which have all the 
tags on this object) */
 @Override
 public Set getTasks() { return 
executionManager.getTasksWithAllTags(tags); }
- 
+
+/** performs execution without spawning a new task thread, though it 
does temporarily set a fake task for the purpose of getting context;
+ * currently supports suppliers or callables  */
+@SuppressWarnings("unchecked")
+@Override
+public  Maybe getImmediately(Object callableOrSupplier) {
+BasicTask fakeTaskForContext;
+if (callableOrSupplier instanceof BasicTask) {
+fakeTaskForContext = (BasicTask)callableOrSupplier;
--- End diff --

as noted above leaving the task running in background (or possibly not 
submitting it) is the right thing to do for immediate execution of a task; 
cancelling it is risky unless we know the task is dedicated to this one usage 
(hence preferring `TaskFactory`); in particular 
`EntityConfigTest.testGetTaskNonBlockingKey()` fails if 
`allowImmediateExecution=true`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r103499754
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java
 ---
@@ -96,7 +98,50 @@ public ExecutionManager getExecutionManager() {
 /** returns tasks started by this context (or tasks which have all the 
tags on this object) */
 @Override
 public Set getTasks() { return 
executionManager.getTasksWithAllTags(tags); }
- 
+
+/** performs execution without spawning a new task thread, though it 
does temporarily set a fake task for the purpose of getting context;
+ * currently supports suppliers or callables  */
+@SuppressWarnings("unchecked")
+@Override
+public  Maybe getImmediately(Object callableOrSupplier) {
+BasicTask fakeTaskForContext;
+if (callableOrSupplier instanceof BasicTask) {
+fakeTaskForContext = (BasicTask)callableOrSupplier;
--- End diff --

As for the particular code block marked here, it is invoked from 
`ValueResolver` when you supply a `TaskFactory`; `VR` generates the `Task` and 
passes it in with `allowImmediateExecution=true`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r103494117
  
--- Diff: 
camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/DslComponent.java
 ---
@@ -554,15 +562,55 @@ protected String resolveKeyName(boolean immediately) {
 .displayName("retrieving config for "+keyName)
 .tag(BrooklynTaskTags.TRANSIENT_TASK_TAG)
 .dynamic(false)
-.body(new Callable() {
-@Override
-public Object call() throws Exception {
-Entity targetEntity = component.get();
-String keyNameS = resolveKeyName(true);
-ConfigKey key = 
targetEntity.getEntityType().getConfigKey(keyNameS);
-return targetEntity.getConfig(key != null ? 
key : ConfigKeys.newConfigKey(Object.class, keyNameS));
-}})
-.build();
+.body(newCallable(false)).build();
+}
+
+private Callable newCallable(final boolean immediate) {
--- End diff --

yeah it's ugly; at least it's private.  have renamed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r103461269
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java ---
@@ -352,33 +357,73 @@ public T get() {
 
 //if the expected type is a closure or map and that's what we 
have, we're done (or if it's null);
 //but not allowed to return a future or DeferredSupplier as the 
resolved value
-if (v==null || (!forceDeep && type.isInstance(v) && 
!Future.class.isInstance(v) && !DeferredSupplier.class.isInstance(v)))
+if (v==null || (!forceDeep && type.isInstance(v) && 
!Future.class.isInstance(v) && !DeferredSupplier.class.isInstance(v) && 
!TaskFactory.class.isInstance(v)))
 return Maybe.of((T) v);
 
 try {
-if (immediately && v instanceof ImmediateSupplier) {
-final ImmediateSupplier supplier = 
(ImmediateSupplier) v;
+boolean allowImmediateExecution = false;
+boolean bailOutAfterImmediateExecution = false;
+
+if (v instanceof ImmediateSupplier) {
+allowImmediateExecution = true;
+
+} else {
+if ((v instanceof TaskFactory) && !(v instanceof 
DeferredSupplier)) {
+v = ((TaskFactory)v).newTask();
+allowImmediateExecution = true;
+bailOutAfterImmediateExecution = true;
+
BrooklynTaskTags.setTransient(((TaskAdaptable)v).asTask());
+if (isEvaluatingImmediately()) {
+// not needed if executing immediately
+BrooklynTaskTags.addTagDynamically( 
((TaskAdaptable)v).asTask(), BrooklynTaskTags.IMMEDIATE_TASK_TAG );
+}
+}
+
+//if it's a task or a future, we wait for the task to 
complete
+if (v instanceof TaskAdaptable) {
+v = ((TaskAdaptable) v).asTask();
+}
+}
+
+if (allowImmediateExecution && isEvaluatingImmediately()) {
+// TODO could allow for everything, when evaluating 
immediately -- but if the target isn't safe to run again
+// then we have to fail if immediate didn't work; to avoid 
breaking semantics we only do that for a few cases;
+// might be nice to get to the point where we can break 
those semantics however, 
+// ie weakening what getImmediate supports and making it 
be non-blocking, so that bailOut=true is the default.
+// if: v instanceof TaskFactory -- it is safe, it's a new 
API (but it is currently the only one supported);
+// more than safe, we have to do it -- or add code 
here to cancel tasks -- because it spawns new tasks
+// (other objects passed through here don't get 
cancelled, because other things might try again later;
+// ie a task or future passed in here might naturally 
be long-running so cancelling is wrong,
+// but with a task factory generated task it would 
leak if we submitted and didn't cancel!)
--- End diff --

DST should cancel -- this looks like a bug


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r103454836
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java ---
@@ -352,33 +357,73 @@ public T get() {
 
 //if the expected type is a closure or map and that's what we 
have, we're done (or if it's null);
 //but not allowed to return a future or DeferredSupplier as the 
resolved value
-if (v==null || (!forceDeep && type.isInstance(v) && 
!Future.class.isInstance(v) && !DeferredSupplier.class.isInstance(v)))
+if (v==null || (!forceDeep && type.isInstance(v) && 
!Future.class.isInstance(v) && !DeferredSupplier.class.isInstance(v) && 
!TaskFactory.class.isInstance(v)))
 return Maybe.of((T) v);
 
 try {
-if (immediately && v instanceof ImmediateSupplier) {
-final ImmediateSupplier supplier = 
(ImmediateSupplier) v;
+boolean allowImmediateExecution = false;
+boolean bailOutAfterImmediateExecution = false;
+
+if (v instanceof ImmediateSupplier) {
+allowImmediateExecution = true;
+
+} else {
+if ((v instanceof TaskFactory) && !(v instanceof 
DeferredSupplier)) {
+v = ((TaskFactory)v).newTask();
+allowImmediateExecution = true;
+bailOutAfterImmediateExecution = true;
+
BrooklynTaskTags.setTransient(((TaskAdaptable)v).asTask());
+if (isEvaluatingImmediately()) {
+// not needed if executing immediately
+BrooklynTaskTags.addTagDynamically( 
((TaskAdaptable)v).asTask(), BrooklynTaskTags.IMMEDIATE_TASK_TAG );
+}
+}
+
+//if it's a task or a future, we wait for the task to 
complete
+if (v instanceof TaskAdaptable) {
+v = ((TaskAdaptable) v).asTask();
+}
+}
+
+if (allowImmediateExecution && isEvaluatingImmediately()) {
+// TODO could allow for everything, when evaluating 
immediately -- but if the target isn't safe to run again
+// then we have to fail if immediate didn't work; to avoid 
breaking semantics we only do that for a few cases;
+// might be nice to get to the point where we can break 
those semantics however, 
+// ie weakening what getImmediate supports and making it 
be non-blocking, so that bailOut=true is the default.
+// if: v instanceof TaskFactory -- it is safe, it's a new 
API (but it is currently the only one supported);
+// more than safe, we have to do it -- or add code 
here to cancel tasks -- because it spawns new tasks
+// (other objects passed through here don't get 
cancelled, because other things might try again later;
+// ie a task or future passed in here might naturally 
be long-running so cancelling is wrong,
+// but with a task factory generated task it would 
leak if we submitted and didn't cancel!)
--- End diff --

Another failing test I wrote in `ValueResolverTest`, which leaks tasks 
(because the outer task is cancelled, while the inner tasks are not).

I'm a bit surprised that it fails - I had assumed that the `SequentialTask` 
would cancel its children when it was cancelled.

```
public void testTaskFactoryGetImmediatelyDoesNotBlockWithNestedTasks() {
final int NUM_CALLS = 3;
final AtomicInteger executingCount = new AtomicInteger();
final List outerTasks = Lists.newArrayList();

TaskFactory taskFactory = new TaskFactory() {
@Override public Task newTask() {
SequentialTask result = new 
SequentialTask<>(ImmutableList.of(new Callable() {
public String call() {
executingCount.incrementAndGet();
try {
Time.sleep(Duration.ONE_MINUTE);
return "myval";
} finally {
executingCount.decrementAndGet();
}
}}));
outerTasks.add(result);
return result;
}
};
for (int i = 0; i < NUM_CALLS; i++) {
Maybe result = 
Tasks.resolving(taskFactory).as(String.class).context(app).immediately(true).getMaybe();

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r103446823
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java
 ---
@@ -96,7 +98,50 @@ public ExecutionManager getExecutionManager() {
 /** returns tasks started by this context (or tasks which have all the 
tags on this object) */
 @Override
 public Set getTasks() { return 
executionManager.getTasksWithAllTags(tags); }
- 
+
+/** performs execution without spawning a new task thread, though it 
does temporarily set a fake task for the purpose of getting context;
+ * currently supports suppliers or callables  */
+@SuppressWarnings("unchecked")
+@Override
+public  Maybe getImmediately(Object callableOrSupplier) {
+BasicTask fakeTaskForContext;
+if (callableOrSupplier instanceof BasicTask) {
+fakeTaskForContext = (BasicTask)callableOrSupplier;
--- End diff --

Under what code path do we get here? Looking at 
`ValueResolver.getMaybeInternal(...)`, `allowImmediateExecution` will be false 
if the object is a task (rather than a `TaskFactory` or `DeferredSupplier`). 

For example, the test below fails (added to `ValueResolverTest`). It leaves 
a single instance of the task executing.

```
public void testTaskGetImmediatelyDoesNotBlock() {
final AtomicInteger executingCount = new AtomicInteger();

final Task task = new BasicTask<>(new Callable() {
public String call() {
executingCount.incrementAndGet();
try {
Time.sleep(Duration.ONE_MINUTE);
return "myval";
} finally {
executingCount.decrementAndGet();
}
}});

for (int i = 0; i < 3; i++) {
Maybe result = 
Tasks.resolving(task).as(String.class).context(app).immediately(true).getMaybe();
Asserts.assertTrue(result.isAbsent(), "result="+result);
}

Asserts.assertFalse(task.isSubmitted());

// The call below default times out after 30s while the task above 
is still running
// Expect the task to not be left running.
Asserts.succeedsEventually(new Runnable() {
public void run() {
Asserts.assertEquals(executingCount.get(), 0);
}
});
}
```

I'm not convinced that we want to support setting a config key with a value 
of type `Task` (long term). But I guess that will require more work, to change 
how `DependentConfiguration` is implemented. So we do need to support it 
short-term :-(


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r103439058
  
--- Diff: 
camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/DslComponent.java
 ---
@@ -554,15 +562,55 @@ protected String resolveKeyName(boolean immediately) {
 .displayName("retrieving config for "+keyName)
 .tag(BrooklynTaskTags.TRANSIENT_TASK_TAG)
 .dynamic(false)
-.body(new Callable() {
-@Override
-public Object call() throws Exception {
-Entity targetEntity = component.get();
-String keyNameS = resolveKeyName(true);
-ConfigKey key = 
targetEntity.getEntityType().getConfigKey(keyNameS);
-return targetEntity.getConfig(key != null ? 
key : ConfigKeys.newConfigKey(Object.class, keyNameS));
-}})
-.build();
+.body(newCallable(false)).build();
+}
+
+private Callable newCallable(final boolean immediate) {
--- End diff --

Minor: I'd have included a comment on this method to say that if immediate 
then the return value will be `Maybe` whereas if `!immediate` then the 
return value of the callable will be the actual `Object`. The difference of 
those semantics are not obvious in the method, but are relied upon by the 
caller.

It's surprising that the same method is used to return the two different 
styles of result, but I see why you did it (to reduce duplication).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r103455566
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java ---
@@ -512,6 +559,24 @@ public Object call() throws Exception {
 }
 }
 
+protected Maybe execImmediate(ExecutionContext exec, Object 
immediateSupplierOrImmediateTask) {
+Maybe result;
+try {
+result = exec.getImmediately(immediateSupplierOrImmediateTask);
+} catch (ImmediateSupplier.ImmediateUnsupportedException e) {
+return null;
+}
+// let 
InterruptingImmediateSupplier.InterruptingImmediateSupplierNotSupportedForObject
 
+// bet thrown, and caller who cares will catch that to know it can 
continue
--- End diff --

typo: `be thrown`.
And could add javadoc:
```
/**
 * @throws 
InterruptingImmediateSupplier.InterruptingImmediateSupplierNotSupportedForObject
 if ...
 */
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r103448474
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java
 ---
@@ -96,7 +98,50 @@ public ExecutionManager getExecutionManager() {
 /** returns tasks started by this context (or tasks which have all the 
tags on this object) */
 @Override
 public Set getTasks() { return 
executionManager.getTasksWithAllTags(tags); }
- 
+
+/** performs execution without spawning a new task thread, though it 
does temporarily set a fake task for the purpose of getting context;
+ * currently supports suppliers or callables  */
+@SuppressWarnings("unchecked")
+@Override
+public  Maybe getImmediately(Object callableOrSupplier) {
+BasicTask fakeTaskForContext;
+if (callableOrSupplier instanceof BasicTask) {
+fakeTaskForContext = (BasicTask)callableOrSupplier;
+if (fakeTaskForContext.isQueuedOrSubmitted()) {
+if (fakeTaskForContext.isDone()) {
+return Maybe.of((T)fakeTaskForContext.getUnchecked());
+} else {
+throw new ImmediateUnsupportedException("Task is in 
progress and incomplete: "+fakeTaskForContext);
+}
+}
+callableOrSupplier = fakeTaskForContext.getJob();
--- End diff --

Very clever, getting the underlying `Callable` to execute from the job!

However, I wonder if this just reduces the chance of leaving tasks behind 
(rather than preventing it).

The test below fails when added to `ValueResolverTest` (when also including 
the change to `ValueResolver.getMaybeInternal()` to set 
`allowImmediateExecution = true` so that this code path is taken). 

```
public void testTaskGetImmediatelyDoesNotBlockWithNestedTasks() {
final AtomicInteger executingCount = new AtomicInteger();

final SequentialTask outerTask = new 
SequentialTask<>(ImmutableList.of(new Callable() {
public String call() {
executingCount.incrementAndGet();
try {
Time.sleep(Duration.ONE_MINUTE);
return "myval";
} finally {
executingCount.decrementAndGet();
}
}}));

for (int i = 0; i < 3; i++) {
Maybe result = 
Tasks.resolving(outerTask).as(String.class).context(app).immediately(true).getMaybe();
Asserts.assertTrue(result.isAbsent(), "result="+result);
}

Asserts.assertFalse(outerTask.isSubmitted());

// the call below default times out after 30s while the task above 
is still running
Asserts.succeedsEventually(new Runnable() {
public void run() {
Asserts.assertEquals(executingCount.get(), 0);
}
});
}
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r103452425
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java ---
@@ -352,33 +357,73 @@ public T get() {
 
 //if the expected type is a closure or map and that's what we 
have, we're done (or if it's null);
 //but not allowed to return a future or DeferredSupplier as the 
resolved value
-if (v==null || (!forceDeep && type.isInstance(v) && 
!Future.class.isInstance(v) && !DeferredSupplier.class.isInstance(v)))
+if (v==null || (!forceDeep && type.isInstance(v) && 
!Future.class.isInstance(v) && !DeferredSupplier.class.isInstance(v) && 
!TaskFactory.class.isInstance(v)))
 return Maybe.of((T) v);
 
 try {
-if (immediately && v instanceof ImmediateSupplier) {
-final ImmediateSupplier supplier = 
(ImmediateSupplier) v;
+boolean allowImmediateExecution = false;
+boolean bailOutAfterImmediateExecution = false;
+
+if (v instanceof ImmediateSupplier) {
+allowImmediateExecution = true;
+
+} else {
+if ((v instanceof TaskFactory) && !(v instanceof 
DeferredSupplier)) {
+v = ((TaskFactory)v).newTask();
+allowImmediateExecution = true;
+bailOutAfterImmediateExecution = true;
+
BrooklynTaskTags.setTransient(((TaskAdaptable)v).asTask());
+if (isEvaluatingImmediately()) {
+// not needed if executing immediately
+BrooklynTaskTags.addTagDynamically( 
((TaskAdaptable)v).asTask(), BrooklynTaskTags.IMMEDIATE_TASK_TAG );
+}
+}
+
+//if it's a task or a future, we wait for the task to 
complete
+if (v instanceof TaskAdaptable) {
+v = ((TaskAdaptable) v).asTask();
--- End diff --

When playing around with the extra tests (see previous comments), do we 
need to add `allowImmediateExecution = true;` when `v instanceof 
TaskAdaptable`? Or will that have other unwanted? side-effects?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r103455943
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java ---
@@ -512,6 +559,24 @@ public Object call() throws Exception {
 }
 }
 
+protected Maybe execImmediate(ExecutionContext exec, Object 
immediateSupplierOrImmediateTask) {
+Maybe result;
+try {
+result = exec.getImmediately(immediateSupplierOrImmediateTask);
+} catch (ImmediateSupplier.ImmediateUnsupportedException e) {
+return null;
--- End diff --

I'd prefer this to propagate the exception, rather than return null. I 
don't like the use of null to indicate that it couldn't be evaluated.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r103447467
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java
 ---
@@ -96,7 +98,50 @@ public ExecutionManager getExecutionManager() {
 /** returns tasks started by this context (or tasks which have all the 
tags on this object) */
 @Override
 public Set getTasks() { return 
executionManager.getTasksWithAllTags(tags); }
- 
+
+/** performs execution without spawning a new task thread, though it 
does temporarily set a fake task for the purpose of getting context;
+ * currently supports suppliers or callables  */
+@SuppressWarnings("unchecked")
+@Override
+public  Maybe getImmediately(Object callableOrSupplier) {
+BasicTask fakeTaskForContext;
+if (callableOrSupplier instanceof BasicTask) {
+fakeTaskForContext = (BasicTask)callableOrSupplier;
--- End diff --

If I change `ValueResolver.getMaybeInternal()` to set 
`allowImmediateExecution = true;` when it is passed a `TaskAdaptable`, then the 
above test does pass.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101893858
  
--- Diff: 
camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/DslComponent.java
 ---
@@ -539,8 +550,9 @@ protected String resolveKeyName(boolean immediately) {
 @Override
 public final Maybe getImmediately() {
 Maybe targetEntityMaybe = component.getImmediately();
-if (targetEntityMaybe.isAbsent()) return Maybe.absent("Target 
entity not available");
+if (targetEntityMaybe.isAbsent()) return 
Maybe.cast(targetEntityMaybe);
 EntityInternal targetEntity = (EntityInternal) 
targetEntityMaybe.get();
+checkAndTagForRecursiveReference(targetEntity);
--- End diff --

did it, but without an official task type -- easy enough to get the 
routines to share code and trigger a dedicated (non-thread) task


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101891562
  
--- Diff: 
camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/DslComponent.java
 ---
@@ -539,8 +550,9 @@ protected String resolveKeyName(boolean immediately) {
 @Override
 public final Maybe getImmediately() {
 Maybe targetEntityMaybe = component.getImmediately();
-if (targetEntityMaybe.isAbsent()) return Maybe.absent("Target 
entity not available");
+if (targetEntityMaybe.isAbsent()) return 
Maybe.cast(targetEntityMaybe);
 EntityInternal targetEntity = (EntityInternal) 
targetEntityMaybe.get();
+checkAndTagForRecursiveReference(targetEntity);
--- End diff --

but actually this won't work either will it --

consider P1 needs to evaluate key C1, then key C2, and C2 refers to C1.  if 
it isn't a dedicated subtask the second check will fail.

guess we need to ensure a dedicated subtask. :( . i'll see whether we can 
do that, probably with an "official" task tag type.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101878724
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java ---
@@ -352,33 +357,51 @@ public T get() {
 
 //if the expected type is a closure or map and that's what we 
have, we're done (or if it's null);
 //but not allowed to return a future or DeferredSupplier as the 
resolved value
-if (v==null || (!forceDeep && type.isInstance(v) && 
!Future.class.isInstance(v) && !DeferredSupplier.class.isInstance(v)))
+if (v==null || (!forceDeep && type.isInstance(v) && 
!Future.class.isInstance(v) && !DeferredSupplier.class.isInstance(v) && 
!TaskFactory.class.isInstance(v)))
 return Maybe.of((T) v);
 
 try {
-if (immediately && v instanceof ImmediateSupplier) {
-final ImmediateSupplier supplier = 
(ImmediateSupplier) v;
+if (isEvaluatingImmediately() && v instanceof 
ImmediateSupplier) {
+final ImmediateSupplier supplier = 
(ImmediateSupplier) v;
 try {
-Maybe result = supplier.getImmediately();
+Maybe result = exec.getImmediately(supplier);
 
 // Recurse: need to ensure returned value is cast, etc
 return (result.isPresent())
 ? recursive
 ? new ValueResolver(result.get(), type, 
this).getMaybe()
 : result
-: Maybe.absent();
+: result;
 } catch (ImmediateSupplier.ImmediateUnsupportedException 
e) {
 log.debug("Unable to resolve-immediately for 
"+description+" ("+v+"); falling back to executing with timeout", e);
 }
 }
 
+// TODO if evaluating immediately should use a new 
ExecutionContext.submitImmediate(...)
+// and sets a timeout but which wraps a task but does not 
spawn a new thread
+
+if ((v instanceof TaskFactory) && !(v instanceof 
DeferredSupplier)) {
--- End diff --

TODO think about this


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101878631
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/InterruptingImmediateSupplier.java
 ---
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.util.core.task;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.Semaphore;
+
+import org.apache.brooklyn.util.exceptions.Exceptions;
+import org.apache.brooklyn.util.exceptions.RuntimeInterruptedException;
+import org.apache.brooklyn.util.guava.Maybe;
+
+import com.google.common.annotations.Beta;
+import com.google.common.base.Supplier;
+
+/**
+ * Wraps a {@link Supplier} as an {@link ImmediateSupplier} by 
interrupting the thread before calling {@link Supplier#get()}.
+ * If the call succeeds, the result is returned.
+ * If the call throws any trace including an {@link InterruptedException} 
or {@link RuntimeInterruptedException} 
+ * (ie the call failed due to the interruption, typically because it tried 
to wait) 
+ * then this class concludes that there is no value available immediately 
and returns {@link Maybe#absent()}.
+ * If the call throws any other error, that is returned.
+ * The interruption is cleared afterwards (unless the thread was 
interrupted when the method was entered).
+ * 
+ * Note that some "immediate" methods, such as {@link Semaphore#acquire()} 
when a semaphore is available,
+ * will throw if the thread is interrupted.  Typically there are 
workarounds, for instance:
+ * if (semaphore.tryAcquire()) semaphore.acquire();. 
+ */
+@Beta
+public class InterruptingImmediateSupplier implements 
ImmediateSupplier, DeferredSupplier {
--- End diff --

seeing as you wrote it it would be churlish not to add this!  :)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101878497
  
--- Diff: 
core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java ---
@@ -244,59 +252,152 @@ public void 
testGetConfigMapWithSubValueAsStringNotCoerced() throws Exception {
 // of the previous "test.confMapThing.obj".
 //
 // Presumably an earlier call to task.get() timed out, causing it to 
cancel the task?
+// Alex: yes, a task.cancel is performed for maps in
+// 
AbstractEntity$BasicConfigurationSupport(AbstractConfigurationSupportInternal).getNonBlockingResolvingStructuredKey(ConfigKey)

+ 
+//
 // I (Aled) question whether we want to support passing a task (rather 
than a 
 // DeferredSupplier or TaskFactory, for example). Our 
EntitySpec.configure is overloaded
 // to take a Task, but that feels wrong!?
-@Test(groups="Broken")
-public void testGetTaskNonBlocking() throws Exception {
-final CountDownLatch latch = new CountDownLatch(1);
-Task task = Tasks.builder().body(
+//
+// If starting clean I (Alex) would agree, we should use TaskFactory. 
However the
+// DependentConfiguration methods -- including the ubiquitous 
AttributeWhenReady --
+// return Task instances so they should survive a getNonBlocking or 
get with a short timeout 
+// access, and if a value is subsequently available it should be 
returned 
+// (which this test asserts, but is currently failing). If TaskFactory 
is used the
+// intended semantics are clear -- you create a new task on each 
access, and can interrupt it
+// and discard it if needed. For a Task it's less clear: probably the 
semantics are that the
+// first returned value is what the value is forevermore. Probably it 
should not be interrupted
+// on a non-blocking / short-wait access, or possibly it should simply 
be re-run if a previous
+// execution was interrupted (but take care if we have a simultaneous 
non-blocking and blocking
+// access, if the first one interrupts the second one should still get 
a value).
+// I tend to think ideally we should switch to using TaskFactory in 
DependentConfiguration.
+class ConfigNonBlockingFixture {
+final Semaphore latch = new Semaphore(0);
+final String expectedVal = "myval";
+Object blockingVal;
+
+protected ConfigNonBlockingFixture usingTask() {
+blockingVal = taskFactory().newTask();
+return this;
+}
+
+protected ConfigNonBlockingFixture usingTaskFactory() {
+blockingVal = taskFactory();
+return this;
+}
+
+protected ConfigNonBlockingFixture usingDeferredSupplier() {
+blockingVal = deferredSupplier();
+return this;
+}
+
+protected ConfigNonBlockingFixture usingImmediateSupplier() {
+blockingVal = new 
InterruptingImmediateSupplier(deferredSupplier());
+return this;
+}
+
+private TaskFactory taskFactory() {
+return Tasks.builder().body(
 new Callable() {
 @Override
 public String call() throws Exception {
-latch.await();
+if (!latch.tryAcquire()) latch.acquire();
+latch.release();
 return "myval";
 }})
-.build();
-runGetConfigNonBlocking(latch, task, "myval");
-}
-
-@Test
-public void testGetDeferredSupplierNonBlocking() throws Exception {
-final CountDownLatch latch = new CountDownLatch(1);
-DeferredSupplier task = new DeferredSupplier() {
-@Override public String get() {
-try {
-latch.await();
-} catch (InterruptedException e) {
-throw Exceptions.propagate(e);
-}
-return "myval";
-}
-};
-runGetConfigNonBlocking(latch, task, "myval");
-}
-
-@SuppressWarnings({"unchecked", "rawtypes"})
-protected void runGetConfigNonBlocking(CountDownLatch latch, Object 
blockingVal, String expectedVal) throws Exception {
-TestEntity entity = (TestEntity) 
mgmt.getEntityManager().createEntity(EntitySpec.create(TestEntity.class)
-.configure(TestEntity.CONF_MAP_OBJ_THING, 
ImmutableMap.of("mysub", blockingVal))
-.configure((ConfigKey)TestEntity.CONF_NAME, 

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101878445
  
--- Diff: 
core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java ---
@@ -244,59 +252,152 @@ public void 
testGetConfigMapWithSubValueAsStringNotCoerced() throws Exception {
 // of the previous "test.confMapThing.obj".
 //
 // Presumably an earlier call to task.get() timed out, causing it to 
cancel the task?
+// Alex: yes, a task.cancel is performed for maps in
+// 
AbstractEntity$BasicConfigurationSupport(AbstractConfigurationSupportInternal).getNonBlockingResolvingStructuredKey(ConfigKey)

+ 
+//
 // I (Aled) question whether we want to support passing a task (rather 
than a 
 // DeferredSupplier or TaskFactory, for example). Our 
EntitySpec.configure is overloaded
 // to take a Task, but that feels wrong!?
-@Test(groups="Broken")
-public void testGetTaskNonBlocking() throws Exception {
-final CountDownLatch latch = new CountDownLatch(1);
-Task task = Tasks.builder().body(
+//
+// If starting clean I (Alex) would agree, we should use TaskFactory. 
However the
+// DependentConfiguration methods -- including the ubiquitous 
AttributeWhenReady --
+// return Task instances so they should survive a getNonBlocking or 
get with a short timeout 
+// access, and if a value is subsequently available it should be 
returned 
+// (which this test asserts, but is currently failing). If TaskFactory 
is used the
+// intended semantics are clear -- you create a new task on each 
access, and can interrupt it
+// and discard it if needed. For a Task it's less clear: probably the 
semantics are that the
+// first returned value is what the value is forevermore. Probably it 
should not be interrupted
+// on a non-blocking / short-wait access, or possibly it should simply 
be re-run if a previous
+// execution was interrupted (but take care if we have a simultaneous 
non-blocking and blocking
+// access, if the first one interrupts the second one should still get 
a value).
+// I tend to think ideally we should switch to using TaskFactory in 
DependentConfiguration.
+class ConfigNonBlockingFixture {
+final Semaphore latch = new Semaphore(0);
+final String expectedVal = "myval";
+Object blockingVal;
+
+protected ConfigNonBlockingFixture usingTask() {
+blockingVal = taskFactory().newTask();
+return this;
+}
+
+protected ConfigNonBlockingFixture usingTaskFactory() {
+blockingVal = taskFactory();
+return this;
+}
+
+protected ConfigNonBlockingFixture usingDeferredSupplier() {
+blockingVal = deferredSupplier();
+return this;
+}
+
+protected ConfigNonBlockingFixture usingImmediateSupplier() {
+blockingVal = new 
InterruptingImmediateSupplier(deferredSupplier());
+return this;
+}
+
+private TaskFactory taskFactory() {
+return Tasks.builder().body(
 new Callable() {
 @Override
 public String call() throws Exception {
-latch.await();
+if (!latch.tryAcquire()) latch.acquire();
+latch.release();
 return "myval";
 }})
-.build();
-runGetConfigNonBlocking(latch, task, "myval");
-}
-
-@Test
-public void testGetDeferredSupplierNonBlocking() throws Exception {
-final CountDownLatch latch = new CountDownLatch(1);
-DeferredSupplier task = new DeferredSupplier() {
-@Override public String get() {
-try {
-latch.await();
-} catch (InterruptedException e) {
-throw Exceptions.propagate(e);
-}
-return "myval";
-}
-};
-runGetConfigNonBlocking(latch, task, "myval");
-}
-
-@SuppressWarnings({"unchecked", "rawtypes"})
-protected void runGetConfigNonBlocking(CountDownLatch latch, Object 
blockingVal, String expectedVal) throws Exception {
-TestEntity entity = (TestEntity) 
mgmt.getEntityManager().createEntity(EntitySpec.create(TestEntity.class)
-.configure(TestEntity.CONF_MAP_OBJ_THING, 
ImmutableMap.of("mysub", blockingVal))
-.configure((ConfigKey)TestEntity.CONF_NAME, 

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101878328
  
--- Diff: 
core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java ---
@@ -244,59 +252,152 @@ public void 
testGetConfigMapWithSubValueAsStringNotCoerced() throws Exception {
 // of the previous "test.confMapThing.obj".
 //
 // Presumably an earlier call to task.get() timed out, causing it to 
cancel the task?
+// Alex: yes, a task.cancel is performed for maps in
+// 
AbstractEntity$BasicConfigurationSupport(AbstractConfigurationSupportInternal).getNonBlockingResolvingStructuredKey(ConfigKey)

+ 
+//
 // I (Aled) question whether we want to support passing a task (rather 
than a 
 // DeferredSupplier or TaskFactory, for example). Our 
EntitySpec.configure is overloaded
 // to take a Task, but that feels wrong!?
-@Test(groups="Broken")
-public void testGetTaskNonBlocking() throws Exception {
-final CountDownLatch latch = new CountDownLatch(1);
-Task task = Tasks.builder().body(
+//
+// If starting clean I (Alex) would agree, we should use TaskFactory. 
However the
--- End diff --

Interesting idea that it is a single (shared) task.  I feel like each 
caller gets its own instance/subscription hence `TaskFactory`.  But for another 
day... (and in any case I'd love us to move to a JS promise/then model)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101878072
  
--- Diff: 
camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java
 ---
@@ -91,6 +96,62 @@ public void testConfigInConfigBlock() throws Exception {
 assertNull(entity.getMyField()); // field with @SetFromFlag
 assertNull(entity.getMyField2()); // field with 
@SetFromFlag("myField2Alias"), set using alias
 }
+
+
+@Test
+public void testRecursiveConfigFailsGracefully() throws Exception {
+doTestRecursiveConfigFailsGracefully(false);
+}
+
+// TODO this test fails because entities aren't available when 
evaluating immediately
+@Test
+public void testRecursiveConfigImmediateFailsGracefully() throws 
Exception {
+doTestRecursiveConfigFailsGracefully(true);
+}
+
+protected void doTestRecursiveConfigFailsGracefully(boolean immediate) 
throws Exception {
+String yaml = Joiner.on("\n").join(
+"services:",
+"- type: org.apache.brooklyn.core.test.entity.TestEntity",
+"  brooklyn.config:",
+"infinite_loop: $brooklyn:config(\"infinite_loop\")");
+
+final Entity app = createStartWaitAndLogApplication(yaml);
+TestEntity entity = (TestEntity) 
Iterables.getOnlyElement(app.getChildren());
+
+Thread t = new Thread(new Runnable() {
+@Override
+public void run() {
+try {
+Time.sleep(Duration.FIVE_SECONDS);
+// error, loop wasn't interrupted or detected
+LOG.warn("Timeout elapsed, destroying items; usage: "+
+
((LocalManagementContext)mgmt()).getGarbageCollector().getUsageString());
+//Entities.destroy(app);
+} catch (RuntimeInterruptedException e) {
+// expected on normal execution
+Thread.interrupted();
--- End diff --

exactly, comment added


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101877907
  
--- Diff: 
camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/DslComponent.java
 ---
@@ -539,8 +550,9 @@ protected String resolveKeyName(boolean immediately) {
 @Override
 public final Maybe getImmediately() {
 Maybe targetEntityMaybe = component.getImmediately();
-if (targetEntityMaybe.isAbsent()) return Maybe.absent("Target 
entity not available");
+if (targetEntityMaybe.isAbsent()) return 
Maybe.cast(targetEntityMaybe);
 EntityInternal targetEntity = (EntityInternal) 
targetEntityMaybe.get();
+checkAndTagForRecursiveReference(targetEntity);
--- End diff --

the tag is left on the task.  you're right that could cause problems if the 
calling code isn't in a task (maybe it always will be but safer not to assume). 
 TODO remove tag afterwards


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101877735
  
--- Diff: 
api/src/main/java/org/apache/brooklyn/api/mgmt/ExecutionContext.java ---
@@ -64,4 +65,6 @@
 
 boolean isShutdown();
 
+ Maybe getImmediately(Object callableOrSupplier);
--- End diff --

good idea.  we should maybe move `ImmediateSupplier` to the utils package, 
then we could reference its javadoc?  we might also change the return type to 
be `ReferenceWithError` so the "can't immediately tell if there's a 
value" problem state can be detected without throwing.  have marked `@Beta` for 
now.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101853047
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/InterruptingImmediateSupplier.java
 ---
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.util.core.task;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.Semaphore;
+
+import org.apache.brooklyn.util.exceptions.Exceptions;
+import org.apache.brooklyn.util.exceptions.RuntimeInterruptedException;
+import org.apache.brooklyn.util.guava.Maybe;
+
+import com.google.common.annotations.Beta;
+import com.google.common.base.Supplier;
+
+/**
+ * Wraps a {@link Supplier} as an {@link ImmediateSupplier} by 
interrupting the thread before calling {@link Supplier#get()}.
+ * If the call succeeds, the result is returned.
+ * If the call throws any trace including an {@link InterruptedException} 
or {@link RuntimeInterruptedException} 
+ * (ie the call failed due to the interruption, typically because it tried 
to wait) 
+ * then this class concludes that there is no value available immediately 
and returns {@link Maybe#absent()}.
+ * If the call throws any other error, that is returned.
+ * The interruption is cleared afterwards (unless the thread was 
interrupted when the method was entered).
+ * 
+ * Note that some "immediate" methods, such as {@link Semaphore#acquire()} 
when a semaphore is available,
+ * will throw if the thread is interrupted.  Typically there are 
workarounds, for instance:
+ * if (semaphore.tryAcquire()) semaphore.acquire();. 
+ */
+@Beta
+public class InterruptingImmediateSupplier implements 
ImmediateSupplier, DeferredSupplier {
+
+final Supplier nestedSupplier;
--- End diff --

I'd make this private (so it's clear to others that it's not being accessed 
directly from elsewhere - if such a use-case arises, we can change it or add a 
getter or whatever). Or if you thinking about users sub-classing it, then make 
it protected.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101790138
  
--- Diff: 
camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java
 ---
@@ -91,6 +96,62 @@ public void testConfigInConfigBlock() throws Exception {
 assertNull(entity.getMyField()); // field with @SetFromFlag
 assertNull(entity.getMyField2()); // field with 
@SetFromFlag("myField2Alias"), set using alias
 }
+
+
+@Test
+public void testRecursiveConfigFailsGracefully() throws Exception {
+doTestRecursiveConfigFailsGracefully(false);
+}
+
+// TODO this test fails because entities aren't available when 
evaluating immediately
--- End diff --

This test passes for me - when/why does it fail, or does this comment need 
deleted?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101864959
  
--- Diff: 
core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java ---
@@ -244,59 +252,152 @@ public void 
testGetConfigMapWithSubValueAsStringNotCoerced() throws Exception {
 // of the previous "test.confMapThing.obj".
 //
 // Presumably an earlier call to task.get() timed out, causing it to 
cancel the task?
+// Alex: yes, a task.cancel is performed for maps in
+// 
AbstractEntity$BasicConfigurationSupport(AbstractConfigurationSupportInternal).getNonBlockingResolvingStructuredKey(ConfigKey)

+ 
+//
 // I (Aled) question whether we want to support passing a task (rather 
than a 
 // DeferredSupplier or TaskFactory, for example). Our 
EntitySpec.configure is overloaded
 // to take a Task, but that feels wrong!?
-@Test(groups="Broken")
-public void testGetTaskNonBlocking() throws Exception {
-final CountDownLatch latch = new CountDownLatch(1);
-Task task = Tasks.builder().body(
+//
+// If starting clean I (Alex) would agree, we should use TaskFactory. 
However the
--- End diff --

I suspect our `DependentConfiguration` class could do with a re-write, but 
not sure off-hand exactly what it should look like. And doesn't feel like it's 
worth doing right now.

It's trick because `WaitInTaskForAttributeReady extends Callable` expects 
to subscribe to (multiple) sensors, and then block for a matching value. We 
could most likely re-implement that (see below).

Using a `TaskFactory` would be tricky, because we only want to do the 
subscriptions once. It is conceptually a single "task" (i.e. we don't want to 
do the subscriptions multiple times).

My gut-feel is therefore we could implement it as a `DeferredSupplier`. 
When first called (or maybe when created), it would create the subscriptions. 
The thread executing the `get` would just be blocking on a `CountDownLatch`, 
which would be set by the subscription callbacks when the value was available 
(or when the entity had failed). Multiple calls to `get()` would share the same 
instance, so would all be blocking on the same thing.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101855532
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/InterruptingImmediateSupplier.java
 ---
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.util.core.task;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.Semaphore;
+
+import org.apache.brooklyn.util.exceptions.Exceptions;
+import org.apache.brooklyn.util.exceptions.RuntimeInterruptedException;
+import org.apache.brooklyn.util.guava.Maybe;
+
+import com.google.common.annotations.Beta;
+import com.google.common.base.Supplier;
+
+/**
+ * Wraps a {@link Supplier} as an {@link ImmediateSupplier} by 
interrupting the thread before calling {@link Supplier#get()}.
+ * If the call succeeds, the result is returned.
+ * If the call throws any trace including an {@link InterruptedException} 
or {@link RuntimeInterruptedException} 
+ * (ie the call failed due to the interruption, typically because it tried 
to wait) 
+ * then this class concludes that there is no value available immediately 
and returns {@link Maybe#absent()}.
+ * If the call throws any other error, that is returned.
+ * The interruption is cleared afterwards (unless the thread was 
interrupted when the method was entered).
+ * 
+ * Note that some "immediate" methods, such as {@link Semaphore#acquire()} 
when a semaphore is available,
+ * will throw if the thread is interrupted.  Typically there are 
workarounds, for instance:
+ * if (semaphore.tryAcquire()) semaphore.acquire();. 
+ */
+@Beta
+public class InterruptingImmediateSupplier implements 
ImmediateSupplier, DeferredSupplier {
--- End diff --

I'd add a test specifically for `InterruptingImmediateSupplierTest` (rather 
than just testing it indirectly in `EntityConfigTest`). For example:
```
public class InterruptingImmediateSupplierTest {

@Test(expectedExceptions=UnsupportedOperationException.class)
public void testOfInvalidType() throws Exception {
InterruptingImmediateSupplier.of("myval");
}

@Test
public void testRunnable() throws Exception {
assertImmediatelyPresent(Runnables.doNothing(), null);
assertImmediatelyAbsent(new SleepingRunnable());
assertImmediatelyFails(new FailingRunnable(), 
MarkerException.class);
}

@Test
public void testCallable() throws Exception {
assertImmediatelyPresent(Callables.returning("myval"), "myval");
assertImmediatelyAbsent(new SleepingCallable());
assertImmediatelyFails(new FailingCallable(), 
MarkerException.class);
}

@Test
public void testSupplier() throws Exception {
assertImmediatelyPresent(Suppliers.ofInstance("myval"), "myval");
assertImmediatelyAbsent(new SleepingSupplier());
assertImmediatelyFails(new FailingSupplier(), 
MarkerException.class);
}

private void assertImmediatelyPresent(Object orig, Object expected) {
Maybe result = getImmediately(orig);
assertEquals(result.get(), expected);
assertFalse(Thread.currentThread().isInterrupted());
}

private void assertImmediatelyAbsent(Object orig) {
Maybe result = getImmediately(orig);
assertTrue(result.isAbsent(), "result="+result);
assertFalse(Thread.currentThread().isInterrupted());
}

private void assertImmediatelyFails(Object orig, Class expected) {
try {
Maybe result = getImmediately(orig);
Asserts.shouldHaveFailedPreviously("result="+result);
} catch (Exception e) {
Asserts.expectedFailureOfType(e, expected);
}
assertFalse(Thread.currentThread().isInterrupted());
}

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101836841
  
--- Diff: 
camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java
 ---
@@ -91,6 +96,62 @@ public void testConfigInConfigBlock() throws Exception {
 assertNull(entity.getMyField()); // field with @SetFromFlag
 assertNull(entity.getMyField2()); // field with 
@SetFromFlag("myField2Alias"), set using alias
 }
+
+
+@Test
+public void testRecursiveConfigFailsGracefully() throws Exception {
+doTestRecursiveConfigFailsGracefully(false);
+}
+
+// TODO this test fails because entities aren't available when 
evaluating immediately
+@Test
+public void testRecursiveConfigImmediateFailsGracefully() throws 
Exception {
+doTestRecursiveConfigFailsGracefully(true);
+}
+
+protected void doTestRecursiveConfigFailsGracefully(boolean immediate) 
throws Exception {
+String yaml = Joiner.on("\n").join(
+"services:",
+"- type: org.apache.brooklyn.core.test.entity.TestEntity",
+"  brooklyn.config:",
+"infinite_loop: $brooklyn:config(\"infinite_loop\")");
+
+final Entity app = createStartWaitAndLogApplication(yaml);
+TestEntity entity = (TestEntity) 
Iterables.getOnlyElement(app.getChildren());
+
+Thread t = new Thread(new Runnable() {
+@Override
+public void run() {
+try {
+Time.sleep(Duration.FIVE_SECONDS);
+// error, loop wasn't interrupted or detected
+LOG.warn("Timeout elapsed, destroying items; usage: "+
+
((LocalManagementContext)mgmt()).getGarbageCollector().getUsageString());
+//Entities.destroy(app);
+} catch (RuntimeInterruptedException e) {
+// expected on normal execution
+Thread.interrupted();
--- End diff --

Are you calling this to clear the interrupted status? Why? Do you get an 
ugly exception or something if we don't?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101851346
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionManager.java
 ---
@@ -753,7 +753,10 @@ protected void beforeStartAtomicTask(Map flags, 
Task task) {
 /** invoked in a task's thread when a task is starting to run (may be 
some time after submitted), 
  * but before doing any of the task's work, so that we can update 
bookkeeping and notify callbacks */
 protected void internalBeforeStart(Map flags, Task task) {
-activeTaskCount.incrementAndGet();
+int count = activeTaskCount.incrementAndGet();
+if (count % 1000==0) {
--- End diff --

If we hover around the 999 to 1001 mark for the number of active tasks, 
then we'll get this log message lots of times. But I think that's acceptable, 
in exchange for simpler code. So fine as it is.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101847631
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java
 ---
@@ -146,7 +146,6 @@ public T call() {
 .immediately(true)
 .deep(true)
 .context(getContext())
-.swallowExceptions()
--- End diff --

I probably agree with this change, but don't feel confident about the full 
implications of it throwing the exception rather than returning the default 
value versus absent. In your test `doTestRecursiveConfigFailsGracefully` it 
certainly makes sense, but not sure what else this will affect.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101836610
  
--- Diff: 
camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java
 ---
@@ -91,6 +96,62 @@ public void testConfigInConfigBlock() throws Exception {
 assertNull(entity.getMyField()); // field with @SetFromFlag
 assertNull(entity.getMyField2()); // field with 
@SetFromFlag("myField2Alias"), set using alias
 }
+
+
+@Test
+public void testRecursiveConfigFailsGracefully() throws Exception {
+doTestRecursiveConfigFailsGracefully(false);
+}
+
+// TODO this test fails because entities aren't available when 
evaluating immediately
+@Test
+public void testRecursiveConfigImmediateFailsGracefully() throws 
Exception {
+doTestRecursiveConfigFailsGracefully(true);
+}
+
+protected void doTestRecursiveConfigFailsGracefully(boolean immediate) 
throws Exception {
+String yaml = Joiner.on("\n").join(
+"services:",
+"- type: org.apache.brooklyn.core.test.entity.TestEntity",
+"  brooklyn.config:",
+"infinite_loop: $brooklyn:config(\"infinite_loop\")");
+
+final Entity app = createStartWaitAndLogApplication(yaml);
+TestEntity entity = (TestEntity) 
Iterables.getOnlyElement(app.getChildren());
+
+Thread t = new Thread(new Runnable() {
+@Override
+public void run() {
+try {
+Time.sleep(Duration.FIVE_SECONDS);
+// error, loop wasn't interrupted or detected
+LOG.warn("Timeout elapsed, destroying items; usage: "+
+
((LocalManagementContext)mgmt()).getGarbageCollector().getUsageString());
+//Entities.destroy(app);
--- End diff --

Delete commented out code, or add additional comment to say when one would 
uncomment it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101868849
  
--- Diff: 
utils/common/src/main/java/org/apache/brooklyn/util/exceptions/Exceptions.java 
---
@@ -263,8 +264,41 @@ public static Throwable 
collapseIncludingAllCausalMessages(Throwable source) {
 public static Throwable collapse(Throwable source, boolean 
collapseCausalChain) {
 return collapse(source, collapseCausalChain, false, 
ImmutableSet.of(), new Object[0]);
 }
+
+/** As {@link Throwables#getCausalChain(Throwable)} but safe in the 
face of perverse classes which return themselves as their cause or otherwise 
have a recursive causal chain. */
+public static List getCausalChain(Throwable t) {
--- End diff --

Add to `ExceptionsTest`:
```
@Test
public void testGetCausalChain() throws Exception {
Exception e1 = new Exception("e1");
Exception e2 = new Exception("e2", e1);
assertEquals(Exceptions.getCausalChain(e2), ImmutableList.of(e2, 
e1));
}

@Test
public void testGetCausalChainRecursive() throws Exception {
Exception e1 = new Exception("e1") {
public synchronized Throwable getCause() {
return this;
}
};
Exception e2 = new Exception("e2", e1);
assertEquals(Exceptions.getCausalChain(e2), ImmutableList.of(e2, 
e1));
}
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101868262
  
--- Diff: 
core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java ---
@@ -244,59 +252,152 @@ public void 
testGetConfigMapWithSubValueAsStringNotCoerced() throws Exception {
 // of the previous "test.confMapThing.obj".
 //
 // Presumably an earlier call to task.get() timed out, causing it to 
cancel the task?
+// Alex: yes, a task.cancel is performed for maps in
+// 
AbstractEntity$BasicConfigurationSupport(AbstractConfigurationSupportInternal).getNonBlockingResolvingStructuredKey(ConfigKey)

+ 
+//
 // I (Aled) question whether we want to support passing a task (rather 
than a 
 // DeferredSupplier or TaskFactory, for example). Our 
EntitySpec.configure is overloaded
 // to take a Task, but that feels wrong!?
-@Test(groups="Broken")
-public void testGetTaskNonBlocking() throws Exception {
-final CountDownLatch latch = new CountDownLatch(1);
-Task task = Tasks.builder().body(
+//
+// If starting clean I (Alex) would agree, we should use TaskFactory. 
However the
+// DependentConfiguration methods -- including the ubiquitous 
AttributeWhenReady --
+// return Task instances so they should survive a getNonBlocking or 
get with a short timeout 
+// access, and if a value is subsequently available it should be 
returned 
+// (which this test asserts, but is currently failing). If TaskFactory 
is used the
+// intended semantics are clear -- you create a new task on each 
access, and can interrupt it
+// and discard it if needed. For a Task it's less clear: probably the 
semantics are that the
+// first returned value is what the value is forevermore. Probably it 
should not be interrupted
+// on a non-blocking / short-wait access, or possibly it should simply 
be re-run if a previous
+// execution was interrupted (but take care if we have a simultaneous 
non-blocking and blocking
+// access, if the first one interrupts the second one should still get 
a value).
+// I tend to think ideally we should switch to using TaskFactory in 
DependentConfiguration.
+class ConfigNonBlockingFixture {
+final Semaphore latch = new Semaphore(0);
+final String expectedVal = "myval";
+Object blockingVal;
+
+protected ConfigNonBlockingFixture usingTask() {
+blockingVal = taskFactory().newTask();
+return this;
+}
+
+protected ConfigNonBlockingFixture usingTaskFactory() {
+blockingVal = taskFactory();
+return this;
+}
+
+protected ConfigNonBlockingFixture usingDeferredSupplier() {
+blockingVal = deferredSupplier();
+return this;
+}
+
+protected ConfigNonBlockingFixture usingImmediateSupplier() {
+blockingVal = new 
InterruptingImmediateSupplier(deferredSupplier());
+return this;
+}
+
+private TaskFactory taskFactory() {
+return Tasks.builder().body(
 new Callable() {
 @Override
 public String call() throws Exception {
-latch.await();
+if (!latch.tryAcquire()) latch.acquire();
+latch.release();
 return "myval";
 }})
-.build();
-runGetConfigNonBlocking(latch, task, "myval");
-}
-
-@Test
-public void testGetDeferredSupplierNonBlocking() throws Exception {
-final CountDownLatch latch = new CountDownLatch(1);
-DeferredSupplier task = new DeferredSupplier() {
-@Override public String get() {
-try {
-latch.await();
-} catch (InterruptedException e) {
-throw Exceptions.propagate(e);
-}
-return "myval";
-}
-};
-runGetConfigNonBlocking(latch, task, "myval");
-}
-
-@SuppressWarnings({"unchecked", "rawtypes"})
-protected void runGetConfigNonBlocking(CountDownLatch latch, Object 
blockingVal, String expectedVal) throws Exception {
-TestEntity entity = (TestEntity) 
mgmt.getEntityManager().createEntity(EntitySpec.create(TestEntity.class)
-.configure(TestEntity.CONF_MAP_OBJ_THING, 
ImmutableMap.of("mysub", blockingVal))
-.configure((ConfigKey)TestEntity.CONF_NAME, 

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101849614
  
--- Diff: 
core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java ---
@@ -244,59 +252,152 @@ public void 
testGetConfigMapWithSubValueAsStringNotCoerced() throws Exception {
 // of the previous "test.confMapThing.obj".
 //
 // Presumably an earlier call to task.get() timed out, causing it to 
cancel the task?
+// Alex: yes, a task.cancel is performed for maps in
+// 
AbstractEntity$BasicConfigurationSupport(AbstractConfigurationSupportInternal).getNonBlockingResolvingStructuredKey(ConfigKey)

+ 
+//
 // I (Aled) question whether we want to support passing a task (rather 
than a 
 // DeferredSupplier or TaskFactory, for example). Our 
EntitySpec.configure is overloaded
 // to take a Task, but that feels wrong!?
-@Test(groups="Broken")
-public void testGetTaskNonBlocking() throws Exception {
-final CountDownLatch latch = new CountDownLatch(1);
-Task task = Tasks.builder().body(
+//
+// If starting clean I (Alex) would agree, we should use TaskFactory. 
However the
+// DependentConfiguration methods -- including the ubiquitous 
AttributeWhenReady --
+// return Task instances so they should survive a getNonBlocking or 
get with a short timeout 
+// access, and if a value is subsequently available it should be 
returned 
+// (which this test asserts, but is currently failing). If TaskFactory 
is used the
+// intended semantics are clear -- you create a new task on each 
access, and can interrupt it
+// and discard it if needed. For a Task it's less clear: probably the 
semantics are that the
+// first returned value is what the value is forevermore. Probably it 
should not be interrupted
+// on a non-blocking / short-wait access, or possibly it should simply 
be re-run if a previous
+// execution was interrupted (but take care if we have a simultaneous 
non-blocking and blocking
+// access, if the first one interrupts the second one should still get 
a value).
+// I tend to think ideally we should switch to using TaskFactory in 
DependentConfiguration.
+class ConfigNonBlockingFixture {
+final Semaphore latch = new Semaphore(0);
+final String expectedVal = "myval";
+Object blockingVal;
+
+protected ConfigNonBlockingFixture usingTask() {
+blockingVal = taskFactory().newTask();
+return this;
+}
+
+protected ConfigNonBlockingFixture usingTaskFactory() {
+blockingVal = taskFactory();
+return this;
+}
+
+protected ConfigNonBlockingFixture usingDeferredSupplier() {
+blockingVal = deferredSupplier();
+return this;
+}
+
+protected ConfigNonBlockingFixture usingImmediateSupplier() {
+blockingVal = new 
InterruptingImmediateSupplier(deferredSupplier());
+return this;
+}
+
+private TaskFactory taskFactory() {
+return Tasks.builder().body(
 new Callable() {
 @Override
 public String call() throws Exception {
-latch.await();
+if (!latch.tryAcquire()) latch.acquire();
+latch.release();
 return "myval";
 }})
-.build();
-runGetConfigNonBlocking(latch, task, "myval");
-}
-
-@Test
-public void testGetDeferredSupplierNonBlocking() throws Exception {
-final CountDownLatch latch = new CountDownLatch(1);
-DeferredSupplier task = new DeferredSupplier() {
-@Override public String get() {
-try {
-latch.await();
-} catch (InterruptedException e) {
-throw Exceptions.propagate(e);
-}
-return "myval";
-}
-};
-runGetConfigNonBlocking(latch, task, "myval");
-}
-
-@SuppressWarnings({"unchecked", "rawtypes"})
-protected void runGetConfigNonBlocking(CountDownLatch latch, Object 
blockingVal, String expectedVal) throws Exception {
-TestEntity entity = (TestEntity) 
mgmt.getEntityManager().createEntity(EntitySpec.create(TestEntity.class)
-.configure(TestEntity.CONF_MAP_OBJ_THING, 
ImmutableMap.of("mysub", blockingVal))
-.configure((ConfigKey)TestEntity.CONF_NAME, 

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101782727
  
--- Diff: 
api/src/main/java/org/apache/brooklyn/api/mgmt/ExecutionContext.java ---
@@ -64,4 +65,6 @@
 
 boolean isShutdown();
 
+ Maybe getImmediately(Object callableOrSupplier);
--- End diff --

Worth adding javadoc here - e.g. similar to what you've added in the impl 
`BasicExecutionContext`. Worth saying when it will return Maybe.absent (e.g. if 
the task execution requires blocking for other work, and can't complete in a 
timely fashion).



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101789828
  
--- Diff: 
camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/DslComponent.java
 ---
@@ -539,8 +550,9 @@ protected String resolveKeyName(boolean immediately) {
 @Override
 public final Maybe getImmediately() {
 Maybe targetEntityMaybe = component.getImmediately();
-if (targetEntityMaybe.isAbsent()) return Maybe.absent("Target 
entity not available");
+if (targetEntityMaybe.isAbsent()) return 
Maybe.cast(targetEntityMaybe);
 EntityInternal targetEntity = (EntityInternal) 
targetEntityMaybe.get();
+checkAndTagForRecursiveReference(targetEntity);
--- End diff --

If I'm reading this right... it adds the tag to the current task, but then 
the tag is not removed at the end of this method - should it be?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #480: Config self reference fix

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/480#discussion_r101862310
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java ---
@@ -352,33 +357,51 @@ public T get() {
 
 //if the expected type is a closure or map and that's what we 
have, we're done (or if it's null);
 //but not allowed to return a future or DeferredSupplier as the 
resolved value
-if (v==null || (!forceDeep && type.isInstance(v) && 
!Future.class.isInstance(v) && !DeferredSupplier.class.isInstance(v)))
+if (v==null || (!forceDeep && type.isInstance(v) && 
!Future.class.isInstance(v) && !DeferredSupplier.class.isInstance(v) && 
!TaskFactory.class.isInstance(v)))
 return Maybe.of((T) v);
 
 try {
-if (immediately && v instanceof ImmediateSupplier) {
-final ImmediateSupplier supplier = 
(ImmediateSupplier) v;
+if (isEvaluatingImmediately() && v instanceof 
ImmediateSupplier) {
+final ImmediateSupplier supplier = 
(ImmediateSupplier) v;
 try {
-Maybe result = supplier.getImmediately();
+Maybe result = exec.getImmediately(supplier);
 
 // Recurse: need to ensure returned value is cast, etc
 return (result.isPresent())
 ? recursive
 ? new ValueResolver(result.get(), type, 
this).getMaybe()
 : result
-: Maybe.absent();
+: result;
 } catch (ImmediateSupplier.ImmediateUnsupportedException 
e) {
 log.debug("Unable to resolve-immediately for 
"+description+" ("+v+"); falling back to executing with timeout", e);
 }
 }
 
+// TODO if evaluating immediately should use a new 
ExecutionContext.submitImmediate(...)
+// and sets a timeout but which wraps a task but does not 
spawn a new thread
+
+if ((v instanceof TaskFactory) && !(v instanceof 
DeferredSupplier)) {
--- End diff --

I'd add something like the following to `ValueResolverTest`:
```
public void testTaskFactoryGet() {
TaskFactory taskFactory = new 
TaskFactory() {
@Override public TaskAdaptable newTask() {
return new BasicTask<>(Callables.returning("myval"));
}
};
String result = 
Tasks.resolving(taskFactory).as(String.class).context(app).get();
assertEquals(result, "myval");
}

public void testTaskFactoryGetImmediately() {
TaskFactory taskFactory = new 
TaskFactory() {
@Override public TaskAdaptable newTask() {
return new BasicTask<>(Callables.returning("myval"));
}
};
String result = 
Tasks.resolving(taskFactory).as(String.class).context(app).immediately(true).get();
assertEquals(result, "myval");
}

public void testTaskFactoryGetImmediatelyDoesNotBlock() {
final AtomicBoolean executing = new AtomicBoolean();
TaskFactory taskFactory = new 
TaskFactory() {
@Override public TaskAdaptable newTask() {
return new BasicTask<>(new Callable() {
public String call() {
executing.set(true);
try {
Time.sleep(Duration.ONE_MINUTE);
return "myval";
} finally {
executing.set(false);
}
}});
}
};
Maybe result = 
Tasks.resolving(taskFactory).as(String.class).context(app).immediately(true).getMaybe();
assertTrue(result.isAbsent(), "result="+result);
Asserts.succeedsEventually(new Runnable() {
public void run() {
assertFalse(executing.get());
}
});
}
```
However, the last assertion fails - using `immediately()` with 
`TaskFactory` is dangerous because the task is left running. It is particularly 
dangerous because `immediately()` is most often used when it's being called 
regularly (e.g. the rest api wants to list all the config values).

Whose responsibility is it to cancel the task? (It feels like the task 
instance is buried inside the `ValueResolver`, so it