Re: RFR(XS): 8174950: Gracefully handle null Supplier in Objects.requireNonNull

[Volker Simonis]

Thanks everybody! This was a real tough one :)
Maybe I'll ask Mark to put it on the Java 9 achievements slide at next
JavaOne :)


On Wed, Feb 15, 2017 at 3:03 AM, David Holmes  wrote:
> Looks good to me!
>
> Thanks,
> David
>
>
> On 15/02/2017 4:52 AM, Volker Simonis wrote:
>>
>> Hi,
>>
>> can I please have another review for the following trivial fix:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8174950/
>> https://bugs.openjdk.java.net/browse/JDK-8174950
>>
>> This change has already been discussed in length on the mailing list:
>>
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-February/thread.html#46324
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-February/thread.html#24989
>>
>> and in the bug comments at:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8033909
>>
>> So I'll give just (yet another) short summary here:
>>
>> - Objects.requireNonNull(T, Supplier) does not check for the Supplier
>> argument being null. Instead it relies on the fact, that the VM will
>> implicitly throw a NullPointerException when it calls .get on the
>> Supplier argument during the creation of the explicit
>> NullPointerException which it is supposed to throw.
>>
>> - this behavior slightly differs from Objects.requireNonNull(T,
>> String) which simply creates a NullPointerException with a null
>> message in the case where the String argument is null.
>>
>> - the difference is not evident in the OpenJDK, because the HotSpot VM
>> creates a NPE with a null message by default if we call a method on a
>> null object.
>>
>> - however creating such a NPE with a null message when invoking a
>> method on a null object is not enforced by the standard, and other
>> implementations can do better :) For the following trivial program:
>>
>> public class NonNull {
>>   public static void main(String[] args) {
>> Supplier ss = null;
>> Object o = Objects.requireNonNull(null, ss);
>>   }
>> }
>>
>> the current OpenJDK implementation returns:
>>
>> Exception in thread "main" java.lang.NullPointerException
>> at java.util.Objects.requireNonNull(Objects.java:290)
>> at NonNull.main(NonNull.java:8)
>>
>> but the SAP JVM will print:
>>
>> Exception in thread "main" java.lang.NullPointerException: while
>> trying to invoke the method java.util.function.Supplier.get() of a
>> null object loaded from local variable 'messageSupplier'
>> at java.util.Objects.requireNonNull(Objects.java:290)
>> at NonNull.main(NonNull.java:8)
>>
>> which is at least confusing for a call to Objects.requireNonNul() with
>> a null Supplier. We think the exception should be the same like the
>> one we get when invoking Objects.requireNonNul() with a null String.
>>
>> - because of this difference (and because we like our extended
>> Exception messages :) the JCK test for Objects.requireNonNul(T,
>> Supplier) (i.e.
>> api/java_util/Objects/index.html#RequireNonNullMessageSupplier) was
>> removed from TCK 8 and is still commented out in TCK 9
>> (npe_checkingNullSupplier() in RequireNonNullMessageSupplier.java).
>>
>> I really think that the simplest and most natural fix for this problem
>> is to simply check for a null Supplier argument and create the NPE
>> with an explicit null message in that case (as outlined in the
>> webrev). This:
>>  - makes the two requireNonNul() overloads for String and Supplier
>> behave the same (which I think was the initial intention).
>>  - doesn't require documentation changes
>>  - makes it possible to write a simple and conforming TCK test
>>
>> Regards,
>> Volker
>>
>

Re: RFR(XS): 8174950: Gracefully handle null Supplier in Objects.requireNonNull

[David Holmes]

Looks good to me!

Thanks,
David

On 15/02/2017 4:52 AM, Volker Simonis wrote:

Hi,

can I please have another review for the following trivial fix:

http://cr.openjdk.java.net/~simonis/webrevs/2017/8174950/
https://bugs.openjdk.java.net/browse/JDK-8174950

This change has already been discussed in length on the mailing list:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-February/thread.html#46324
http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-February/thread.html#24989

and in the bug comments at:

https://bugs.openjdk.java.net/browse/JDK-8033909

So I'll give just (yet another) short summary here:

- Objects.requireNonNull(T, Supplier) does not check for the Supplier
argument being null. Instead it relies on the fact, that the VM will
implicitly throw a NullPointerException when it calls .get on the
Supplier argument during the creation of the explicit
NullPointerException which it is supposed to throw.

- this behavior slightly differs from Objects.requireNonNull(T,
String) which simply creates a NullPointerException with a null
message in the case where the String argument is null.

- the difference is not evident in the OpenJDK, because the HotSpot VM
creates a NPE with a null message by default if we call a method on a
null object.

- however creating such a NPE with a null message when invoking a
method on a null object is not enforced by the standard, and other
implementations can do better :) For the following trivial program:

public class NonNull {
  public static void main(String[] args) {
Supplier ss = null;
Object o = Objects.requireNonNull(null, ss);
  }
}

the current OpenJDK implementation returns:

Exception in thread "main" java.lang.NullPointerException
at java.util.Objects.requireNonNull(Objects.java:290)
at NonNull.main(NonNull.java:8)

but the SAP JVM will print:

