[mqtt][HA] ConcurrentHashMap to avoid ConcurrentModificationException

[bodiroga]

Hi @jochen314!

I've been able to fix the ConcurrentModifiedException errors using a ConcurrentHashMap for the 'results' of the discovery process.

What do you think?

Sadly the problem described in #7716 (comment) is still present 😞

Fixes #7661

Signed-off-by: Aitor Iturrioz [email protected]

[TravisBuddy]

Travis tests have failed

Hey @bodiroga,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

[J-N-K]

Keep the Map in the declaration. But another suggestion:

Why don‘t you create an inner class

public static class HADiscoveryResult {
    public final Set<HaId> components = new HashSet<>();
    public @Nullable DiscoveryResult discoveryResult;
}

and use one Map<ThingUID, HADiscoveryResult> instead of the two sets you have now? Makes it easier to be thread safe late when clearing the Map.

[bodiroga]

Hi @J-N-K!

I have changed the results declaration type, but I would prefer not to change the rest of the implementation, as it works fine. Do you think it's necessary?

[TravisBuddy]

Travis tests have failed

Hey @bodiroga,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

[bodiroga]

Hi @J-N-K!

Sorry to bother you, but is there any option to get this merged for OH 2.5.8? And also: should I pay attention to Travis?

Thank you!

[J-N-K]

Travis fails in disabled itest

[jochen314]

I do not think, that this will fix #7661
We would need to make componentsPerThingID a ConcurrentHashMap and also the Set, which we put into it:

Set<HaID> components = componentsPerThingID.computeIfAbsent(thingID, key -> ConcurrentHashMap.newKeySet());

When we are at it, we should make thingIDPerTopic a ConcurrentHashMap to be sure.

The changes to results should not be needed, as all access is synchonized.

[bodiroga]

I do not think, that this will fix #7661
We would need to make componentsPerThingID a ConcurrentHashMap and also the Set, which we put into it:

Set<HaID> components = componentsPerThingID.computeIfAbsent(thingID, key -> ConcurrentHashMap.newKeySet());

When we are at it, we should make thingIDPerTopic a ConcurrentHashMap to be sure.

The changes to results should not be needed, as all access is synchonized.

Hi @jochen314!

As you guys know more than me, I don't know if your proposed changes are really required or not, all I can say is that with the PR merged, I haven't seeing the error again. Was I the only one with that error message?

[jochen314]

I see. I did not notice, that you had my second change (line 168) already in.
This reduces the change of a CME by a lot.
But there still is a chance for one, because componentsPerThingID is not thread safe

[bodiroga]

So it is just a matter of changing componentsPerThingID and thingIDPerTopic to ConcurrentHashMaps? Nothing more? I can test it and provide a PR if you want!