potential dead lock (thing manager impl. + custom addon)

[maggu2810]

The thingAdded method of the thing manager implementation if called by the ThingRegistryImpl instance's method notifyTrackers if a provider notifies its listener about an added thing.

https://github.com/openhab/openhab-core/blob/master/bundles/org.openhab.core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/ThingManagerImpl.java#L454

This method calls registerAndInitializeHandler.

https://github.com/openhab/openhab-core/blob/master/bundles/org.openhab.core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/ThingManagerImpl.java#L458

https://github.com/openhab/openhab-core/blob/master/bundles/org.openhab.core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/ThingManagerImpl.java#L1069

The registerAndInitializeHandler calls initializeHandler.

https://github.com/openhab/openhab-core/blob/master/bundles/org.openhab.core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/ThingManagerImpl.java#L1074

https://github.com/openhab/openhab-core/blob/master/bundles/org.openhab.core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/ThingManagerImpl.java#L636

The initializeHandler method looks up for the lock of the given thing and locks that lock (in the calling thread -- let's name it thread_a).

https://github.com/openhab/openhab-core/blob/master/bundles/org.openhab.core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/ThingManagerImpl.java#L645-L647

It calls doInitializeHandler while the lock is hold.

https://github.com/openhab/openhab-core/blob/master/bundles/org.openhab.core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/ThingManagerImpl.java#L672

https://github.com/openhab/openhab-core/blob/master/bundles/org.openhab.core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/ThingManagerImpl.java#L758

The doInitializeHandler method calls the initialize method of the thing handler using the safe caller.

https://github.com/openhab/openhab-core/blob/master/bundles/org.openhab.core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/ThingManagerImpl.java#L758

It does not force async execution so a synchronous execution is used.

The synchronous invocation handler submit the execution to a scheduler.

https://github.com/openhab/openhab-core/blob/master/bundles/org.openhab.core/src/main/java/org/eclipse/smarthome/core/internal/common/InvocationHandlerSync.java#L65

As the execution is synchronous we wait until another thread (let's name it thread_b) finished its execution.

https://github.com/openhab/openhab-core/blob/master/bundles/org.openhab.core/src/main/java/org/eclipse/smarthome/core/internal/common/InvocationHandlerSync.java#L66

So, thread_a holds the lock of the thing and waits until thread_b finished the execution of the thing handler's initialize logic.

The thing handler's initialize logic is implemented by third party developers. As soon as the custom thing handler's implementation contains some code that triggers another thing manager implementation code that would like to lock the thing's lock, there is a dead lock.

• thread_a holds the thing's lock
• thread_a delegate thing handler's initialize method execution to thread_b
• thread_a waits until thread_b is finished
• thread_b starts the execution of the thing handler's initialize method
• thread_b contians some custom code that triggers a thing manager implementation method that tries to lock the thing's lock
• thread_b waits until the lock is "free" and can be locked (so it waits until thread_a calls the unlock method)
• thread_a still waits until thread_b is finished
• thread_a and thread_b waits forever

If I am correct this can be provoked by calling the update method for the thing of the provider (in the initialize method).

sun.misc.Unsafe.park(Native Method)
java.util.concurrent.locks.LockSupport.park(LockSupport.java:175)
java.util.concurrent.locks.AbstractQueuedSynchronizer.parkAndCheckInterrupt(AbstractQueuedSynchronizer.java:836)
java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireQueued(AbstractQueuedSynchronizer.java:870)
java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire(AbstractQueuedSynchronizer.java:1199)
java.util.concurrent.locks.ReentrantLock$NonfairSync.lock(ReentrantLock.java:209)
java.util.concurrent.locks.ReentrantLock.lock(ReentrantLock.java:285)
org.eclipse.smarthome.core.thing.internal.ThingManagerImpl.thingUpdated
org.eclipse.smarthome.core.thing.internal.ThingRegistryImpl.notifyTrackers
org.eclipse.smarthome.core.thing.internal.ThingRegistryImpl.notifyListenersAboutUpdatedElement
org.eclipse.smarthome.core.common.registry.AbstractRegistry.updated
org.eclipse.smarthome.core.common.registry.AbstractProvider.notifyListeners
org.eclipse.smarthome.core.common.registry.AbstractProvider.notifyListenersAboutUpdatedElement
provider
thing handler's initialize method

We can state that this should not be done. I just want to show that this way the safe caller threads can be eaten up.

IMHO it is very dangerous to hold a lock and delegate work to another thread + waits until the execution is done -- if you cannot ensure that the lock will not be tried to lock again in that execution.

[J-N-K]

Besides a complete re-design: Wouldn't using tryLock(5, TimeUnit.SECONDS) at least prevent the deadlock? We could log a warning (or better an error) stating that this is a bug if we run into that condition.