From 0d77dbc17235c9bfb833467fcb158ee3a69843eb Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Mon, 20 Feb 2017 14:39:06 +0000 Subject: [PATCH 1/6] use new maybe assertions --- .../org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java index 1686f55d9f..e1f1708979 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java @@ -372,10 +372,10 @@ public void testDeferredSupplierToAttributeWhenReadyInSpecialTypes() throws Exce final TestEntity entity = (TestEntity) Iterables.getOnlyElement(app.getChildren()); // Attribute not yet set; non-blocking will return promptly without the value - assertTrue(entity.config().getNonBlocking(TestEntity.CONF_NAME).isAbsent()); - assertTrue(entity.config().getNonBlocking(TestEntity.CONF_MAP_THING).isAbsent()); - assertTrue(entity.config().getNonBlocking(TestEntity.CONF_LIST_THING).isAbsent()); - assertTrue(entity.config().getNonBlocking(TestEntity.CONF_SET_THING).isAbsent()); + Asserts.assertNotPresent(entity.config().getNonBlocking(TestEntity.CONF_NAME)); + Asserts.assertNotPresent(entity.config().getNonBlocking(TestEntity.CONF_MAP_THING)); + Asserts.assertNotPresent(entity.config().getNonBlocking(TestEntity.CONF_LIST_THING)); + Asserts.assertNotPresent(entity.config().getNonBlocking(TestEntity.CONF_SET_THING)); // Now set the attribute: get will return once that has happened executor.submit(new Callable() { From 3821e02c504382cb5a5a2411ddecda5b58b73136 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Mon, 20 Feb 2017 15:48:46 +0000 Subject: [PATCH 2/6] Switch more of the "immediate" / "non-blocking" calls to be truly non-blocking. Also updates tests. Mainly uses ImmediateSupplier and InterruptingImmediateSupplier for true non-blocking evaluation, with some other tricks used in other places. Some non-reliable calls may still fail, but most have been repaired, and the rest should be. (If the old semantics are _really_ needed you can resolve with a short wait.) Re-enables many of the tests disabled for https://issues.apache.org/jira/browse/BROOKLYN-272 --- .../spi/dsl/DslDeferredFunctionCall.java | 3 +- .../spi/dsl/methods/BrooklynDslCommon.java | 15 +--- .../spi/dsl/methods/DslComponent.java | 10 +-- .../camp/brooklyn/ConfigYamlTest.java | 32 +------- .../internal/AbstractConfigMapImpl.java | 3 + .../AbstractConfigurationSupportInternal.java | 57 +++++++------- .../core/objs/BrooklynObjectInternal.java | 13 +++- .../core/sensor/DependentConfiguration.java | 15 ++-- .../util/core/task/ImmediateSupplier.java | 43 +++++++++- .../task/InterruptingImmediateSupplier.java | 7 +- .../util/core/task/ValueResolver.java | 78 +++++++------------ .../core/entity/EntityConfigTest.java | 18 ++--- .../util/core/task/ValueResolverTest.java | 34 +++++--- .../org/apache/brooklyn/util/guava/Maybe.java | 9 +++ 14 files changed, 182 insertions(+), 155 deletions(-) diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslDeferredFunctionCall.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslDeferredFunctionCall.java index b8387b3dbd..1c1cef520c 100644 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslDeferredFunctionCall.java +++ b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslDeferredFunctionCall.java @@ -27,6 +27,7 @@ import org.apache.brooklyn.camp.brooklyn.spi.dsl.methods.BrooklynDslCommon; import org.apache.brooklyn.camp.brooklyn.spi.dsl.methods.DslToStringHelpers; import org.apache.brooklyn.core.mgmt.BrooklynTaskTags; +import org.apache.brooklyn.util.core.task.ImmediateSupplier; import org.apache.brooklyn.util.core.task.Tasks; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.guava.Maybe; @@ -94,7 +95,7 @@ protected Maybe invokeOnDeferred(Object obj, boolean immediate) { return invokeOn(instance); } else { if (immediate) { - return Maybe.absent("Could not evaluate immediately: " + obj); + return Maybe.absent(new ImmediateSupplier.ImmediateValueNotAvailableException("Could not evaluate immediately: " + obj)); } else { return Maybe.absent(Maybe.getException(resolvedMaybe)); } diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/BrooklynDslCommon.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/BrooklynDslCommon.java index 2836895d16..e0ba90fa0f 100644 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/BrooklynDslCommon.java +++ b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/BrooklynDslCommon.java @@ -589,18 +589,11 @@ public Maybe getImmediately() { final Class clazz = getOrLoadType(); final ExecutionContext executionContext = entity().getExecutionContext(); - // Marker exception that one of our component-parts cannot yet be resolved - - // throwing and catching this allows us to abort fast. - // A bit messy to use exceptions in normal control flow, but this allows the Maps util methods to be used. - @SuppressWarnings("serial") - class UnavailableException extends RuntimeException { - } - final Function resolver = new Function() { @Override public Object apply(Object value) { Maybe result = Tasks.resolving(value, Object.class).context(executionContext).deep(true).immediately(true).getMaybe(); if (result.isAbsent()) { - throw new UnavailableException(); + throw new ImmediateValueNotAvailableException(); } else { return result.get(); } @@ -620,8 +613,8 @@ class UnavailableException extends RuntimeException { result = create(clazz, factoryMethodName, resolvedFactoryMethodArgs, resolvedFields, resolvedConfig); } return Maybe.of(result); - } catch (UnavailableException e) { - return Maybe.absent(); + } catch (ImmediateValueNotAvailableException e) { + return ImmediateValueNotAvailableException.newAbsentWithExceptionSupplier(); } } @@ -873,7 +866,7 @@ public EntitySupplier(String entityId) { public Maybe getImmediately() { EntityInternal entity = entity(); if (entity == null) { - return Maybe.absent(); + return Maybe.absent("No entity available"); } Entity targetEntity = entity.getManagementContext().getEntityManager().getEntity(entityId); return Maybe.of(targetEntity); diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/DslComponent.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/DslComponent.java index 1cce90e678..0d2213a8f2 100644 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/DslComponent.java +++ b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/DslComponent.java @@ -288,7 +288,7 @@ protected Maybe callImpl(boolean immediate) throws Exception { if (immediate) { if (maybeComponentId.isAbsent()) { - return Maybe.absent(Maybe.getException(maybeComponentId)); + return ImmediateValueNotAvailableException.newAbsentWrapping("Cannot find component ID", maybeComponentId); } } @@ -418,7 +418,7 @@ public EntityId(DslComponent component) { @Override public Maybe getImmediately() { Maybe targetEntityMaybe = component.getImmediately(); - if (targetEntityMaybe.isAbsent()) return Maybe.absent("Target entity not available"); + if (targetEntityMaybe.isAbsent()) return ImmediateValueNotAvailableException.newAbsentWrapping("Target entity is not available: "+component, targetEntityMaybe); Entity targetEntity = targetEntityMaybe.get(); return Maybe.of(targetEntity.getId()); @@ -477,7 +477,7 @@ protected String resolveSensorName(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 ImmediateValueNotAvailableException.newAbsentWrapping("Target entity not available: "+component, targetEntityMaybe); Entity targetEntity = targetEntityMaybe.get(); String sensorNameS = resolveSensorName(true); @@ -486,7 +486,7 @@ public final Maybe getImmediately() { targetSensor = Sensors.newSensor(Object.class, sensorNameS); } Object result = targetEntity.sensors().get(targetSensor); - return GroovyJavaMethods.truth(result) ? Maybe.of(result) : Maybe.absent(); + return GroovyJavaMethods.truth(result) ? Maybe.of(result) : ImmediateValueNotAvailableException.newAbsentWithExceptionSupplier(); } @SuppressWarnings("unchecked") @@ -660,7 +660,7 @@ protected Maybe> getImmediately(Object si, boolean resolved) { return Maybe.>of((Sensor)si); } else if (si instanceof String) { Maybe targetEntityMaybe = component.getImmediately(); - if (targetEntityMaybe.isAbsent()) return Maybe.absent("Target entity not available"); + if (targetEntityMaybe.isAbsent()) return ImmediateValueNotAvailableException.newAbsentWrapping("Target entity is not available: "+component, targetEntityMaybe); Entity targetEntity = targetEntityMaybe.get(); Sensor result = null; diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java index e1f1708979..53a6be84ce 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java @@ -307,16 +307,7 @@ public void testDeferredSupplierToConfig() throws Exception { assertEquals(entity.config().get(TestEntity.CONF_SET_PLAIN), ImmutableSet.of("myOther")); } - /** - * TODO The {@code entity.config().getNonBlocking()} can return absent. When it's called with - * a deferred supplier value, it will kick off a task and then wait just a few millis for that - * task to execute deferredSupplier.get(). If it times out, then it returns Maybe.absent. - * However, on apache jenkins the machine is often slow so the task doesn't complete in the - * given number of millis (even though deferredSupplier.get() doesn't need to block for anything). - * Same for {@link #testDeferredSupplierToAttributeWhenReadyInSpecialTypes()}. - * See https://issues.apache.org/jira/browse/BROOKLYN-272. - */ - @Test(groups="Broken") + @Test public void testDeferredSupplierToAttributeWhenReady() throws Exception { String yaml = Joiner.on("\n").join( "services:", @@ -344,17 +335,9 @@ public Object call() { /** * This tests config keys of type {@link org.apache.brooklyn.core.config.MapConfigKey}, etc. - * For plain maps, see {@link #testDeferredSupplierToAttributeWhenReadyInPlainCollections()}. - * - * TODO The {@code entity.config().getNonBlocking()} can return absent. When it's called with - * a deferred supplier value, it will kick off a task and then wait just a few millis for that - * task to execute deferredSupplier.get(). If it times out, then it returns Maybe.absent. - * However, on apache jenkins the machine is often slow so the task doesn't complete in the - * given number of millis (even though deferredSupplier.get() doesn't need to block for anything). - * Same for {@link #testDeferredSupplierToAttributeWhenReady()}. - * See https://issues.apache.org/jira/browse/BROOKLYN-272. + * For plain maps, see {@link #testDeferredSupplierToAttributeWhenReadyInPlainCollections()} */ - @Test(groups="Broken") + @Test public void testDeferredSupplierToAttributeWhenReadyInSpecialTypes() throws Exception { String yaml = Joiner.on("\n").join( "services:", @@ -399,15 +382,8 @@ public Object call() { * This tests config keys of type {@link java.util.Map}, etc. * For special types (e.g. {@link org.apache.brooklyn.core.config.MapConfigKey}), see * {@link #testDeferredSupplierToAttributeWhenReadyInPlainCollections()}. - * - * TODO test doesn't work because getNonBlocking returns even when no value. - * For example, we get back: Present[value={mykey=attributeWhenReady("myOtherSensor")}]. - * However, the `config().get()` does behave as desired. - * - * Including the "WIP" group because this test would presumably have never worked! - * Added to demonstrate the short-coming. */ - @Test(groups={"Broken", "WIP"}) + @Test public void testDeferredSupplierToAttributeWhenReadyInPlainCollections() throws Exception { String yaml = Joiner.on("\n").join( "services:", diff --git a/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java b/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java index b736beb27e..091f8746fe 100644 --- a/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java +++ b/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java @@ -313,6 +313,9 @@ protected Maybe resolveRawValueFromContainer(TContainer container, Confi // wasteful to make a copy to look up; maybe try once opportunistically? ownCopy = MutableMap.copyOf(oc); } + // would be cleaner here to have an extractValueMaybe but semantics can get confusing whether absent + // means no value can be extracted (getRaw semantics) and immediate mode is on but blocking is needed (ImmediateSupplier semantics); + // simpler not to support maybe, in which case here null means the former, and the latter throws something (which the caller catches) Maybe result = Maybe.of((Object) ((ConfigKeySelfExtracting) key).extractValue(ownCopy, getExecutionContext(container)) ); postLocalEvaluate(key, bo, value, result); return result; diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java b/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java index 796ab137da..113daac3fa 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java @@ -22,8 +22,6 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.Callable; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.TimeoutException; import javax.annotation.Nullable; @@ -41,8 +39,9 @@ import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.core.config.ConfigBag; import org.apache.brooklyn.util.core.flags.TypeCoercions; +import org.apache.brooklyn.util.core.task.ImmediateSupplier.ImmediateUnsupportedException; +import org.apache.brooklyn.util.core.task.ImmediateSupplier.ImmediateValueNotAvailableException; import org.apache.brooklyn.util.core.task.Tasks; -import org.apache.brooklyn.util.core.task.ValueResolver; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.exceptions.RuntimeInterruptedException; import org.apache.brooklyn.util.guava.Maybe; @@ -53,6 +52,7 @@ public abstract class AbstractConfigurationSupportInternal implements BrooklynObjectInternal.ConfigurationSupportInternal { + @SuppressWarnings("unused") private static final Logger LOG = LoggerFactory.getLogger(AbstractConfigurationSupportInternal.class); @Override @@ -77,10 +77,16 @@ public Maybe getNonBlocking(HasConfigKey key) { @Override public Maybe getNonBlocking(final ConfigKey key) { - if (key instanceof StructuredConfigKey || key instanceof SubElementConfigKey) { - return getNonBlockingResolvingStructuredKey(key); - } else { - return getNonBlockingResolvingSimple(key); + try { + if (key instanceof StructuredConfigKey || key instanceof SubElementConfigKey) { + return getNonBlockingResolvingStructuredKey(key); + } else { + return getNonBlockingResolvingSimple(key); + } + } catch (ImmediateValueNotAvailableException e) { + return Maybe.absent(e); + } catch (ImmediateUnsupportedException e) { + return Maybe.absent(e); } } @@ -89,12 +95,6 @@ public Maybe getNonBlocking(final ConfigKey key) { * execute the custom logic, as is done by {@link #get(ConfigKey)}, but non-blocking! */ protected Maybe getNonBlockingResolvingStructuredKey(final ConfigKey key) { - // TODO This is a poor implementation. We risk timing out when it's just doing its - // normal work (e.g. because job's thread was starved), rather than when it's truly - // blocked. Really we'd need to dig into the implementation of get(key), so that the - // underlying work can be configured with a timeout, for when it finally calls - // ValueResolver. - Callable job = new Callable() { @Override public T call() { @@ -106,22 +106,15 @@ public T call() { } }; - Task t = getContext().submit(Tasks.builder().body(job) + Task t = Tasks.builder().body(job) .displayName("Resolving dependent value") .description("Resolving "+key.getName()) .tag(BrooklynTaskTags.TRANSIENT_TASK_TAG) - .build()); + .build(); try { - T result = t.get(ValueResolver.NON_BLOCKING_WAIT); - return Maybe.of(result); - } catch (TimeoutException e) { - t.cancel(true); - return Maybe.absent(); - } catch (ExecutionException e) { - LOG.debug("Problem resolving "+key.getName()+", returning ", e); - return Maybe.absent(); - } catch (InterruptedException e) { - throw Exceptions.propagate(e); + return getContext().getImmediately(t); + } catch (ImmediateUnsupportedException e) { + return Maybe.absent(); } } @@ -139,17 +132,19 @@ protected Maybe getNonBlockingResolvingSimple(ConfigKey key) { // or Absent if the config key was unset. Object unresolved = getRaw(key).or(key.getDefaultValue()); final Object marker = new Object(); - // Give tasks a short grace period to resolve. - Object resolved = Tasks.resolving(unresolved) + Maybe resolved = Tasks.resolving(unresolved) .as(Object.class) .defaultValue(marker) .immediately(true) .deep(true) .context(getContext()) - .get(); - return (resolved != marker) - ? TypeCoercions.tryCoerce(resolved, key.getTypeToken()) - : Maybe.absent(); + .getMaybe(); + if (resolved.isAbsent()) return Maybe.Absent.castAbsent(resolved); + if (resolved.get()==marker) { + // TODO changed Feb 2017, previously returned absent, in contrast to what the javadoc says + return Maybe.of((T)null); + } + return TypeCoercions.tryCoerce(resolved.get(), key.getTypeToken()); } @Override diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java b/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java index 86daf1f2d3..3fc2c99be2 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java @@ -28,6 +28,8 @@ import org.apache.brooklyn.config.ConfigKey.HasConfigKey; import org.apache.brooklyn.config.ConfigMap.ConfigMapWithInheritance; import org.apache.brooklyn.util.core.config.ConfigBag; +import org.apache.brooklyn.util.core.task.ImmediateSupplier; +import org.apache.brooklyn.util.core.task.ImmediateSupplier.ImmediateUnsupportedException; import org.apache.brooklyn.util.guava.Maybe; import com.google.common.annotations.Beta; @@ -113,13 +115,20 @@ public interface ConfigurationSupportInternal extends Configurable.Configuration /** * Attempts to coerce the value for this config key, if available, - * taking a default and {@link Maybe#absent absent} if the uncoerced - * cannot be resolved within a short timeframe. + * including returning a default if the config key is unset, + * returning a {@link Maybe#absent absent} if the uncoerced + * does not support immediate resolution. *

