-
Notifications
You must be signed in to change notification settings - Fork 52
Config self reference fix #480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
d7c6036
4121554
5324f82
a679682
faeeb1b
f84d886
49f0e22
72eff85
3f3e3d6
7476d3b
b073349
0aa29ef
cd3d486
99ccc0f
2e6f11f
4602114
a3f42d3
e6fd10c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,7 @@ | |
| import org.apache.brooklyn.util.core.task.DeferredSupplier; | ||
| import org.apache.brooklyn.util.core.task.ImmediateSupplier; | ||
| import org.apache.brooklyn.util.core.task.TaskBuilder; | ||
| import org.apache.brooklyn.util.core.task.TaskTags; | ||
| import org.apache.brooklyn.util.core.task.Tasks; | ||
| import org.apache.brooklyn.util.exceptions.Exceptions; | ||
| import org.apache.brooklyn.util.groovy.GroovyJavaMethods; | ||
|
|
@@ -206,6 +207,15 @@ public Maybe<Entity> getImmediately() { | |
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Entity get() { | ||
| try { | ||
| return call(); | ||
| } catch (Exception e) { | ||
| throw Exceptions.propagate(e); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Entity call() throws Exception { | ||
| return callImpl(false).get(); | ||
|
|
@@ -219,7 +229,7 @@ protected Maybe<Entity> getEntity(boolean immediate) { | |
| return Maybe.of(scopeComponent.get()); | ||
| } | ||
| } else { | ||
| return Maybe.<Entity>of(entity()); | ||
| return Maybe.<Entity>ofDisallowingNull(entity()).or(Maybe.<Entity>absent("Context entity not available when trying to evaluate Brooklyn DSL")); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -311,10 +321,11 @@ protected Maybe<Entity> callImpl(boolean immediate) throws Exception { | |
| return Maybe.of(result.get()); | ||
| } | ||
|
|
||
| // TODO may want to block and repeat on new entities joining? | ||
| throw new NoSuchElementException("No entity matching id " + desiredComponentId+ | ||
| // could be nice if DSL has an extra .block() method to allow it to wait for a matching entity. | ||
| // previously we threw if nothing existed; now we return an absent with a detailed error | ||
| return Maybe.absent(new NoSuchElementException("No entity matching id " + desiredComponentId+ | ||
| (scope==Scope.GLOBAL ? "" : ", in scope "+scope+" wrt "+entity+ | ||
| (scopeComponent!=null ? " ("+scopeComponent+" from "+entity()+")" : ""))); | ||
| (scopeComponent!=null ? " ("+scopeComponent+" from "+entity()+")" : "")))); | ||
| } | ||
|
|
||
| private ExecutionContext getExecutionContext() { | ||
|
|
@@ -539,8 +550,9 @@ protected String resolveKeyName(boolean immediately) { | |
| @Override | ||
| public final Maybe<Object> getImmediately() { | ||
| Maybe<Entity> targetEntityMaybe = component.getImmediately(); | ||
| if (targetEntityMaybe.isAbsent()) return Maybe.absent("Target entity not available"); | ||
| if (targetEntityMaybe.isAbsent()) return Maybe.<Object>cast(targetEntityMaybe); | ||
| EntityInternal targetEntity = (EntityInternal) targetEntityMaybe.get(); | ||
| checkAndTagForRecursiveReference(targetEntity); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). UPDATE: we now always use a dedicated tag
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually i've done a better strategy -- @aledsage appreciate any thoughts on this:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did it, but without an official task type -- easy enough to get the routines to share code and trigger a dedicated (non-thread) task |
||
|
|
||
| String keyNameS = resolveKeyName(true); | ||
| ConfigKey<?> key = targetEntity.getEntityType().getConfigKey(keyNameS); | ||
|
|
@@ -558,11 +570,26 @@ public Task<Object> newTask() { | |
| @Override | ||
| public Object call() throws Exception { | ||
| Entity targetEntity = component.get(); | ||
| checkAndTagForRecursiveReference(targetEntity); | ||
|
|
||
| String keyNameS = resolveKeyName(true); | ||
| ConfigKey<?> key = targetEntity.getEntityType().getConfigKey(keyNameS); | ||
| return targetEntity.getConfig(key != null ? key : ConfigKeys.newConfigKey(Object.class, keyNameS)); | ||
| }}) | ||
| .build(); | ||
| } | ||
| }).build(); | ||
| } | ||
|
|
||
| private void checkAndTagForRecursiveReference(Entity targetEntity) { | ||
| String tag = "DSL:entity('"+targetEntity.getId()+"').config('"+keyName+"')"; | ||
| Task<?> ancestor = Tasks.current(); | ||
| while (ancestor!=null) { | ||
| if (TaskTags.hasTag(ancestor, tag)) { | ||
| throw new IllegalStateException("Recursive config reference "+tag); | ||
| } | ||
| ancestor = ancestor.getSubmittedByTask(); | ||
| } | ||
|
|
||
| Tasks.addTagDynamically(tag); | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,8 +28,14 @@ | |
|
|
||
| import org.apache.brooklyn.api.entity.Entity; | ||
| import org.apache.brooklyn.core.config.ConfigKeys; | ||
| import org.apache.brooklyn.core.entity.Entities; | ||
| import org.apache.brooklyn.core.mgmt.internal.LocalManagementContext; | ||
| import org.apache.brooklyn.core.sensor.Sensors; | ||
| import org.apache.brooklyn.core.test.entity.TestEntity; | ||
| import org.apache.brooklyn.test.Asserts; | ||
| import org.apache.brooklyn.util.exceptions.RuntimeInterruptedException; | ||
| import org.apache.brooklyn.util.time.Duration; | ||
| import org.apache.brooklyn.util.time.Time; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.testng.annotations.AfterMethod; | ||
|
|
@@ -44,7 +50,6 @@ | |
|
|
||
| public class ConfigYamlTest extends AbstractYamlTest { | ||
|
|
||
| @SuppressWarnings("unused") | ||
| private static final Logger LOG = LoggerFactory.getLogger(ConfigYamlTest.class); | ||
|
|
||
| private ExecutorService executor; | ||
|
|
@@ -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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test passes for me - when/why does it fail, or does this comment need deleted?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. stale comment, good catch |
||
| @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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Delete commented out code, or add additional comment to say when one would uncomment it. |
||
| } catch (RuntimeInterruptedException e) { | ||
| // expected on normal execution | ||
| Thread.interrupted(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you calling this to clear the interrupted status? Why? Do you get an ugly exception or something if we don't?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. exactly, comment added |
||
| } | ||
| } | ||
| }); | ||
| t.start(); | ||
| try { | ||
| String c; | ||
| if (immediate) { | ||
| // this should throw rather than return "absent", because the error is definitive (absent means couldn't resolve in time) | ||
| c = entity.config().getNonBlocking(ConfigKeys.newStringConfigKey("infinite_loop")).or("FAILED"); | ||
| } else { | ||
| c = entity.config().get(ConfigKeys.newStringConfigKey("infinite_loop")); | ||
| } | ||
| Asserts.shouldHaveFailedPreviously("Expected recursive error, instead got: "+c); | ||
| } catch (Exception e) { | ||
| Asserts.expectedFailureContainsIgnoreCase(e, "infinite_loop", "recursive"); | ||
| } finally { | ||
| if (!Entities.isManaged(app)) { | ||
| t.interrupt(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testConfigAtTopLevel() throws Exception { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -146,7 +146,6 @@ protected <T> Maybe<T> getNonBlockingResolvingSimple(ConfigKey<T> key) { | |
| .immediately(true) | ||
| .deep(true) | ||
| .context(getContext()) | ||
| .swallowExceptions() | ||
| .get(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, the immediate stuff is quite new so i'd prefer to try this. if it's a problem we should perhaps look at wrapping in a |
||
| return (resolved != marker) | ||
| ? TypeCoercions.tryCoerce(resolved, key.getTypeToken()) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -591,7 +591,7 @@ public boolean cancel(TaskCancellationMode mode) { | |
| if (!task.isCancelled()) result |= ((TaskInternal<T>)task).cancel(mode); | ||
| result |= delegate().cancel(mode.isAllowedToInterruptTask()); | ||
|
|
||
| if (mode.isAllowedToInterruptAllSubmittedTasks() || mode.isAllowedToInterruptDependentSubmittedTasks()) { | ||
| if (mode.isAllowedToInterruptDependentSubmittedTasks()) { | ||
| int subtasksFound=0; | ||
| int subtasksReallyCancelled=0; | ||
|
|
||
|
|
@@ -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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| log.warn("High number of active tasks: task #"+count+" is "+task); | ||
| } | ||
|
|
||
| //set thread _before_ start time, so we won't get a null thread when there is a start-time | ||
| if (log.isTraceEnabled()) log.trace(""+this+" beforeStart, task: "+task + " running on thread " + Thread.currentThread().getName()); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea. we should maybe move
ImmediateSupplierto the utils package, then we could reference its javadoc? we might also change the return type to beReferenceWithError<Maybe<T>>so the "can't immediately tell if there's a value" problem state can be detected without throwing. have marked@Betafor now.