Copilot commented on code in PR #434:
URL:
https://github.com/apache/jackrabbit-filevault/pull/434#discussion_r4114937295
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/io/ZipArchive.java:
##########
@@ -153,8 +158,11 @@ 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);
+ if (watcher != null) {
+ CloseWatcher.unregister(watcher);
+ }
+ watcher =
+ CloseWatcher.register(this, new Closer(isTempFile ? this.file
: null, jar), SHOULD_CREATE_STACK_TRACE);
Review Comment:
For a temporary archive the constructor watcher owns `new Closer(file,
null)`. `open()` assigns `jar` before scanning metadata, so if that scan
throws, this replacement registration is never reached; `close()` then deletes
only the path and leaves the already-open `JarFile` unclosed (and a later
`open()` returns early because `jar` is still non-null). Close the partially
opened jar and reset/unregister the watcher on the failure path before relying
on this watcher.
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/JcrPackageImpl.java:
##########
@@ -322,22 +322,14 @@ 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 {
- Binary bin = getData().getBinary();
- try (FileOutputStream out =
FileUtils.openOutputStream(tmpFile);
- InputStream in = bin.getStream()) {
- IOUtils.copy(in, out);
- } finally {
- bin.dispose();
- }
- pack = new ZipVaultPackage(tmpFile, true);
- } catch (RepositoryException | IOException | RuntimeException
e) {
- FileUtils.deleteQuietly(tmpFile);
- throw e;
+ Binary bin = getData().getBinary();
+ try (FileOutputStream out =
FileUtils.openOutputStream(tmpFile);
+ InputStream in = bin.getStream()) {
+ IOUtils.copy(in, out);
+ } finally {
+ bin.dispose();
}
Review Comment:
The `CloseWatcher` is only registered inside `ZipVaultPackage` after this
copy completes, so any failure while obtaining the binary, opening the output,
copying the stream, or disposing the binary leaves `tmpFile` orphaned with no
cleanup owner. This is the same pre-construction failure covered by the deleted
regression test; retain an explicit quiet delete fallback around the copy and
package construction, while using the watcher for files that have been wrapped
successfully.
--
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]
