Make TextFileBasedViolationStore thread-safe under parallel test execution#1656
Make TextFileBasedViolationStore thread-safe under parallel test execution#1656kelunik wants to merge 3 commits into
Conversation
StefanGraeber
left a comment
StefanGraeber
left a comment
There was a problem hiding this comment.
Thanks for contributing 🙂
The code itself looks good, just one minor edge case in the case of a non-existing directory which remains as race condition.
to make the pipeline pass, you must include a Signed-off-by: footer to your commit
https://github.com/TNG/ArchUnit/pull/1656/checks?check_run_id=85373415790
|
you might have a merge conflict with #1655 which will be submitted soon. |
…ution Multiple FreezingArchRule instances each create a fresh TextFileBasedViolationStore and load stored.rules into their own private snapshot. Concurrent saves caused a lost-update: the last writer overwrote the file with only its own entries. Fix by sharing one FileSyncedProperties per stored.rules canonical path via a static ConcurrentHashMap, and synchronizing writes with a putIfAbsent method that atomically checks, sets, and flushes to disk under one lock. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Niklas Keller <niklas@chrono24.com>
- Move directory creation into FileSyncedProperties.initializePropertiesFile, removing the separate ensureExistence call from initialize() - Use mkdirs() || isDirectory() to handle the race where a concurrent process creates the directory between our mkdirs() call returning false and us checking - Extract concurrency tests into TextFileBasedViolationStoreConcurrencyTest, separate initialize-race and save-race scenarios, and call future.get() to surface exceptions that would otherwise be silently swallowed by the executor - Convert both test classes from JUnit 4 to JUnit 5 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Niklas Keller <niklas@chrono24.com>
7518e4c to
85656a5
Compare
Thanks for the heads up! It didn't have a conflict, but I rebased anyway and converted the test to JUnit 5.
True, I added a separate test for that. |
Signed-off-by: Niklas Keller <me@kelunik.com>
|
Oh, these old Java versions, should be fixed. |
2 participants
Multiple FreezingArchRule instances each create a fresh TextFileBasedViolationStore and load stored.rules into their own private snapshot. Concurrent saves caused a lost-update: the last writer overwrote the file with only its own entries.
Fix by sharing one FileSyncedProperties per stored.rules canonical path via a static ConcurrentHashMap, and synchronizing writes with a putIfAbsent method that atomically checks, sets, and flushes to disk under one lock.
Fixes #1654.