brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (BROOKLYN-544) Deadlock in enricher's subscription
Date Thu, 12 Oct 2017 09:02:00 GMT

    [ https://issues.apache.org/jira/browse/BROOKLYN-544?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16201662#comment-16201662
] 

ASF GitHub Bot commented on BROOKLYN-544:
-----------------------------------------

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

    https://github.com/apache/brooklyn-server/pull/860#discussion_r144226175
  
    --- Diff: core/src/main/java/org/apache/brooklyn/core/sensor/AttributeMap.java ---
    @@ -138,62 +165,91 @@ private void checkPath(Collection<String> path) {
             Preconditions.checkArgument(!path.isEmpty(), "path can't be empty");
         }
     
    +    /**
    +     * Sets a value and published it.
    +     * <p>
    +     * Constraints of {@link #getLockInternal()} apply, with lock-holder being the supplied
{@link Function}.
    +     */
         public <T> T update(AttributeSensor<T> attribute, T newValue) {
             // 2017-10 this was unsynched which meant if two threads updated
             // the last publication would not correspond to the last value.
    -        // could introduce deadlock but emit internal and publish should
    -        // not seek any locks. _subscribe_ and _delivery_ might, but they
    -        // won't be in this block. an issue with _subscribe-and-get-initial_
    -        // should be resolved by initial subscription queueing the publication
    -        // to a context where locks are not held.
    -        synchronized (values) {
    -            T oldValue = updateWithoutPublishing(attribute, newValue);
    +        // a crude `synchronized (values)` could cause deadlock as
    +        // this does publish (doing `synch(LSM)`) whereas _subscribe_ 
    +        // would have the LSM lock and try to `synch(values)`
    +        // as described at https://issues.apache.org/jira/browse/BROOKLYN-544.
    +        // the addition of getLockInternal should make this much cleaner,
    +        // as no one holding `synch(LSM)` should be updating here, 
    +        // ands gets here won't call any other code at all
    +        return withLock(() -> {
    +            T oldValue = updateInternalWithoutLockOrPublish(attribute, newValue);
                 entity.emitInternal(attribute, newValue);
                 return oldValue;
    -        }
    +        });
         }
         
    +    /** @deprecated since 0.13.0 this is becoming an internal method, {@link #updateInternalWithoutLockOrPublish(AttributeSensor,
Object)} */
    +    @Deprecated
         public <T> T updateWithoutPublishing(AttributeSensor<T> attribute, T
newValue) {
    -        if (log.isTraceEnabled()) {
    -            Object oldValue = getValue(attribute);
    -            if (!Objects.equal(oldValue, newValue != null)) {
    -                log.trace("setting attribute {} to {} (was {}) on {}", new Object[] {attribute.getName(),
newValue, oldValue, entity});
    -            } else {
    -                log.trace("setting attribute {} to {} (unchanged) on {}", new Object[]
{attribute.getName(), newValue, this});
    +        return updateInternalWithoutLockOrPublish(attribute, newValue);
    +    }
    +    
    +    @Beta
    +    public <T> T updateInternalWithoutLockOrPublish(AttributeSensor<T> attribute,
T newValue) {
    +        synchronized (values) {
    +            if (log.isTraceEnabled()) {
    +                Object oldValue = getValue(attribute);
    +                if (!Objects.equal(oldValue, newValue != null)) {
    +                    log.trace("setting attribute {} to {} (was {}) on {}", new Object[]
{attribute.getName(), newValue, oldValue, entity});
    +                } else {
    +                    log.trace("setting attribute {} to {} (unchanged) on {}", new Object[]
{attribute.getName(), newValue, this});
    +                }
                 }
    +    
    +            T oldValue = update(attribute.getNameParts(), newValue);
    +            return (isNull(oldValue)) ? null : oldValue;
             }
    -
    -        T oldValue = update(attribute.getNameParts(), newValue);
    -        
    -        return (isNull(oldValue)) ? null : oldValue;
         }
     
    +    private <T> T withLock(Callable<T> body) { return Locks.withLock(getLockInternal(),
body); }
    +    private void withLock(Runnable body) { Locks.withLock(getLockInternal(), body); }
    +    
         /**
    -     * Where atomicity is desired, the methods in this class synchronize on the {@link
#values} map.
    +     * Where atomicity is desired, this method wraps an acquisition of {@link #getLockInternal()}
    +     * while applying the given {@link Function}.
    +     * <p>
    +     * Constraints of {@link #getLockInternal()} apply, with lock-holder being the supplied
{@link Function}.
          */
         public <T> T modify(AttributeSensor<T> attribute, Function<? super
T, Maybe<T>> modifier) {
    -        synchronized (values) {
    +        return withLock(() -> {
                 T oldValue = getValue(attribute);
                 Maybe<? extends T> newValue = modifier.apply(oldValue);
    -
    +            
                 if (newValue.isPresent()) {
                     if (log.isTraceEnabled()) log.trace("modified attribute {} to {} (was
{}) on {}", new Object[] {attribute.getName(), newValue, oldValue, entity});
                     return update(attribute, newValue.get());
                 } else {
                     if (log.isTraceEnabled()) log.trace("modified attribute {} unchanged;
not emitting on {}", new Object[] {attribute.getName(), newValue, this});
                     return oldValue;
                 }
    -        }
    +        });
         }
     
    +    /** Removes a sensor.  The {@link #getLockInternal()} is acquired,
    +     * and constraints on the caller described there apply to callers of this method.
    +     * <p>
    +     * The caller is responsible for any subsequent actions needed, including publishing
    +     * the removal of the sensor and triggering persistence. Caller may wish to 
    +     * have the {@link #getLockInternal()} while doing that. */
         public void remove(AttributeSensor<?> attribute) {
             BrooklynLogging.log(log, BrooklynLogging.levelDebugOrTraceIfReadOnly(entity),
                 "removing attribute {} on {}", attribute.getName(), entity);
    -
    -        remove(attribute.getNameParts());
    +        withLock(() -> remove(attribute.getNameParts()) );
         }
     
         // TODO path must be ordered(and legal to contain duplicates like "a.b.a"; list would
be better
    +    /** @deprecated since 0.13.0 becoming private; callers should only ever use {@link
#remove(AttributeSensor)}
    --- End diff --
    
    `since 1.0.0`


> Deadlock in enricher's subscription
> -----------------------------------
>
>                 Key: BROOKLYN-544
>                 URL: https://issues.apache.org/jira/browse/BROOKLYN-544
>             Project: Brooklyn
>          Issue Type: Bug
>            Reporter: Aled Sage
>            Priority: Blocker
>
> With brooklyn 1.0.0-SNAPSHOT, running the test {{YamlRollingTimeWindowMeanEnricherTest}}
many times, it hung with the deadlock shown below:
> {noformat}
> Found one Java-level deadlock:
> =============================
> "brooklyn-execmanager-CkWF5Jg7-1":
>   waiting to lock monitor 0x00007f8e9c025de8 (object 0x00000007b5fee528, a org.apache.brooklyn.core.mgmt.internal.LocalSubscriptionManager),
>   which is held by "main"
> "main":
>   waiting to lock monitor 0x00007f8e9b95ebe8 (object 0x00000007b6042cf8, a java.util.Collections$SynchronizedMap),
>   which is held by "brooklyn-execmanager-CkWF5Jg7-1"
> Java stack information for the threads listed above:
> ===================================================
> "brooklyn-execmanager-CkWF5Jg7-1":
>         at org.apache.brooklyn.core.mgmt.internal.LocalSubscriptionManager.getSubscriptionsForEntitySensor(LocalSubscriptionManager.java:207)
>         - waiting to lock <0x00000007b5fee528> (a org.apache.brooklyn.core.mgmt.internal.LocalSubscriptionManager)
>         at org.apache.brooklyn.core.mgmt.internal.LocalSubscriptionManager.publish(LocalSubscriptionManager.java:255)
>         at org.apache.brooklyn.core.mgmt.internal.BasicSubscriptionContext.publish(BasicSubscriptionContext.java:176)
>         at org.apache.brooklyn.core.entity.AbstractEntity$BasicSensorSupport.emitInternal(AbstractEntity.java:1059)
>         at org.apache.brooklyn.core.sensor.AttributeMap.update(AttributeMap.java:151)
>         - locked <0x00000007b6042cf8> (a java.util.Collections$SynchronizedMap)
>         at org.apache.brooklyn.core.entity.AbstractEntity$BasicSensorSupport.set(AbstractEntity.java:957)
>         at org.apache.brooklyn.core.enricher.AbstractEnricher.emit(AbstractEnricher.java:144)
>         at org.apache.brooklyn.enricher.stock.AbstractTransformer.onEvent(AbstractTransformer.java:157)
>         at org.apache.brooklyn.core.mgmt.internal.LocalSubscriptionManager$1.run(LocalSubscriptionManager.java:336)
>         at org.apache.brooklyn.core.mgmt.internal.LocalSubscriptionManager.submitPublishEvent(LocalSubscriptionManager.java:352)
>         at org.apache.brooklyn.core.mgmt.internal.LocalSubscriptionManager.lambda$0(LocalSubscriptionManager.java:192)
>         at org.apache.brooklyn.core.mgmt.internal.LocalSubscriptionManager$$Lambda$3/871160466.run(Unknown
Source)
>         at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
>         at org.apache.brooklyn.util.core.task.BasicExecutionManager$SubmissionCallable.call(BasicExecutionManager.java:565)
>         at org.apache.brooklyn.util.core.task.SingleThreadedScheduler$1.call(SingleThreadedScheduler.java:116)
>         at java.util.concurrent.FutureTask.run(FutureTask.java:266)
>         at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
>         at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
>         at java.lang.Thread.run(Thread.java:745)
> "main":
>         at java.util.Collections$SynchronizedMap.get(Collections.java:2584)
>         - waiting to lock <0x00000007b6042cf8> (a java.util.Collections$SynchronizedMap)
>         at org.apache.brooklyn.core.sensor.AttributeMap.getValue(AttributeMap.java:219)
>         at org.apache.brooklyn.core.sensor.AttributeMap.getValue(AttributeMap.java:225)
>         at org.apache.brooklyn.core.entity.AbstractEntity$BasicSensorSupport.get(AbstractEntity.java:932)
>         at org.apache.brooklyn.core.mgmt.internal.LocalSubscriptionManager.subscribe(LocalSubscriptionManager.java:147)
>         - locked <0x00000007b5fee528> (a org.apache.brooklyn.core.mgmt.internal.LocalSubscriptionManager)
>         at org.apache.brooklyn.core.mgmt.internal.AbstractSubscriptionManager.subscribe(AbstractSubscriptionManager.java:106)
>         at org.apache.brooklyn.core.mgmt.internal.BasicSubscriptionContext.subscribe(BasicSubscriptionContext.java:98)
>         at org.apache.brooklyn.core.mgmt.internal.SubscriptionTracker.subscribe(SubscriptionTracker.java:67)
>         at org.apache.brooklyn.core.objs.AbstractEntityAdjunct$BasicSubscriptionSupport.subscribe(AbstractEntityAdjunct.java:238)
>         at org.apache.brooklyn.enricher.stock.AbstractTransformer.setEntity(AbstractTransformer.java:120)
>         at org.apache.brooklyn.enricher.stock.YamlRollingTimeWindowMeanEnricher.setEntity(YamlRollingTimeWindowMeanEnricher.java:93)
>         at org.apache.brooklyn.core.entity.AbstractEntity$BasicEnricherSupport.add(AbstractEntity.java:1443)
>         at org.apache.brooklyn.core.entity.AbstractEntity$BasicEnricherSupport.add(AbstractEntity.java:1429)
>         at org.apache.brooklyn.enricher.stock.YamlRollingTimeWindowMeanEnricherTest.setUp(YamlRollingTimeWindowMeanEnricherTest.java:68)
>         at sun.reflect.GeneratedMethodAccessor16.invoke(Unknown Source)
>         at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>         at java.lang.reflect.Method.invoke(Method.java:498)
>         at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
>         at org.testng.internal.Invoker.invokeConfigurationMethod(Invoker.java:515)
>         at org.testng.internal.Invoker.invokeConfigurations(Invoker.java:217)
>         at org.testng.internal.Invoker.invokeMethod(Invoker.java:590)
>         at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851)
>         at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177)
>         at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
>         at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
>         at org.testng.TestRunner.privateRun(TestRunner.java:756)
>         at org.testng.TestRunner.run(TestRunner.java:610)
>         at org.testng.SuiteRunner.runTest(SuiteRunner.java:387)
>         at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:382)
>         at org.testng.SuiteRunner.privateRun(SuiteRunner.java:340)
>         at org.testng.SuiteRunner.run(SuiteRunner.java:289)
>         at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
>         at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86)
>         at org.testng.TestNG.runSuitesSequentially(TestNG.java:1293)
>         at org.testng.TestNG.runSuitesLocally(TestNG.java:1218)
>         at org.testng.TestNG.runSuites(TestNG.java:1133)
>         at org.testng.TestNG.run(TestNG.java:1104)
>         at org.testng.remote.AbstractRemoteTestNG.run(AbstractRemoteTestNG.java:132)
>         at org.testng.remote.RemoteTestNG.initAndRun(RemoteTestNG.java:230)
>         at org.testng.remote.RemoteTestNG.main(RemoteTestNG.java:76)
> Found 1 deadlock.
> {noformat}



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Mime
View raw message