Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: The last added folder defines progress monitor total work in asynchronous zipping task #313

Closed
Searen1984 opened this issue Apr 25, 2021 · 4 comments
Assignees
Labels
bug Something isn't working resolved

Comments

@Searen1984
Copy link

Hello,

I think I found a bug when adding multiple folders for asynchronous zipping. The last added folder sets the total work to be done. So the information about the running zip task is not correct in terms of percentage done.

At first the example corresponding to the documentation:

File targetFile = new File...
ZipFile zipFile = new ZipFile(targetFile);
ProgressMonitor progressMonitor = zipFile.getProgressMonitor();
zipFile.setRunInThread(true);

for (File path : paths) {
  zipFile.addFolder(path);
}

while (!progressMonitor.getState().equals(ProgressMonitor.State.READY)) {
  System.out.println("Percentage done: " + progressMonitor.getPercentDone());
  System.out.println("Current file: " + progressMonitor.getFileName());
  System.out.println("Current task: " + progressMonitor.getCurrentTask());

  Thread.sleep(100);
}

Here the problem: Adding a folder resets the total work to be done (see line 28 AsyncZipTask.java). The method calculateTotalWork(taskParameters) is defined through AddFolderToZipTask.java. There you can see that the total work corresponds to the size of all files inside only this folder. It is not accumulated. Therefore, the last folder defines the total work to be done and the percentage is not correct.

Here a solution proposal: Just accumulating the work to be done might lead to strange effects because adding a folder increases the total work to be done and decreases the percentage. So instead create an AccumulatedProgressMonitor class which derives from the ProgressMonitor and allows to combine the state of multiple progress monitors into this single class. Then this AccumulatedProgressMonitor could be returned from the ZipFile class instead of the ProgressMonitor. Thus, there would be no interface break. Additionally each call to addFolder/addFile/addStream should return the ProgressMonitor for this zipping operation. In that way the user could also create an own AccumulatedProgressMonitor accumulating only specific operations. I did not think about the states but it might work ;)

@srikanth-lingala
Copy link
Owner

First of all, thank you for the detailed analysis and explanation of the issue. It makes it much easier to investigate/discuss with such explanation.

This is an interesting scenario you have mentioned. I need to play around a bit to find a solution for this. I will consider your proposal as well, and get back soon when I have more information.

@srikanth-lingala
Copy link
Owner

The actual bug here is that it is (or was before my fix) possible to call addFolder() multiple times before the current task was complete. Zip4j thread mode uses a single thread executor, and there will be only one task execution possible at any given moment (A task can be adding a folder, extracting a file, etc). When a task is in progress, triggering any new task will result in an exception being thrown. I fixed this bug now. You should wait until the current task (in your case adding a folder) is complete before calling addFolder again. Please have a look at the documentation here on how to wait for task execution to be complete. You don't need all the code from the sample, it works if you just check if the state is busy.

@Searen1984
Copy link
Author

Great. Thanks a lot.

@srikanth-lingala
Copy link
Owner

Fixed in v2.8.0 released today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working resolved
Projects
None yet
Development

No branches or pull requests

2 participants