8293977: jdk/modules/etc/VerifyModuleDelegation.java fail with jfx #10328

xiangzhai · 2022-09-19T00:45:32Z

Hi,

@dumasun reported the issue:

Configured with jfx-ls-modular-sdk:

configure --with-import-modules=modular-sdk

make run-test CONF=fastdebug TEST="jdk/modules/etc/VerifyModuleDelegation.java" failed:

----------System.out:(46/3114)----------
test VerifyModuleDelegation.checkJavaBase(): success
test VerifyModuleDelegation.checkLoaderDelegation(): failure
java.lang.Error: platform/javafx.swing can't delegate to find classes from app/jdk.unsupported.desktop
    at VerifyModuleDelegation.lambda$checkLoaderDelegation$1(VerifyModuleDelegation.java:77)
    at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133)
    at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1921)
    at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:762)
    at VerifyModuleDelegation.lambda$checkLoaderDelegation$2(VerifyModuleDelegation.java:68)
    at java.base/java.util.HashMap$KeySpliterator.forEachRemaining(HashMap.java:1715)
    at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:762)
    at VerifyModuleDelegation.checkLoaderDelegation(VerifyModuleDelegation.java:68)
    at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
    at java.base/java.lang.reflect.Method.invoke(Method.java:578)
    at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
    at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599)
    at org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174)
    at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46)
    at org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822)
    at org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147)
    at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
    at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
    at org.testng.TestRunner.privateRun(TestRunner.java:764)
    at org.testng.TestRunner.run(TestRunner.java:585)
    at org.testng.SuiteRunner.runTest(SuiteRunner.java:384)
    at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378)
    at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337)
    at org.testng.SuiteRunner.run(SuiteRunner.java:286)
    at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53)
    at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96)
    at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218)
    at org.testng.TestNG.runSuitesLocally(TestNG.java:1140)
    at org.testng.TestNG.runSuites(TestNG.java:1069)
    at org.testng.TestNG.run(TestNG.java:1037)
    at com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94)
    at com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:54)
    at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
    at java.base/java.lang.reflect.Method.invoke(Method.java:578)
    at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
    at java.base/java.lang.Thread.run(Thread.java:1589)

===============================================
jdk/modules/etc/VerifyModuleDelegation.java
Total tests run: 2, Passes: 1, Failures: 1, Skips: 0
===============================================

----------System.err:(13/753)----------
WARNING: Using incubator modules: jdk.incubator.vector, jdk.incubator.concurrent
java.lang.Exception: failures: 1
    at com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:96)
    at com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:54)
    at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
    at java.base/java.lang.reflect.Method.invoke(Method.java:578)
    at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
    at java.base/java.lang.Thread.run(Thread.java:1589)

Thanks,
Leslie Zhai

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8293977: jdk/modules/etc/VerifyModuleDelegation.java fail with jfx

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10328/head:pull/10328
$ git checkout pull/10328

Update a local copy of the PR:
$ git checkout pull/10328
$ git pull https://git.openjdk.org/jdk pull/10328/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10328

View PR using the GUI difftool:
$ git pr show -t 10328

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10328.diff

bridgekeeper · 2022-09-19T00:45:53Z

👋 Welcome back lzhai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-09-19T00:49:01Z

@xiangzhai The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-09-19T00:52:52Z

Webrevs

00: Full (ee6c7070)

AlanBateman · 2022-09-19T06:14:25Z

I don't think the test should be changed until there is more information on the issue. jdk.unsupported.desktop should be mapped to the application class loader. Can you tell use which class loader that javafx.swing is mapped to? Has the module-loader-map.conf changed in your environment? I'm wondering if there is any residual configuration somewhere that is mapping the javafx.* modules the platform class loader.

xiangzhai · 2022-09-19T07:02:40Z

Hi @AlanBateman

Thanks for your kind response!

Can you tell use which class loader that javafx.swing is mapped to?

