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

Commit

Permalink
fix reference mess when updating uninitialized things (#5234)
Browse files Browse the repository at this point in the history 
Whenever there was a ThingHandler which failed to initialize,
e.g. because of a missing/non-configured bridge, then the framework
would not updated this ThingHandler's Thing reference as soon
as there was an update on that Thing. As a result, the ThingHandler
operated on the old Thing instance with all its consequences.

fixes #5215
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
  • Loading branch information
@sjsf @maggu2810
sjsf authored and maggu2810 committed Mar 14, 2018
1 parent c19a641 commit 3d01f35
Show file tree
Hide file tree
Showing 2 changed files with 254 additions and 12 deletions.
234 changes: 232 additions & 2 deletions ...est/src/test/java/org/eclipse/smarthome/core/thing/internal/ThingManagerOSGiJavaTest.java
View file
Expand Up @@ -25,6 +25,7 @@
import java.util.Hashtable;
import java.util.List;
import java.util.Map;
import java.util.concurrent.Semaphore;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Function;

Expand All @@ -40,6 +41,7 @@
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.Bridge;
import org.eclipse.smarthome.core.thing.Channel;
import org.eclipse.smarthome.core.thing.ChannelUID;
import org.eclipse.smarthome.core.thing.ManagedThingProvider;
Expand All @@ -48,12 +50,14 @@
import org.eclipse.smarthome.core.thing.ThingStatusDetail;
import org.eclipse.smarthome.core.thing.ThingTypeUID;
import org.eclipse.smarthome.core.thing.ThingUID;
import org.eclipse.smarthome.core.thing.binding.BaseBridgeHandler;
import org.eclipse.smarthome.core.thing.binding.BaseThingHandler;
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.BridgeBuilder;
import org.eclipse.smarthome.core.thing.binding.builder.ChannelBuilder;
import org.eclipse.smarthome.core.thing.binding.builder.ThingBuilder;
import org.eclipse.smarthome.core.thing.link.ItemChannelLinkRegistry;
Expand Down Expand Up @@ -88,8 +92,12 @@ public class ThingManagerOSGiJavaTest extends JavaOSGiTest {

private final String CONFIG_PARAM_NAME = "test";
private final ChannelTypeUID CHANNEL_TYPE_UID = new ChannelTypeUID("binding", "channel");
private final ThingTypeUID THING_TYPE_UID = new ThingTypeUID("binding:type");
private final ThingUID THING_UID = new ThingUID(THING_TYPE_UID, "id");
private final ThingTypeUID THING_TYPE_UID = new ThingTypeUID("binding:thing");
private final ThingUID THING_UID = new ThingUID(THING_TYPE_UID, "thing");

private final ThingTypeUID BRIDGE_TYPE_UID = new ThingTypeUID("binding:bridge");
private final ThingUID BRIDGE_UID = new ThingUID(BRIDGE_TYPE_UID, "bridge");

private final ChannelUID CHANNEL_UID = new ChannelUID(THING_UID, "channel");
private Thing THING;
private URI CONFIG_DESCRIPTION_THING;
Expand Down Expand Up @@ -257,6 +265,228 @@ public void handleCommand(@NonNull ChannelUID channelUID, @NonNull Command comma
Collections.singletonMap(CONFIG_PARAM_NAME, "value"), ThingStatus.ONLINE, ThingStatusDetail.NONE);
}

@Test
public void testChildHandlerInitialized_replacedUnitializedThing() {
Semaphore childHandlerInitializedSemaphore = new Semaphore(1);
Semaphore thingUpdatedSemapthore = new Semaphore(1);

registerThingHandlerFactory(BRIDGE_TYPE_UID, bridge -> new BaseBridgeHandler((Bridge) bridge) {
@Override
public void handleCommand(@NonNull ChannelUID channelUID, @NonNull Command command) {
}

@Override
public void initialize() {
updateStatus(ThingStatus.ONLINE);
}

@Override
public void childHandlerInitialized(ThingHandler childHandler, Thing childThing) {
try {
childHandlerInitializedSemaphore.acquire();
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
};
});
registerThingHandlerFactory(THING_TYPE_UID, thing -> new BaseThingHandler(thing) {
@Override
public void handleCommand(@NonNull ChannelUID channelUID, @NonNull Command command) {
}

@Override
public void initialize() {
if (getBridge() == null) {
throw new RuntimeException("Fail because of missing bridge");
}
updateStatus(ThingStatus.ONLINE);
}

@Override
public void thingUpdated(Thing thing) {
this.thing = thing;
try {
thingUpdatedSemapthore.acquire();
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
};
});

Bridge bridge = BridgeBuilder.create(BRIDGE_TYPE_UID, BRIDGE_UID).build();
managedThingProvider.add(bridge);
waitForAssert(() -> {
assertEquals(ThingStatus.ONLINE, bridge.getStatus());
});

Thing thing = ThingBuilder.create(THING_TYPE_UID, THING_UID).build();
managedThingProvider.add(thing);
waitForAssert(() -> {
assertEquals(ThingStatus.UNINITIALIZED, thing.getStatus());
});

assertEquals(1, childHandlerInitializedSemaphore.availablePermits());

Thing thing2 = ThingBuilder.create(THING_TYPE_UID, THING_UID).withBridge(BRIDGE_UID).build();
managedThingProvider.update(thing2);
waitForAssert(() -> {
assertEquals(ThingStatus.ONLINE, thing2.getStatus());
});

// childHandlerInitialized(...) must be called
assertEquals(0, childHandlerInitializedSemaphore.availablePermits());

// thingUpdated(...) is not called
assertEquals(1, thingUpdatedSemapthore.availablePermits());
}

@Test
public void testChildHandlerInitialized_modifiedUninitializedThing() {
Semaphore childHandlerInitializedSemaphore = new Semaphore(1);
Semaphore thingUpdatedSemapthore = new Semaphore(1);

registerThingHandlerFactory(BRIDGE_TYPE_UID, bridge -> new BaseBridgeHandler((Bridge) bridge) {
@Override
public void handleCommand(@NonNull ChannelUID channelUID, @NonNull Command command) {
}

@Override
public void initialize() {
updateStatus(ThingStatus.ONLINE);
}

@Override
public void childHandlerInitialized(ThingHandler childHandler, Thing childThing) {
try {
childHandlerInitializedSemaphore.acquire();
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
};
});
registerThingHandlerFactory(THING_TYPE_UID, thing -> new BaseThingHandler(thing) {
@Override
public void handleCommand(@NonNull ChannelUID channelUID, @NonNull Command command) {
}

@Override
public void initialize() {
if (getBridge() == null) {
throw new RuntimeException("Fail because of missing bridge");
}
updateStatus(ThingStatus.ONLINE);
}

@Override
public void thingUpdated(Thing thing) {
this.thing = thing;
try {
thingUpdatedSemapthore.acquire();
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
};
});

Bridge bridge = BridgeBuilder.create(BRIDGE_TYPE_UID, BRIDGE_UID).build();
managedThingProvider.add(bridge);
waitForAssert(() -> {
assertEquals(ThingStatus.ONLINE, bridge.getStatus());
});

Thing thing = ThingBuilder.create(THING_TYPE_UID, THING_UID).build();
managedThingProvider.add(thing);
waitForAssert(() -> {
assertEquals(ThingStatus.UNINITIALIZED, thing.getStatus());
});

assertEquals(1, childHandlerInitializedSemaphore.availablePermits());

thing.setBridgeUID(bridge.getUID());
managedThingProvider.update(thing);
waitForAssert(() -> {
assertEquals(ThingStatus.ONLINE, thing.getStatus());
});

// childHandlerInitialized(...) must be called
assertEquals(0, childHandlerInitializedSemaphore.availablePermits());

// thingUpdated(...) is not called
assertEquals(1, thingUpdatedSemapthore.availablePermits());
}

@Test
public void testChildHandlerInitialized_replacedInitializedThing() {
Semaphore childHandlerInitializedSemaphore = new Semaphore(1);
Semaphore thingUpdatedSemapthore = new Semaphore(1);

registerThingHandlerFactory(BRIDGE_TYPE_UID, bridge -> new BaseBridgeHandler((Bridge) bridge) {
@Override
public void handleCommand(@NonNull ChannelUID channelUID, @NonNull Command command) {
}

@Override
public void initialize() {
updateStatus(ThingStatus.ONLINE);
}

@Override
public void childHandlerInitialized(ThingHandler childHandler, Thing childThing) {
try {
childHandlerInitializedSemaphore.acquire();
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
};
});
registerThingHandlerFactory(THING_TYPE_UID, thing -> new BaseThingHandler(thing) {
@Override
public void handleCommand(@NonNull ChannelUID channelUID, @NonNull Command command) {
}

@Override
public void initialize() {
updateStatus(ThingStatus.ONLINE);
}

@Override
public void thingUpdated(Thing thing) {
this.thing = thing;
try {
thingUpdatedSemapthore.acquire();
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
};
});

Bridge bridge = BridgeBuilder.create(BRIDGE_TYPE_UID, BRIDGE_UID).build();
managedThingProvider.add(bridge);
waitForAssert(() -> {
assertEquals(ThingStatus.ONLINE, bridge.getStatus());
});

Thing thing = ThingBuilder.create(THING_TYPE_UID, THING_UID).build();
managedThingProvider.add(thing);
waitForAssert(() -> {
assertEquals(ThingStatus.ONLINE, thing.getStatus());
});

assertEquals(1, childHandlerInitializedSemaphore.availablePermits());

Thing thing2 = ThingBuilder.create(THING_TYPE_UID, THING_UID).withBridge(BRIDGE_UID).build();
managedThingProvider.update(thing2);
waitForAssert(() -> {
assertEquals(ThingStatus.ONLINE, thing2.getStatus());
});

// childHandlerInitialized(...) is not be called - framework calls ThingHandler.thingUpdated(...) instead.
assertEquals(1, childHandlerInitializedSemaphore.availablePermits());

// ThingHandler.thingUpdated(...) must be called
assertEquals(0, thingUpdatedSemapthore.availablePermits());
}

private void assertThingStatus(Map<String, Object> propsThing, Map<String, Object> propsChannel, ThingStatus status,
ThingStatusDetail statusDetail) {
Configuration configThing = new Configuration(propsThing);
Expand Down
32 changes: 22 additions & 10 deletions ...home.core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/ThingManager.java
View file
Expand Up @@ -428,15 +428,33 @@ public void thingUpdated(final Thing thing, ThingTrackerEvent thingTrackerEvent)
Lock lock1 = getLockForThing(thing.getUID());
try {
lock1.lock();
ThingHandler thingHandler = replaceThing(getThing(thingUID), thing);
Thing oldThing = getThing(thingUID);
ThingHandler thingHandler = replaceThing(oldThing, thing);
if (thingHandler != null) {
if (ThingHandlerHelper.isHandlerInitialized(thing) || isInitializing(thing)) {
if (oldThing != null) {
oldThing.setHandler(null);
}
thing.setHandler(thingHandler);
safeCaller.create(thingHandler, ThingHandler.class).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.",
"Cannot notify handler about updated thing '{}', because handler is not initialized (thing must be in status UNKNOWN, ONLINE or OFFLINE).",
thing.getThingTypeUID());
initializeHandler(thing);
if (thingHandler.getThing() == thing) {
logger.debug("Initializing handler of thing '{}'", thing.getThingTypeUID());
if (oldThing != null) {
oldThing.setHandler(null);
}
thing.setHandler(thingHandler);
initializeHandler(thing);
} else {
logger.debug("Replacing uninitialized handler for updated thing '{}'",
thing.getThingTypeUID());
ThingHandlerFactory thingHandlerFactory = getThingHandlerFactory(thing);
unregisterHandler(thingHandler.getThing(), thingHandlerFactory);
registerAndInitializeHandler(thing, thingHandlerFactory);
}
}
} else {
registerAndInitializeHandler(thing, getThingHandlerFactory(thing));
Expand All @@ -453,12 +471,6 @@ private ThingHandler replaceThing(Thing oldThing, Thing newThing) {
if (oldThing != newThing) {
this.things.remove(oldThing);
this.things.add(newThing);
if (thingHandler != null) {
newThing.setHandler(thingHandler);
}
if (oldThing != null) {
oldThing.setHandler(null);
}
}
return thingHandler;
}
Expand Down Expand Up @@ -569,7 +581,7 @@ private void initializeHandler(Thing thing) {
throw new IllegalStateException("Handler should not be null here");
} else {
if (handler.getThing() != thing) {
logger.debug("The model of {} is inconsistent [thing.getHandler().getThing() != thing]",
logger.warn("The model of {} is inconsistent [thing.getHandler().getThing() != thing]",
thing.getUID());
}
}
Expand Down

0 comments on commit 3d01f35

Please sign in to comment.