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

bundles/core/org.eclipse.smarthome.core.thing.test/src/test/java/org/eclipse/smarthome/core/thing/internal/ThingManagerOSGiJavaTest.java

@@ -0,0 +1,196 @@
+/**
+ * Copyright (c) 2014,2017 Contributors to the Eclipse Foundation
+ *
+ * See the NOTICE file(s) distributed with this work for additional
+ * information regarding copyright ownership.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ */
+package org.eclipse.smarthome.core.thing.internal;
+
+import static org.hamcrest.CoreMatchers.*;
+import static org.junit.Assert.*;
+import static org.mockito.Mockito.*;
+
+import java.io.IOException;
+import java.net.URI;
+import java.util.Collections;
+import java.util.Dictionary;
+import java.util.Hashtable;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.eclipse.jdt.annotation.NonNull;
+import org.eclipse.jdt.annotation.Nullable;
+import org.eclipse.smarthome.core.common.SafeCaller;
+import org.eclipse.smarthome.core.items.ItemRegistry;
+import org.eclipse.smarthome.core.service.ReadyMarker;
+import org.eclipse.smarthome.core.service.ReadyService;
+import org.eclipse.smarthome.core.thing.Channel;
+import org.eclipse.smarthome.core.thing.ChannelUID;
+import org.eclipse.smarthome.core.thing.ManagedThingProvider;
+import org.eclipse.smarthome.core.thing.Thing;
+import org.eclipse.smarthome.core.thing.ThingStatus;
+import org.eclipse.smarthome.core.thing.ThingTypeUID;
+import org.eclipse.smarthome.core.thing.ThingUID;
+import org.eclipse.smarthome.core.thing.binding.BaseThingHandlerFactory;
+import org.eclipse.smarthome.core.thing.binding.ThingHandler;
+import org.eclipse.smarthome.core.thing.binding.ThingHandlerCallback;
+import org.eclipse.smarthome.core.thing.binding.ThingHandlerFactory;
+import org.eclipse.smarthome.core.thing.binding.ThingTypeProvider;
+import org.eclipse.smarthome.core.thing.binding.builder.ThingBuilder;
+import org.eclipse.smarthome.core.thing.link.ItemChannelLinkRegistry;
+import org.eclipse.smarthome.core.thing.type.ThingType;
+import org.eclipse.smarthome.core.thing.type.ThingTypeBuilder;
+import org.eclipse.smarthome.core.thing.type.ThingTypeRegistry;
+import org.eclipse.smarthome.test.java.JavaOSGiTest;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Matchers;
+import org.osgi.framework.InvalidSyntaxException;
+import org.osgi.service.cm.Configuration;
+import org.osgi.service.cm.ConfigurationAdmin;
+
+/**
+ *
+ * @author Simon Kaufmann - initial contribution and API.
+ *
+ */
+public class ThingManagerOSGiJavaTest extends JavaOSGiTest {
+
+    private ManagedThingProvider managedThingProvider;
+    private ItemRegistry itemRegistry;
+    private ReadyService readyService;
+    private ItemChannelLinkRegistry itemChannelLinkRegistry;
+
+    private final ThingTypeUID THING_TYPE_UID = new ThingTypeUID("binding:type");
+    private final ThingUID THING_UID = new ThingUID(THING_TYPE_UID, "id");
+    private final ChannelUID CHANNEL_UID = new ChannelUID(THING_UID, "channel");
+    private Thing THING;
+
+    @Before
+    public void setUp() throws Exception {
+        THING = ThingBuilder.create(THING_TYPE_UID, THING_UID)
+                .withChannels(Collections.singletonList(new Channel(CHANNEL_UID, "Switch"))).build();
+        registerVolatileStorageService();
+
+        configureAutoLinking(false);
+
+        managedThingProvider = getService(ManagedThingProvider.class);
+
+        itemRegistry = getService(ItemRegistry.class);
+        assertNotNull(itemRegistry);
+
+        itemChannelLinkRegistry = getService(ItemChannelLinkRegistry.class);
+        assertNotNull(itemChannelLinkRegistry);
+
+        readyService = getService(ReadyService.class);
+        assertNotNull(readyService);
+
+        waitForAssert(() -> {
+            try {
+                assertThat(
+                        bundleContext.getServiceReferences(ReadyMarker.class,
+                                "(esh.xmlThingTypes=" + bundleContext.getBundle().getSymbolicName() + ")"),
+                        is(notNullValue()));
+            } catch (InvalidSyntaxException e) {
+                throw new RuntimeException(e);
+            }
+        });
+        waitForAssert(() -> {
+            try {
+                assertThat(bundleContext.getServiceReferences(ChannelItemProvider.class, null), is(notNullValue()));
+            } catch (InvalidSyntaxException e) {
+                throw new RuntimeException(e);
+            }
+        });
+
+    }
+
+    @After
+    public void teardown() throws Exception {
+        managedThingProvider.getAll().forEach(it -> {
+            managedThingProvider.remove(it.getUID());
+        });
+        configureAutoLinking(true);
+    }
+
+    @Test
+    public void testInitializeCallsThingUpdated() throws Exception {
+        registerThingTypeProvider();
+        AtomicReference<ThingHandlerCallback> thc = new AtomicReference<>();
+        AtomicReference<Boolean> initializeRunning = new AtomicReference<>(false);
+        ThingHandlerFactory thingHandlerFactory = new BaseThingHandlerFactory() {
+            @Override
+            public boolean supportsThingType(@NonNull ThingTypeUID thingTypeUID) {
+                return true;
+            }
+
+            @Override
+            protected @Nullable ThingHandler createHandler(@NonNull Thing thing) {
+                ThingHandler mockHandler = mock(ThingHandler.class);
+                doAnswer(a -> {
+                    thc.set((ThingHandlerCallback) a.getArguments()[0]);
+                    return null;
+                }).when(mockHandler).setCallback(Matchers.isA(ThingHandlerCallback.class));
+                doAnswer(a -> {
+                    initializeRunning.set(true);
+
+                    // call thingUpdated() from within initialize()
+                    thc.get().thingUpdated(THING);
+
+                    // hang on a little to provoke a potential dead-lock
+                    Thread.sleep(1000);
+
+                    initializeRunning.set(false);
+                    return null;
+                }).when(mockHandler).initialize();
+                when(mockHandler.getThing()).thenReturn(THING);
+                return mockHandler;
+            }
+        };
+        registerService(thingHandlerFactory, ThingHandlerFactory.class.getName());
+        new Thread((Runnable) () -> managedThingProvider.add(THING)).start();
+
+        waitForAssert(() -> {
+            assertThat(THING.getStatus(), is(ThingStatus.INITIALIZING));
+        });
+
+        // ensure it didn't run into a dead-lock which gets resolved by the SafeCaller.
+        waitForAssert(() -> {
+            assertThat(initializeRunning.get(), is(false));
+        }, SafeCaller.DEFAULT_TIMEOUT - 100, 50);
+    }
+
+    private void registerThingTypeProvider() throws Exception {
+        URI configDescriptionUri = new URI("test:test");
+        ThingType thingType = ThingTypeBuilder.instance(new ThingTypeUID("binding", "type"), "label")
+                .withConfigDescriptionURI(configDescriptionUri).build();
+
+        ThingTypeProvider mockThingTypeProvider = mock(ThingTypeProvider.class);
+        when(mockThingTypeProvider.getThingType(Matchers.isA(ThingTypeUID.class), Matchers.isA(Locale.class)))
+                .thenReturn(thingType);
+        registerService(mockThingTypeProvider);
+
+        ThingTypeRegistry mockThingTypeRegistry = mock(ThingTypeRegistry.class);
+        when(mockThingTypeRegistry.getThingType(Matchers.isA(ThingTypeUID.class))).thenReturn(thingType);
+        registerService(mockThingTypeRegistry);
+    }
+
+    private void configureAutoLinking(Boolean on) throws IOException {
+        ConfigurationAdmin configAdmin = getService(ConfigurationAdmin.class);
+        Configuration config = configAdmin.getConfiguration("org.eclipse.smarthome.links", null);
+        Dictionary<String, Object> properties = config.getProperties();
+        if (properties == null) {
+            properties = new Hashtable<>();
+        }
+        properties.put("autoLinks", on.toString());
+        config.update(properties);
+    }
+
+}

bundles/core/org.eclipse.smarthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/binding/BaseThingHandler.java

@@ -475,6 +475,10 @@ protected ThingBuilder editThing() {
      *             if handler is not initialized correctly, because no callback is present
      */
     protected void updateThing(Thing thing) {
+        if (thing == this.thing) {
+            throw new IllegalArgumentException(
+                    "Changes must not be done on the current thing - create a copy, e.g. via editThing()");
+        }
         synchronized (this) {
             if (this.callback != null) {
                 this.thing = thing;
@@ -527,8 +531,7 @@ protected void updateConfiguration(Configuration configuration) {
 
     /**
      * Returns a copy of the properties map, that can be modified. The method {@link
-     * BaseThingHandler#updateProperties(Map<String, String> properties)} must then be called to change the
-     * properties values for the thing that is handled by this thing handler instance.
+     * BaseThingHandler#updateProperties(Map<String, String> properties)} must be called to persist the properties.
      *
      * @return copy of the thing properties (not null)
      */
@@ -538,20 +541,33 @@ protected Map<String, String> editProperties() {
     }
 
     /**
-     * Updates multiple properties for the thing that is handled by this thing handler instance. Each value is only
-     * set for the given property name if there has not been set any value yet or if the value has been changed. If the
-     * value of the property to be set is null then the property is removed.
+     * Informs the framework, that the given properties map of the thing was updated. This method performs a check, if
+     * the properties were updated. If the properties did not change, the framework is not informed about changes.
      *
      * @param properties
-     *            properties map, that was updated
+     *            properties map, that was updated and should be persisted
+     *
+     * @throws IllegalStateException
+     *             if handler is not initialized correctly, because no callback is present
      */
     protected void updateProperties(Map<String, String> properties) {
+        boolean propertiesUpdated = false;
         for (Entry<String, String> property : properties.entrySet()) {
             String propertyName = property.getKey();
             String propertyValue = property.getValue();
             String existingPropertyValue = thing.getProperties().get(propertyName);
             if (existingPropertyValue == null || !existingPropertyValue.equals(propertyValue)) {
                 this.thing.setProperty(propertyName, propertyValue);
+                propertiesUpdated = true;
+            }
+        }
+        if (propertiesUpdated) {
+            synchronized (this) {
+                if (this.callback != null) {
+                    this.callback.thingUpdated(thing);
+                } else {
+                    throw new IllegalStateException("Could not update properties, because callback is missing");
+                }
             }
         }
     }
@@ -562,7 +578,8 @@ protected void updateProperties(Map<String, String> properties) {
      * set for the given property name if there has not been set any value yet or if the value has been changed. If the
      * value of the property to be set is null then the property is removed.
      *
-     * If multiple properties should be changed at the same time, the {@link BaseThingHandler#editProperties()} method
+     * This method also informs the framework about the updated thing, which in fact will persists the changes. So, if
+     * multiple properties should be changed at the same time, the {@link BaseThingHandler#editProperties()} method
      * should be used.
      *
      * @param name the name of the property to be set
@@ -572,6 +589,13 @@ protected void updateProperty(String name, String value) {
         String existingPropertyValue = thing.getProperties().get(name);
         if (existingPropertyValue == null || !existingPropertyValue.equals(value)) {
             thing.setProperty(name, value);
+            synchronized (this) {
+                if (this.callback != null) {
+                    this.callback.thingUpdated(thing);
+                } else {
+                    throw new IllegalStateException("Could not update properties, because callback is missing");
+                }
+            }
         }
     }
 

bundles/core/org.eclipse.smarthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/ThingManager.java

@@ -401,43 +401,49 @@ public void thingRemoved(final Thing thing, ThingTrackerEvent thingTrackerEvent)
 
     @Override
     public void thingUpdated(final Thing thing, ThingTrackerEvent thingTrackerEvent) {
-        Lock lock = getLockForThing(thing.getUID());
-        try {
-            lock.lock();
-            ThingUID thingUID = thing.getUID();
-            Thing oldThing = getThing(thingUID);
-
-            if (oldThing != thing) {
-                this.things.remove(oldThing);
-                this.things.add(thing);
-            }
-
-            final ThingHandler thingHandler = thingHandlers.get(thingUID);
-            if (thingHandler != null) {
-                if (oldThing != thing) {
-                    thing.setHandler(thingHandler);
-                }
-                if (ThingHandlerHelper.isHandlerInitialized(thing) || isInitializing(thing)) {
-                    // prevent infinite loops by not informing handler about self-initiated update
-                    if (!thingUpdatedLock.contains(thingUID)) {
+        ThingUID thingUID = thing.getUID();
+        if (thingUpdatedLock.contains(thingUID)) {
+            // called from the thing handler itself, therefore
+            // it exists, is initializing/initialized and
+            // must not be informed (in order to prevent infinite loops)
+            replaceThing(getThing(thingUID), thing);
+        } else {
+            Lock lock1 = getLockForThing(thing.getUID());
+            try {
+                lock1.lock();
+                ThingHandler thingHandler = replaceThing(getThing(thingUID), thing);
+                if (thingHandler != null) {
+                    if (ThingHandlerHelper.isHandlerInitialized(thing) || isInitializing(thing)) {
                         safeCaller.create(thingHandler).build().thingUpdated(thing);
+                    } else {
+                        logger.debug(
+                                "Cannot notify handler about updated thing '{}', because handler is not initialized (thing must be in status UNKNOWN, ONLINE or OFFLINE). Starting handler initialization instead.",
+                                thing.getThingTypeUID());
+                        initializeHandler(thing);
                     }
                 } else {
-                    logger.debug(
-                            "Cannot notify handler about updated thing '{}', because handler is not initialized (thing must be in status UNKNOWN, ONLINE or OFFLINE). Starting handler initialization instead.",
-                            thing.getThingTypeUID());
-                    initializeHandler(thing);
+                    registerAndInitializeHandler(thing, getThingHandlerFactory(thing));
                 }
-            } else {
-                registerAndInitializeHandler(thing, getThingHandlerFactory(thing));
+
+            } finally {
+                lock1.unlock();
             }
+        }
+    }
 
-            if (oldThing != thing && oldThing != null) {
+    private ThingHandler replaceThing(Thing oldThing, Thing newThing) {
+        final ThingHandler thingHandler = thingHandlers.get(newThing.getUID());
+        if (oldThing != newThing) {
+            this.things.remove(oldThing);
+            this.things.add(newThing);
+            if (thingHandler != null) {
+                newThing.setHandler(thingHandler);
+            }
+            if (oldThing != null) {
                 oldThing.setHandler(null);
             }
-        } finally {
-            lock.unlock();
         }
+        return thingHandler;
     }
 
     private Thing getThing(ThingUID id) {