On Fri, 23 Jul 2021 10:28:02 GMT, Alan Bateman <al...@openjdk.org> wrote:
> 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. I hadn't paid any thoughts on the "synchronized" part. You are right - the new `FileRolloverOutputStream` doesn't get sent back to the callers directly and instead either the `EntryOutputStream` or the `DeflatingEntryOutputStream` get returned. Both of them have the necessary syncrhonizations in place for `write` and `close` operations. The `flush` of the `FileRolloverOutputStream` calls the `flush` on the `BufferedOutputStream` which already has the necessary synchronization. I've updated the PR to remove the use of `synchronized` from this new class and added a brief note about this for future maintainers, just like the existing `EntryOutputStreamDef` has. > 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. The updated version of this PR now fixes this part to open it just once. I had reviewed this `transferTo` multiple times before, but clearly I overlooked this part of the implementation. Thank you for these inputs. The updated PR continues to pass the new tests and the existing ones in `test/jdk/jdk/nio/zipfs/`. ------------- PR: https://git.openjdk.java.net/jdk/pull/4607
