Copilot commented on code in PR #434:
URL:
https://github.com/apache/jackrabbit-filevault/pull/434#discussion_r4124304414
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/JcrPackageImpl.java:
##########
@@ -334,8 +334,11 @@ protected VaultPackage getPackage(boolean
forceFileArchive) throws RepositoryExc
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;
}
Review Comment:
`tmpFile.delete()` ignores the return value and failures leave the temp file
behind. Also, the previous implementation cleaned up on `RuntimeException`, but
the new one no longer does—any unchecked exception during stream copy/IO could
leak the temp file. Consider using a `finally` cleanup guarded by whether
`pack` was successfully created, and use a more reliable deletion approach
(e.g., `Files.deleteIfExists(...)` with logging on failure or
`FileUtils.deleteQuietly(...)`) to avoid leaks.
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/io/ZipArchive.java:
##########
@@ -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;
+ }
Review Comment:
Similar to `ZipNioArchive.close()`: swallowing all exceptions without
logging makes close failures invisible and harder to debug. Logging at `warn`
level would preserve observability without changing control flow.
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/io/ZipNioArchive.java:
##########
@@ -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;
}
Review Comment:
The `catch (Exception e)` silently swallows unexpected failures during
close, which makes diagnosing IO/resource issues difficult. At minimum, log at
`warn` level (as the prior implementation did) so operational issues (e.g.,
failure to close FS / delete temp file) aren’t hidden.
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/JcrPackageImpl.java:
##########
@@ -322,10 +322,10 @@ protected VaultPackage getPackage(boolean
forceFileArchive) throws RepositoryExc
}
} 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();
Review Comment:
The PR removes `JcrPackageImplTempFileTest`, but the updated
`JcrPackageImpl` still has important temp-file cleanup behavior (especially on
exceptions during package creation). The new archive tests validate
`dumpUnclosedArchives()` behavior, but they don’t cover the original regression
scenario (copying the JCR binary fails before `ZipVaultPackage` is
constructed). Consider reintroducing a targeted test for
`JcrPackageImpl#getPackage(...)` failure cleanup to prevent regressions in the
packaging layer.
##########
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));
+ }
Review Comment:
These tests are likely to be flaky: `System.gc()` is not guaranteed, and a
fixed `Thread.sleep(100)` can be too short on slower CI machines. A more
reliable pattern is to clear strong references (assign the archive variable to
`null`), then poll with a timeout (e.g., repeatedly call
`dumpUnclosedArchives()` and check `Files.exists(tmpFile)` until it becomes
false or a reasonable time limit is reached).
##########
vault-core/src/test/java/org/apache/jackrabbit/vault/fs/io/ZipArchiveCloseTest.java:
##########
@@ -177,4 +181,37 @@ public void testCloseWithoutOpenIsSafe() throws
URISyntaxException {
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));
+ }
Review Comment:
Same flakiness concern as in `ZipNioArchiveTest`: relying on `System.gc()`
and a fixed sleep can cause intermittent failures. Prefer a bounded
retry/eventual-consistency approach (poll until deletion occurs or timeout) so
CI timing variance doesn’t break the build.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
