Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v3]
On Tue, 2 Jan 2024 22:06:36 GMT, Eirik Bjørsnøs wrote: > A CSR for this change has been proposed and is ready for review: > [JDK-8322871](https://bugs.openjdk.org/browse/JDK-8322871) I've reviewed the CSR so you can finalize. The implementation change looks fine. - PR Comment: https://git.openjdk.org/jdk/pull/17209#issuecomment-1892440727
Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v3]
On Tue, 2 Jan 2024 12:21:21 GMT, Eirik Bjørsnøs 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 two additional
> commits since the last revision:
>
> - Add more extensive testing of combined write/close failure modes
> - Don't suppress if finishException is null, mark stream as closed even when
> closing the wrapped stream failed
test/jdk/java/util/zip/ZipOutputStream/CloseWrappedStream.java line 47:
> 45: */
> 46: @Test
> 47: public void exceptionDuringFinish() {
Suggestion:
public void exceptionDuringFinish() {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17209#discussion_r1452026379
Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v3]
On Tue, 2 Jan 2024 12:21:21 GMT, Eirik Bjørsnøs 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 two additional > commits since the last revision: > > - Add more extensive testing of combined write/close failure modes > - Don't suppress if finishException is null, mark stream as closed even when > closing the wrapped stream failed A CSR for this change has been proposed and is ready for review: [JDK-8322871](https://bugs.openjdk.org/browse/JDK-8322871) - PR Comment: https://git.openjdk.org/jdk/pull/17209#issuecomment-1874623292
Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v3]
> 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 two additional commits since the last revision: - Add more extensive testing of combined write/close failure modes - Don't suppress if finishException is null, mark stream as closed even when closing the wrapped stream failed - Changes: - all: https://git.openjdk.org/jdk/pull/17209/files - new: https://git.openjdk.org/jdk/pull/17209/files/02707d58..33e7756e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17209=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17209=01-02 Stats: 149 lines in 2 files changed: 123 ins; 2 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/17209.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17209/head:pull/17209 PR: https://git.openjdk.org/jdk/pull/17209
