On Mon, 15 May 2023 15:10:06 GMT, Volker Simonis <simo...@openjdk.org> wrote:
>> Can I please get a review of this test only change which addresses the issue >> noted in https://bugs.openjdk.org/browse/JDK-8307403? >> >> When we recently did a change in >> https://bugs.openjdk.org/browse/JDK-8299748, there was a oversight that the >> `deflate.deflate(tempBuffer)` could potentially end up taking in a zero >> sized array. That would then mean the loop in which this `deflate` happens, >> would never exit leading to the test timing out. >> I've added additional details as a comment to the JBS issue >> https://bugs.openjdk.org/browse/JDK-8307403?focusedCommentId=14581202&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14581202. >> >> The commit in this PR uses a fixed size temporary buffer, unrelated to the >> input length, to get past this problem. The use of fixed size here will not >> re-introduce the original issue which >> https://bugs.openjdk.org/browse/JDK-8299748 fixed, because unlike >> previously, this is just a temporary size and we continue to deflate the >> content into a dynamically growing `ByteArrayOutputStream` till the deflater >> is "finished". >> >> Additionally I've added a log message to help debug any future issues in >> this call. > > test/jdk/java/util/zip/DeInflate.java line 146: > >> 144: def.finish(); >> 145: >> 146: try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) { > > I don't think this is really required because the `ByteArrayOutputStream` > will grow dynamically anyway, even if initialized to zero but for all other > cases `len` is probably a better estimation than the default size of `32`. > But I leave it up to you if you want to change it back. Hello Volker, you are right that even initializing this with `len` should be fine since the `ByteArrayOutputStream` will grow if/when needed. However, the reason I decided to remove the usage of `len` here was to avoid any kind of confusion and make it easier to read that `len` is only for the input length (as noted in the javadoc of that method) and plays no other role in deciding sizes of (temporary) outputs. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13955#discussion_r1194498736
