[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-jigsaw-dev
Subject: Re: Ping - Re: RFR 8078812, Test RMI with client and servers as modules
From: Felix Yang <felix.yang () oracle ! com>
Date: 2016-04-29 5:08:10
Message-ID: bfbb3bfe-6a60-49d0-ef7a-33ca5f7e94f8 () oracle ! com
[Download RAW message or body]
Hi Stuart,
thanks a lot for the comments!
-Felix
On 2016/4/28 9:06, Stuart Marks wrote:
> Hi Felix,
>
> I finally got a chance to take a good look at this. Again, sorry for
> the delays. I was able to reproduce the failure and find a fix for it.
> There are also some structural issues with the test that will require
> some improvements.
>
> * * *
>
> First is the way the test fixture is set up. The test now has a single
> copy of the source code for the client, server, and app, and they're
> compiled once. This is good. Unfortunately, the tests modify the
> filesystem in order to set up different cases for the classes' modular
> structure.
>
> The problem is that makes the tests dependent on each other. The
> Test-NG directives enforce this execution order, but it makes them
> really hard to work with. For example, if I were to disable test #2,
> this would cause test #3 to fail since #3 depends on side effects from
> test #2. Also, if test #2 were to fail, it's hard to debug, since test
> #3 modifies the fixture after test #2 runs, potentially obscuring any
> problems that #2 might have run into.
>
> Ideally the test should set up a single test fixture and not modify
> it, allowing the different cases to be run independently. Offhand I'm
> not sure of the best way to do this.
>
> One approach that might be worth exploring is to create several
> different jar files from the class files; the jar files would have the
> classes in different modular configurations. Then, each test would
> invoke a JVM with arguments to put the different jar files on the
> classpath or the module path. I haven't investigated this approach,
> though, so I don't know if it's practical.
>
> Other approaches may be viable as well. It's probably worth discussing
> and prototyping an approach first before you spend too much time
> implementing any particular scheme.
It was chosen to compile once considering efficiency. If we prefer each
test can be executed separately, it looks enough to just change
"@BeforeTest" to "@BeforeMethod" and update tests slightly.
With jar files, may I understand it is a different scenario with
automatic modules. I suggest to add a new test in future enhancement.
See http://cr.openjdk.java.net/~xiaofeya/8078812/webrev.02
-Felix
>
> * * *
>
> Moving on to the actual test failure, the message from
> testUnnamedApp() is as follows:
>
> ==================================================
> Command line:
> [/Users/src/jdk-dev/jdk9-dev/build/macosx-x86_64-normal-server-fastdebug/jdk/bin/java \
>
> -ea -esa -mp mods
> -Djava.rmi.server.codebase=file:/Users/src/jdk-dev/jdk9-dev/jdk/testoutput/JTwork/java/rmi/module/ModuleTest/./mods/mserver/ \
>
> -cp classes testpkg.DummyApp 65071 ]
>
> Error: A JNI error has occurred, please check your installation and
> try again
> Exception in thread "main" java.lang.NoClassDefFoundError:
> serverpkg/Hello
> at java.lang.Class.getDeclaredMethods0(java.base/Native Method)
> at
> java.lang.Class.privateGetDeclaredMethods(java.base/Class.java:2937)
> at
> java.lang.Class.privateGetMethodRecursive(java.base/Class.java:3282)
> at java.lang.Class.getMethod0(java.base/Class.java:3252)
> at java.lang.Class.getMethod(java.base/Class.java:1961)
> at
> sun.launcher.LauncherHelper.validateMainClass(java.base/LauncherHelper.java:648)
> at
> sun.launcher.LauncherHelper.checkAndLoadMain(java.base/LauncherHelper.java:499)
> Caused by: java.lang.ClassNotFoundException: serverpkg.Hello
> at
> jdk.internal.loader.BuiltinClassLoader.loadClass(java.base/BuiltinClassLoader.java:366)
> at
> jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(java.base/ClassLoaders.java:184)
> at java.lang.ClassLoader.loadClass(java.base/ClassLoader.java:419)
> ... 7 more
>
> test ModuleTest.testUnnamedApp(): failure
> java.lang.AssertionError: expected [0] but found [1]
> [remainder of stack trace elided]
> ==================================================
>
> The main class is testpkg.DummyApp, but the launcher fails with
> NoClassDefFoundError on serverpkg/Hello. This was a bit of a puzzle,
> but I finally figured out that this is occurring during the
> verification of the DummyApp class. DummyApp depends on Hello, which
> can't be found, so it fails verification before DummyApp.main() gets
> called.
>
> It's also strange that it says "A JNI error has occurred" but that
> might be a red herring.
>
> In any case the real issue is that serverpkg.Hello can't be found. The
> reason it can't be found is that it's in a module. That module is on
> the module path, but the module isn't resolved by default. Thus its
> classes aren't visible to classes in the unnamed module.
>
> To add modules to the set of resolved modules, use the -addmods
> option. In this case, testUnnamedApp() has the client and server
> classes in modules, so it needs to have the arguments
>
> -addmods mclient,mserver
>
> added to its command line. Since the mclient module explicitly depends
> on the mserver module, this could instead just e
>
> -addmods mclient
>
> The testUnnamedAppandClient() method has only the server in a named
> module, so it needs to have
>
> -addmods mserver
>
> on its command line.
>
> With these changes, all the tests pass.
Thanks for figuring this out. But do you think it is worth to
investigate the " NoClassDefFoundError " and "A JNI error has occurred".
The root cause is not well-indicated.
-Felix
>
> * * *
>
> There are a couple other things that could be improved as well. I'll
> just mention them here (so I don't forget) but we should work on these
> later:
>
> 1) The registry and codebase stuff is probably unnecessary and can be
> removed. These might be interesting test cases for the future. It's
> not clear to me that we need to support modules in the RMI codebase,
> but at least there should be tests for cases we think are important.
Excuse me, could you explain what to remove in detail? I agree the
scenarios can be various beyond the sanity ones.
-Felix
>
> 2) In DummyApp, if creating the Client fails (e.g., because of
> NoClassDefFoundError), that error will propagate out of the main()
> method, bypassing the calls to System.exit(). The Server object has
> already been exported, which will cause the JVM to live indefinitely.
> This causes the test to hang and eventually time out, and to leave
> stale JVM processes after the test run.
There is explicit System.exit(-1) for error situation, so I didn't
observe such stale process. Any way, I updated it slightly.
-Felix
>
> In any case, I think the next step is to come up with a good design
> for varying the modular structure of the classes for the different
> test cases, without modifying the filesystem. Let's work on that, and
> we can work on the other stuff later.
>
> s'marks
>
>
>
>
>
> On 4/14/16 1:45 AM, Felix Yang wrote:
> > Hi Stuart,
> > any comment on the new tests and issue?
> >
> > Thanks,
> > Felix
> > On 2016/4/8 15:52, Felix Yang wrote:
> > > Hi Stuart,
> > > thanks for the comments. I adjusted the test. The test failed
> > > with
> > > NoClassDefFoundErr if client/server are in named module but dummy
> > > app in
> > > unnamed module. See https://bugs.openjdk.java.net/browse/JDK-8153833
> > >
> > > New webrev: http://cr.openjdk.java.net/~xiaofeya/8078812/webrev.01/
> > > <http://cr.openjdk.java.net/%7Exiaofeya/8078812/webrev.01/>
> > >
> > > About simplifying the scenario, I replaced inheritance by moving
> > > classes in
> > > runtime.
> > >
> > > Thanks,
> > > Felix
> > >
> > > On 2016/3/2 10:28, Stuart Marks wrote:
> > > > Hi Felix, sorry for the delay.
> > > >
> > > > Several comments below.
> > > >
> > > > --------------------------------------------------
> > > >
> > > > 119 // wait for rmiregistry to be fully ready
> > > > 120 Thread.sleep(3000);
> > > >
> > > > There are some RMI test utilities that will handle starting up and
> > > > waiting
> > > > for the registry to be ready. Unfortunately they're in the unnamed
> > > > package
> > > > (still haven't cleaned that up) so you can't use them from this test.
> > > >
> > > > For now I'd recommend scaling the timeout by the
> > > > test.timeout.factor, so that
> > > > this won't fail on slow systems. There's a test utility for that; see
> > > > Utils.adjustTimeout().
> > > >
> > > > --------------------------------------------------
> > > >
> > > > The directory "unamed" is misspelled; it has classes in the unnamed
> > > > module.
> > > > This misspelling is reflected in variable names and values, e.g.,
> > > >
> > > > 59 private static final Path UNAMED_SRC_DIR =
> > > > Paths.get(TEST_SRC,
> > > > "unamed");
> > > > 60 private static final Path MODS_OUTPUT_DIR =
> > > > Paths.get("mods");
> > > > 61 private static final Path UNAMED_OUTPUT_DIR =
> > > > Paths.get("unamed");
> > > >
> > > > While spelling errors aren't that big a deal, since this involves
> > > > file and
> > > > directory names, I'd recommend fixing this now. It'll just be
> > > > harder to fix
> > > > it later.
> > > >
> > > > Also, "SeperateModuleClient" => "SeparateModuleClient"
> > > >
> > > > --------------------------------------------------
> > > >
> > > > Overall the structure of the test seems reasonable to test various
> > > > clients
> > > > and servers in combinations of the same, different, and unnamed
> > > > modules.
> > > >
> > > > I'm not entirely sure what p2.SeperateModuleClient is testing. It
> > > > extends
> > > > p1.Client and its constructor and testStub() method simply call the
> > > > corresponding methods on super, so it doesn't seem to be testing
> > > > anything
> > > > different from p1.Client running against p3.Server.
> > > >
> > > > Also, p4.UnamedModuleClient seems to want to run the client from
> > > > the unnamed
> > > > module, but it too extends p1.Client and simply defers all of its
> > > > execution
> > > > to its superclass. So I don't think this actually testing an RMI
> > > > client call
> > > > originating from an unnamed module.
> > > >
> > > > There is an uncomfortable amount of duplication between
> > > > mtest/test/DummyApp.java and p4/UnamedModuleDummyApp. I see what
> > > > this is
> > > > trying to do, which is to test a RMI server from an unnamed module. It
> > > > instantiates p3.Server, which resides in a named module, but
> > > > exports it from
> > > > an unnamed module.
> > > >
> > > > So I think there's some tension here. There's subclassing among the
> > > > clients
> > > > that attempts to avoid duplication, which is good, but it doesn't
> > > > truly seem
> > > > to be testing clients in different modules and in an unnamed
> > > > module. The
> > > > server, on the other hand, does seem to be testing things properly in
> > > > different modules, at the cost of duplicating all the code into
> > > > another class
> > > > that resides in a different (unnamed) module.
> > > >
> > > > I'm not entirely sure what the best approach is here. Ideally you'd
> > > > want to
> > > > have a single implementation of client, server + remote interface, and
> > > > application, and compile each of them once. Then since you're
> > > > invoking a new
> > > > JVM for each test, invoke it with different options to put the
> > > > client, or the
> > > > server, or the app, into modules, or onto the classpath (to get
> > > > into an
> > > > unnamed module). You might have to copy compiled class files to
> > > > different
> > > > locations so that different classes can be either on the classpath
> > > > or in a
> > > > named module without causing any conflicts.
> > > >
> > > > I'm willing to entertain some simplifications here as well. It
> > > > might be
> > > > sufficient to deal with just clients and servers in different/unnamed
> > > > modules, and not worry about putting the application into different
> > > > modules.
> > > > That should reduce the number of cases to cover.
> > > >
> > > > s'marks
> > > >
> > > > On 2/29/16 10:05 PM, Felix Yang wrote:
> > > > > Ping...
> > > > >
> > > > > -Felix
> > > > > On 2016/2/24 14:06, Felix Yang wrote:
> > > > > > Hi all,
> > > > > > please review the new tests to use RMI in module world.
> > > > > >
> > > > > > Webrev:
> > > > > > http://cr.openjdk.java.net/~xiaofeya/8078812/webrev.00/
> > > > > > Bug:
> > > > > > https://bugs.openjdk.java.net/browse/JDK-8078812
> > > > > >
> > > > > > Thanks,
> > > > > > Felix
> > > > >
> > >
> >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic