Fix concurrency in YamlThingProvider

[Nadahar]

This is an attempt to fix #5664. I don't know that much about YamlThingProvider, so it's a bit hard for me to properly assess whether everything works as it should, but I don't think I've broken anything.

There was a general lack of thread-safety in the class, in addition to modifying a collection inside a loop. I've addressed both, but making it thread-safe was a bit challenging because of how the logic is structured.

Calling listeners while holding a lock is always a bad idea, because it can easily lead to deadlocks, so I've had to do some refactoring to make it possible to notify listeners outside the synchronization blocks.

@lolodomo I'm creating it as a draft for now, not because I have more to do, but because I want you to look at it and/or test it before putting it up for review.

[wborn]

Thanks for working on this. Protecting thingsMap and the retry queue with a common lock addresses the direct collection-safety problem that caused the reported ConcurrentModificationException. Moving listener calls outside the lock is also sensible because listeners are external code and may call back into other components, which could otherwise introduce deadlocks.

I think there is still a correctness issue around notification ordering, though. The provider state is updated while holding the lock, but the corresponding listener notification is sent only after releasing it. This means another thread can make and notify a newer change before the older notification has been delivered.

Because the registry processes updates by UID and does not verify that the notification's oldElement is still its current value, a delayed older notification can overwrite newer registry state. This can leave YamlThingProvider containing one Thing while ThingRegistry contains an older version of the same thing.

I also think this PR should include deterministic concurrency regression tests. The current test changes continue to exercise only the sequential retry flow and mostly adapt assertions to the renamed retry-thread field. They would not fail for either the original concurrent modification or the remaining notification-ordering race.

I left inline comments with a concrete interleaving and a possible latch-based testing approach.

[wborn]

Could we make sure that state changes and their corresponding listener notifications are observed in the same order?

Moving listener calls outside synchronized (this) is sensible because listeners can call into other components and potentially cause a deadlock. However, releasing the lock between updating thingsMap and notifying the listeners introduces a different race: another thread can perform and notify a newer update before this thread sends its older notification.

For example, suppose the same thing initially has state A:

1. The retry thread acquires the lock, changes the thing from A to B, records an A → B notification, and releases the lock.
2. Before that notification is sent, a YAML file update acquires the lock and changes the thing from B to C.
3. The YAML update sends its B → C notification first, so the registry now contains C.
4. The retry thread then sends its delayed A → B notification.

AbstractRegistry.updated() looks up the currently registered element by UID, but it does not check whether that element is still equal to the supplied oldElement. It can therefore accept the delayed notification and replace C with the older B. At that point, YamlThingProvider contains C while ThingRegistry contains B.

The same general issue can occur between add, update, remove, and retry notifications whenever two operations overlap.

Could notification dispatch therefore be serialized in the same order as the state mutations, without invoking listeners while holding the state lock? A separate ordered event queue or single notification dispatcher might be possible approaches.

[wborn]

Could we add deterministic concurrency tests for the behavior this PR is intended to fix?

The current test changes update the field name from lazyRetryThread to retryThread, but the tests still execute the provider operations sequentially and wait using Thread.sleep(). They therefore confirm that the normal retry path still works, but they do not exercise the concurrent access that caused the original ConcurrentModificationException.

They also would not detect the notification-ordering race described in the provider comment. A test could, for example:

1. Use latches in a listener or test handler factory to pause the retry thread after it has updated the provider state but before its listener notification is completed.
2. Perform a newer update or removal from another thread.
3. Allow the newer notification to complete first.
4. Release the older retry notification.
5. Verify that both YamlThingProvider and the listener or registry still contain the newest state.

Using latches or barriers rather than timing-based sleeps should make the interleaving reproducible and keep the test reliable on slower CI systems.

[Nadahar]

I must admit that I don't remember anything of this situation now. But, I thought that the whole retry logic was there for handling issues during startup, as a way to "handle" the fact that things don't start in a well-orchestrated order, so that some waiting might be needed to avoid that things fail to register "randomly"?

If that's the situation, then it might be "defendable" to not expect the rest of the system to behave normally at the time when the retry logic might actually be activated.

I still have a "feeling" that there are a general potential problem with creating all these different element types from YAML files, some of which might depend on others, or even others from other files. I've raised the concern before, but I'm not able to say if it's a "real" problem or not. I guess we'll learn as people take this into use. But, if my "feeling" is warranted, there might be a general need for a parse ⇒ queue ⇒ register logic for all YAML created elements, not just Things. If so, a general solution that could be utilized by any element type might be the best solution.

When it comes to event notification order, there's always a risk that the order might be off when moving from synchronous to asynchronous delivery. Whether that is an "actual problem" or just a hypothetical one is hard to know, which also affects how much work it's sensible to put into managing the order. In a way, synchronous - asynchronous is more of a scale than a binary choice. When you enforce order, you move "further towards synchronous" on the scale, which can also counter some of the benefits of doing it asynchronously, that things can move swiftly and without being "bogged down" by single events that get stuck for a while for some reason. It depends to some degree on the assumptions made by the recipient as well, and how the event information is structured. I'm not saying that's a good idea, but one could, in theory, just embed a (nano) timestamp with every event, and say that it's the responsibility of the recipient to figure it out, if the order matters.

The easiest route is often to let the recipients live on in the belief that events arrive in order, so that they can keep functioning inside a "synchronous illusion". But, my impression is that many asynchronous systems don't make that guarantee, it just happens to be so that events usually arrive "in the order in which they occurred" anyway. If it's determined that the right solution is to "shield the recipients" from the true chaos asynchronous behavior, a queue system is probably the best solution. Perhaps some of the logic from #5708 can be reused.

But, whatever solution is chosen, there are usually new "edge cases" that must either be ignored or handled. If a queue is used for example, should it be unbound? How long should events be allowed to be stuck in the queue before it's considered "a problem"? How do we monitor this? Can new queue entries be rejected? If so, how to we handle that. I'm not saying that all of this needs to be handled, in many situations, such occurrences are rare enough or unlikely enough that it doesn't have to be handled. I'm just trying to point out that you can rarely "get completely out of the jam", most solutions just make the problem manifest somewhere else or in some other form. So, just like with whether or not out-of-sync events is or isn't a problem, there needs to be some level of judgment regarding which situations are "real problems" and not.

I never set out to solve all this when I created this PR, so it shouldn't be seen as an attempt to solve it all. My goal was just to make it work a little bit better than it currently does, hopefully "better enough" that it never manifests as a real problem.