Hi Max,
Do you want me to go though official contribution (I already have signed OCA
for glassfish) or
someone from oracle team will take care about it?
Thanks,
Tigran.
----- Original Message -----
> From: "Weijun Wang" <weijun.w...@oracle.com>
> To: "Tigran Mkrtchyan" <tigran.mkrtch...@desy.de>
> Cc: "security-dev" <security-dev@openjdk.java.net>
> Sent: Friday, April 24, 2020 3:29:02 PM
> Subject: Re: NPE is used in javax.security.auth.Subject for flowcontrol
> Hi Tigran,
>
> I read "Collection.html#optional-restrictions" carefully, and yes you're
> correct. Sorry for my ignorance.
>
> This method is called quite a lot. It might be worth an enhancement.
>
> Thanks,
> Max
>
>> On Apr 24, 2020, at 9:00 PM, Mkrtchyan, Tigran <tigran.mkrtch...@desy.de>
>> wrote:
>>
>>
>>
>> Here is the benchmark results:
>>
>> Benchmark Mode Cnt Score Error Units
>> SubjectBenchmark.jdkSubject thrpt 20 473086.025 ± 7661.215 ops/s
>> SubjectBenchmark.patchedSubject thrpt 20 2529016.530 ± 50982.465 ops/s
>>
>> On a one hand, patched version has 5x higher throughput. However, on an other
>> hand
>> the throughput of the original code is sufficient for any real application.
>> The
>> benchmark code is attached.
>>
>> Sorry for the noise. Should have done benchmarks before trying to optimize
>> it.
>>
>> Tigran.
>>
>> ----- Original Message -----
>>> From: "Tigran Mkrtchyan" <tigran.mkrtch...@desy.de>
>>> To: "Weijun Wang" <weijun.w...@oracle.com>
>>> Cc: "security-dev" <security-dev@openjdk.java.net>
>>> Sent: Friday, April 24, 2020 12:50:03 PM
>>> Subject: Re: NPE is used in javax.security.auth.Subject for flowcontrol
>>
>>> I read it differently:
>>>
>>> Attempting to query the presence of an ineligible element may throw an
>>> exception, or it may simply return false; some implementations
>>> will exhibit the former behavior and some will exhibit the latter.
>>>
>>> Implementation might thrown an exception or return false, but sill don't
>>> allow
>>> null values.
>>>
>>> ... would not result in the insertion of an ineligible element into the
>>> collection may throw an exception or it may
>>> succeed, at the option of the implementation.
>>>
>>>
>>> For example, TreeSet throws NPE depending on Comparator.
>>>
>>> Well, I understand, this is a corner case that possibly not worth to
>>> optimize.
>>> Let me write
>>> a benchmark to see how much difference it makes.
>>>
>>> Tigran.
>>>
>>>
>>> ----- Original Message -----
>>>> From: "Weijun Wang" <weijun.w...@oracle.com>
>>>> To: "Tigran Mkrtchyan" <tigran.mkrtch...@desy.de>
>>>> Cc: "security-dev" <security-dev@openjdk.java.net>
>>>> Sent: Friday, April 24, 2020 12:23:48 PM
>>>> Subject: Re: NPE is used in javax.security.auth.Subject for flowcontrol
>>>
>>>> My understanding is by optional you have 2 options while designing the Set:
>>>>
>>>> 1. Allow null
>>>>
>>>> 2. Not allow null
>>>>
>>>> Now that SecureSet has chosen the 2nd way, it is now in the "this set does
>>>> not
>>>> permit null elements" category and therefore it should "@throws
>>>> NullPointerException if the specified element is null".
>>>>
>>>> --Max
>>>>
>>>>> On Apr 24, 2020, at 6:13 PM, Mkrtchyan, Tigran <tigran.mkrtch...@desy.de>
>>>>> wrote:
>>>>>
>>>>> Hi Max,
>>>>>
>>>>> that's right, but java doc as well mentions that this exception is
>>>>> *optional*,
>>>>> and has a corresponding note:
>>>>>
>>>>> Some collection implementations have restrictions on the elements that
>>>>> they may
>>>>> contain. For example, some implementations prohibit null elements, and
>>>>> some
>>>>> have restrictions on the types of their elements. Attempting to add an
>>>>> ineligible element throws an unchecked exception, typically
>>>>> NullPointerException or ClassCastException. Attempting to query the
>>>>> presence of
>>>>> an ineligible element may throw an exception, or it may simply return
>>>>> false;
>>>>> some implementations will exhibit the former behavior and some will
>>>>> exhibit the
>>>>> latter. More generally, attempting an operation on an ineligible element
>>>>> whose
>>>>> completion would not result in the insertion of an ineligible element
>>>>> into the
>>>>> collection may throw an exception or it may succeed, at the option of the
>>>>> implementation. Such exceptions are marked as "optional" in the
>>>>> specification
>>>>> for this interface.
>>>>>
>>>>> Thus, returning *false* is justified and spec compliant.
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Tigran.
>>>>>
>>>>>
>>>>> ----- Original Message -----
>>>>>> From: "Weijun Wang" <weijun.w...@oracle.com>
>>>>>> To: "Tigran Mkrtchyan" <tigran.mkrtch...@desy.de>
>>>>>> Cc: "security-dev" <security-dev@openjdk.java.net>
>>>>>> Sent: Friday, April 24, 2020 11:42:41 AM
>>>>>> Subject: Re: NPE is used in javax.security.auth.Subject for flowcontrol
>>>>>
>>>>>> Hi Tigran,
>>>>>>
>>>>>> In java.util.Set, we have:
>>>>>>
>>>>>> * @throws NullPointerException if the specified element is null and
>>>>>> this
>>>>>> * set does not permit null elements
>>>>>> * (<a href="Collection.html#optional-restrictions">optional</a>)
>>>>>> */
>>>>>> boolean contains(Object o);
>>>>>>
>>>>>> As an implementation, SecureSet must follow the spec to throw an NPE. If
>>>>>> it
>>>>>> returns null, some unexpected thing might happen when the contains()
>>>>>> method is
>>>>>> called somewhere else.
>>>>>>
>>>>>> Thanks,
>>>>>> Max
>>>>>>
>>>>>>> On Apr 24, 2020, at 4:21 PM, Mkrtchyan, Tigran
>>>>>>> <tigran.mkrtch...@desy.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Dear Java-SE security developers,
>>>>>>>
>>>>>>>
>>>>>>> Imagine a following code:
>>>>>>>
>>>>>>> ```
>>>>>>> Subject s1 = ... ;
>>>>>>>
>>>>>>> Subject s2 = ... ;
>>>>>>>
>>>>>>>
>>>>>>> s2.getPrincipals().addAll(s1.getPrincipals());
>>>>>>>
>>>>>>> ```
>>>>>>>
>>>>>>> The Subject's SecureSet.addAll checks that provided Set doesn't
>>>>>>> contains 'null'
>>>>>>> values
>>>>>>> by calling collectionNullClean, which calls SecureSet#contains:
>>>>>>>
>>>>>>> ```
>>>>>>> try {
>>>>>>> hasNullElements = coll.contains(null);
>>>>>>> } catch (NullPointerException npe) {
>>>>>>>
>>>>>>> ```
>>>>>>>
>>>>>>> The SecureSet#contains itself checks for 'null' values, the NPE is
>>>>>>> always
>>>>>>> generated.
>>>>>>>
>>>>>>> This as introduced by commit e680ab7f208e
>>>>>>>
>>>>>>> https://hg.openjdk.java.net/jdk/jdk/diff/e680ab7f208e/jdk/src/share/classes/javax/security/auth/Subject.java
>>>>>>>
>>>>>>>
>>>>>>> As SecureSet doesn't allow null values, it will be much simpler to
>>>>>>> return false
>>>>>>> right away:
>>>>>>>
>>>>>>> ```
>>>>>>>
>>>>>>> public boolean contains(Object o) {
>>>>>>> if (o == null) {
>>>>>>> // null values rejected by add
>>>>>>> return false;
>>>>>>> }
>>>>>>>
>>>>>>> ...
>>>>>>> }
>>>>>>>
>>>>>>> ```
>>>>>>>
>>>>>>>
>>>>>>> Thanks in advance,
>>>>>>> Tigran.
> > <SubjectBenchmark.java>
