On Sat, 1 Mar 2025 07:50:44 GMT, Jaikiran Pai <j...@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 to introduce a class level (linkable) text explaining the differences between these constructors and how/when the Inflater/Deflater is closed. The proposed `@implSpec` on the constructors has been changed to a brief `@apiNote` linking back to this class level text. The javadoc of `close()` methods too is updated to include a brief `@apiNote` linking back to the class level javadoc. Does this look better? > perhaps with an example snippet demonstrating a safe use of the API class I haven't included a snippet in the class level doc, because the class level doc of `Deflater/Inflater` already has the necessary snippet on how to use (and close) a `Deflater/Inflater`. Adding one in these classes would need to take into account the fact that a `Deflater`/`Inflater` that has been passed to these classes need not necessarily be closed by the application after these Input/OutputStream instances have been closed. The application is free to keep using those `Deflater`/`Inflater` instances as long as it wants to and in fact, the application can even invoke `reset()` on those instances to reuse them for additional operations. ------------- PR Comment: https://git.openjdk.org/jdk/pull/23655#issuecomment-2692048697
