Servient internal caching of ExposedThings works incorrectly with MqttProtocolServer

[Dnnd]

Expected behavior

It's possible to interact with thing, which was exposed (ExposedThing::expose), destroyed (ExposedThing::destroy) and exposed again with the same id.

Observed behavior

It's impossible to use events, actions and properties of exposed ExposedThing if it's id is equal to the id of the previously destroyed Thing.

Example

public class Test {
    private static final String THING_ID = "thing";
    private static final String PROPERTY_NAME = "property-1";

    public static void exposeWriteDestroy(Wot wot,
                                          String value) throws ExecutionException, InterruptedException {
        Thing thing = new Thing.Builder()
                .addProperty(PROPERTY_NAME, new ThingProperty.Builder().setType("string").build())
                .setId(THING_ID)
                .build();
        ExposedThing exposed = wot.produce(thing);
        exposed.expose().get();
        exposed.getProperty(PROPERTY_NAME).write(value).get();
        exposed.destroy().get();
    }

    public static void main(String[] args) throws IOException, WotException, ExecutionException, InterruptedException {
        Config config = ConfigFactory.parseString(
                "wot.servient.client-factories = [\"city.sane.wot.binding.mqtt.MqttProtocolClientFactory\"], wot.servient.servers = [\"city.sane.wot.binding.mqtt.MqttProtocolServer\"]");
        config = config.withFallback(ConfigFactory.load());
        Wot wot = new DefaultWot(config);
        exposeWriteDestroy(wot, "first invocation");
        exposeWriteDestroy(wot, "second invocation");
    }
}

While the string "first invocation" was successfully transmitted to MQTT broker, the second string ("second invocation") wasn't. I believe, the cause is interaction between Servient's things map https://github.com/sane-city/wot-servient/blob/24918ef49c3963304a4e8000b1f9fb9b52a15e97/wot-servient/src/main/java/city/sane/wot/Servient.java#L54 and MqttProtocolServer. Servient::destroy won't remove any ExposedThing from things map, so Servient::addThing method won't replace ExposedThing instance if it's was created earlier: https://github.com/sane-city/wot-servient/blob/24918ef49c3963304a4e8000b1f9fb9b52a15e97/wot-servient/src/main/java/city/sane/wot/Servient.java#L208 It leads to the situation, where ExposedThing instance which I created is different from one inside Servient and MqttProtocolServer internal maps. After ExposedThing::expose invocation these instances won't share the same PublisherSubject field inside states of exposed properties, so it's impossible to pass any data to property observer https://github.com/sane-city/wot-servient/blob/24918ef49c3963304a4e8000b1f9fb9b52a15e97/wot-servient-binding-mqtt/src/main/java/city/sane/wot/binding/mqtt/MqttProtocolServer.java#L130-L138 using instance of ExposedThing returned from Wot::produce method. One may suggest, that problem can be mitigated by using instance of ExposedThing returned from ExposedThing::expose method, but these will lead to another problem. Consider the following code:

public class Test {
    private static final String THING_ID = "thing";
    private static final String PROPERTY_NAME = "property-1";
    private static ExposedThing exposedThing;

    public static void setExposedThing(ExposedThing thing) {
        Test.exposedThing = thing;
    }

    public static void exposeWriteDestroy(Wot wot,
                                          String value) throws ExecutionException, InterruptedException {
        Thing thing = new Thing.Builder()
                .addProperty(PROPERTY_NAME, new ThingProperty.Builder().setType("string").build())
                .setId(THING_ID)
                .build();
        ExposedThing exposed = wot.produce(thing);
        exposed.expose().whenComplete((t, ignore ) -> setExposedThing(t)).get();

        exposedThing.getProperty(PROPERTY_NAME).write(value).get();
        exposedThing.destroy().get();
    }

    public static void main(String[] args) throws IOException, WotException, ExecutionException, InterruptedException {
        Config config = ConfigFactory.parseString(
                "wot.servient.client-factories = [\"city.sane.wot.binding.mqtt.MqttProtocolClientFactory\"], wot.servient.servers = [\"city.sane.wot.binding.mqtt.MqttProtocolServer\"]");
        config = config.withFallback(ConfigFactory.load());
        Wot wot = new DefaultWot(config);
        exposeWriteDestroy(wot, "first invocation");
        exposeWriteDestroy(wot, "second invocation");
    }
}

In this example, the string "second invocation" will be sent to MQTT broker twice. It will happen because MqttProtocolServer won't dispose subscriptions, created in MqttProtocolServer::exposeProperties.

[HeikoBornholdt]

Hello,

thanks for your bug report.

So far, I was able to fix the following issues:

1. When calling ExposedThing::destroy all previously created subscriptions are now disposed (https://github.com/sane-city/wot-servient/commit/6ff9eac7d044e9128d35079db586a8b6784388bb)
2. The servient now refuses to expose multiple things with the same id (https://github.com/sane-city/wot-servient/commit/a393b5ba100f01195110c27a3c5efe8822c42a12).
3. Exposed things will be removed from MQTT broker when calling ExposedThing::destroy (https://github.com/sane-city/wot-servient/commit/1030f1688955ee294928ac7531488b33e644682e)

But there is still no function to remove a thing from the servient. I am still undecided what is the best way to solve this.

[Dnnd]

I simply patched Servient to remove thing from things map in Servient::destroy call:

        return CompletableFuture.allOf(serverFutures).whenComplete((result, t) -> things.remove(id)).thenApply(result -> thing);

The single test was broken https://github.com/sane-city/wot-servient/blob/1030f1688955ee294928ac7531488b33e644682e/wot-servient/src/test/java/city/sane/wot/ServientTest.java#L127 due to immutability issues, but I didn't encounter any other problems. You suppose there would be any issues with this approach?

[HeikoBornholdt]

This approach would prevent a thing once destroyed from being exposed again. We could work around this by moving the Servient::addThing call from DefaultWot::produce to Servient::expose.