According to the output:

java.lang.Error: platform/javafx.swing can't delegate to find classes from app/jdk.unsupported.desktop

app as the loader2 that javafx.swing is mapped to:

    @Test
    public void checkLoaderDelegation() {
        ModuleLayer boot = ModuleLayer.boot();
        MREFS.stream()
             .forEach(md -> md.requires().stream().forEach(req ->
                 {
                     // check if M requires D and D's loader must be either the
                     // same or an ancestor of M's loader
                     ClassLoader loader1 = boot.findLoader(md.name());
                     ClassLoader loader2 = boot.findLoader(req.name());
                     if (loader1 != loader2 && !isAncestor(loader2, loader1) && !md.name().equals("javafx.swing")) {
                         throw new Error(loader1.getName() + "/" + md.name() +
                             " can't delegate to find classes from " +
                             loader2.getName() + "/" + req.name());
                     }
                 }));
    }

Has the module-loader-map.conf changed in your environment?

Nope. https://github.com/loongson/jdk/blame/loongarch-port/make/conf/module-loader-map.conf

And @dumasun also reproduced the issue for x86_64. Perhaps she could provide more information.

Thanks,
Leslie Zhai

AlanBateman · 2022-09-19T07:29:52Z

java.lang.Error: platform/javafx.swing can't delegate to find classes from app/jdk.unsupported.desktop

There must be configuration somewhere that is mapping the javafx.* modules to the platform class loader So I think the test is doing its job and has found an issue. Hopefully your colleague can provide more Info.

dumasun · 2022-09-19T11:53:19Z

The replication step on x86:

cd openjdk/jdk
jtreg/bin/jtreg -dir:test/jdk -testjdk:./jdk-fx-jdk-20+15-56-g4020ed53dd6-linux_x64 \
-verbose:all jdk/modules/etc/VerifyModuleDelegation.java

test inputs:
testjdk(jdk-fx-jdk-20+15-56-g4020ed53dd6-linux_x64) consists of two parts, jdk and jfx(20+3-3-ga27840e)
test repo: https://github.com/openjdk/jdk

Thanks,
dumasun

AlanBateman · 2022-09-20T06:12:06Z

jtreg/bin/jtreg -dir:test/jdk -testjdk:./jdk-fx-jdk-20+15-56-g4020ed53dd6-linux_x64
-verbose:all jdk/modules/etc/VerifyModuleDelegation.java

Is jdk-fx-jdk-20+15-56-g4020ed53dd6-linux_x64 your build or is it coming from somewhere else? Also can you confirm that make/conf/module-loader-map.conf has not been edit to map the javafx.* modules to the platform class loader?

dumasun · 2022-09-20T08:04:48Z

jtreg/bin/jtreg -dir:test/jdk -testjdk:./jdk-fx-jdk-20+15-56-g4020ed53dd6-linux_x64
-verbose:all jdk/modules/etc/VerifyModuleDelegation.java

Is jdk-fx-jdk-20+15-56-g4020ed53dd6-linux_x64 your build or is it coming from somewhere else?

yes, jdk-fx-jdk-20+15-56-g4020ed53dd6-linux_x64 my build
build steps:
step 1. git clone https://github.com/openjdk/jfx.git and buid modular-sdk.
step 2. git clone https://github.com/openjdk/jdk, configure --with-import-modules=/path/modular-sdk and make images.
then jdk-fx-jdk-20+15-56-g4020ed53dd6-linux_x64 as testjdk.

Is there something wrong with the way I test?

Also can you confirm that make/conf/module-loader-map.conf has not been edit to map the javafx.* modules to the platform class loader?

I donot edit make/conf/module-loader-map.conf.

AlanBateman · 2022-09-20T08:53:00Z

I donot edit make/conf/module-loader-map.conf.

@magicus @kevinrushforth In the distant past, the javafx.* modules were mapped to the platform class loader. Do you recall how that worked with configure --with-import-modules? If someone uses jlink to create a run-time image that includes the javafx.* modules then they will be mapped to the application class loader. Somehow, using --with-import-modules we seem to end up with these modules in the value of PLATFORM_MODULE_NAMES used by build. I'm wondering if there is some residual configuration somewhere.

kevinrushforth · 2022-09-20T12:27:39Z

I haven't built a JDK using the --with-import-modules to include the javafx.* modules in quite a while, but my recollection is that yes, that did cause the JavaFX modules to be loaded by the platform class loader. The following is defined in the build.properties file for the javafx.swing module (as well as all other JavaFX modules), which is used by the JDK build when using --with-import-modules:

classloader=ext

I would guess that ext (which used to stand for the extension class loader) is causing it be included in the list of modules that are loaded by the platform class loader, but that needs to be checked.

AlanBateman · 2022-09-20T12:35:58Z

I haven't built a JDK using the --with-import-modules to include the javafx.* modules in quite a while, but my recollection is that yes, that did cause the JavaFX modules to be loaded by the platform class loader. The following is defined in the build.properties file for the javafx.swing module (as well as all other JavaFX modules), which is used by the JDK build when using --with-import-modules:
classloader=ext

Thanks for this. In make/common/Modules.gmk, ReadSingleImportMetaData is adding to PLATFORM_MODULES. So I think this needs to re-examined as there should be no reason to have imported modules mapped to the boot or platform class loaders now.

kevinrushforth · 2022-09-20T14:11:55Z

In that case, a solution might be to remove ext from the classloader property from all of the modules/javafx.*/make/build.properties files in JavaFX:

classloader=

or

classloader=app

xiangzhai · 2022-09-21T00:41:46Z

Hi @kevinrushforth

Thanks for your hint!

@dumasun is building jfx with the quick debug patch:

diff --git a/modules/javafx.swing/make/build.properties b/modules/javafx.swing/make/build.properties
index f913584cb4..5d04f77111 100644
--- a/modules/javafx.swing/make/build.properties
+++ b/modules/javafx.swing/make/build.properties
@@ -29,4 +29,4 @@ include_in_jre=true
 include_in_jdk=true
 include_in_jdk_server=false
 include_in_docs=true
-classloader=ext
+classloader=app

Thanks,
Leslie Zhai

dumasun · 2022-09-21T07:38:58Z

jdk/modules/etc/VerifyModuleDelegation.java has passed for building jfx with the debug patch.
Thank you very much.

diff --git a/modules/javafx.swing/make/build.properties b/modules/javafx.swing/make/build.properties
index f913584cb4..5d04f77111 100644
--- a/modules/javafx.swing/make/build.properties
+++ b/modules/javafx.swing/make/build.properties
@@ -29,4 +29,4 @@ include_in_jre=true
 include_in_jdk=true
 include_in_jdk_server=false
 include_in_docs=true
-classloader=ext
+classloader=app

xiangzhai · 2022-09-21T07:48:55Z

Hi @kevinrushforth

In that case, a solution might be to remove ext from the classloader property from all of the modules/javafx.*/make/build.properties files in JavaFX:

Thanks for your patch!

So I will close this PR. Then create a new PR for the jfx repository instead? or just wait JDK-8294095.

Also thank @dumasun for your good job!

Cheers,
Leslie Zhai

magicus · 2022-09-21T08:35:30Z

@AlanBateman While this particular issue was closed now, this is perhaps yet another indication that the "import modules" thing is causing problems, and should be removed now that it is not used anymore. I created JDK-8294095 to explore this possibility.

kevinrushforth · 2022-09-21T12:49:17Z

While this particular issue was closed now, this is perhaps yet another indication that the "import modules" thing is causing problems, and should be removed now that it is not used anymore. I created JDK-8294095 to explore this possibility.

