commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From scolebou...@apache.org
Subject svn commit: r385293 - in /jakarta/commons/proper/io/trunk: RELEASE-NOTES.txt src/java/org/apache/commons/io/output/LockableFileWriter.java src/test/org/apache/commons/io/output/LockableFileWriterTest.java xdocs/upgradeto1_2.xml
Date Sun, 12 Mar 2006 15:11:20 GMT
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: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message