On Thu, 22 Jul 2021 16:01:30 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review for this proposed fix for the issue reported in >> https://bugs.openjdk.java.net/browse/JDK-8190753? >> >> The commit here checks for the size of the zip entry before trying to create >> a `ByteArrayOutputStream` for that entry's content. A new jtreg test has >> been included in this commit to reproduce the issue and verify the fix. >> >> P.S: It's still a bit arguable whether it's a good idea to create the >> `ByteArrayOutputStream` with an initial size potentially as large as the >> `MAX_ARRAY_SIZE` or whether it's better to just use some smaller default >> value. However, I think that can be addressed separately while dealing with >> https://bugs.openjdk.java.net/browse/JDK-8011146 > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - remove no longer necessary javadoc comment on test > - review suggestion - wrap ByteArrayOutputStream instead of extending it Thanks for changing it to wrap the BOAS rather than existing it, that avoids the annoying wrapping/unwrapping of exceptions. So I think the approach looks good but I think the synchronization needs to be re-checked it is not obvious that is correct or needed. Are there any cases where FileRolloverOutputStream is returned to user code? I don't think so, instead users of the zip file system will get an EntryOutputStream that wraps the FileRolloverOutputStream. The EntryOutputStream methods are synchronized so I assume that FileRolloverOutputStream does not need to it and that would avoid the inconsistency between the write methods and the flush/close methods. One other thing to point out is that transferToFile shouldn't need to open the file twice, instead it should be able to open the tmp file for writing once. ------------- PR: https://git.openjdk.java.net/jdk/pull/4607
