From 53ac8b61e3d51f7800bfad9409719c662262c532 Mon Sep 17 00:00:00 2001 From: Felix Marx Date: Wed, 30 Aug 2023 11:06:24 +0200 Subject: [PATCH 1/3] FELIX-6515: Add global write lock to write back cfg files * 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. --- .../fileinstall/internal/ConfigInstaller.java | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java b/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java index cf0a4b539f..dabd504c35 100644 --- a/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java +++ b/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java @@ -282,13 +282,24 @@ public void doConfigurationEvent(ConfigurationEvent configurationEvent) { props.remove(key); } - try (Writer fw = new OutputStreamWriter(new FileOutputStream(file), encoding())) + + // re-writing a config file must be guarded by a write lock as otherwise DirectoryWatcher + // might pick up an empty file and as a result, write this file to disk in a follow-up event + fileInstall.lock.writeLock().lockInterruptibly(); + try + { + try (Writer fw = new OutputStreamWriter(new FileOutputStream(file), encoding())) + { + props.save( fw ); + } + // we're just writing out what's already loaded into ConfigAdmin, so + // update file checksum since lastModified gets updated when writing + fileInstall.updateChecksum(file); + } + finally { - props.save( fw ); + fileInstall.lock.writeLock().unlock(); } - // we're just writing out what's already loaded into ConfigAdmin, so - // update file checksum since lastModified gets updated when writing - fileInstall.updateChecksum(file); } } catch (Exception e) From 9ecb4231beaef9abfe9f7160ba0350291bc4f8f2 Mon Sep 17 00:00:00 2001 From: Felix Marx Date: Thu, 12 Nov 2026 08:20:58 +0100 Subject: [PATCH 2/3] FELIX-6515 - Test that fileinstall rewrites config files under the write 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]>. --- .../internal/ConfigInstallerTest.java | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java b/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java index f423379041..b5250585f8 100644 --- a/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java +++ b/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java @@ -23,10 +23,14 @@ import java.io.OutputStream; import java.nio.file.Files; import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Dictionary; import java.util.Hashtable; +import java.util.List; import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.locks.ReentrantReadWriteLock; import junit.framework.TestCase; import org.easymock.Capture; @@ -435,6 +439,74 @@ public Boolean answer() throws Throwable { assertEquals("networkInterface = \"wlp3s1\"", Files.readAllLines( Paths.get(file.toURI()) ).get(0)); } + /** + * FELIX-6515: rewriting a config file truncates it before filling it. The write must be guarded by the + * global write lock so that a DirectoryWatcher (which holds the read lock while processing) can never + * observe, and subsequently write back, an empty file. + */ + public void testDoConfigurationEventRewritesFileUnderWriteLock() throws Exception + { + final File file = File.createTempFile("test", ".config"); + try (OutputStream os = new FileOutputStream(file)) { + os.write("networkInterface = \"wlp3s1\"\n".getBytes("UTF-8")); + } + + final String pid = file.getName().substring(0, file.getName().indexOf(".config")); + + final ServiceReference sr = EasyMock.createMock(ServiceReference.class); + + final AtomicReference lockRef = new AtomicReference<>(); + // write lock state each time the encoding is looked up: first when loading the file (before the + // lock is taken), then while the file is being rewritten + final List lockedOnEncodingLookup = new ArrayList<>(); + + EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes(); + EasyMock.expect(mockBundle.loadClass(ConfigurationAttribute.class.getName())).andReturn((Class)ConfigurationAttribute.class).anyTimes(); + EasyMock.expect(mockBundleContext.getProperty((String) EasyMock.anyObject())) + .andAnswer(new IAnswer() { + @Override + public String answer() throws Throwable { + if (DirectoryWatcher.CONFIG_ENCODING.equals(EasyMock.getCurrentArguments()[0])) { + lockedOnEncodingLookup.add(lockRef.get().isWriteLockedByCurrentThread()); + } + return null; + } + }) + .anyTimes(); + + final Dictionary configProps = new Hashtable() { + { + put("networkInterface", "wlp3s0"); + put(DirectoryWatcher.FILENAME, file.toURI().toString()); + } + }; + EasyMock.expect(mockConfiguration.getProperties()).andReturn(configProps).anyTimes(); + EasyMock.expect(mockConfiguration.getPid()).andReturn(pid).anyTimes(); + EasyMock.expect(mockConfigurationAdmin.listConfigurations("(service.pid=" + pid + ")")) + .andReturn(new Configuration[] { mockConfiguration }); + + EasyMock.replay(mockBundleContext, mockConfigurationAdmin, mockBundle, mockConfiguration, sr); + + final AtomicReference lockedOnChecksumUpdate = new AtomicReference<>(); + final FileInstall fileInstall = new FileInstall() { + @Override + public void updateChecksum(File f) { + lockedOnChecksumUpdate.set(lockRef.get().isWriteLockedByCurrentThread()); + } + }; + final ReentrantReadWriteLock lock = (ReentrantReadWriteLock) fileInstall.lock; + lockRef.set(lock); + final ConfigInstaller ci = new ConfigInstaller( mockBundleContext, mockConfigurationAdmin, fileInstall ); + + ci.doConfigurationEvent( new ConfigurationEvent(sr , ConfigurationEvent.CM_UPDATED, null, pid ) ); + + assertEquals("networkInterface = \"wlp3s0\"", Files.readAllLines( Paths.get(file.toURI()) ).get(0)); + assertEquals("Encoding is read once to load and once to write the file", + Arrays.asList(Boolean.FALSE, Boolean.TRUE), lockedOnEncodingLookup); + assertEquals("Checksum must be updated before the write lock is released", Boolean.TRUE, lockedOnChecksumUpdate.get()); + assertFalse("Write lock must be released after write-back", lock.isWriteLocked()); + } + public void testSetConfiguration() throws Exception { EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes(); From c2a0e9c82cb110081a70bfe9b4fec866b1af2644 Mon Sep 17 00:00:00 2001 From: Felix Marx Date: Thu, 12 Nov 2026 08:45:56 +0100 Subject: [PATCH 3/3] FELIX-6515 - Do not rewrite unchanged config files and narrow the write 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. --- .../fileinstall/internal/ConfigInstaller.java | 25 +++++-- .../internal/ConfigInstallerTest.java | 69 ++++++++++++++----- 2 files changed, 70 insertions(+), 24 deletions(-) diff --git a/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java b/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java index d156780a8d..4c1b198d8a 100644 --- a/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java +++ b/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java @@ -19,12 +19,14 @@ package org.apache.felix.fileinstall.internal; import java.io.BufferedInputStream; +import java.io.ByteArrayOutputStream; import java.io.File; import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; +import java.io.OutputStream; import java.io.OutputStreamWriter; import java.io.Reader; import java.io.Writer; @@ -32,9 +34,11 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.net.URI; +import java.nio.file.Files; import java.security.AccessController; import java.security.PrivilegedAction; import java.util.ArrayList; +import java.util.Arrays; import java.util.Dictionary; import java.util.Enumeration; import java.util.HashMap; @@ -289,18 +293,29 @@ public void doConfigurationEvent(ConfigurationEvent configurationEvent) props.remove(key); } + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + try (Writer fw = new OutputStreamWriter(baos, encoding())) + { + props.save( fw ); + } + byte[] newContent = baos.toByteArray(); + // re-writing a config file must be guarded by a write lock as otherwise DirectoryWatcher // might pick up an empty file and as a result, write this file to disk in a follow-up event fileInstall.lock.writeLock().lockInterruptibly(); try { - try (Writer fw = new OutputStreamWriter(new FileOutputStream(file), encoding())) + // do not touch the file (and its lastModified) if there is nothing to change + if (!Arrays.equals(newContent, Files.readAllBytes(file.toPath()))) { - props.save( fw ); + try (OutputStream os = new FileOutputStream(file)) + { + os.write(newContent); + } + // we're just writing out what's already loaded into ConfigAdmin, so + // update file checksum since lastModified gets updated when writing + fileInstall.updateChecksum(file); } - // we're just writing out what's already loaded into ConfigAdmin, so - // update file checksum since lastModified gets updated when writing - fileInstall.updateChecksum(file); } finally { diff --git a/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java b/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java index b5250585f8..f3edf78e0d 100644 --- a/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java +++ b/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java @@ -23,12 +23,9 @@ import java.io.OutputStream; import java.nio.file.Files; import java.nio.file.Paths; -import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.Dictionary; import java.util.Hashtable; -import java.util.List; import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.locks.ReentrantReadWriteLock; @@ -456,23 +453,10 @@ public void testDoConfigurationEventRewritesFileUnderWriteLock() throws Exceptio final ServiceReference sr = EasyMock.createMock(ServiceReference.class); final AtomicReference lockRef = new AtomicReference<>(); - // write lock state each time the encoding is looked up: first when loading the file (before the - // lock is taken), then while the file is being rewritten - final List lockedOnEncodingLookup = new ArrayList<>(); EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes(); EasyMock.expect(mockBundle.loadClass(ConfigurationAttribute.class.getName())).andReturn((Class)ConfigurationAttribute.class).anyTimes(); - EasyMock.expect(mockBundleContext.getProperty((String) EasyMock.anyObject())) - .andAnswer(new IAnswer() { - @Override - public String answer() throws Throwable { - if (DirectoryWatcher.CONFIG_ENCODING.equals(EasyMock.getCurrentArguments()[0])) { - lockedOnEncodingLookup.add(lockRef.get().isWriteLockedByCurrentThread()); - } - return null; - } - }) - .anyTimes(); + EasyMock.expect(mockBundleContext.getProperty((String) EasyMock.anyObject())).andReturn(null).anyTimes(); final Dictionary configProps = new Hashtable() { { @@ -501,12 +485,59 @@ public void updateChecksum(File f) { ci.doConfigurationEvent( new ConfigurationEvent(sr , ConfigurationEvent.CM_UPDATED, null, pid ) ); assertEquals("networkInterface = \"wlp3s0\"", Files.readAllLines( Paths.get(file.toURI()) ).get(0)); - assertEquals("Encoding is read once to load and once to write the file", - Arrays.asList(Boolean.FALSE, Boolean.TRUE), lockedOnEncodingLookup); assertEquals("Checksum must be updated before the write lock is released", Boolean.TRUE, lockedOnChecksumUpdate.get()); assertFalse("Write lock must be released after write-back", lock.isWriteLocked()); } + /** + * FELIX-6515: a config file whose content would not change must not be touched at all. + */ + public void testDoConfigurationEventDoesNotRewriteUnchangedFile() throws Exception + { + final File file = File.createTempFile("test", ".config"); + try (OutputStream os = new FileOutputStream(file)) { + os.write("networkInterface = \"wlp3s0\"\n".getBytes("UTF-8")); + } + final long lastModified = 1_000_000_000L; + assertTrue(file.setLastModified(lastModified)); + + final String pid = file.getName().substring(0, file.getName().indexOf(".config")); + final ServiceReference sr = EasyMock.createMock(ServiceReference.class); + + EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes(); + EasyMock.expect(mockBundle.loadClass(ConfigurationAttribute.class.getName())).andReturn((Class)ConfigurationAttribute.class).anyTimes(); + EasyMock.expect(mockBundleContext.getProperty((String) EasyMock.anyObject())).andReturn(null).anyTimes(); + + final Dictionary configProps = new Hashtable() { + { + put("networkInterface", "wlp3s0"); + put(DirectoryWatcher.FILENAME, file.toURI().toString()); + } + }; + EasyMock.expect(mockConfiguration.getProperties()).andReturn(configProps).anyTimes(); + EasyMock.expect(mockConfiguration.getPid()).andReturn(pid).anyTimes(); + EasyMock.expect(mockConfigurationAdmin.listConfigurations("(service.pid=" + pid + ")")) + .andReturn(new Configuration[] { mockConfiguration }); + + EasyMock.replay(mockBundleContext, mockConfigurationAdmin, mockBundle, mockConfiguration, sr); + + final AtomicReference checksumUpdated = new AtomicReference<>(false); + final FileInstall fileInstall = new FileInstall() { + @Override + public void updateChecksum(File f) { + checksumUpdated.set(true); + } + }; + final ConfigInstaller ci = new ConfigInstaller( mockBundleContext, mockConfigurationAdmin, fileInstall ); + + ci.doConfigurationEvent( new ConfigurationEvent(sr , ConfigurationEvent.CM_UPDATED, null, pid ) ); + + assertEquals("Unchanged file must not be modified", lastModified, file.lastModified()); + assertFalse("Checksum must not be updated for an unchanged file", checksumUpdated.get()); + } + + + public void testSetConfiguration() throws Exception { EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes();