On Wed, 27 Oct 2021 13:06:54 GMT, Larry-N <d...@openjdk.java.net> wrote:
>> This fix adds a cache of service provider classes to LoginContext (in
>> particular, it's a cache of LoginModules classes). The approach helps to
>> increase the performance of the LoginContext.login() method significantly,
>> especially in a multi-threading environment. Service Loader is used for
>> polling on Service Provider classes, without instantiating LoginModules
>> object if Service Provider name doesn't match record in .config file. The
>> set of service providers is cached for each Context Loader separately.
>> This code passed successfully tier1 and tier2 tests on mac os.
>
> Larry-N has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Corrected trailing whitespaces
Source code change looks mostly fine to me. I'm running some tests now. Will
get back once they finish.
As for the test, IMO, it was meant to show that `SecondLoginModule` is still
needed even if it's not in the JAAS login config file. Your updated test only
prove it's not really instantiated. How about this:
Change the test directives to
* @build FirstLoginModule
* @run main/othervm/fail Loader
* @build SecondLoginModule
* @run main/othervm Loader
so that the login succeeds only after there exists a `SecondLoginModule`. All
those `isLoaded` flags should be removed so their enclosing classes are not
compiled automatically.
src/java.base/share/classes/javax/security/auth/login/LoginContext.java line
229:
> 227: private static final sun.security.util.Debug debug =
> 228: sun.security.util.Debug.getInstance("logincontext",
> "\t[LoginContext]");
> 229: private static final WeakHashMap<ClassLoader,
> Set<Provider<LoginModule>>> cacheServiceProviders = new WeakHashMap<>();
This line can be a little shorter. There are some other lines below which is
also quite long. The original limit was 80 chars but please be at most 90.
src/java.base/share/classes/javax/security/auth/login/LoginContext.java line
704:
> 702: lmProviders =
> cacheServiceProviders.get(contextClassLoader);
> 703: if (lmProviders == null){
> 704: if (debug != null)
Please enclose the following line in a pair of braces.
src/java.base/share/classes/javax/security/auth/login/LoginContext.java line
717:
> 715: }
> 716:
> cacheServiceProviders.put(contextClassLoader,lmProviders);
> 717: }
Is it necessary the lines below still be inside the synchronized block?
src/java.base/share/classes/javax/security/auth/login/LoginContext.java line
723:
> 721: if (debug != null) {
> 722: debug.println(name + " loaded as a
> service");
> 723: }
Please de-indent the if block above.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5748
