[shelly] Improve stability through better thread safety

[markus7017]

The PR implements synchronization for improve thread safety of the class.

[markus7017]

@Nadahar I think that's all for ShellyHttpClient.
Should we add Shelly1HttpApi to this PR? Shelly2ApiClient will be a bigger one.

[Nadahar]

Should we add Shelly1HttpApi to this PR?

That's entirely up to you. I think it's fine to do more than one class.

[Nadahar]

I don't think you can safely do timeoutErrors++ with volatiles. I'm not 100% sure, but I've never taken the chance. It's just "syntactic sugar", what it actually does is timeoutErrors = timeoutErrors + 1 - and if that is done by multiple threads in parallel, one thread will overwrite the result of the other. That's why I think AtomicInteger is a better fit for these two, because when you call increment(), you can be sure that it is incremented exactly by one.

[Nadahar]

Now I found where they are called, from fillDeviceStatus(). Using AtomicInteger should be straight forward then.

[Copilot]

Pull request overview

This PR updates the Shelly binding’s ShellyHttpClient to reduce concurrency issues by adding synchronization around configuration updates and reads, and by adjusting several state fields to be volatile.

Changes:

• Synchronize setConfig(...) and snapshot selected config fields under a lock for request execution.
• Change several mutable fields (thingName, timeout counters, basicAuth) to volatile.
• Use local snapshots (e.g., deviceIp, userId, password) to avoid unsynchronized reads of config fields during HTTP calls.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[markus7017]

changes applied

[markus7017]

@Nadahar I also added Shelly1HttpClient, please verify.

[Nadahar]

The "stats counters" looks good. I've started to look at the rest, and I only came to config. Merely synchronizing and grabbing a local reference isn't enough when the object itself is mutable/unprotected.

So, either everything that includes any fields from config must be inside locks, in all the classes that accesses it, or we will have to find another solution. I'm thinking about making ShellyThingConfiguration itself thread-safe. If we did that, then config itself could get away with being volatile, and not need synchronization.

Synchronizing all use of config in all the classes will lead to quite a lot of synchronized blocks, so I'm wondering if the other solution would be better.

[Nadahar]

@markus7017 I pushed a commit here with a proposal to make ShellyThingConfiguration thread-safe. There were quite a lot of changes, so I didn't "fully optimize" everything, some things can probably be done in a better way. But, since I don't even know if we shall keep it, there's no point of "optimizing it".

It's rather messy. The reason is that you use the configuration class for "other purposes". If you only used it for what it's for, there would be no need for synchronization, since getConfigAs() takes care of synchronization and since you should never modify it from the code, it would be "effectively immutable".

In my opinion, these two things should be split, with one class just representing the configuration, and one with the "operational state", where you set the things you set in the configuration itself now. I think this would simplify things - but it should work this way as well.

[lsiepel]

In my opinion, these two things should be split, with one class just representing the configuration, and one with the "operational state", where you set the things you set in the configuration itself now. I think this would simplify things - but it should work this way as well.

+1 for the seperation.

[Nadahar]

I assumed that nobody had checked this out yet, so I amended the last commit. All I did was move the comments in ShellyThingConfiguration to be Javadocs instead.

[Nadahar]

