On Mon, 15 Jan 2024 10:26:53 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> Please consider this PR which makes `DeflaterOutputStream.close()` always >> close its wrapped output stream exactly once. >> >> Currently, closing of the wrapped output stream happens outside the finally >> block where `finish()` is called. If `finish()` throws, this means the >> wrapped stream will not be closed. This can potentially lead to leaking >> resources such as file descriptors or sockets. >> >> This fix is to move the closing of the wrapped stream inside the finally >> block. >> >> Additionally, the `closed = true;` statement is moved to the start of the >> close method. This makes sure we only ever close the wrapped stream once >> (this aligns with the overridden method `FilterOutputStream.close´) >> >> Specification: This change brings the implementation of >> `DeflaterOutputStream.close()` in line with its specification: *Writes >> remaining compressed data to the output stream and closes the underlying >> stream.* >> >> Risk: This is a behavioural change. There is a small risk that existing code >> depends on the close method not following its specification. >> >> Testing: The PR adds a new JUnit 5 test `CloseWrappedStream.java` which >> simulates the failure condition and verifies that the wrapped stream was >> closed under failing and non-failing conditions. > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Update test/jdk/java/util/zip/ZipOutputStream/CloseWrappedStream.java > > Remove extra whitespace > > Co-authored-by: Andrey Turbanov <turban...@gmail.com> The changes look good overall. See suggestion for comment improvement but not required, just makes it clearer test/jdk/java/util/zip/ZipOutputStream/CloseWrappedStream.java line 91: > 89: /** > 90: * Check that the exception handling is correct when the > 91: * wrapped stream throws while being closed This comment could use a bit of wordsmithing to indicate what "correct" means ------------- Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17209#pullrequestreview-1824541172 PR Review Comment: https://git.openjdk.org/jdk/pull/17209#discussion_r1453817997
