Copilot commented on code in PR #434:
URL:
https://github.com/apache/jackrabbit-filevault/pull/434#discussion_r4123810520
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/JcrPackageImpl.java:
##########
@@ -334,8 +333,8 @@ 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;
}
Review Comment:
The new error path only deletes `tmpFile` for `IOException`, but failures
here can also occur as `RepositoryException` (e.g., `getData()`,
`bin.getStream()`) or `RuntimeException`, which will now leak the temp file
(previously it was deleted in those cases). Also, `tmpFile.delete()` ignores
its boolean result, so even the `IOException` path may silently fail to delete.
Restore cleanup for all relevant exception types and perform deletion in a
`finally` (or equivalent) using a best-effort delete that doesn’t silently fail
(e.g., delete-if-exists / quiet delete), while still rethrowing the original
exception.
##########
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);
+ zipFileSystem = null;
}
- if (deleteAtClose) {
- try {
- Files.delete(path);
- } catch (IOException e) {
- log.warn("Could not delete " + path, e);
+ }
Review Comment:
`close()` unregisters the watcher but leaves `watcher` non-null, so repeated
`close()` calls will repeatedly invoke the closer (and can trigger noisy warn
logs when deleting a file twice). Make `close()` idempotent by nulling
`watcher` after unregistering (and consider avoiding a blanket swallowed
exception here—at least log at debug/warn if it occurs).
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/io/ZipArchive.java:
##########
@@ -219,19 +227,44 @@ 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);
Review Comment:
Same pattern as `ZipNioArchive`: `watcher` is not set to null after
unregistering, which makes `close()` non-idempotent and can lead to repeated
cleanup and/or log noise on subsequent closes. Set `watcher = null` after
unregistering to keep repeated `close()` calls safe and quiet.
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/JcrPackageImpl.java:
##########
@@ -334,8 +333,8 @@ 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;
Review Comment:
The prior regression test covering temp-file cleanup on
`JcrPackageImpl#getPackage(true)` failure was removed, but this code path still
has nuanced cleanup behavior and is easy to regress (especially with exception
type changes). Add an equivalent regression test targeting
`JcrPackageImpl#getPackage(...)` that simulates failures from both stream IO
and JCR (`RepositoryException`), asserting the created temp file is removed.
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/io/ZipArchive.java:
##########
@@ -219,19 +227,44 @@ 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);
+ jar = null;
+ }
+ }
+
+ /**
+ * This class is used to close the zip file system and delete the zip file
if requested.
Review Comment:
This Javadoc is inaccurate for `ZipArchive`: it refers to closing a “zip
file system”, but the implementation closes a `JarFile`. Update the comment to
reflect the actual resource being managed (JarFile) to avoid confusion.
##########
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 rely on `System.gc()` + a fixed `Thread.sleep(100)` to make the
archive eligible for leak detection, which is inherently flaky under CI and
different JVMs. Prefer polling with a bounded timeout (retrying `System.gc()`
and `dumpUnclosedArchives()` until success) or using a deterministic hook from
the watcher/leak-detection mechanism (if available) so the test doesn’t depend
on timing/GC behavior.
##########
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(ZipArchiveCloseTest.class
Review Comment:
`ZipNioArchiveTest` loading resources via `ZipArchiveCloseTest.class`
creates unnecessary coupling between test classes. Use
`ZipNioArchiveTest.class.getResource(...)` (or a shared utility) to keep the
test self-contained.
--
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]