* Note: if no value for the key is available, not even as a default, * this returns a {@link Maybe#isPresent()} containing null * (following the semantics of {@link #get(ConfigKey)} * rather than {@link #getRaw(ConfigKey)}). + * Thus a {@link Maybe#absent()} definitively indicates that + * the absence is due to the request to evaluate immediately. + *

+ * This will include catching {@link ImmediateUnsupportedException} + * and returning it as an absence, thus making the semantics here slightly + * "safer" than that of {@link ImmediateSupplier#getImmediately()}. */ @Beta Maybe getNonBlocking(ConfigKey key); diff --git a/core/src/main/java/org/apache/brooklyn/core/sensor/DependentConfiguration.java b/core/src/main/java/org/apache/brooklyn/core/sensor/DependentConfiguration.java index ca79d73d0c..32a409ff02 100644 --- a/core/src/main/java/org/apache/brooklyn/core/sensor/DependentConfiguration.java +++ b/core/src/main/java/org/apache/brooklyn/core/sensor/DependentConfiguration.java @@ -68,6 +68,7 @@ import org.apache.brooklyn.util.groovy.GroovyJavaMethods; import org.apache.brooklyn.util.guava.Functionals; import org.apache.brooklyn.util.guava.Maybe; +import org.apache.brooklyn.util.guava.Maybe.Absent; import org.apache.brooklyn.util.net.Urls; import org.apache.brooklyn.util.text.StringFunctions; import org.apache.brooklyn.util.text.StringFunctions.RegexReplacer; @@ -499,7 +500,7 @@ public static Maybe formatStringImmediately(final String spec, final Obj List resolvedArgs = Lists.newArrayList(); for (Object arg : args) { Maybe argVal = resolveImmediately(arg); - if (argVal.isAbsent()) return Maybe.absent(); + if (argVal.isAbsent()) return Maybe.Absent.castAbsent(argVal); resolvedArgs.add(argVal.get()); } @@ -511,7 +512,7 @@ public static Maybe formatStringImmediately(final String spec, final Obj */ public static Maybe urlEncodeImmediately(Object arg) { Maybe resolvedArg = resolveImmediately(arg); - if (resolvedArg.isAbsent()) return Maybe.absent(); + if (resolvedArg.isAbsent()) return Absent.castAbsent(resolvedArg); if (resolvedArg.isNull()) return Maybe.of((String)null); String resolvedString = resolvedArg.get().toString(); @@ -564,15 +565,15 @@ protected static Maybe resolveImmediately(Object val) { public static Maybe regexReplacementImmediately(Object source, Object pattern, Object replacement) { Maybe resolvedSource = resolveImmediately(source); - if (resolvedSource.isAbsent()) return Maybe.absent(); + if (resolvedSource.isAbsent()) return Absent.castAbsent(resolvedSource); String resolvedSourceStr = String.valueOf(resolvedSource.get()); Maybe resolvedPattern = resolveImmediately(pattern); - if (resolvedPattern.isAbsent()) return Maybe.absent(); + if (resolvedPattern.isAbsent()) return Absent.castAbsent(resolvedPattern); String resolvedPatternStr = String.valueOf(resolvedPattern.get()); Maybe resolvedReplacement = resolveImmediately(replacement); - if (resolvedReplacement.isAbsent()) return Maybe.absent(); + if (resolvedReplacement.isAbsent()) return Absent.castAbsent(resolvedReplacement); String resolvedReplacementStr = String.valueOf(resolvedReplacement.get()); String result = new StringFunctions.RegexReplacer(resolvedPatternStr, resolvedReplacementStr).apply(resolvedSourceStr); @@ -591,11 +592,11 @@ public static Task regexReplacement(Object source, Object pattern, Objec public static Maybe> regexReplacementImmediately(Object pattern, Object replacement) { Maybe resolvedPattern = resolveImmediately(pattern); - if (resolvedPattern.isAbsent()) return Maybe.absent(); + if (resolvedPattern.isAbsent()) return Absent.castAbsent(resolvedPattern); String resolvedPatternStr = String.valueOf(resolvedPattern.get()); Maybe resolvedReplacement = resolveImmediately(replacement); - if (resolvedReplacement.isAbsent()) return Maybe.absent(); + if (resolvedReplacement.isAbsent()) return Absent.castAbsent(resolvedReplacement); String resolvedReplacementStr = String.valueOf(resolvedReplacement.get()); RegexReplacer result = new StringFunctions.RegexReplacer(resolvedPatternStr, resolvedReplacementStr); diff --git a/core/src/main/java/org/apache/brooklyn/util/core/task/ImmediateSupplier.java b/core/src/main/java/org/apache/brooklyn/util/core/task/ImmediateSupplier.java index 5ec8d68d1b..522c3610dd 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/task/ImmediateSupplier.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/task/ImmediateSupplier.java @@ -45,9 +45,48 @@ public ImmediateUnsupportedException(String message, Throwable cause) { } /** - * Gets the value promptly, or returns {@link Maybe#absent()} if the value is not yet available. + * Indicates that an attempt was made to forcibly get a requested immediate value + * where blocking is required. See {@link ImmediateSupplier#getImmediately()}, which if + * it returns an absent result, that absent will throw this. + *

+ * This is useful for passing between contexts that support immediate evaluation, + * through contexts that do not, to outer contexts which do, as the outer context + * will be able to use this exception to return a {@link Maybe#absent()} rather than throwing. + */ + public static class ImmediateValueNotAvailableException extends RuntimeException { + private static final long serialVersionUID = -5860437285154375232L; + + public ImmediateValueNotAvailableException() { } + public ImmediateValueNotAvailableException(String message) { + super(message); + } + public ImmediateValueNotAvailableException(String message, Throwable cause) { + super(message, cause); + } + public static Maybe newAbsentWithExceptionSupplier() { + return Maybe.Absent.changeExceptionSupplier(Maybe.absent(), ImmediateValueNotAvailableException.class); + } + public static Maybe newAbsentWrapping(String message, Maybe inner) { + return Maybe.absent(new ImmediateValueNotAvailableException(message, Maybe.getException(inner))); + } + } + + /** + * Gets the value promptly, or returns {@link Maybe#absent()} if the value requires blocking, + * or throws {@link ImmediateUnsupportedException} if it cannot be determined whether the value requires blocking or not. + *

+ * The {@link Maybe#absent()} returned here indicates that a value definitively is pending, just it is not yet available, + * and an attempt to {@link Maybe#get()} it should throw an {@link ImmediateValueNotAvailableException}; + * it can be created with {@link ImmediateValueNotAvailableException#newAbsentWithExceptionSupplier()} to + * avoid creating traces (or simply with Maybe.absent(new ImmediateValueNotAvailableException(...))). + * This is in contrast with this method throwing a {@link ImmediateUnsupportedException} which should be done + * if the presence of an eventual value cannot even be determined in a non-blocking way. + *

+ * Implementations of this method should typically catch the former exception if encountered and return a + * {@link Maybe#absent()} wrapping it, whereas {@link ImmediateUnsupportedException} instances should be propagated. * - * @throws ImmediateUnsupportedException if cannot determine whether a value is immediately available + * @throws ImmediateUnsupportedException as above, if cannot be determined whether a value is or might eventually be available */ Maybe getImmediately(); + } diff --git a/core/src/main/java/org/apache/brooklyn/util/core/task/InterruptingImmediateSupplier.java b/core/src/main/java/org/apache/brooklyn/util/core/task/InterruptingImmediateSupplier.java index 84b1bb4fee..0a34a18506 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/task/InterruptingImmediateSupplier.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/task/InterruptingImmediateSupplier.java @@ -58,9 +58,12 @@ public Maybe getImmediately() { if (!interrupted) Thread.currentThread().interrupt(); return Maybe.ofAllowingNull(get()); } catch (Throwable t) { + if (Exceptions.getFirstThrowableOfType(t, ImmediateValueNotAvailableException.class)!=null) { + return Maybe.absent(Exceptions.getFirstThrowableOfType(t, ImmediateValueNotAvailableException.class)); + } if (Exceptions.getFirstThrowableOfType(t, InterruptedException.class)!=null || Exceptions.getFirstThrowableOfType(t, RuntimeInterruptedException.class)!=null) { - return Maybe.absent(new UnsupportedOperationException("Immediate value not available", t)); + return Maybe.absent(new ImmediateValueNotAvailableException("Immediate value not available, required non-blocking execution", t)); } throw Exceptions.propagate(t); } finally { @@ -105,7 +108,7 @@ public T get() { } } - public static class InterruptingImmediateSupplierNotSupportedForObject extends UnsupportedOperationException { + public static class InterruptingImmediateSupplierNotSupportedForObject extends ImmediateUnsupportedException { private static final long serialVersionUID = 307517409005386500L; public InterruptingImmediateSupplierNotSupportedForObject(Object o) { diff --git a/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java b/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java index f8cb91b66c..4446d924d5 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java @@ -33,6 +33,8 @@ import org.apache.brooklyn.core.entity.EntityInternal; import org.apache.brooklyn.core.mgmt.BrooklynTaskTags; import org.apache.brooklyn.util.core.flags.TypeCoercions; +import org.apache.brooklyn.util.core.task.ImmediateSupplier.ImmediateUnsupportedException; +import org.apache.brooklyn.util.core.task.ImmediateSupplier.ImmediateValueNotAvailableException; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.guava.Maybe; import org.apache.brooklyn.util.javalang.JavaClassNames; @@ -275,8 +277,10 @@ public ValueResolver timeout(Duration timeout) { } /** - * Whether the value should be resolved immediately (and if not available immediately, - * return absent). + * Whether the value should be resolved immediately + * (following {@link ImmediateSupplier#getImmediately()} semantics with regards to when errors may be thrown, + * except some cases where {@link ImmediateUnsupportedException} is thrown may be re-run with a {@link #NON_BLOCKING_WAIT} + * after which the more definitive {@link ImmediateValueNotAvailableException} will be thrown/wrapped. */ @Beta public ValueResolver immediately(boolean val) { @@ -364,19 +368,15 @@ protected Maybe getMaybeInternal() { boolean allowImmediateExecution = false; boolean bailOutAfterImmediateExecution = false; - if (v instanceof ImmediateSupplier) { + if (v instanceof ImmediateSupplier || v instanceof DeferredSupplier) { allowImmediateExecution = true; } else { - if ((v instanceof TaskFactory) && !(v instanceof DeferredSupplier)) { + if (v instanceof TaskFactory) { 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 @@ -384,31 +384,32 @@ protected Maybe getMaybeInternal() { 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!) - // if: v instanceof ImmediateSupplier -- it probably is safe to change to bailOut = true ? - // if: v instanceof Task or other things -- it currently isn't safe, there are places where - // we expect to getImmediate on things which don't support it nicely, - // and we rely on the blocking-short-wait behaviour, e.g. QuorumChecks in ConfigYamlTest + // Feb 2017 - many things now we try to run immediate; notable exceptions are: + // * where the target isn't safe to run again (such as a Task which someone else might need), + // * or where he can't be run in an "interrupting" mode even if non-blocking (eg Future.get(), some other tasks) + // (the latter could be tried here, with bailOut false, but in most cases it will just throw so we still need to + // have the timings as in SHORT_WAIT etc as a fallack) + + Maybe result = null; try { - Maybe result = execImmediate(exec, v); - if (result!=null) return result; + result = exec.getImmediately(v); + + return (result.isPresent()) + ? recursive + ? new ValueResolver(result.get(), type, this).getMaybe() + : result + : result; + } catch (ImmediateSupplier.ImmediateUnsupportedException e) { if (bailOutAfterImmediateExecution) { throw new ImmediateSupplier.ImmediateUnsupportedException("Cannot get immediately: "+v); } - } catch (InterruptingImmediateSupplier.InterruptingImmediateSupplierNotSupportedForObject o) { - // ignore, continue below - log.debug("Unable to resolve-immediately for "+description+" ("+v+", wrong type "+v.getClass()+"); falling back to executing with timeout"); - } + // else proceed to below + } catch (ImmediateSupplier.ImmediateValueNotAvailableException e) { + // definitively not available + return ImmediateSupplier.ImmediateValueNotAvailableException.newAbsentWithExceptionSupplier(); + } } if (v instanceof Task) { @@ -472,8 +473,7 @@ public Object call() throws Exception { .description(description); if (isTransientTask) tb.tag(BrooklynTaskTags.TRANSIENT_TASK_TAG); - // Note that immediate resolution is handled by using ImmediateSupplier (using an instanceof check), - // so that it executes in the current thread instead of using task execution. + // immediate resolution is handled above Task vt = exec.submit(tb.build()); Maybe vm = Durations.get(vt, timer); vt.cancel(true); @@ -559,24 +559,6 @@ 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 - - // Recurse: need to ensure returned value is cast, etc - return (result.isPresent()) - ? recursive - ? new ValueResolver(result.get(), type, this).getMaybe() - : result - : result; - } - protected String getDescription() { return description!=null ? description : ""+value; } diff --git a/core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java b/core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java index 2f40fe9c30..90f8a12d3c 100644 --- a/core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java @@ -374,7 +374,7 @@ protected void runGetConfigNonBlockingInMap() throws Exception { assertAllOurConfigTasksCancelled(); } else { // TaskFactory tasks are cancelled, but others are not, - // things (ValueResolver?) are smart enough to know to leave it running + // things (ValueResolver.getMaybeInternal()) are smart enough to know to leave it running assertAllOurConfigTasksNotCancelled(); } @@ -411,30 +411,30 @@ private void assertAllTasksDone() { } } - @Test(groups="Integration") // because takes 1s+ + @Test(groups="Integration") // still takes 1s+ -- because we don't want to interrupt the task, we have to run it in BG public void testGetTaskNonBlockingKey() throws Exception { new ConfigNonBlockingFixture().usingTask().runGetConfigNonBlockingInKey(); } - @Test(groups="Integration") // because takes 1s+ + @Test public void testGetTaskNonBlockingMap() throws Exception { new ConfigNonBlockingFixture().usingTask().runGetConfigNonBlockingInMap(); } - @Test(groups="Integration") // because takes 1s+ + @Test public void testGetTaskFactoryNonBlockingKey() throws Exception { new ConfigNonBlockingFixture().usingTaskFactory().runGetConfigNonBlockingInKey(); } - @Test(groups="Integration") // because takes 1s+ + @Test public void testGetTaskFactoryNonBlockingMap() throws Exception { new ConfigNonBlockingFixture().usingTaskFactory().runGetConfigNonBlockingInMap(); } - @Test(groups="Integration") // because takes 1s+ + @Test public void testGetSupplierNonBlockingKey() throws Exception { new ConfigNonBlockingFixture().usingDeferredSupplier().runGetConfigNonBlockingInKey(); } - @Test(groups="Integration") // because takes 1s+ - public void testGetSuppierNonBlockingMap() throws Exception { + @Test + public void testGetSupplierNonBlockingMap() throws Exception { new ConfigNonBlockingFixture().usingDeferredSupplier().runGetConfigNonBlockingInMap(); } @@ -442,7 +442,7 @@ public void testGetSuppierNonBlockingMap() throws Exception { public void testGetImmediateSupplierNonBlockingKey() throws Exception { new ConfigNonBlockingFixture().usingImmediateSupplier().runGetConfigNonBlockingInKey(); } - @Test(groups="Integration") // because takes 1s+ + @Test public void testGetImmediateSupplierNonBlockingMap() throws Exception { new ConfigNonBlockingFixture().usingImmediateSupplier().runGetConfigNonBlockingInMap(); } diff --git a/core/src/test/java/org/apache/brooklyn/util/core/task/ValueResolverTest.java b/core/src/test/java/org/apache/brooklyn/util/core/task/ValueResolverTest.java index 550d4751af..c9c76fb1a8 100644 --- a/core/src/test/java/org/apache/brooklyn/util/core/task/ValueResolverTest.java +++ b/core/src/test/java/org/apache/brooklyn/util/core/task/ValueResolverTest.java @@ -32,6 +32,7 @@ import org.apache.brooklyn.core.mgmt.BrooklynTaskTags; import org.apache.brooklyn.core.test.BrooklynAppUnitTestSupport; import org.apache.brooklyn.test.Asserts; +import org.apache.brooklyn.util.core.task.ImmediateSupplier.ImmediateValueNotAvailableException; import org.apache.brooklyn.util.guava.Maybe; import org.apache.brooklyn.util.time.Duration; import org.apache.brooklyn.util.time.Time; @@ -169,11 +170,26 @@ private CallInfo myUniquelyNamedMethod() { assertImmediateFakeTaskFromMethod(callInfo, "myUniquelyNamedMethod"); } - public void testGetImmediatelyFallsBackToDeferredCallInTask() throws Exception { - final MyImmediateAndDeferredSupplier supplier = new MyImmediateAndDeferredSupplier(true); + public void testGetImmediatelyFallsBackToDeferredCallInTaskOnUnsupported() throws Exception { + final MyImmediateAndDeferredSupplier supplier = new MyImmediateAndDeferredSupplier(new ImmediateSupplier.ImmediateUnsupportedException("Simulate immediate unsupported")); CallInfo callInfo = Tasks.resolving(supplier).as(CallInfo.class).context(app).immediately(true).get(); - assertRealTaskNotFromMethod(callInfo, "testGetImmediatelyFallsBackToDeferredCallInTask"); + assertNotNull(callInfo.task); assertEquals(BrooklynTaskTags.getContextEntity(callInfo.task), app); + assertNotContainsCallingMethod(callInfo.stackTrace, "testGetImmediatelyFallsBackToDeferredCallInTask"); + } + + public void testGetImmediatelyDoesntFallBackToDeferredCallOnNotAvailable() throws Exception { + final MyImmediateAndDeferredSupplier supplier = new MyImmediateAndDeferredSupplier(new ImmediateSupplier.ImmediateValueNotAvailableException()); + Maybe callInfo = Tasks.resolving(supplier).as(CallInfo.class).context(app).immediately(true).getMaybe(); + Asserts.assertNotPresent(callInfo); + + try { + callInfo.get(); + Asserts.shouldHaveFailedPreviously("resolution should have failed now the ImmediateSupplier is not expected to fallback to other evaluation forms; instead got "+callInfo); + + } catch (Exception e) { + Asserts.expectedFailureOfType(e, ImmediateValueNotAvailableException.class); + } } public void testNonRecursiveBlockingFailsOnNonObjectType() throws Exception { @@ -271,20 +287,20 @@ public void run() { } private static class MyImmediateAndDeferredSupplier implements ImmediateSupplier, DeferredSupplier { - private final boolean failImmediately; + private final RuntimeException failImmediately; public MyImmediateAndDeferredSupplier() { - this(false); + this(null); } - public MyImmediateAndDeferredSupplier(boolean simulateImmediateUnsupported) { - this.failImmediately = simulateImmediateUnsupported; + public MyImmediateAndDeferredSupplier(RuntimeException failImmediately) { + this.failImmediately = failImmediately; } @Override public Maybe getImmediately() { - if (failImmediately) { - throw new ImmediateSupplier.ImmediateUnsupportedException("Simulate immediate unsupported"); + if (failImmediately!=null) { + throw failImmediately; } else { return Maybe.of(CallInfo.newInstance()); } diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java b/utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java index 2dfe6276f7..b115acb7d8 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java @@ -365,6 +365,15 @@ public static Maybe changeExceptionSupplier(Maybe original, Function)supplier)); } + /** Like {@link #cast(Maybe)} but allows any casting because that is valid for absents. + * Enforces that the argument really is absent. */ + @SuppressWarnings("unchecked") + public static Maybe castAbsent(Maybe absent) { + if (absent!=null && absent.isPresent()) { + throw new IllegalArgumentException("Expected an absent, but instead got: "+absent); + } + return (Maybe)absent; + } } public static class AbsentNull extends Absent { From ccc80485c551dd132763dbc37306b95ca20f240c Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Fri, 3 Mar 2017 16:20:17 +0000 Subject: [PATCH 3/6] remove commented out and no-longer needed ValueResolver.execImmediately method --- .../brooklyn/util/core/task/ValueResolver.java | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java b/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java index c5316fce3c..e98bb922a2 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java @@ -562,21 +562,6 @@ public Object call() throws Exception { } } -// /** tries to get immediately, then resolve recursively (including for casting) if {@link #recursive} is set -// * -// * @throws InterruptingImmediateSupplier.InterruptingImmediateSupplierNotSupportedForObject -// * ImmediateSupplier.ImmediateUnsupportedException -// * if underlying call to {@link ExecutionContext#getImmediately(Object)} does so */ -// protected Maybe execImmediate(ExecutionContext exec, Object immediateSupplierOrImmediateTask) { -// Maybe result = exec.getImmediately(immediateSupplierOrImmediateTask); -// -// return (result.isPresent()) -// ? recursive -// ? new ValueResolver(result.get(), type, this).getMaybe() -// : result -// : result; -// } - protected String getDescription() { return description!=null ? description : ""+value; } From ed81635e3d4f85bf5d9bf978447d50080140e7e8 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Tue, 7 Mar 2017 15:20:42 +0000 Subject: [PATCH 4/6] remove unnecessary marker default value --- .../core/objs/AbstractConfigurationSupportInternal.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java b/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java index 113daac3fa..95b631272d 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java @@ -131,19 +131,13 @@ protected Maybe getNonBlockingResolvingSimple(ConfigKey key) { // getRaw returns Maybe(val) if the key was explicitly set (where val can be null) // or Absent if the config key was unset. Object unresolved = getRaw(key).or(key.getDefaultValue()); - final Object marker = new Object(); Maybe resolved = Tasks.resolving(unresolved) .as(Object.class) - .defaultValue(marker) .immediately(true) .deep(true) .context(getContext()) .getMaybe(); if (resolved.isAbsent()) return Maybe.Absent.castAbsent(resolved); - if (resolved.get()==marker) { - // TODO changed Feb 2017, previously returned absent, in contrast to what the javadoc says - return Maybe.of((T)null); - } return TypeCoercions.tryCoerce(resolved.get(), key.getTypeToken()); } From d214ca99a865707db6d82b5f843c8a921657c886 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Tue, 7 Mar 2017 15:21:17 +0000 Subject: [PATCH 5/6] clear interrupt if evaluation a (nested) ImmediateSupplier --- .../util/core/task/BasicExecutionContext.java | 9 ++- .../core/entity/EntityConfigTest.java | 57 +++++++++++++------ 2 files changed, 47 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java b/core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java index f35a68af4c..6bccf10d80 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java @@ -140,7 +140,14 @@ public Maybe getImmediately(Object callableOrSupplier) { if (!(callableOrSupplier instanceof ImmediateSupplier)) { callableOrSupplier = InterruptingImmediateSupplier.of(callableOrSupplier); } - return ((ImmediateSupplier)callableOrSupplier).getImmediately(); + boolean wasAlreadyInterrupted = Thread.interrupted(); + try { + return ((ImmediateSupplier)callableOrSupplier).getImmediately(); + } finally { + if (wasAlreadyInterrupted) { + Thread.currentThread().interrupt(); + } + } } finally { BasicExecutionManager.getPerThreadCurrentTask().set(previousTask); diff --git a/core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java b/core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java index c551ba6415..b583b1ef10 100644 --- a/core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java @@ -303,8 +303,13 @@ protected ConfigNonBlockingFixture usingInterruptingImmediateSupplier() { return this; } - protected ConfigNonBlockingFixture usingImmediateSupplier() { - blockingVal = immediateSupplier(); + protected ConfigNonBlockingFixture usingImmediateSupplierNoSleep() { + blockingVal = immediateSupplier(false); + return this; + } + + protected ConfigNonBlockingFixture usingImmediateSupplierWithSleep() { + blockingVal = immediateSupplier(true); return this; } @@ -344,24 +349,18 @@ private DeferredSupplier deferredSupplier() { }; } - private DeferredSupplier immediateSupplier() { + private DeferredSupplier immediateSupplier(final boolean withSleep) { class DeferredImmediateSupplier implements DeferredSupplier, ImmediateSupplier { @Override public Maybe getImmediately() { try { - // Do a blocking operation (which would cause interrupt, if called in - // InterruptingImmediateSupplier). This is to simulate use-cases like - // use of `$brooklyn:external("vault", "aws.secretKey")`, which is not - // blocked by other Brooklyn tasks, but will do IO operations. - Thread.sleep(1); + sleepIfNeeded(); log.trace("acquiring"); if (latch.tryAcquire()) { latch.release(); return Maybe.of("myval"); } else { - // TODO After Alex's changes in PR #565, use: - // Maybe.absent(new ImmediateSupplier.ImmediateValueNotAvailableException())); - return Maybe.absent(); + return Maybe.absent(new ImmediateSupplier.ImmediateValueNotAvailableException()); } } catch (InterruptedException e) { log.trace("interrupted"); @@ -370,7 +369,7 @@ public Maybe getImmediately() { } @Override public String get() { try { - Thread.sleep(1); // See explanation in getImmediately() + sleepIfNeeded(); log.trace("acquiring"); if (!latch.tryAcquire()) latch.acquire(); latch.release(); @@ -381,6 +380,20 @@ public Maybe getImmediately() { } return "myval"; } + private void sleepIfNeeded() throws InterruptedException { + if (withSleep) { + try { + // Do a blocking operation (which would cause interrupt, if called in + // InterruptingImmediateSupplier). This is to simulate use-cases like + // use of `$brooklyn:external("vault", "aws.secretKey")`, which is not + // blocked by other Brooklyn tasks, but will do IO operations. + Thread.sleep(1); + } catch (InterruptedException e) { + log.debug("Sleep was interrupted during eval (expected in many cases)", e); + throw Exceptions.propagate(e); + } + } + } } return new DeferredImmediateSupplier(); } @@ -487,7 +500,7 @@ public void testGetSupplierNonBlockingMap() throws Exception { new ConfigNonBlockingFixture().usingDeferredSupplier().runGetConfigNonBlockingInMap(); } - @Test // fast + @Test public void testGetInterruptingImmediateSupplierNonBlockingKey() throws Exception { new ConfigNonBlockingFixture().usingInterruptingImmediateSupplier().runGetConfigNonBlockingInKey(); } @@ -496,13 +509,21 @@ public void testGetInterruptingImmediateSupplierNonBlockingMap() throws Exceptio new ConfigNonBlockingFixture().usingInterruptingImmediateSupplier().runGetConfigNonBlockingInMap(); } - @Test // fast - public void testGetImmediateSupplierNonBlockingKey() throws Exception { - new ConfigNonBlockingFixture().usingImmediateSupplier().runGetConfigNonBlockingInKey(); + @Test + public void testGetImmediateSupplierNoSleepNonBlockingKey() throws Exception { + new ConfigNonBlockingFixture().usingImmediateSupplierNoSleep().runGetConfigNonBlockingInKey(); + } + @Test + public void testGetImmediateSupplierNoSleepNonBlockingMap() throws Exception { + new ConfigNonBlockingFixture().usingImmediateSupplierNoSleep().runGetConfigNonBlockingInMap(); + } + @Test + public void testGetImmediateSupplierWithSleepNonBlockingKey() throws Exception { + new ConfigNonBlockingFixture().usingImmediateSupplierWithSleep().runGetConfigNonBlockingInKey(); } @Test - public void testGetImmediateSupplierNonBlockingMap() throws Exception { - new ConfigNonBlockingFixture().usingImmediateSupplier().runGetConfigNonBlockingInMap(); + public void testGetImmediateSupplierWithSleepNonBlockingMap() throws Exception { + new ConfigNonBlockingFixture().usingImmediateSupplierWithSleep().runGetConfigNonBlockingInMap(); } @Test From f4dc19d9631801c4472e46367e2c94e12e6446f4 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Mon, 20 Mar 2017 15:54:34 +0000 Subject: [PATCH 6/6] address PR review -- just with comments here --- .../core/objs/AbstractConfigurationSupportInternal.java | 3 +++ .../org/apache/brooklyn/util/core/task/ValueResolver.java | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java b/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java index 95b631272d..4b3876bac3 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java @@ -138,6 +138,9 @@ protected Maybe getNonBlockingResolvingSimple(ConfigKey key) { .context(getContext()) .getMaybe(); if (resolved.isAbsent()) return Maybe.Absent.castAbsent(resolved); + + // likely we don't need this coercion if we set as(key.getType()) above, + // but that needs confirmation and quite a lot of testing return TypeCoercions.tryCoerce(resolved.get(), key.getTypeToken()); } diff --git a/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java b/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java index e98bb922a2..ffaa817457 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java @@ -392,6 +392,13 @@ protected Maybe getMaybeInternal() { // (the latter could be tried here, with bailOut false, but in most cases it will just throw so we still need to // have the timings as in SHORT_WAIT etc as a fallack) + // TODO svet suggested at https://github.com/apache/brooklyn-server/pull/565#pullrequestreview-27124074 + // that we might flip the immediately bit if interrupted -- or maybe instead (alex's idea) + // enter this block + // if (allowImmediateExecution && (isEvaluatingImmediately() || Thread.isInterrupted()) + // -- feels right, and would help with some recursive immediate values but no compelling + // use case yet and needs some deep thought which we're deferring for now + Maybe result = null; try { result = exec.getImmediately(v);