[YAML thing provider] Clean retry queue when a thing is updated/removed#5361
[YAML thing provider] Clean retry queue when a thing is updated/removed#5361lolodomo wants to merge 1 commit intoopenhab:mainfrom
Conversation
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
lolodomo
force-pushed
the
yaml_thing_clean_queue
branch
from
9983375 to
75b0aa3
Compare
lolodomo
changed the title
|
Put the PR in draft mode to do more tests. |
There was a problem hiding this comment.
Pull request overview
This pull request enhances the YamlThingProvider to properly manage the retry queue when things are updated or removed from YAML configuration models. The changes prevent stale entries from accumulating in the retry queue by cleaning them up when things are updated or removed, and adds synchronization to ensure thread-safe access to the queue.
Changes:
- Added synchronization (queueLock) to all retry queue operations to prevent race conditions
- Implemented removeFromRetryQueue method to clean up entries when things are updated or removed
- Integrated queue cleanup into updatedModel and removedModel methods for non-isolated models
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (QueueContent qc : queue) { | ||
| if (thingUID.equals(qc.thingUID)) { | ||
| queue.remove(qc); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new removeFromRetryQueue method and its integration with updatedModel/removedModel lack test coverage. Consider adding tests to verify: 1) items are correctly removed from the retry queue when a thing is updated/removed, 2) the method handles concurrent access correctly, 3) the cleanup happens only for non-isolated models. This is especially important given the concurrency aspects of this code.
| for (QueueContent qc : queue) { | |
| if (thingUID.equals(qc.thingUID)) { | |
| queue.remove(qc); | |
| } | |
| } | |
| queue.removeIf(qc -> thingUID.equals(qc.thingUID)); |
| for (QueueContent qc : queue) { | ||
| if (thingUID.equals(qc.thingUID)) { | ||
| queue.remove(qc); | ||
| } | ||
| } |
There was a problem hiding this comment.
The removeFromRetryQueue method continues iterating through all queue elements even after finding and removing a match. This is inefficient and could also remove multiple entries if duplicate thingUID entries exist in the queue. Consider adding a break statement after queue.remove(qc) to exit the loop once a match is found, or use queue.removeIf(qc -> thingUID.equals(qc.thingUID)) which is more efficient and idiomatic.
| for (QueueContent qc : queue) { | |
| if (thingUID.equals(qc.thingUID)) { | |
| queue.remove(qc); | |
| } | |
| } | |
| queue.removeIf(qc -> thingUID.equals(qc.thingUID)); |
| @Override | ||
| public void run() { | ||
| logger.debug("Starting lazy retry thread"); | ||
| while (!queue.isEmpty()) { |
There was a problem hiding this comment.
The queue.isEmpty() check at line 112 is performed outside the synchronized block, which could lead to a race condition. If the queue becomes empty between the check and entering the synchronized block, or if items are added/removed by other threads, the loop might not behave as expected. Consider moving the isEmpty() check inside the synchronized block or restructuring the loop to check emptiness within the synchronized section.
| } | ||
| } | ||
| } | ||
| if (!queue.isEmpty()) { |
There was a problem hiding this comment.
Similar to line 112, this queue.isEmpty() check is performed outside the synchronized block, which could lead to a race condition. The queue state might change between this check and the next iteration of the while loop at line 112. Consider performing this check within a synchronized block for consistency with the rest of the queue operations.
1 participant
No description provided.