RFR: 8066583: DeflaterInput/OutputStream and InflaterInput/OutputStream should explain responsibility for freeing resources [v3]

[Eirik Bjørsnøs]

On Sat, 1 Mar 2025 07:50:44 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review of this doc-only change which proposes to improve the API documentation of `DeflaterInputStream`, `DeflaterOutputStream`, `InflaterInputStream` and `InflaterOutputStream` classes?
>> 
>> As noted in https://bugs.openjdk.org/browse/JDK-8066583 some of the constructors of these classes allow callers to pass a `Deflater`/`Inflater` instance. The implementation of these classes do not close the given `Deflater`/`Inflater` when the corresponding instance of the class itself is closed. This isn't documented and can lead to situations where callers aren't aware that they are responsible for closing the given `Deflater`/`Inflater` instance. That can then lead to resource leaks of resources held by the `Deflater`/`Inflater`.
>> 
>> The commit in this PR updates the relevant constructors of these classes to add an `@implSpec` explaining the responsibility of closing the given `Inflater`/`Deflater`. I chose the `@implSpec` since each of these classes whose documentation is being updated are `public` and can be sub-classed and the `close()` method overridden. The text being added merely specifies the implementation of these classes and not the sub-classes.
>> 
>> I'll draft a CSR once we agree on the proposed text.
>
> Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - change "@implSpec" to "@apiNote" and update their text
>  - add class level documentation
>  - merge latest from master branch
>  - add tests
>  - update DeflaterInputStream and InflaterInputStream
>  - 8066583: DeflaterOutputStream and InflaterOutputStream should explain responsibility for freeing resources

> Given the inputs from Alan, Eirik and Lance, I have now updated this PR [...]

> Does this look better?

I like the structure this is shaping into, with the affected constructors linking into a note. I like that the note explains that some forms of constructors require special attention to manage resources.

As noted by @vy, I find the note perhaps a bit verbose, and I agree that focus should be given to what actually needs attention, which is the constructors which allow passing a `Deflater`. I'm worried that the lengthy "introduction" means we loose readers before they get the message.

I also think it could maybe help understanding/importance if we explicitly mention that closing the deflater will release resources used by the deflater. Otherwise, the resource leak concequence of not closing the deflater is not made abundantly clear.

I tried rewording this with input from @vy in mind, and came up with this. Take whatever inspiration you find useful:

```  
 * <p>Some of the constructors in this class allow passing a {@link Deflater} for
 * compressing data. When a {@code DeflaterOutputStream} is constructed by passing
 * it a {@code Deflater}, then closing the stream will not close the passed
 * {@code Deflater}. In such cases, it is the responsibility of the caller to
 * to release resources used by the {@code Deflater} by
 * {@linkplain Deflater#close() closing} it as and when appropriate,
 * after the {@code DeflaterOutputStream} has been closed.
 ```

> > perhaps with an example snippet demonstrating a safe use of the API class

Yes, I agree there is too much variabilty in the potential uses of Deflater here, the important message is that the caller is responsible for releasing resources by closing.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/23655#issuecomment-2694136936