Resources: closing outputstream bug, recursive delete

geoserver · Aug 6, 2015 · 5860960 · 5860960
1 parent 9592882
commit 5860960
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 10 deletions.
diff --git a/src/platform/src/main/java/org/geoserver/platform/resource/FileSystemResourceStore.java b/src/platform/src/main/java/org/geoserver/platform/resource/FileSystemResourceStore.java
@@ -228,13 +228,16 @@ public OutputStream out() {
                     @Override
                     public void close() throws IOException {
                         delegate.close();
-                        Lock lock = lock();
-                        try {
-                            // no errors, overwrite the original file
-                            Files.move(temp, actualFile);
-                        }
-                        finally {
-                            lock.release();
+                        //if already closed, there should be no exception (see spec Closeable)
+                        if (temp.exists()) {
+                            Lock lock = lock();
+                            try {
+                                // no errors, overwrite the original file
+                                Files.move(temp, actualFile);
+                            }
+                            finally {
+                                lock.release();
+                            }
                         }
                     }
 
@@ -429,7 +432,10 @@ public String toString() {
 
         @Override
         public boolean delete() {
-            return file.delete();
+            if (!file.exists()) {
+                return false;
+            }
+            return Files.delete(file);
         }
 
         @Override

diff --git a/src/platform/src/main/java/org/geoserver/platform/resource/Files.java b/src/platform/src/main/java/org/geoserver/platform/resource/Files.java
@@ -103,9 +103,12 @@ public OutputStream out() {
                     FileOutputStream delegate = new FileOutputStream(temp);
 
                     @Override
-                    public void close() throws IOException {
+                    public void close() throws IOException {                        
                         delegate.close();
-                        Files.move(temp, file);
+                        //if already closed, there should be no exception (see spec Closeable)
+                        if (temp.exists()) {
+                            Files.move(temp, file);
+                        }
                     }
 
                     @Override

diff --git a/...platform/src/test/java/org/geoserver/platform/resource/FileWrapperResourceTheoryTest.java b/...platform/src/test/java/org/geoserver/platform/resource/FileWrapperResourceTheoryTest.java
@@ -111,6 +111,12 @@ public void theoryAddingFileToDirectoryAddsResource(String path)
             throws Exception {
 
     }
+
+    @Override @Ignore
+    public void theoryRecursiveDelete(String path)
+            throws Exception {
+
+    }
 
     // paths for file wrapper are special so ignore this test
     @Override @Ignore

diff --git a/src/platform/src/test/java/org/geoserver/platform/resource/ResourceTheoryTest.java b/src/platform/src/test/java/org/geoserver/platform/resource/ResourceTheoryTest.java
@@ -486,4 +486,27 @@ public void theoryMultipleOutputStreamsAreSafe(String path) throws Exception {
         assertThat(resultContent, anyOf(equalTo(thread1Content), equalTo(thread2Content)));
 
     }
+
+    @Theory
+    public void theoryDoubleClose(String path) throws Exception {
+        final Resource res = getResource(path);
+        assumeThat(res, is(resource()));
+
+        OutputStream os = res.out();
+        os.close();
+        os.close();
+    }
+
+    @Theory
+    public void theoryRecursiveDelete(String path) throws Exception {
+        final Resource res = getResource(path);
+        assumeThat(res, is(directory()));
+        assumeThat(res, is(directory()));
+
+        Collection<Resource> result = res.list();        
+        assumeThat(result.size(), greaterThan(0));
+
+        assertTrue(res.delete());
+
+    }
 }