Tibor17 commented on a change in pull request #78: COMPRESS-485 keep zip
entries order in parallel zip creation
URL: https://github.com/apache/commons-compress/pull/78#discussion_r283110877
##########
File path:
src/main/java/org/apache/commons/compress/archivers/zip/ParallelScatterZipCreator.java
##########
@@ -255,8 +257,13 @@ public void writeTo(final ZipArchiveOutputStream
targetStream)
compressionDoneAt = System.currentTimeMillis();
synchronized (streams) {
+ // write zip entries in the order they were added (kept as
futures)
+ for (final Future<ScatterZipOutputStream> future : futures) {
Review comment:
This loop was added. And it is a problem in this synchronized block.
The monitor lock is `streams` and not `futures`. They cannot be both in one
statement.
Problem: this will cause NPE when the CPU is overloaded. The same issue was
fixed in Surefire (see memory model).
Additionally the `futures.add()` is not synchronized!
One way would be for you to introduce double synchro block.
I would not do it! Instead I would avoid synchronizations of `ArrayList` and
on both. Then I would use modern collections which do not need external
synchronizatrions (i.e. `Collections.synch...`) and this way there would not
be any `synchronized`. Use `ConcurrentLinkedDequeue`. It is useful when you add
to the tail of the queue and do not remove, and if you iterate, because this
implementation safely iterates even if you change it : no issue.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
