Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Persist properties again & unblock thingUpdated() when called from within initialize() #4688

Merged
merged 5 commits into from Dec 8, 2017
Merged

Persist properties again & unblock thingUpdated() when called from within initialize() #4688

merged 5 commits into from Dec 8, 2017

Conversation

sjsf
Copy link
Contributor

@sjsf sjsf commented Dec 7, 2017 •
edited

This PR reverts #1682 as it doesn't hurt anymore to try persisting properties now since #2629 made the framework handle non-managed things gracefully.

In order to make this work it also resolves a dead-lock: While #3351 introduced a lock in order to prevent that in a binding ThingHandler.thingUpdated() can be called in parallel with ThingHandler.initialize() from the framework, this mechanism led to a 5s dead-lock if a thing handler actually updates the thing itself during initialization as in #4375 & #4666.

Also it disallows bindings to call BaseThingHandler.updateThing(...) with the current thing instance by throwing an IAE as mutations should be done via BaseThingHandler.editThing().

fixes #4375
fixes #4666
Signed-off-by: Simon Kaufmann simon.kfm@googlemail.com

@sjsf
Revert "Thing properties are never persisted - removed callback.thing…
8e883ac 
…Updated() from BaseThingHandler#updateProperties and #updateProperty (eclipse-archived#1682)"

...now that the framework handles non-managed things gracefully (see eclipse-archived#2629).

reverts eclipse-archived#1682
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
@sjsf
enhance docu
b66fdf2 
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
@sjsf
use long for timeouts in JavaTest
0df98df 
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
@sjsf
unblock thingUpdated() when calling from initialize()
1747b9e 
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
@sjsf sjsf mentioned this pull request Dec 7, 2017
Merged
@afuechsel
Copy link
Contributor

Are you sure, that we will not see a lot of IAE because of the change in updateThing?

@sjsf
Copy link
Contributor Author

sjsf commented Dec 7, 2017

Yes, pretty much. A quick search on eclipse/smarthome and openhab/openhab2-addons revealed only the Hue binding to misbehave in that respect.

maggu2810
maggu2810 reviewed Dec 7, 2017
View reviewed changes
...arthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/ThingManager.java Outdated

if (oldThing != thing && oldThing != null) {
private void replaceThing(Thing oldThing, Thing newThing) {
final ThingHandler thingHandler = thingHandlers.get(newThing.getUID());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is nothing to do if oldThing == newThing, why not moving this line into your if branch, so it is not executed if not necessary?

...arthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/ThingManager.java Outdated
try {
lock1.lock();
replaceThing(getThing(thingUID), thing);
ThingHandler thingHandler = thingHandlers.get(thing.getUID());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaceThing is a private method and access the new thing handler already.
So, if your replaceThing returns the thing handler of the second argument, there is no need to do it twice.

@sjsf
get thing handler only once
dd982ed 
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
@sjsf
Copy link
Contributor Author

sjsf commented Dec 7, 2017

@maggu2810 both your comments are valid of course, but you cannot have it all 😉

PR is updated

@kaikreuzer kaikreuzer mentioned this pull request Dec 7, 2017
Closed
kaikreuzer
kaikreuzer approved these changes Dec 8, 2017
View reviewed changes
Copy link
Contributor

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

@kaikreuzer kaikreuzer merged commit 6ecdd96 into eclipse-archived:master Dec 8, 2017
@sjsf sjsf deleted the persistProperties branch December 8, 2017 10:28
@mhilbush
Copy link
Contributor

mhilbush commented Dec 8, 2017 •
edited

A quick search on eclipse/smarthome and openhab/openhab2-addons revealed only the Hue binding to misbehave in that respect.

I am seeing similar behavior with the zwave binding when updating the Thing config. Was scratching my head why PaperUI would hang for so long (5 sec) on a simple config update... The specific use case is when changing the temp scale of a channel.

Edit: I'm on OH build 1115.

@SJKA Do you believe this is the same root cause that will be solved by this PR?

2017-12-08 07:01:10.464 [WARN ] [nal.common.AbstractInvocationHandler] - Timeout of 5000ms exceeded while calling method 'ThingHandler.thingUpdated()' on 'org.openhab.binding.zwave.handler.ZWaveThingHandler@489b25ab'. Thread 'safeCall-410' (29194) is in state 'WAITING'
        at sun.misc.Unsafe.park(Native Method)
        at java.util.concurrent.locks.LockSupport.park(LockSupport.java:175)
        at java.util.concurrent.locks.AbstractQueuedSynchronizer.parkAndCheckInterrupt(AbstractQueuedSynchronizer.java:836)
        at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireQueued(AbstractQueuedSynchronizer.java:870)
        at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire(AbstractQueuedSynchronizer.java:1199)
        at java.util.concurrent.locks.ReentrantLock$NonfairSync.lock(ReentrantLock.java:209)
        at java.util.concurrent.locks.ReentrantLock.lock(ReentrantLock.java:285)
        at org.eclipse.smarthome.core.thing.internal.ThingManager.thingUpdated(ThingManager.java:406)
        at org.eclipse.smarthome.core.thing.internal.ThingRegistryImpl.notifyTrackers(ThingRegistryImpl.java:222)
        at org.eclipse.smarthome.core.thing.internal.ThingRegistryImpl.notifyListenersAboutUpdatedElement(ThingRegistryImpl.java:145)
        at org.eclipse.smarthome.core.thing.internal.ThingRegistryImpl.notifyListenersAboutUpdatedElement(ThingRegistryImpl.java:1)
        at org.eclipse.smarthome.core.common.registry.AbstractRegistry.updated(AbstractRegistry.java:188)
        at org.eclipse.smarthome.core.common.registry.AbstractRegistry.updated(AbstractRegistry.java:1)
        at org.eclipse.smarthome.core.common.registry.AbstractProvider.notifyListeners(AbstractProvider.java:62)
        at org.eclipse.smarthome.core.common.registry.AbstractProvider.notifyListenersAboutUpdatedElement(AbstractProvider.java:86)
        at org.eclipse.smarthome.core.common.registry.AbstractManagedProvider.update(AbstractManagedProvider.java:132)
        at org.eclipse.smarthome.core.thing.internal.ThingManager$1$1.run(ThingManager.java:233)
        at org.eclipse.smarthome.core.thing.internal.ThingManager$1$1.run(ThingManager.java:1)
        at java.security.AccessController.doPrivileged(Native Method)
        ...

@mhilbush
Copy link
Contributor

mhilbush commented Dec 8, 2017

I loaded up OH build 1118 (which includes this PR) and the above problem no longer occurs.

@openhab-bot
Copy link
Contributor

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/blacklist-troubleshooting/37055/5

@sjsf
Copy link
Contributor Author

sjsf commented Dec 9, 2017

@SJKA Do you believe this is the same root cause that will be solved by this PR?

Yes. And it's awesome to see your positive confirmation of it, thanks @mhilbush!

@htreu htreu added this to the 0.10.0 milestone Oct 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@maggu2810 maggu2810 maggu2810 left review comments

@kaikreuzer kaikreuzer kaikreuzer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.10.0
7 participants
@sjsf @afuechsel @mhilbush @openhab-bot @maggu2810 @kaikreuzer @htreu