On Tue, 2 Jan 2024 09:14:14 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> Please consider this PR which makes `DeflaterOutputStream.close()` always
>> close its wrapped output stream.
>>
>> 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.
>>
>> 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:
>
> Prevent IOException thrown during finish() from being lost if an
> IOException is thrown while closing the wrapped stream
src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 257:
> 255: } catch (IOException ioe) {
> 256: if (finishException != ioe) {
> 257: ioe.addSuppressed(finishException);
A null check for `finishException` will be needed here to prevent a
`NullPointerException` being thrown from within `addSuppressed`. I think it's
better to just copy over (rest of) the finally block from
`FilterOutputStream`'s close() method.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17209#discussion_r1439328181
