This is an automated email from the ASF dual-hosted git repository.
hansva pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/hop.git
The following commit(s) were added to refs/heads/main by this push:
new 061d847b71 issue #2235 : stop closing the zip filesystem after every
Unzip entry (#7711)
061d847b71 is described below
commit 061d847b712202b10b4aa15e7568e8029a2c56d5
Author: Matt Casters <[email protected]>
AuthorDate: Fri Jul 31 09:06:34 2026 +0200
issue #2235 : stop closing the zip filesystem after every Unzip entry
(#7711)
Closing the VFS zip filesystem in the per-entry finally block forced
java.util.zip.ZipFile to re-parse the entire central directory for each
file, which made archives with hundreds of thousands of small files
catastrophically slow. Release the zip filesystem once after all entries
are processed, and add a regression test that asserts a single release.
---
.../hop/workflow/actions/unzip/ActionUnZip.java | 362 +++++++++++----------
.../actions/unzip/WorkflowActionUnZipTest.java | 87 +++++
2 files changed, 280 insertions(+), 169 deletions(-)
diff --git
a/plugins/actions/unzip/src/main/java/org/apache/hop/workflow/actions/unzip/ActionUnZip.java
b/plugins/actions/unzip/src/main/java/org/apache/hop/workflow/actions/unzip/ActionUnZip.java
index dc3b2adaa9..921357d62d 100644
---
a/plugins/actions/unzip/src/main/java/org/apache/hop/workflow/actions/unzip/ActionUnZip.java
+++
b/plugins/actions/unzip/src/main/java/org/apache/hop/workflow/actions/unzip/ActionUnZip.java
@@ -585,210 +585,208 @@ public class ActionUnZip extends ActionBase implements
Cloneable, IAction {
// Try to read the entries from the VFS object...
//
String zipFilename = "zip:" +
sourceFileObject.getName().getFriendlyURI();
- FileObject zipFile = HopVfs.getFileObject(zipFilename, getVariables());
- FileObject[] items =
- zipFile.findFiles(
- new AllFileSelector() {
- @Override
- public boolean traverseDescendents(FileSelectInfo info) {
- return true;
- }
-
- @Override
- public boolean includeFile(FileSelectInfo info) {
- // Never return the parent directory of a file list.
- if (info.getDepth() == 0) {
- return false;
+ FileObject zipFile = null;
+ try {
+ zipFile = HopVfs.getFileObject(zipFilename, getVariables());
+ FileObject[] items =
+ zipFile.findFiles(
+ new AllFileSelector() {
+ @Override
+ public boolean traverseDescendents(FileSelectInfo info) {
+ return true;
}
- FileObject fileObject = info.getFile();
- return fileObject != null;
- }
- });
-
- Pattern pattern = null;
- if (!Utils.isEmpty(realWildcard)) {
- pattern = Pattern.compile(realWildcard);
- }
- Pattern patternexclude = null;
- if (!Utils.isEmpty(realWildcardExclude)) {
- patternexclude = Pattern.compile(realWildcardExclude);
- }
+ @Override
+ public boolean includeFile(FileSelectInfo info) {
+ // Never return the parent directory of a file list.
+ if (info.getDepth() == 0) {
+ return false;
+ }
- for (FileObject item : items) {
+ FileObject fileObject = info.getFile();
+ return fileObject != null;
+ }
+ });
- if (successConditionBroken) {
- if (!successConditionBrokenExit) {
- logError(
- BaseMessages.getString(
- PKG, CONST_ACTION_UN_ZIP_ERROR_SUCCESS_CONDITIONBROKEN, ""
+ nrErrors));
- successConditionBrokenExit = true;
- }
- return false;
+ Pattern pattern = null;
+ if (!Utils.isEmpty(realWildcard)) {
+ pattern = Pattern.compile(realWildcard);
+ }
+ Pattern patternexclude = null;
+ if (!Utils.isEmpty(realWildcardExclude)) {
+ patternexclude = Pattern.compile(realWildcardExclude);
}
- synchronized (HopVfs.getFileSystemManager(getVariables())) {
- FileObject newFileObject = null;
- try {
- if (isDetailed()) {
- logDetailed(
+ for (FileObject item : items) {
+
+ if (successConditionBroken) {
+ if (!successConditionBrokenExit) {
+ logError(
BaseMessages.getString(
- PKG,
- "ActionUnZip.Log.ProcessingZipEntry",
- item.getName().getURI(),
- sourceFileObject.toString()));
+ PKG, CONST_ACTION_UN_ZIP_ERROR_SUCCESS_CONDITIONBROKEN,
"" + nrErrors));
+ successConditionBrokenExit = true;
}
+ return false;
+ }
- // get real destination filename
- //
- String newFileName = unzipToFolder + Const.FILE_SEPARATOR +
getTargetFilename(item);
- newFileObject = HopVfs.getFileObject(newFileName, getVariables());
-
- if (item.getType().equals(FileType.FOLDER)) {
- // Directory
- //
+ synchronized (HopVfs.getFileSystemManager(getVariables())) {
+ FileObject newFileObject = null;
+ try {
if (isDetailed()) {
logDetailed(
BaseMessages.getString(
- PKG, "ActionUnZip.CreatingDirectory.Label",
newFileName));
+ PKG,
+ "ActionUnZip.Log.ProcessingZipEntry",
+ item.getName().getURI(),
+ sourceFileObject.toString()));
}
- // Create Directory if necessary ...
+ // get real destination filename
//
- if (!newFileObject.exists()) {
- newFileObject.createFolder();
- }
- } else {
- // File
- //
- boolean getIt = true;
- boolean getItexclude = false;
-
- // First see if the file matches the regular expression!
- //
- if (pattern != null) {
- getIt = matchesWildcard(pattern, item);
- }
+ String newFileName = unzipToFolder + Const.FILE_SEPARATOR +
getTargetFilename(item);
+ newFileObject = HopVfs.getFileObject(newFileName,
getVariables());
- if (patternexclude != null) {
- getItexclude = matchesWildcard(patternexclude, item);
- }
-
- boolean take = takeThisFile(item, newFileName);
-
- if (getIt && !getItexclude && take) {
+ if (item.getType().equals(FileType.FOLDER)) {
+ // Directory
+ //
if (isDetailed()) {
logDetailed(
BaseMessages.getString(
- PKG,
- "ActionUnZip.ExtractingEntry.Label",
- item.getName().getURI(),
- newFileName));
+ PKG, "ActionUnZip.CreatingDirectory.Label",
newFileName));
}
- if (ifFileExist == FileExistsEnum.UNIQ) {
- // Create file with unique name
+ // Create Directory if necessary ...
+ //
+ if (!newFileObject.exists()) {
+ newFileObject.createFolder();
+ }
+ } else {
+ // File
+ //
+ boolean getIt = true;
+ boolean getItexclude = false;
- int lenstring = newFileName.length();
- int lastindexOfDot = newFileName.lastIndexOf('.');
- if (lastindexOfDot == -1) {
- lastindexOfDot = lenstring;
- }
+ // First see if the file matches the regular expression!
+ //
+ if (pattern != null) {
+ getIt = matchesWildcard(pattern, item);
+ }
- newFileName =
- newFileName.substring(0, lastindexOfDot)
- + StringUtil.getFormattedDateTimeNow(true)
- + newFileName.substring(lastindexOfDot, lenstring);
+ if (patternexclude != null) {
+ getItexclude = matchesWildcard(patternexclude, item);
+ }
+
+ boolean take = takeThisFile(item, newFileName);
- if (isDebug()) {
- logDebug(
+ if (getIt && !getItexclude && take) {
+ if (isDetailed()) {
+ logDetailed(
BaseMessages.getString(
- PKG, "ActionUnZip.Log.CreatingUniqFile",
newFileName));
+ PKG,
+ "ActionUnZip.ExtractingEntry.Label",
+ item.getName().getURI(),
+ newFileName));
}
- }
- // See if the folder to the target file exists...
- //
- if (!newFileObject.getParent().exists()) {
- newFileObject.getParent().createFolder(); // creates the
whole path.
- }
- InputStream is = null;
- OutputStream os = null;
+ if (ifFileExist == FileExistsEnum.UNIQ) {
+ // Create file with unique name
- try {
- is = HopVfs.getInputStream(item);
- os = HopVfs.getOutputStream(newFileObject, false);
-
- if (is != null) {
- byte[] buff = new byte[2048];
- int len;
- long entryBytes = 0L;
- while ((len = is.read(buff)) > 0) {
- os.write(buff, 0, len);
-
result.setBytesWrittenThisAction(result.getBytesWrittenThisAction() + len);
- entryBytes += len;
+ int lenstring = newFileName.length();
+ int lastindexOfDot = newFileName.lastIndexOf('.');
+ if (lastindexOfDot == -1) {
+ lastindexOfDot = lenstring;
}
- if (entryBytes > 0) {
- emitFileIoLineage(
- parentWorkflow,
- FileIoOperation.COPY,
- item,
- newFileObject,
- entryBytes,
- true,
- null);
+
+ newFileName =
+ newFileName.substring(0, lastindexOfDot)
+ + StringUtil.getFormattedDateTimeNow(true)
+ + newFileName.substring(lastindexOfDot, lenstring);
+
+ if (isDebug()) {
+ logDebug(
+ BaseMessages.getString(
+ PKG, "ActionUnZip.Log.CreatingUniqFile",
newFileName));
}
+ }
- // Add filename to result filenames
- addFilenameToResultFilenames(result, parentWorkflow,
newFileName);
+ // See if the folder to the target file exists...
+ //
+ if (!newFileObject.getParent().exists()) {
+ newFileObject.getParent().createFolder(); // creates the
whole path.
}
- } finally {
- if (is != null) {
- is.close();
+ InputStream is = null;
+ OutputStream os = null;
+
+ try {
+ is = HopVfs.getInputStream(item);
+ os = HopVfs.getOutputStream(newFileObject, false);
+
+ if (is != null) {
+ byte[] buff = new byte[2048];
+ int len;
+ long entryBytes = 0L;
+ while ((len = is.read(buff)) > 0) {
+ os.write(buff, 0, len);
+
result.setBytesWrittenThisAction(result.getBytesWrittenThisAction() + len);
+ entryBytes += len;
+ }
+ if (entryBytes > 0) {
+ emitFileIoLineage(
+ parentWorkflow,
+ FileIoOperation.COPY,
+ item,
+ newFileObject,
+ entryBytes,
+ true,
+ null);
+ }
+
+ // Add filename to result filenames
+ addFilenameToResultFilenames(result, parentWorkflow,
newFileName);
+ }
+ } finally {
+ if (is != null) {
+ is.close();
+ }
+ if (os != null) {
+ os.close();
+ }
}
- if (os != null) {
- os.close();
+ } // end if take
+ }
+ } catch (Exception e) {
+ updateErrors();
+ logError(
+ BaseMessages.getString(
+ PKG,
+ "ActionUnZip.Error.CanNotProcessZipEntry",
+ item.getName().getURI(),
+ sourceFileObject.toString()),
+ e);
+ } finally {
+ if (newFileObject != null) {
+ try {
+ newFileObject.close();
+ if (setOriginalModificationDate) {
+ // Change last modification date
+ newFileObject
+ .getContent()
+
.setLastModifiedTime(item.getContent().getLastModifiedTime());
}
- }
- } // end if take
- }
- } catch (Exception e) {
- updateErrors();
- logError(
- BaseMessages.getString(
- PKG,
- "ActionUnZip.Error.CanNotProcessZipEntry",
- item.getName().getURI(),
- sourceFileObject.toString()),
- e);
- } finally {
- if (newFileObject != null) {
- try {
- newFileObject.close();
- if (setOriginalModificationDate) {
- // Change last modification date
- newFileObject
- .getContent()
-
.setLastModifiedTime(item.getContent().getLastModifiedTime());
- }
- } catch (Exception e) {
- /* Ignore */
- } // ignore this
- }
- // Close file object
- // close() does not release resources!
-
HopVfs.getFileSystemManager(getVariables()).closeFileSystem(item.getFileSystem());
- if (items != null) {
- items = null;
+ } catch (Exception e) {
+ /* Ignore */
+ } // ignore this
+ }
}
- }
- } // Synchronized block on HopVfs.getInstance().getFileSystemManager()
- } // End for
-
- // Here gc() is explicitly called if e.g. createfile is used in the same
- // workflow for the same file. The problem is that after creating the
file the
- // file object is not properly garbaged collected and thus the file
cannot
- // be deleted anymore. This is a known problem in the JVM.
+ } // Synchronized block on
HopVfs.getInstance().getFileSystemManager()
+ } // End for
+ } finally {
+ // Close the zip filesystem once after all entries are processed.
+ // Closing it per entry (as we used to) forces java.util.zip.ZipFile
to re-parse the
+ // entire central directory for every file — catastrophic for archives
with hundreds of
+ // thousands of entries (issue #2235).
+ releaseZipFileSystem(zipFile);
+ }
// Unzip done...
if (afterUnzip > 0) {
@@ -806,6 +804,32 @@ public class ActionUnZip extends ActionBase implements
Cloneable, IAction {
return retval;
}
+ /**
+ * Releases the VFS zip filesystem after all entries have been processed.
+ *
+ * <p>Must be called once per archive (not per entry). Closing the
filesystem after every entry
+ * re-opens {@code java.util.zip.ZipFile} and re-parses the central
directory for each file, which
+ * is catastrophic for archives with hundreds of thousands of entries (issue
#2235).
+ *
+ * <p>Package-private so unit tests can override and count invocations.
+ */
+ void releaseZipFileSystem(FileObject zipFile) {
+ if (zipFile == null) {
+ return;
+ }
+ try {
+ // close() alone does not release the layered zip filesystem resources
+
HopVfs.getFileSystemManager(getVariables()).closeFileSystem(zipFile.getFileSystem());
+ } catch (Exception e) {
+ /* Ignore */
+ }
+ try {
+ zipFile.close();
+ } catch (Exception e) {
+ /* Ignore */
+ }
+ }
+
private void emitFileIoLineage(
IWorkflowEngine<WorkflowMeta> wf,
FileIoOperation operation,
diff --git
a/plugins/actions/unzip/src/test/java/org/apache/hop/workflow/actions/unzip/WorkflowActionUnZipTest.java
b/plugins/actions/unzip/src/test/java/org/apache/hop/workflow/actions/unzip/WorkflowActionUnZipTest.java
index 190b5490f1..7a1eec1717 100644
---
a/plugins/actions/unzip/src/test/java/org/apache/hop/workflow/actions/unzip/WorkflowActionUnZipTest.java
+++
b/plugins/actions/unzip/src/test/java/org/apache/hop/workflow/actions/unzip/WorkflowActionUnZipTest.java
@@ -28,6 +28,7 @@ import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.util.ArrayList;
import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
import java.util.zip.ZipEntry;
import java.util.zip.ZipOutputStream;
import org.apache.commons.vfs2.FileContent;
@@ -198,6 +199,46 @@ class WorkflowActionUnZipTest extends
WorkflowActionLoadSaveTestSupport<ActionUn
"city.txt should have been filtered out by the entry wildcard");
}
+ /**
+ * Issue #2235: the zip VFS filesystem must be released once after all
entries are processed, not
+ * once per entry. Closing it per entry re-parses the zip central directory
for every file and is
+ * catastrophic on archives with hundreds of thousands of small files.
+ */
+ @Test
+ void unzipReleasesZipFileSystemOncePerArchive() throws Exception {
+ final int entryCount = 50;
+
+ File workDir = Files.createTempDirectory("unzip2235").toFile();
+ workDir.deleteOnExit();
+ File zipFile = new File(workDir, "many-entries.zip");
+ File targetDir = new File(workDir, "extract");
+ createZipWithManySmallEntries(zipFile, entryCount);
+
+ AtomicInteger releaseCount = new AtomicInteger();
+ ActionUnZip action =
+ new ActionUnZip() {
+ @Override
+ void releaseZipFileSystem(FileObject zipFileObject) {
+ releaseCount.incrementAndGet();
+ super.releaseZipFileSystem(zipFileObject);
+ }
+ };
+
+ Result result = runUnzipSingleFile(action, zipFile, targetDir,
FileExistsEnum.OVERWRITE);
+
+ assertEquals(0, result.getNrErrors(), "The action should not report
errors");
+ assertTrue(result.getResult(), "The action should succeed");
+ assertEquals(
+ entryCount,
+ countFiles(targetDir),
+ "All " + entryCount + " entries should have been extracted");
+ assertEquals(
+ 1,
+ releaseCount.get(),
+ "Zip filesystem must be released exactly once per archive (issue
#2235); "
+ + "releasing per entry re-parses the central directory for every
file");
+ }
+
/**
* Builds a temp folder with two {@code geoip_*.zip} archives, each holding
one distinct entry.
*/
@@ -236,6 +277,29 @@ class WorkflowActionUnZipTest extends
WorkflowActionLoadSaveTestSupport<ActionUn
return action.execute(new Result(), 0);
}
+ /** Configures and runs the unzip action on a single archive file. */
+ private static Result runUnzipSingleFile(
+ File zipFile, File targetDir, FileExistsEnum ifFileExist) {
+ return runUnzipSingleFile(new ActionUnZip(), zipFile, targetDir,
ifFileExist);
+ }
+
+ private static Result runUnzipSingleFile(
+ ActionUnZip action, File zipFile, File targetDir, FileExistsEnum
ifFileExist) {
+ IWorkflowEngine<WorkflowMeta> workflow = new LocalWorkflowEngine(new
WorkflowMeta());
+ workflow.setStopped(false);
+
+ workflow.getWorkflowMeta().addAction(new ActionMeta(action));
+ action.setParentWorkflow(workflow);
+ action.setParentWorkflowMeta(workflow.getWorkflowMeta());
+
+ action.setZipFilename(zipFile.getPath());
+ action.setSourceDirectory(targetDir.getPath());
+ action.setCreateFolder(true);
+ action.setIfFileExist(ifFileExist);
+
+ return action.execute(new Result(), 0);
+ }
+
/** Creates a zip file containing the given text entries. */
private static void createZipWithEntries(File zipFile, String[] entryNames,
String[] contents)
throws java.io.IOException {
@@ -248,4 +312,27 @@ class WorkflowActionUnZipTest extends
WorkflowActionLoadSaveTestSupport<ActionUn
}
}
}
+
+ /**
+ * Creates a zip with many tiny entries (same shape as SEC EDGAR bulk zips:
lots of small files).
+ * Compression level 0 keeps archive creation cheap in unit tests.
+ */
+ private static void createZipWithManySmallEntries(File zipFile, int
entryCount)
+ throws java.io.IOException {
+ byte[] payload = "{\"a\":1}".getBytes(StandardCharsets.UTF_8);
+ try (OutputStream fos = new FileOutputStream(zipFile);
+ ZipOutputStream zos = new ZipOutputStream(fos)) {
+ zos.setLevel(0);
+ for (int i = 0; i < entryCount; i++) {
+ zos.putNextEntry(new ZipEntry(String.format("f%06d.json", i)));
+ zos.write(payload);
+ zos.closeEntry();
+ }
+ }
+ }
+
+ private static int countFiles(File dir) {
+ File[] children = dir.listFiles(File::isFile);
+ return children == null ? 0 : children.length;
+ }
}
