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 020ee0bb52..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; @@ -288,13 +292,35 @@ public void doConfigurationEvent(ConfigurationEvent configurationEvent) { props.remove(key); } - try (Writer fw = new OutputStreamWriter(new FileOutputStream(file), encoding())) + + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + try (Writer fw = new OutputStreamWriter(baos, 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); + 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 + { + // do not touch the file (and its lastModified) if there is nothing to change + if (!Arrays.equals(newContent, Files.readAllBytes(file.toPath()))) + { + 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); + } + } + finally + { + fileInstall.lock.writeLock().unlock(); + } } } catch (Exception e) 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..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 @@ -27,6 +27,7 @@ import java.util.Dictionary; import java.util.Hashtable; import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.locks.ReentrantReadWriteLock; import junit.framework.TestCase; import org.easymock.Capture; @@ -435,6 +436,108 @@ 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<>(); + + 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 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("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();