RFR: 8325304: Several classes in java.util.jar and java.util.zip don't specify the behaviour for null arguments [v5]

[Jaikiran Pai]

On Wed, 7 Feb 2024 10:47:31 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> These 2 classes, the `CheckedInputStream` and the `CheckedOutputStream` are slightly different from the rest of the classes in this changeset. This javadoc here is for the constructor of the `CheckedInputStream`. The implementation of the constructor just blindly assigns the passed argument to an internal member field and doesn't do any null check on the passed arguments (any subsequent usage of these instance fields within that class, then leads to a NullPointerException). So the constructor isn't throwing any `NullPointerException` here and thus adding a `@throws` here would be incorrect. In theory, we could just change the implementation of this constructor to throw a `NullPointerException` if either of these arguments were null, but that might mean a potential breakage of some weird usage of the CheckedInputStream. So I decided to add the `NullPointerException` detail to the constructor description.
>> 
>> Do you think we should instead do something like this for this class and the `CheckedOutputStream` class:
>> 
>> 
>> diff --git a/src/java.base/share/classes/java/util/zip/CheckedInputStream.java b/src/java.base/share/classes/java/util/zip/CheckedInputStream.java
>> index 10f72b416d1..76cba26986e 100644
>> --- a/src/java.base/share/classes/java/util/zip/CheckedInputStream.java
>> +++ b/src/java.base/share/classes/java/util/zip/CheckedInputStream.java
>> @@ -41,10 +41,7 @@ public class CheckedInputStream extends FilterInputStream {
>>      private final Checksum cksum;
>>  
>>      /**
>> -     * Creates an input stream using the specified Checksum. A null
>> -     * value to either {@code in} or {@code cksum} will cause
>> -     * a {@link NullPointerException} to be thrown from the
>> -     * {@code read} methods of this {@code CheckedInputStream}.
>> +     * Creates an input stream using the specified Checksum.
>>       * @param in the input stream
>>       * @param cksum the Checksum
>>       */
>> @@ -57,6 +54,8 @@ public CheckedInputStream(InputStream in, Checksum cksum) {
>>       * Reads a byte. Will block if no input is available.
>>       * @return the byte read, or -1 if the end of the stream is reached.
>>       * @throws    IOException if an I/O error has occurred
>> +     * @throws    NullPointerException if this {@code CheckedInputStream} was
>> +     *            constructed with a {@code null} value for {@code in} or {@code cksum}
>>       */
>>      public int read() throws IOException {
>>          int b = in.read();
>> @@ -75,7 +74,9 @@ public int read() thro...
>
>> These 2 classes, the `CheckedInputStream` and the `CheckedOutputStream` are slightly different from the rest of the classes in this changeset. This javadoc here is for the constructor of the `CheckedInputStream`. The implementation of the constructor just blindly assigns the passed argument to an internal member field and doesn't do any null check on the passed arguments (any subsequent usage of these instance fields within that class, then leads to a NullPointerException). 
> 
> The superclasses FilterInputStream and FilterOutputStream have a a protected field for the underlying stream so it's possible for a subclass to set the underlying stream lazily. I can't recall seeing code in the wild availing of that but it is possible.
> 
> CheckedInputStream and CheckedOutputStream date from JDK 1.1 and it's not clear what the intention was. My guess is that it was an oversight/bug to not check for null when constructing directly. Fixing this will help catch buggy code but it does mean a behavior change. I think we have to keep existing behavior for the subclassing case because it is possible for the subclass to set the stream lazily.

Given that subclasses could set these fields lazily (however remote the case might be), do you think we should then not specify the `NullPointerException` for the read methods on these 2 classes. In which case I can exclude these 2 classes from this PR.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17728#discussion_r1481314236