Exception in thread "main" java.lang.NullPointerException: while
trying to invoke the method java.util.function.Supplier.get() of a
null object loaded from local variable 'messageSupplier'
at java.util.Objects.requireNonNull(Objects.java:290)
at NonNull.main(NonNull.java:8)

which is at least confusing for a call to Objects.requireNonNul() with
a null Supplier. We think the exception should be the same like the
one we get when invoking Objects.requireNonNul() with a null String.

- because of this difference (and because we like our extended
Exception messages :) the JCK test for Objects.requireNonNul(T,
Supplier) (i.e.
api/java_util/Objects/index.html#RequireNonNullMessageSupplier) was
removed from TCK 8 and is still commented out in TCK 9
(npe_checkingNullSupplier() in RequireNonNullMessageSupplier.java).

I really think that the simplest and most natural fix for this problem
is to simply check for a null Supplier argument and create the NPE
with an explicit null message in that case (as outlined in the
webrev). This:
 - makes the two requireNonNul() overloads for String and Supplier
behave the same (which I think was the initial intention).
 - doesn't require documentation changes
 - makes it possible to write a simple and conforming TCK test

Regards,
Volker

Re: RFR(XS): 8174950: Gracefully handle null Supplier in Objects.requireNonNull

[Alan Bateman]

On 14/02/2017 18:52, Volker Simonis wrote:


Hi,

can I please have another review for the following trivial fix:

http://cr.openjdk.java.net/~simonis/webrevs/2017/8174950/
https://bugs.openjdk.java.net/browse/JDK-8174950

This change has already been discussed in length on the mailing list:



This looks okay to me.

-Alan

Re: RFR(XS): 8174950: Gracefully handle null Supplier in Objects.requireNonNull

[Chris Hegarty]

On 14 Feb 2017, at 18:52, Volker Simonis  wrote:
> 
> Hi,
> 
> can I please have another review for the following trivial fix:
> 
> http://cr.openjdk.java.net/~simonis/webrevs/2017/8174950/

Looks good.

-Chris.

Re: RFR(XS): 8174950: Gracefully handle null Supplier in Objects.requireNonNull

[Paul Sandoz]

+1

Paul.

> On 14 Feb 2017, at 10:52, Volker Simonis  wrote:
> 
> Hi,
> 
> can I please have another review for the following trivial fix:
> 
> http://cr.openjdk.java.net/~simonis/webrevs/2017/8174950/
> https://bugs.openjdk.java.net/browse/JDK-8174950
> 
> This change has already been discussed in length on the mailing list:
> 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-February/thread.html#46324
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-February/thread.html#24989
> 
> and in the bug comments at:
> 
> https://bugs.openjdk.java.net/browse/JDK-8033909
> 
> So I'll give just (yet another) short summary here:
> 
> - Objects.requireNonNull(T, Supplier) does not check for the Supplier
> argument being null. Instead it relies on the fact, that the VM will
> implicitly throw a NullPointerException when it calls .get on the
> Supplier argument during the creation of the explicit
> NullPointerException which it is supposed to throw.
> 
> - this behavior slightly differs from Objects.requireNonNull(T,
> String) which simply creates a NullPointerException with a null
> message in the case where the String argument is null.
> 
> - the difference is not evident in the OpenJDK, because the HotSpot VM
> creates a NPE with a null message by default if we call a method on a
> null object.
> 
> - however creating such a NPE with a null message when invoking a
> method on a null object is not enforced by the standard, and other
> implementations can do better :) For the following trivial program:
> 
> public class NonNull {
>  public static void main(String[] args) {
>Supplier ss = null;
>Object o = Objects.requireNonNull(null, ss);
>  }
> }
> 
> the current OpenJDK implementation returns:
> 
> Exception in thread "main" java.lang.NullPointerException
>at java.util.Objects.requireNonNull(Objects.java:290)
>at NonNull.main(NonNull.java:8)
> 
> but the SAP JVM will print:
> 
> Exception in thread "main" java.lang.NullPointerException: while
> trying to invoke the method java.util.function.Supplier.get() of a
> null object loaded from local variable 'messageSupplier'
>at java.util.Objects.requireNonNull(Objects.java:290)
>at NonNull.main(NonNull.java:8)
> 
> which is at least confusing for a call to Objects.requireNonNul() with
> a null Supplier. We think the exception should be the same like the
> one we get when invoking Objects.requireNonNul() with a null String.
> 
> - because of this difference (and because we like our extended
> Exception messages :) the JCK test for Objects.requireNonNul(T,
> Supplier) (i.e.
> api/java_util/Objects/index.html#RequireNonNullMessageSupplier) was
> removed from TCK 8 and is still commented out in TCK 9
> (npe_checkingNullSupplier() in RequireNonNullMessageSupplier.java).
> 
> I really think that the simplest and most natural fix for this problem
> is to simply check for a null Supplier argument and create the NPE
> with an explicit null message in that case (as outlined in the
> webrev). This:
> - makes the two requireNonNul() overloads for String and Supplier
> behave the same (which I think was the initial intention).
> - doesn't require documentation changes
> - makes it possible to write a simple and conforming TCK test
> 
> Regards,
> Volker