[shelly] Add additional syncing to Shelly2ApiRc

[markus7017]

Some more improvements on thread-safety.
@Nadahar Could you check while I'm running longer tests. I'm not sure about synchronized on disconnect(), reconnect(), close() method level.

[Nadahar]

@markus7017 It will take some time to analyze is properly, but after a quick glance, it doesn't look bad.

The problem/challenge as I see it, is the inheritance of the classes. Remember that any instance is the result of the class and all superclasses combined, which means that to make one thread-safe, you must handle the superclasses too.

We have:

ShellyHttpClient ─┬─> Shelly1HttpApi
                  └─> Shelly2ApiClient ─> Shelly2ApiRpc ─> ShellyBluApi

So, to make this thread-safe, you really have to deal with all of these classes. It might be slightly complicated.

[Nadahar]

I suggest that we start with ShellyHttpClient and work our way down the hierarchy.

Looking at it, I see:

    protected int timeoutErrors = 0;
    protected int timeoutsRecovered = 0;

But, I can't see that the information gathered is used anywhere. It's a bit of a strange thing to keep track of, what can you possibly use that information for? I mean, these are sums of errors/recovery across all requests for a device. The number of requests isn't counted, nor is the time - so you have no context to decide if this is "little" or "much". What is the idea? Is it better to just remove them, than to try to make them thread-safe?

[markus7017]

@markus7017 It will take some time to analyze is properly, but after a quick glance, it doesn't look bad.

The problem/challenge as I see it, is the inheritance of the classes. Remember that any instance is the result of the class and all superclasses combined, which means that to make one thread-safe, you must handle the superclasses too.

We have:

ShellyHttpClient ─┬─> Shelly1HttpApi
                  └─> Shelly2ApiClient ─> Shelly2ApiRpc ─> ShellyBluApi

So, to make this thread-safe, you really have to deal with all of these classes. It might be slightly complicated.

I think this is a longer journey and splitter in multiple PRs.

Do you see a risk to just merge this PR. It doesn't solve the issues completely, but would add some level of improvement. Then we could proceed module by module, which also creates a clerar scope for each PR

[Nadahar]

I think this is a longer journey and splitter in multiple PRs.

Do you see a risk to just merge this PR. It doesn't solve the issues completely, but would add some level of improvement. Then we could proceed module by module, which also creates a clerar scope for each PR

It's hard to evaluate what the consequences of having some things thread-safe and some things not is. I can't really say, I never do it that way. I'm not sure that the "journey" is that long, but maybe, I can't really tell until I dive into the details.

If you want to do it in multiple PRs, I would start with ShellyHttpClient and then do them in hierarchical order.

[Nadahar]

How did this happen? Was it a mistake in the previous PR? I can't work if it always returns null, so it can't have been like this.

[markus7017]

That was a last-minute fix, but I suppose @lsiepel had already merged the PR, because I posted "done" before pushing the change. I created #20395

[lsiepel]

Other PR was merged. I would expect this to be remvoed from the PR or a conflict.

[markus7017]

    protected int timeoutErrors = 0;
    protected int timeoutsRecovered = 0;

Those are just statistical values, updated in ShellyHttpClient.httpRequest(). They are displayed by ShellyManager in the WebView and are an indicator for an unstable WiFi connection or other issues when communicating with the device.

[markus7017]

If you want to do it in multiple PRs, I would start with ShellyHttpClient and then do them in hierarchical order.

O, then I would say we leave this one open and starte with ShellyHttoClient

[Nadahar]

Those are just statistical values, updated in ShellyHttpClient.httpRequest(). They are displayed by ShellyManager in the WebView and are an indicator for an unstable WiFi connection or other issues when communicating with the device.

Ping me from the new PR about this, the reason I asked is because syncing them will mean a lot of sync blocks. I can't see that they are read anywhere, so how does the WebView get them?

I'm wondering if it would be best to use AtomicInteger for those, so that they are completely independent and don't need sync. But, then they might need to be read slightly differently, which is why I was looking for the code that used them. Do you read them using reflection, or is Eclipse just not able to find the reference?