Author: scolebourne Date: Sun Mar 12 07:11:19 2006 New Revision: 385293 URL: http://svn.apache.org/viewcvs?rev=385293&view=rev Log: LockableFileWriter locking mechanism was broken and only provided limited protection File deletion and locking in case of constructor error was broken bug 38942
Modified: jakarta/commons/proper/io/trunk/RELEASE-NOTES.txt jakarta/commons/proper/io/trunk/src/java/org/apache/commons/io/output/LockableFileWriter.java jakarta/commons/proper/io/trunk/src/test/org/apache/commons/io/output/LockableFileWriterTest.java jakarta/commons/proper/io/trunk/xdocs/upgradeto1_2.xml Modified: jakarta/commons/proper/io/trunk/RELEASE-NOTES.txt URL: http://svn.apache.org/viewcvs/jakarta/commons/proper/io/trunk/RELEASE-NOTES.txt?rev=385293&r1=385292&r2=385293&view=diff ============================================================================== --- jakarta/commons/proper/io/trunk/RELEASE-NOTES.txt (original) +++ jakarta/commons/proper/io/trunk/RELEASE-NOTES.txt Sun Mar 12 07:11:19 2006 @@ -36,6 +36,10 @@ - FileUtils.read* Increase certainty that files are closed in case of error +- LockableFileWriter + Locking mechanism was broken and only provided limited protection [38942] + File deletion and locking in case of constructor error was broken + Enhancements from 1.1 --------------------- Modified: jakarta/commons/proper/io/trunk/src/java/org/apache/commons/io/output/LockableFileWriter.java URL: http://svn.apache.org/viewcvs/jakarta/commons/proper/io/trunk/src/java/org/apache/commons/io/output/LockableFileWriter.java?rev=385293&r1=385292&r2=385293&view=diff ============================================================================== --- jakarta/commons/proper/io/trunk/src/java/org/apache/commons/io/output/LockableFileWriter.java (original) +++ jakarta/commons/proper/io/trunk/src/java/org/apache/commons/io/output/LockableFileWriter.java Sun Mar 12 07:11:19 2006 @@ -19,10 +19,12 @@ import java.io.FileOutputStream; import java.io.FileWriter; import java.io.IOException; +import java.io.OutputStream; import java.io.OutputStreamWriter; import java.io.Writer; import org.apache.commons.io.FileUtils; +import org.apache.commons.io.IOUtils; /** * FileWriter that will create and honor lock files to allow simple @@ -52,9 +54,9 @@ private static final String LCK = ".lck"; /** The writer to decorate. */ - private Writer out; + private final Writer out; /** The lock file. */ - private File lockFile; + private final File lockFile; /** * Constructs a LockableFileWriter. @@ -171,22 +173,13 @@ File lockDirFile = new File(lockDir); FileUtils.forceMkdir(lockDirFile); testLockDir(lockDirFile); - this.lockFile = new File(lockDirFile, file.getName() + LCK); + lockFile = new File(lockDirFile, file.getName() + LCK); + + // check if locked + createLock(); // init wrapped writer - try { - createLock(); - if (encoding == null) { - out = new FileWriter(file.getAbsolutePath(), append); - } else { - FileOutputStream fos = new FileOutputStream(file.getAbsolutePath(), append); - out = new OutputStreamWriter(fos, encoding); - } - } catch (IOException ioe) { - this.lockFile.delete(); - throw ioe; - } - this.out = new FileWriter(file.getAbsolutePath(), append); + out = initWriter(file, encoding, append); } //----------------------------------------------------------------------- @@ -223,6 +216,47 @@ } } + /** + * Initialise the wrapped file writer. + * Ensure that a cleanup occurs if the writer creation fails. + * + * @param file the file to be accessed + * @param encoding the encoding to use + * @param append true to append + * @throws IOException if an error occurs + */ + private Writer initWriter(File file, String encoding, boolean append) throws IOException { + boolean fileExistedAlready = file.exists(); + OutputStream stream = null; + Writer writer = null; + try { + if (encoding == null) { + writer = new FileWriter(file.getAbsolutePath(), append); + } else { + stream = new FileOutputStream(file.getAbsolutePath(), append); + writer = new OutputStreamWriter(stream, encoding); + } + } catch (IOException ex) { + IOUtils.closeQuietly(writer); + IOUtils.closeQuietly(stream); + lockFile.delete(); + if (fileExistedAlready == false) { + file.delete(); + } + throw ex; + } catch (RuntimeException ex) { + IOUtils.closeQuietly(writer); + IOUtils.closeQuietly(stream); + lockFile.delete(); + if (fileExistedAlready == false) { + file.delete(); + } + throw ex; + } + return writer; + } + + //----------------------------------------------------------------------- /** * Closes the file writer. * Modified: jakarta/commons/proper/io/trunk/src/test/org/apache/commons/io/output/LockableFileWriterTest.java URL: http://svn.apache.org/viewcvs/jakarta/commons/proper/io/trunk/src/test/org/apache/commons/io/output/LockableFileWriterTest.java?rev=385293&r1=385292&r2=385293&view=diff ============================================================================== --- jakarta/commons/proper/io/trunk/src/test/org/apache/commons/io/output/LockableFileWriterTest.java (original) +++ jakarta/commons/proper/io/trunk/src/test/org/apache/commons/io/output/LockableFileWriterTest.java Sun Mar 12 07:11:19 2006 @@ -17,8 +17,10 @@ import java.io.File; import java.io.IOException; +import java.io.Writer; -import junit.framework.TestCase; +import org.apache.commons.io.IOUtils; +import org.apache.commons.io.testtools.FileBasedTestCase; /** * Tests that files really lock, although no writing is done as @@ -27,80 +29,198 @@ * @author Henri Yandell (bayard at apache dot org) * @version $Revision$ $Date$ */ - -public class LockableFileWriterTest extends TestCase { +public class LockableFileWriterTest extends FileBasedTestCase { private File file; private File lockDir; private File lockFile; + private File altLockDir; + private File altLockFile; public LockableFileWriterTest(String name) { super(name); } public void setUp() { - file = new File("testlockfile"); + file = new File(getTestDirectory(), "testlockfile"); lockDir = new File(System.getProperty("java.io.tmpdir")); lockFile = new File(lockDir, file.getName() + ".lck"); + altLockDir = getTestDirectory(); + altLockFile = new File(altLockDir, file.getName() + ".lck"); } public void tearDown() { file.delete(); lockFile.delete(); + altLockFile.delete(); } //----------------------------------------------------------------------- public void testFileLocked() throws IOException { - LockableFileWriter lfw = new LockableFileWriter(this.file); + LockableFileWriter lfw1 = null; + LockableFileWriter lfw2 = null; + LockableFileWriter lfw3 = null; try { - new LockableFileWriter(this.file); - fail("Somehow able to open a locked file. "); - } catch(IOException ioe) { - String msg = ioe.getMessage(); - assertTrue( "Exception message does not start correctly. ", - msg.startsWith("Can't write file, lock ") ); + // open a valid locakable writer + lfw1 = new LockableFileWriter(file); + assertEquals(true, file.exists()); + assertEquals(true, lockFile.exists()); + + // try to open a second writer + try { + lfw2 = new LockableFileWriter(file); + fail("Somehow able to open a locked file. "); + } catch(IOException ioe) { + String msg = ioe.getMessage(); + assertTrue( "Exception message does not start correctly. ", + msg.startsWith("Can't write file, lock ") ); + assertEquals(true, file.exists()); + assertEquals(true, lockFile.exists()); + } + + // try to open a third writer + try { + lfw3 = new LockableFileWriter(file); + fail("Somehow able to open a locked file. "); + } catch(IOException ioe) { + String msg = ioe.getMessage(); + assertTrue( "Exception message does not start correctly. ", + msg.startsWith("Can't write file, lock ") ); + assertEquals(true, file.exists()); + assertEquals(true, lockFile.exists()); + } + } finally { - lfw.close(); + IOUtils.closeQuietly(lfw1); + IOUtils.closeQuietly(lfw2); + IOUtils.closeQuietly(lfw3); } + assertEquals(true, file.exists()); assertEquals(false, lockFile.exists()); } - public void testFileNotLocked() throws IOException { - LockableFileWriter lfw = new LockableFileWriter(this.file); - lfw.close(); + //----------------------------------------------------------------------- + public void testAlternateLockDir() throws IOException { + LockableFileWriter lfw1 = null; + LockableFileWriter lfw2 = null; try { - LockableFileWriter lfw2 = new LockableFileWriter(this.file); - lfw2.close(); - } catch(IOException ioe) { - String msg = ioe.getMessage(); - if( msg.startsWith("Can't write file, lock ") ) { - fail("Somehow unable to open a unlocked file. "); + // open a valid locakable writer + lfw1 = new LockableFileWriter(file, true, altLockDir.getAbsolutePath()); + assertEquals(true, file.exists()); + assertEquals(true, altLockFile.exists()); + + // try to open a second writer + try { + lfw2 = new LockableFileWriter(file, true, altLockDir.getAbsolutePath()); + fail("Somehow able to open a locked file. "); + } catch(IOException ioe) { + String msg = ioe.getMessage(); + assertTrue( "Exception message does not start correctly. ", + msg.startsWith("Can't write file, lock ") ); + assertEquals(true, file.exists()); + assertEquals(true, altLockFile.exists()); } + + } finally { + IOUtils.closeQuietly(lfw1); + IOUtils.closeQuietly(lfw2); + } + assertEquals(true, file.exists()); + assertEquals(false, altLockFile.exists()); + } + + //----------------------------------------------------------------------- + public void testFileNotLocked() throws IOException { + // open a valid locakable writer + LockableFileWriter lfw1 = null; + try { + lfw1 = new LockableFileWriter(file); + assertEquals(true, file.exists()); + assertEquals(true, lockFile.exists()); + } finally { + IOUtils.closeQuietly(lfw1); + } + assertEquals(true, file.exists()); + assertEquals(false, lockFile.exists()); + + // open a second valid writer on the same file + LockableFileWriter lfw2 = null; + try { + lfw2 = new LockableFileWriter(file); + assertEquals(true, file.exists()); + assertEquals(true, lockFile.exists()); + } finally { + IOUtils.closeQuietly(lfw2); } + assertEquals(true, file.exists()); assertEquals(false, lockFile.exists()); } + //----------------------------------------------------------------------- public void testConstructor_File_encoding_badEncoding() throws IOException { + Writer writer = null; try { - new LockableFileWriter(file, "BAD-ENCODE"); + writer = new LockableFileWriter(file, "BAD-ENCODE"); fail(); - } catch (IOException ex) {} + } catch (IOException ex) { + // expected + assertEquals(false, file.exists()); + assertEquals(false, lockFile.exists()); + } finally { + IOUtils.closeQuietly(writer); + } + assertEquals(false, file.exists()); assertEquals(false, lockFile.exists()); } + //----------------------------------------------------------------------- + public void testConstructor_File_directory() throws IOException { + Writer writer = null; + try { + writer = new LockableFileWriter(getTestDirectory()); + fail(); + } catch (IOException ex) { + // expected + assertEquals(false, file.exists()); + assertEquals(false, lockFile.exists()); + } finally { + IOUtils.closeQuietly(writer); + } + assertEquals(false, file.exists()); + assertEquals(false, lockFile.exists()); + } + + //----------------------------------------------------------------------- public void testConstructor_File_nullFile() throws IOException { + Writer writer = null; try { - new LockableFileWriter((File) null); + writer = new LockableFileWriter((File) null); fail(); - } catch (NullPointerException ex) {} + } catch (NullPointerException ex) { + // expected + assertEquals(false, file.exists()); + assertEquals(false, lockFile.exists()); + } finally { + IOUtils.closeQuietly(writer); + } + assertEquals(false, file.exists()); assertEquals(false, lockFile.exists()); } + //----------------------------------------------------------------------- public void testConstructor_fileName_nullFile() throws IOException { + Writer writer = null; try { - new LockableFileWriter((String) null); + writer = new LockableFileWriter((String) null); fail(); - } catch (NullPointerException ex) {} + } catch (NullPointerException ex) { + // expected + assertEquals(false, file.exists()); + assertEquals(false, lockFile.exists()); + } finally { + IOUtils.closeQuietly(writer); + } + assertEquals(false, file.exists()); assertEquals(false, lockFile.exists()); } Modified: jakarta/commons/proper/io/trunk/xdocs/upgradeto1_2.xml URL: http://svn.apache.org/viewcvs/jakarta/commons/proper/io/trunk/xdocs/upgradeto1_2.xml?rev=385293&r1=385292&r2=385293&view=diff ============================================================================== --- jakarta/commons/proper/io/trunk/xdocs/upgradeto1_2.xml (original) +++ jakarta/commons/proper/io/trunk/xdocs/upgradeto1_2.xml Sun Mar 12 07:11:19 2006 @@ -55,6 +55,10 @@ - FileUtils.read* Increase certainty that files are closed in case of error +- LockableFileWriter + Locking mechanism was broken and only provided limited protection [38942] + File deletion and locking in case of constructor error was broken + Enhancements from 1.1 --------------------- --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]