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

[Eirik Bjørsnøs]

On Sun, 16 Feb 2025 12:45:04 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.
>
> There are one or two places in existing jtreg tests where this behaviour is already verified, but they are not exhaustive. So I've updated a couple of existing test classes to include new test methods to verify the expectation of all these constructors. The tests continue to pass.

Hey @jaikiran 

I'm wondering if a class level note, perhaps with an example snippet demonstrating a safe use of the API class using custom deflater / inflater would be warranted here?

The reasoning is as follows:

* Users may start out using a "safe" constructor, then switching over to a constructor taking a custom inflater / deflater
* When doing so, they may skip reading special notes for that particular constructor. (I have no data on this, I'm just assuming that constructor Javadoc notes are less frequently visited compared to the class-level description)
* People like to copy-paste code from SO and similar, perhaps better if they have a class level snippet demonstrating safe use. We don't want to repeat this when multiple constructors are "unsafe"
* The "danger" kind-of-sort-of lives at the class level, since some constuctors are safe, other not. It's sort of insidious. 

* Perhaps the class level is a place to note that there some constructors are safe, some not, then each "unsafe" constructor could have a short note as well.

Just a thought, there are probably reasons to not do this as well.

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

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