paulrutter commented on PR #233: URL: https://github.com/apache/felix-dev/pull/233#issuecomment-6824359123
Thanks for the fix, the approach (serialize first, compare, write under the write lock) looks right and the two tests cover the new behaviour well. A few findings, roughly by importance: 1. **Read-modify-write is still partly outside the lock.** `file` is read and merged into `props` (the `props.load(r)` / "removed properties" logic) *before* `writeLock().lockInterruptibly()`. If `DirectoryWatcher` processes a user edit of the file between that read and the lock acquisition, the content computed from the stale read is compared against and then written over the newer file. Consider taking the write lock before loading the file, so load, merge, compare and write are one critical section (the serialization is cheap). 2. **Possible self-deadlock if the event is delivered on a watcher thread.** `DirectoryWatcher` holds `lock.readLock()` while it installs/updates configs, and a `ReentrantReadWriteLock` cannot upgrade read to write. Felix ConfigAdmin delivers `ConfigurationEvent`s asynchronously so this is fine there, but a ConfigAdmin implementation that dispatches synchronously on the calling thread would hang here instead of just logging. Worth a comment, or `tryLock` with a timeout. 3. **Interrupt is swallowed.** `lockInterruptibly()` throws `InterruptedException`, which is caught by the generic `catch (Exception e)` and logged as "Unable to save configuration" without restoring the interrupt flag. Please add `Thread.currentThread().interrupt()` for that case. 4. **`Files.readAllBytes` can now fail where the old code did not.** If the file is deleted between the `file.isFile()` check and the read, this throws `NoSuchFileException` (logged as an error), whereas the old code would have recreated it via `FileOutputStream`. Treating a missing file as "changed" would keep the old behaviour. 5. **The truncate-then-write window still exists for readers outside `fileInstall.lock`** (other processes, editors, backup tools). Writing to a temp file in the same directory and `Files.move(..., ATOMIC_MOVE, REPLACE_EXISTING)` would remove the empty-file state entirely, and would make the lock less critical. Not a blocker for this PR. 6. Nit: the test file adds two trailing blank lines before `testSetConfiguration`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
