JCRVLT-851: Remove package tmp files through closewatcher

vault-core/src/main/java/org/apache/jackrabbit/vault/fs/io/Archive.java

@@ -115,7 +115,7 @@ public interface Archive extends Closeable {
     Archive getSubArchive(@NotNull String root, boolean asJcrRoot) throws IOException;
 
     /**
-     * Closes the archive. Only necessary to call if the archive has been opened.
+     * Closes the archive. Only necessary to call if the archive has been opened or it is based on a tmp file which is supposed to be deleted.
      */
     void close();
 

vault-core/src/main/java/org/apache/jackrabbit/vault/fs/io/ZipArchive.java

@@ -18,6 +18,7 @@
  */
 package org.apache.jackrabbit.vault.fs.io;
 
+import java.io.Closeable;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
@@ -98,6 +99,10 @@ public ZipArchive(@NotNull File zipFile) {
     public ZipArchive(@NotNull File zipFile, boolean isTempFile) {
         this.file = zipFile;
         this.isTempFile = isTempFile;
+        if (isTempFile) {
+            watcher = CloseWatcher.register(this, new Closer(file, null), SHOULD_CREATE_STACK_TRACE);
+        }
+        dumpUnclosedArchives();
     }
 
     @Override
@@ -106,6 +111,11 @@ public void open(boolean strict) throws IOException {
             return;
         }
         jar = new JarFile(file);
+        if (watcher != null) {
+            CloseWatcher.unregister(watcher);
+        }
+        watcher =
+                CloseWatcher.register(this, new Closer(isTempFile ? this.file : null, jar), SHOULD_CREATE_STACK_TRACE);
         root = new EntryImpl("", true);
         inf = new DefaultMetaInf();
 
@@ -153,8 +163,6 @@ public void open(boolean strict) throws IOException {
         if (inf.getNodeTypes().isEmpty()) {
             log.debug("Zip {} does not contain nodetypes.", file.getPath());
         }
-        dumpUnclosedArchives();
-        watcher = CloseWatcher.register(this, jar, SHOULD_CREATE_STACK_TRACE);
     }
 
     @Override
@@ -219,19 +227,45 @@ public long getLastModified() {
 
     @Override
     public void close() {
-        try {
-            if (jar != null) {
-                jar.close();
-                jar = null;
-                if (watcher != null) {
-                    CloseWatcher.unregister(watcher);
-                }
+        if (watcher != null) {
+            try {
+                watcher.getCloseable().close();
+            } catch (Exception e) {
+                // should not happen
             }
-            if (file != null && isTempFile) {
-                FileUtils.deleteQuietly(file);
+            CloseWatcher.unregister(watcher);
+            watcher = null;
+            jar = null;
+        }
+    }
+
+    /**
+     * This class is used to close the {@link JarFile} and delete the zip file if requested.
+     * Needs to be a separate class to avoid a circular reference between the ZipArchive and the CloseWatcher.
+     */
+    private static final class Closer implements Closeable {
+
+        private final File fileToDelete;
+
+        private final JarFile jarFile;
+
+        public Closer(File fileToDelete, JarFile jarFile) {
+            this.fileToDelete = fileToDelete;
+            this.jarFile = jarFile;
+        }
+
+        @Override
+        public void close() {
+            try {
+                if (jarFile != null) {
+                    jarFile.close();
+                }
+                if (fileToDelete != null) {
+                    FileUtils.deleteQuietly(fileToDelete);
+                }
+            } catch (IOException e) {
+                log.warn("Error during close.", e);
             }
-        } catch (IOException e) {
-            log.warn("Error during close.", e);
         }
     }
 

vault-core/src/main/java/org/apache/jackrabbit/vault/fs/io/ZipNioArchive.java

@@ -18,6 +18,7 @@
  */
 package org.apache.jackrabbit.vault.fs.io;
 
+import java.io.Closeable;
 import java.io.IOException;
 import java.io.InputStream;
 import java.nio.file.FileSystem;
@@ -90,6 +91,10 @@ public ZipNioArchive(Path path, boolean deleteAtClose) {
         this.path = path;
         zipFileSystem = null;
         this.deleteAtClose = deleteAtClose;
+        if (deleteAtClose) {
+            watcher = CloseWatcher.register(this, new Closer(this.path, null), SHOULD_CREATE_STACK_TRACE);
+        }
+        dumpUnclosedArchives();
     }
 
     @Override
@@ -103,8 +108,11 @@ public void open(boolean strict) throws IOException {
         } catch (ProviderNotFoundException e) {
             throw new IOException("Can not open zip file '" + path + "'", e);
         }
-        dumpUnclosedArchives();
-        watcher = CloseWatcher.register(this, zipFileSystem, SHOULD_CREATE_STACK_TRACE);
+        if (watcher != null) {
+            CloseWatcher.unregister(watcher);
+        }
+        watcher = CloseWatcher.register(
+                this, new Closer(deleteAtClose ? this.path : null, zipFileSystem), SHOULD_CREATE_STACK_TRACE);
     }
 
     @Override
@@ -199,21 +207,48 @@ private static String getSystemId(Path zipPath, Path pathInZip) {
 
     @Override
     public void close() {
-        if (zipFileSystem != null) {
-            if (watcher != null) {
-                CloseWatcher.unregister(watcher);
-            }
+        if (watcher != null) {
             try {
-                zipFileSystem.close();
-            } catch (IOException e) {
-                log.warn("Error during close.", e);
+                watcher.getCloseable().close();
+            } catch (Exception e) {
+                // should not happen
             }
+            CloseWatcher.unregister(watcher);
+            watcher = null;
+            zipFileSystem = null;
         }
-        if (deleteAtClose) {
-            try {
-                Files.delete(path);
-            } catch (IOException e) {
-                log.warn("Could not delete " + path, e);
+    }
+
+    /**
+     * This class is used to close the zip file system and delete the zip file if requested.
+     * Needs to be a separate class to avoid a circular reference between the ZipNioArchive and the CloseWatcher.
+     */
+    private static final class Closer implements Closeable {
+
+        private final Path pathToDelete;
+
+        private final FileSystem zipFileSystem;
+
+        public Closer(Path pathToDelete, FileSystem zipFileSystem) {
+            this.pathToDelete = pathToDelete;
+            this.zipFileSystem = zipFileSystem;
+        }
+
+        @Override
+        public void close() {
+            if (zipFileSystem != null) {
+                try {
+                    zipFileSystem.close();
+                } catch (IOException e) {
+                    log.warn("Error during close.", e);
+                }
+            }
+            if (pathToDelete != null) {
+                try {
+                    Files.delete(pathToDelete);
+                } catch (IOException e) {
+                    log.warn("Could not delete " + pathToDelete, e);
+                }
             }
         }
     }

vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/JcrPackageImpl.java

@@ -322,10 +322,10 @@
                 }
             } else {
                 File tmpFile = File.createTempFile("vaultpack", ".zip");
-                // make sure the temp file is removed at the latest when the JVM terminates,
-                // in case it is never explicitly closed via ZipArchive.close()/ZipVaultPackage.close()
-                tmpFile.deleteOnExit();
                 try {
+                    // used as last resort, usually tmpFile is deleted via ZipVaultPackage.close() or the enclosed
+                    // CloseWatcher
+                    tmpFile.deleteOnExit();
                     Binary bin = getData().getBinary();
                     try (FileOutputStream out = FileUtils.openOutputStream(tmpFile);
                             InputStream in = bin.getStream()) {
@@ -334,8 +334,11 @@
                         bin.dispose();
                     }
                     pack = new ZipVaultPackage(tmpFile, true);
-                } catch (RepositoryException | IOException | RuntimeException e) {
-                    FileUtils.deleteQuietly(tmpFile);
+                } catch (IOException e) {
+                    tmpFile.delete();
+                    throw e;
+                } catch (RepositoryException e) {
+                    tmpFile.delete();
                     throw e;
                 }
             }

vault-core/src/test/java/org/apache/jackrabbit/vault/fs/io/ZipArchiveCloseTest.java

@@ -22,6 +22,8 @@
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.net.URISyntaxException;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipOutputStream;
@@ -32,7 +34,9 @@
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 
 /**
  *  Test to demonstrate JCRVLT-838.
@@ -177,4 +181,37 @@
         archive.close();
         archive.close(); // Multiple closes also safe
     }
+
+    @Test
+    public void testDumpUnclosedArchivesClosesTmpFile() throws IOException, InterruptedException, URISyntaxException {
+        Path tmpFile = createTempPackage();
+        new ZipArchive(tmpFile.toFile(), true);
+        System.gc();
+        Thread.sleep(100);
+        // Now dump unclosed archives
+        assertTrue("Couldn't find unclosed archives", AbstractArchive.dumpUnclosedArchives());
+        assertFalse("Temp file should have been deleted but still exists", Files.exists(tmpFile));
+    }
+
+    @Test
+    public void testDumpUnclosedArchivesClosesTmpFileAfterOpen()
+            throws IOException, InterruptedException, URISyntaxException {
+        Path tmpFile = createTempPackage();
+        new ZipArchive(tmpFile.toFile(), true).open(true);
+        System.gc();
+        Thread.sleep(100);
+        // Now dump unclosed archives
+        assertTrue("Couldn't find unclosed archives", AbstractArchive.dumpUnclosedArchives());
+        assertFalse("Temp file should have been deleted but still exists", Files.exists(tmpFile));
+    }
+
+    private Path createTempPackage() throws URISyntaxException, IOException {
+        Path zipFile = Paths.get(ZipArchiveCloseTest.class
+                .getResource("/test-packages/atomic-counter-test.zip")
+                .toURI());
+        // copy to tmpFile
+        Path tmpFile = tempFolder.newFile().toPath();
+        Files.copy(zipFile, tmpFile, java.nio.file.StandardCopyOption.REPLACE_EXISTING);
+        return tmpFile;
+    }
 }

vault-core/src/test/java/org/apache/jackrabbit/vault/fs/io/ZipNioArchiveTest.java

@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.vault.fs.io;
+
+import java.io.IOException;
+import java.net.URISyntaxException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import static org.junit.Assert.*;
+
+/**
+ * Most tests in {@link ArchiveTest} are also applicable to {@link ZipNioArchive}.
+ * This class contains additional tests specific to {@link ZipNioArchive}.
+ */
+public class ZipNioArchiveTest {
+
+    @Rule
+    public TemporaryFolder tempFolder = new TemporaryFolder();
+
+    @Test
+    public void testDumpUnclosedArchivesClosesTmpFile() throws IOException, InterruptedException, URISyntaxException {
+        Path tmpFile = createTempPackage();
+        new ZipNioArchive(tmpFile, true);
+        System.gc();
+        Thread.sleep(100);
+        // Now dump unclosed archives
+        assertTrue("Couldn't find unclosed archives", AbstractArchive.dumpUnclosedArchives());
+        assertFalse("Temp file should have been deleted but still exists", Files.exists(tmpFile));
+    }
+
+    @Test
+    public void testDumpUnclosedArchivesClosesTmpFileAfterOpen()
+            throws IOException, InterruptedException, URISyntaxException {
+        Path tmpFile = createTempPackage();
+        new ZipNioArchive(tmpFile, true).open(true);
+        System.gc();
+        Thread.sleep(100);
+        // Now dump unclosed archives
+        assertTrue("Couldn't find unclosed archives", AbstractArchive.dumpUnclosedArchives());
+        assertFalse("Temp file should have been deleted but still exists", Files.exists(tmpFile));
+    }
+
+    private Path createTempPackage() throws URISyntaxException, IOException {
+        Path zipFile = Paths.get(ZipNioArchiveTest.class
+                .getResource("/test-packages/atomic-counter-test.zip")
+                .toURI());
+        // copy to tmpFile
+        Path tmpFile = tempFolder.newFile().toPath();
+        Files.copy(zipFile, tmpFile, java.nio.file.StandardCopyOption.REPLACE_EXISTING);
+        return tmpFile;
+    }
+}