I took the work of commenting on each config parameter which is used where (I don't mean that the code should be merged with these comments, they are only an aid to disentangle this):

    // All access must be guarded by "this"
    /** IP address of the device */
    private String deviceIp = ""; // relay, roller, dimmer, light, rgbw2, battery, basic, relay-gen2, roller-gen2, rgbw-gen2, battery-gen2, dimmer-gen2, blugw

    // All access must be guarded by "this"
    /** IP address or MAC address for BLU devices */
    private String deviceAddress = ""; // blubattery

    // All access must be guarded by "this"
    /** userid for HTTP basic auth */
    private String userId = ""; // relay, roller, dimmer, light, rgbw2, battery, basic

    // All access must be guarded by "this"
    /** password for HTTP basic auth */
    private String password = ""; // relay, roller, dimmer, light, rgbw2, battery, basic, relay-gen2, roller-gen2, rgbw-gen2, battery-gen2, dimmer-gen2

    // All access must be guarded by "this"
    /** schedule interval for the update job */
    private int updateInterval = 60; // relay, roller, dimmer, light, rgbw2, battery, basic, relay-gen2, roller-gen2, rgbw-gen2, battery-gen2, dimmer-gen2

    // All access must be guarded by "this"
    /** threshold for battery value */
    private int lowBattery = 15; // battery, battery-gen2, blubattery

    // All access must be guarded by "this"
    /** {@code true}: turn on device if brightness > 0 is set */
    private boolean brightnessAutoOn = true; // dimmer, light, rgbw2, dimmer-gen2

    // All access must be guarded by "this"
    /** Roller position favorite when control channel receives ON, 0=none */
    private int favoriteUP = 0; // roller, roller-gen2

    // All access must be guarded by "this"
    /** Roller position favorite when control channel receives ON, 0=none */
    private int favoriteDOWN = 0; // roller, roller-gen2

    // All access must be guarded by "this"
    /** {@code true}: register for Relay btn_xxx events */
    private boolean eventsButton = false; // relay, dimmer

    // All access must be guarded by "this"
    /** {@code true}: register for device out_xxx events */
    private boolean eventsSwitch = true; // relay, dimmer, light

    // All access must be guarded by "this"
    /** {@code true}: register for short/long push events */
    private boolean eventsPush = true; // relay, dimmer

    // All access must be guarded by "this"
    /** {@code true}: register for short/long push events */
    private boolean eventsRoller = true; // roller

    // All access must be guarded by "this"
    /** {@code true}: register for sensor events */
    private boolean eventsSensorReport = true; // battery, basic

    // All access must be guarded by "this"
    /** {@code true}: use CoIoT events (based on COAP) */
    private boolean eventsCoIoT = false; // relay, roller, dimmer, light, rgbw2, battery, basic

    // All access must be guarded by "this"
    /** local IP addresses used to create callback URL */
    private String localIp = ""; //

    // All access must be guarded by "this"
    private String localPort = "8080"; //

    // All access must be guarded by "this"
    private String realm = ""; //

    // All access must be guarded by "this"
    private Boolean enableBluGateway = false; // relay-gen2, roller-gen2, rgbw-gen2, dimmer-gen2, blugw

    // All access must be guarded by "this"
    private Boolean enableRangeExtender = true; // relay-gen2, rgbw-gen2

This allows some conclusions to be drawn: realm, localPort and localIp are the only fields that aren't used by any Thing types. They don't belong in the configuration class at all. Other than that, there are pretty clear differences between gen1 and gen2 (and blu for the one device that is defined): deviceIp, password and updateInterval are the only ones that are common for all. userId, eventsButton, eventsSwitch, eventsPush, eventsRoller, eventsSensorReport and eventsCoIoT (in fact all events* fields) are gen1 only. enableBluGateway and enableRangeExtender are gen2 only. deviceAddress is used only be the one defined blu device. lowBattery, brightnessAutoOn, favoriteUP and favoriteDOWN are used across generations.

[markus7017]

From my understanding those values are initially filled

• by the thing configuration
• during thing initialization

I need to check on code level, but assume that those are not changed after thing initialization. In additional a copy is passed to the ShellyHttp class, so I don't get why we need to synchronize access to each member through the code.

I could make the ShellyThingConfiguration representing the thing config only and copy the fields into something. like ShellyThingDynamicConfig, which then gets passed to the other classes.

[Nadahar]

I could make the ShellyThingConfiguration representing the thing config only and copy the fields into something. like ShellyThingDynamicConfig, which then gets passed to the other classes.

Yes, that what I've been suggesting. RuntimeSettings/State/Config or whatever you want to call it. Then the stuff you do with e.g. the password (where you fetch the "default" password if it's blank etc.) could also only be done there, and the config itself never modified.

[Nadahar]

There's one thing I need to explain: The way I approach thread-safety is methodical. I generally don't stop to try to evaluate "is this likely to happen or not", things just get way too complicated that way. So, I'm "strict" in the sense that I look at everything that "can go wrong", without trying to do an analysis of the probability, and then I deal with that. I probably end up protecting some things that "in the real world" would work fine without it, but as I see it, I save lots of time that way because the alternative is very complex analysis, and probably simulation, of what can and can't occur. Many people seem to rely on "guessing" instead, or only handing thread-safety when it's "proven" that it can fail. I reject that, since it only leads to endless subtle bugs, because timing sensitive bugs can be very hard to "prove", and guessing is just that - guessing.

