Repository navigation
FELIX-6515 Fix race condition when rewriting config files in ConfigInstaller - #233
FelixMarxIBM wants to merge 5 commits into
Conversation
* Writing a file is a 2 step process where the file is created empty and then filled * There is a race-condition, where the DirectoryWatcher might pick up the creation of the * empty file. This empty file will be written back to disk on a follow-up CM_UPDATED event. * Using the global write lock will block the DirectoryWatcher to wait for the write to finish * and therefore never to read an empty file.
|
I'll try to create a test for this race-condition but doing that might be hard to accomplish. |
…ite lock ConfigInstaller truncates a .cfg/.config file before filling it when writing a configuration back to disk. The fix for FELIX-6515 guards that rewrite, and the following checksum update, with the global FileInstall write lock so that a DirectoryWatcher, which processes files under the read lock, can never pick up the empty file and write it back on a follow-up CM_UPDATED event. Add ConfigInstallerTest.testDoConfigurationEventRewritesFileUnderWriteLock to cover that. The test is deterministic and does not rely on threads or sleeping: it runs doConfigurationEvent on the test thread and records whether the current thread holds the write lock when the encoding is looked up for loading the file (not held) and for writing it (held), and when the checksum is updated (held). It also checks the rewritten content and that the lock is released afterwards. Without the fix it fails with expected:<[false, true]> but was:<[false, false]>.
…te lock When ConfigInstaller writes a configuration back to its .cfg/.config file after a CM_UPDATED event, it used to truncate and rewrite the file every time, even if the content produced from ConfigAdmin was identical to what is already on disk. Besides needless I/O, this bumps lastModified, which forces a checksum update and gives the DirectoryWatcher a reason to look at the file again. Changes: - Serialize the properties into a byte array (using the same configured encoding() as is used for loading the file) before taking the lock, and compare it with the current file content. Only if the bytes differ the file is written and the checksum updated. If nothing would change, the file is not touched, so lastModified stays the same and no checksum update is needed, as the scanner's checksum (name, lastModified, length, writeable flag) remains valid. - Restrict the FileInstall write lock to the part that needs it: comparing, writing the file and updating the checksum. Building the byte array does not touch the file and is done outside the lock. The lock still guarantees that a DirectoryWatcher, which processes files under the read lock, cannot observe the truncated, empty file. Notes: - The comparison is byte exact. A file that does not round-trip through TypedProperties load/save (e.g. CRLF line endings, missing trailing newline) is rewritten once and is stable afterwards. Files that cannot be decoded with the configured encoding are rewritten as before. - If the file disappears before it is compared, reading it now fails and is logged as "Unable to save configuration" instead of silently recreating it. Tests: - Add testDoConfigurationEventDoesNotRewriteUnchangedFile, which verifies that lastModified is unchanged and updateChecksum is not called for an unchanged file. - Simplify testDoConfigurationEventRewritesFileUnderWriteLock: the encoding is now read outside of the lock, so drop the encoding lookup assertion and keep the checks that the checksum update happens under the write lock and that the lock is released afterwards.
FelixMarxIBM
changed the title
|
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:
|
Summary
This pull request addresses FELIX-6515, a race condition bug where rewriting a config file could result in it being temporarily truncated (empty) before being filled with new content. A
DirectoryWatcherholding a read lock could observe this empty intermediate state and subsequently write it back to disk, corrupting the configuration.The fix introduces two improvements:
fileInstall.lock.writeLock()) to preventDirectoryWatcherfrom observing an empty/partial file during the write operation.lastModifiedtimestamp and avoiding unnecessary checksum updates).Files Changed
📄
fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.javaRefactors the config file write-back logic in the configuration event handler:
ByteArrayOutputStreamfirst, rather than writing directly to the file, allowing content comparison before any disk I/O.Arrays.equalsandFiles.readAllBytes; skips the write entirely if the content is unchanged.writeLock().lockInterruptibly()/unlock()block to preventDirectoryWatcherfrom observing a partially written (empty) file during the operation.ByteArrayOutputStream,OutputStream,Files, andArrays.📄
fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.javaAdds two new test cases to validate the FELIX-6515 fix:
testDoConfigurationEventRewritesFileUnderWriteLock: Verifies that when a config file is rewritten with changed content, the write operation is performed while holding the write lock, and the lock is properly released afterward. Uses a customFileInstallsubclass to capture whether the write lock is held at the timeupdateChecksumis called.testDoConfigurationEventDoesNotRewriteUnchangedFile: Verifies that when the configuration content has not changed, the file'slastModifiedtimestamp is preserved (file is not touched) andupdateChecksumis not called. Uses a knownlastModifiedvalue set viafile.setLastModified()to assert immutability.