Other than the issue of adding the imported FX modules in the platform classes, they are successfully using the "import modules" feature. I don't see why it needs to be removed.

magicus · 2022-09-21T15:05:21Z

See JDK-8294095 for a discussion on the rationale. In short, it is because imported modules are intrusive in the build system, and it advertises a kind of "generality" that it is not designed for.

AlanBateman · 2022-09-25T06:30:46Z

configure --with-import-modules is of its time. It probably should have been removed in JDK 11 but there are people using it now so I think we are stuck with it. It's way more intrusive than it should be. JDK 9/10 had its need but things have moved on significantly. So if it stays then maybe it could be changed so that the imported modules do not participate in the mapping to class loaders or the hashing. Instead, the import could just provide input into the jlink command used to create the final images. That should be a lot less intrusive and would give us the nice property that it would be equivalent to running jlink outside of the JDK build that produces a run-time image that contains the JavaFX or other imported modules.

8293977: jdk/modules/etc/VerifyModuleDelegation.java fail with jfx

ee6c707

openjdk bot added the rfr label Sep 19, 2022

openjdk bot added the core-libs label Sep 19, 2022

xiangzhai closed this Sep 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8293977: jdk/modules/etc/VerifyModuleDelegation.java fail with jfx #10328

8293977: jdk/modules/etc/VerifyModuleDelegation.java fail with jfx #10328

xiangzhai commented Sep 19, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 19, 2022

openjdk bot commented Sep 19, 2022

mlbridge bot commented Sep 19, 2022

AlanBateman commented Sep 19, 2022 •

edited

Loading

xiangzhai commented Sep 19, 2022 •

edited

Loading

AlanBateman commented Sep 19, 2022

dumasun commented Sep 19, 2022 •

edited

Loading

AlanBateman commented Sep 20, 2022

dumasun commented Sep 20, 2022

AlanBateman commented Sep 20, 2022

kevinrushforth commented Sep 20, 2022

AlanBateman commented Sep 20, 2022

kevinrushforth commented Sep 20, 2022

xiangzhai commented Sep 21, 2022

dumasun commented Sep 21, 2022

xiangzhai commented Sep 21, 2022 •

edited

Loading

magicus commented Sep 21, 2022

kevinrushforth commented Sep 21, 2022

magicus commented Sep 21, 2022

AlanBateman commented Sep 25, 2022

8293977: jdk/modules/etc/VerifyModuleDelegation.java fail with jfx #10328

8293977: jdk/modules/etc/VerifyModuleDelegation.java fail with jfx #10328

Conversation

xiangzhai commented Sep 19, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewing

bridgekeeper bot commented Sep 19, 2022

openjdk bot commented Sep 19, 2022

mlbridge bot commented Sep 19, 2022

Webrevs

AlanBateman commented Sep 19, 2022 • edited Loading

xiangzhai commented Sep 19, 2022 • edited Loading

AlanBateman commented Sep 19, 2022

dumasun commented Sep 19, 2022 • edited Loading

AlanBateman commented Sep 20, 2022

dumasun commented Sep 20, 2022

AlanBateman commented Sep 20, 2022

kevinrushforth commented Sep 20, 2022

AlanBateman commented Sep 20, 2022

kevinrushforth commented Sep 20, 2022

xiangzhai commented Sep 21, 2022

dumasun commented Sep 21, 2022

xiangzhai commented Sep 21, 2022 • edited Loading

magicus commented Sep 21, 2022

kevinrushforth commented Sep 21, 2022

magicus commented Sep 21, 2022

AlanBateman commented Sep 25, 2022

xiangzhai commented Sep 19, 2022 •

edited by openjdk bot

Loading

AlanBateman commented Sep 19, 2022 •

edited

Loading

xiangzhai commented Sep 19, 2022 •

edited

Loading

dumasun commented Sep 19, 2022 •

edited

Loading

xiangzhai commented Sep 21, 2022 •

edited

Loading