On Wed, 12 May 2021 05:51:14 GMT, Ioi Lam <ik...@openjdk.org> wrote:
> This bug was discovered during the development of
> [JDK-6824466](https://bugs.openjdk.java.net/browse/JDK-6824466): when CDS is
> enabled, if `BootLoader::loadClassOrNull` is called before `initPhase2`, it
> would trigger the initialization of the archived module graph in the wrong
> order. Because of unanticipated nesting of `<clinit>` methods,
> `BootLoader::SERVICES_CATALOG` becomes empty, causing future `ServiceLoader`
> operations to fail.
>
> The fix has 2 parts:
>
> - `BootLoader::loadClassOrNull` no longer calls `ClassLoaders::bootLoader()`.
> This avoids triggering the archived module graph initialization. Instead, it
> makes a direct call to `Classloader::findBootstrapClassOrNull()`. We don't
> actually need a `ClassLoader` instance for this call, so I changed
> `Classloader::findBootstrapClassOrNull()` to be a static method.
> - The object returned by `BootLoader::getServicesCatalog()` is now maintained
> inside `jdk.internal.loader.ClassLoaders`. Although not strictly required
> for the fix, this simplifies the initialization of the archived module graph.
> It also makes the logic consistent for the 3 built-in loaders
> (boot/platform/app).
>
> Testing: tiers1-4 in progress. I also verified that the bug reported by Mandy
> is no longer reproducible after I applied this patch on her branch.
Changes for CDS usually make the code harder to maintain but I think this patch
is okay and improves a few areas. Just a few cleanups to keep the code
consistent.
src/java.base/share/classes/jdk/internal/loader/BootLoader.java line 75:
> 73: = NativeLibraries.jniNativeLibraries(null);
> 74:
> 75: private static final JavaLangAccess JLA =
> SharedSecrets.getJavaLangAccess();
It be nicer if you could move this to the top to be consistent with the other
files.
src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java line 58:
> 56: private static final AppClassLoader APP_LOADER;
> 57:
> 58: private static void setArchivedServicesCatalog(ArchivedClassLoaders
> archivedClassLoaders, ClassLoader loader) {
I'd prefer if we could keep the code consistent if possible. In this case, the
method declaration can be split over two lines to avoid one really one line in
the file. Also can you add a one line // comment to be consistent with the
other private methods.
-------------
Marked as reviewed by alanb (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3992