[Nadahar]

• during thing initialization

If something is only modified during initialization, I typically make the class immutable (final fields). That way, there's no need for synchronization. When "building" the information, one can either do it using local variables that are figured out before the immutable instance is created, or I make a "builder" class so that you don't need to juggle all the local variables. When the builder builds, it then creates an immutable instance that can safely be shared.

[markus7017]

@Nadahar I separated the persistent thing config from those values used in addition (ShellyThingBasicConfig vs. ShellyThingConfiguration) and implemented thread-safety by synchronized getter/setter methods. I also move initialization of several config settings centrally to the ShellyThingConfiguration constructor.

[markus7017]

@Nadahar Various files are initialized in the constructor, but not being modified later (those having a getter, but not a setter method). From my understanding I could omit synchronized for them and keep it only for those exposing a setter method, correct?

[markus7017]

@Nadahar I reverted the last commit, this became a separate PR #20445

[markus7017]

Test with 85 devices is running well.

@Nadahar From my understanding you found nothing critical, so are we good for @lsiepel to merge this PR?

Any comment on mockito usage in test cases? Make sense, or remove?

[Nadahar]

From my understanding you found nothing critical, so are we good for @lsiepel to merge this PR?

Yes.

Any comment on mockito usage in test cases? Make sense, or remove?

I already commented on it - sometimes I think you're a bit too quick when reading 😉

[markus7017]

Additional test cases are pushed

[markus7017]

@Nadahar Could you please click "Resolve conversation" on those topics you are fine with. From my side there is nothing more, which should be included in this PR.

[Nadahar]

Could you please click "Resolve conversation" on those topics you are fine with. From my side there is nothing more, which should be included in this PR.

Done. You still seem to have skipped some comments, at least you haven't responded or done anything.

[markus7017]

Done. You still seem to have skipped some comments, at least you haven't responded or done anything.

I miss the change from INFO to DEBUG, now changed
All other topics have a reply from my side (and don't show pending), I checked also collapsed comments

[Nadahar]

All other topics have a reply from my side (and don't show pending), I checked also collapsed comments

Have a reply yes, but not to the last things I've said. It's about the TODO that should be removed, that I've now finally made a suggestion for removing, and it's still the comment in ShellyHandlerFactory that is unclear to me.

[markus7017]

TODO: getDeviceProfile(), I'm aware of this, but will not include it in this PR (has nothing to do with config topics)
TODO 2: Shelly2ApiClient.apiRequest() - same
both are left overs from the separation of ShellyDiscoveryInterface

TODO 3: logic on BD address in initializeThingConfig() looks good.

I gave an answer on HandlerFactory. I removed modified() completely and things are re-initialized on on changes to the binding config. For now this is fine with me.

[Nadahar]

TODO: getDeviceProfile(), I'm aware of this, but will not include it in this PR (has nothing to do with config topics) TODO 2: Shelly2ApiClient.apiRequest() - same both are left overs from the separation of ShellyDiscoveryInterface

TODO 3: logic on BD address in initializeThingConfig() looks good.

I'm not sure what you're referring to, the TODO we removed now was the last one I knew off. The point with making them isn't that they should become a part of the merged code, but that they must be somehow "resolved" before we do. Since the logic seems to work well, I wanted the TODO removed before we merge.

I gave an answer on HandlerFactory. I removed modified() completely and things are re-initialized on on changes to the binding config. For now this is fine with me.

You didn't answer my reply. You didn't specify "how it worked", just that "it works without". I don't know what that means - does it mean that the config is updated, or also that the Thing handlers are reinitialized? That wasn't clear to me, but I understand your reply here as if the latter is the case. That would have been enough for me to "resolve" the comment, except that I didn't understand your comment about "native threads" - which I would also like to have clarified, independently of this PR.

[markus7017]

I'm not sure what you're referring to, the TODO we removed now was the last one I knew off. The point with making them isn't that they should become a part of the merged code, but that they must be somehow "resolved" before we do. Since the logic seems to work well, I wanted the TODO removed before we merge.

Nope, the other 2 are still in the code

I don't know what that means - does it mean that the config is updated, or also that the Thing handlers are reinitialized?

Yes, both works as mentioned dozend comments above. This was already remove before the "partial revert" and I just re-applied. I tested with

• Modify binding config and put in a wrong default password -> things go to OFFLINE with auth error
• Modify it again with correct pw -> things go ONLINE

I didn't understand your comment about "native threads

I was referring to this code, which create a new thread rather than using schedule(). Nevertheless, it's removed.

new Thread(() -> {
                for (ShellyThingInterface handler : handlers) {
                    handler.reinitializeThing();
                }
            }, "OH-binding-shelly-reinitializer").start();
        }
    }

