On Wed, 27 Oct 2021 12:46:57 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> renames
>
> src/java.base/share/classes/javax/security/auth/Subject.java line 324:
>
>> 322: }
>> 323:
>> 324: // Store the current subject to a ThreadLocal when a system
>> property is set.
>
> nit: "... in a ThreadLocal ..."
Oops.
> src/java.base/share/classes/javax/security/auth/Subject.java line 360:
>
>> 358: * <p>
>> 359: * No matter what storage is chosen, the current subject will
>> 360: * always be installed by the {@link #callAs} method.
>
> I'm not really sure if this sentence is necessary. It seems like it doesn't
> need to be in the specification to me. The first sentence is clear enough to
> me: "The current subject is installed by the {@link #callAs} method."
Removed.
> src/java.base/share/classes/javax/security/auth/Subject.java line 362:
>
>> 360: * always be installed by the {@link #callAs} method.
>> 361: *
>> 362: * @return the current subject. The return value can be
>
> Suggest to combine this into one sentence: the current subject, or {@code
> null} if a current subject is not installed or the current subject is set to
> {@code null}."
OK.
> src/java.base/share/classes/javax/security/auth/Subject.java line 394:
>
>> 392: *
>> 393: * @param subject the intended current subject for {@code action}.
>> 394: * Can be {@code null}.
>
> Is it necessary to allow `null` in the new `callAs` method? Is there a use
> case where this is useful? I know the `doAs` methods allowed it, but it looks
> like they simply call `AccessController.doPrivileged` in that case w/o a
> subject. Seems it would be simpler to not allow it and throw an NPE.
Maybe in a nested call that wants to clear out the existing subject
temporarily? It could be useful.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5024