[Nadahar]

Nope, the other 2 are still in the code

Yes, but I don't think they are from this PR.

I was referring to this code, which create a new thread rather than using schedule(). Nevertheless, it's removed.

Yes, that's what I thought too. But this would only happen occasionally, and the threads would only run briefly (there is no loop keeping them alive). The treads are just as "native" as those you get from scheduler, in fact they are exactly the same "type of threads". The reason I didn't use scheduler is that it's not available in that class. It exists in the Thing handlers, not the factory. So, the alternative would have been to use ThreadPoolManager to acquire threads from an existing pool, but that would mean hard-coding the pool name, which could potentially clash with future changes in core. All things considered, I therefore thought that it was the best option to just create one short-lived thread for this.

Also, this wasn't the code that was previously removed. That was your code, I wrote this one without ever looking at what you had there. I doubt it was the same. This one was "smart", it would only trigger the reinit if one of the relevant configuration parameters had changed, not for any change to the configuration. I made ShellyBindingRuntimeConfig.update() return a boolean if an actual change to one of the "inherited" parameter resulted, and only if that was the case, the handler reinit would be triggered.

But, if the BaseThingHandlerFactory already handles this, it's not needed. I very much doubt that BaseThingHandlerFactory takes what values have changed into account when determining if a reinit should be triggered though.

I can't find the code in BaseThingHandlerFactory that does this, so I remain somewhat skeptical. This is where the handlers are created:

    @Override
    public ThingHandler registerHandler(Thing thing) {
        ThingHandler thingHandler = createHandler(thing);
        if (thingHandler == null) {
            throw new IllegalStateException(this.getClass().getSimpleName()
                    + " could not create a handler for the thing '" + thing.getUID() + "'.");
        }
        if ((thing instanceof Bridge) && !(thingHandler instanceof BridgeHandler)) {
            throw new IllegalStateException(
                    "Created handler of bridge '" + thing.getUID() + "' must implement the BridgeHandler interface.");
        }
        registerConfigStatusProvider(thing, thingHandler);
        registerFirmwareUpdateHandler(thing, thingHandler);
        registerServices(thing, thingHandler);
        return thingHandler;
    }

It doesn't store the ThingHander reference anywhere, so how can it reinitialize them?

[Nadahar]

@lsiepel As far as I'm concerned, this is ready. I haven't done a full review with all the latest changes, but I've reviewed the details of the changes, so hopefully I haven't missed anything. The issue with handler reinit can be handled outside this PR.

Maybe give Copilot a final round? Otherwise, this LGTM.

[lsiepel]

Thanks, LGTM

A PR i will remember for some time. Reviews, tests all look good, there seems to be some areas that can be further improved. Hopefully we can do that in smaller chunks.
Anyway, special thanks to @markus7017 and @Nadahar.

[Nadahar]

A PR i will remember for some time. Reviews, tests all look good, there seems to be some areas that can be further improved. Hopefully we can do that in smaller chunks.

It's not my first rodeo, I'm not so sure that this makes it that high on the list of what I've been through 😉 That said, not everything can be done in smaller chunks, which is exactly those things that tend to never be done - because they can't be done in small chunks. But, I agree that I've had enough of this PR now...

[markus7017]

This created memories :-)

A big thank you, this provides a big improvement.