答复: RFR for JDK-7190106 RMI benchmark fails intermittently because of use of fixed port
Tristan Yan
tristan.yan at oracle.com
Tue Nov 26 05:12:30 UTC 2013
Hi Stuart
Thanks for your code review. I updated the webrev according your
suggestion. Could you review it again?
http://cr.openjdk.java.net/~ewang/tristan/JDK-7190106/webrev.00/
Tristan
On 11/26/2013 10:36 AM, Stuart Marks wrote:
> Hi Tristan,
>
> Finally getting back to this. Again, sorry for the delay.
>
> The changes look much better now. I've listed a bunch of items below,
> but they're all comparatively minor, even nitpicky. But there are a
> few things that should be cleaned up before we integrate this.
>
> Items listed below.
>
>
> ** bench/serial/Main.java
>
>
> - The description should simply be "The Serialization benchmark
> test". This test has nothing to do with RMI, even though it's under
> the java/rmi hierarchy in the filesystem.
>
> - parseArgs() uses strings-in-switch! Good, but put "break" on its
> own line instead of following the close-brace of the catch clause.
> (rmi/Main.java doesn't have this issue.)
>
>
> ** bench/rmi/Main.java
>
>
> - Imports of java.util.logging.Level and Logger are unused?
>
> - Missing "-server" line from usage().
>
> - Interesting use of enum. Note by convention an enum is like a class
> and names are in mixed case, thus use OutputFormat instead of
> OUTPUT_FORMAT. Also note that the enum doesn't buy much, at least in
> terms of lines of code, since the enum declaration and enum instance
> overrides add about as much code as the case statement that got
> removed from setupReporter(). It does buy a bit of type-safety,
> though, so might as well leave it in.
>
> - Enum instance HTML should be on a new line, like XML.
>
> - Reflection code can be chained instead of declaring several locals.
> This is mainly a matter of taste, but to my eye it's cleaner. The main
> advantage is avoiding the need to come up with names for intermediate
> locals. For example:
>
> port = (int) Class.forName("TestLibrary")
> .getMethod("getUnusedRandomPort")
> .invoke(null);
>
> - Catch clause at 389 can be of ReflectiveOperationException. This
> covers everything except IllegalArgumentException, which is unchecked,
> so you don't need to catch it.
>
> (Not sure why Method.invoke is declared to throw
> IllegalArgumentException, since generally only checked exceptions are
> declared in the throws clause.)
>
> - Line 416, no need to mention RMISecurityManager in a comment, just
> make the change to use SecurityManager.
>
> - It's kind of surprising that TEST_SRC_PATH appends the file
> separator to the test.src property. At line 241 test.src has to be
> fetched again to use it without the file separator.
>
> - Line 234, instead of the java.home property, use the test.jdk
> property. This will use the JDK under test instead of the JDK that's
> running jtreg. In practice it's unclear whether this makes any actual
> difference today, but it's good to try to keep this separation. Also,
> use file separators here instead of appending "/bin/java".
>
> (Hmmm. I observe that the RMI testlibrary invokes JVM subprocesses
> using java.home.)
>
>
> Thanks,
>
>
> s'marks
>
>
> On 11/20/13 1:49 PM, Stuart Marks wrote:
>> Hi, sorry about the delay, I'm still backlogged from traveling. I'll
>> get to this
>> soon.
>>
>> s'marks
>>
>> On 11/19/13 6:27 PM, Tristan Yan wrote:
>>> Hi Stuart
>>> Did you get chance to review it again.
>>> Let me know if you have any new comments or suggestions.
>>> Thanks
>>> Tristan
>>>
>>> -----邮件原件-----
>>> 发件人: Tristan Yan
>>> 发送时间: Thursday, November 14, 2013 11:09 PM
>>> 收件人: Stuart Marks
>>> 抄送: core-libs-dev at openjdk.java.net
>>> 主题: 答复: RFR for JDK-7190106 RMI benchmark fails intermittently
>>> because of
>>> use of fixed port
>>>
>>> Thank you Stuart
>>> It took me a little time to correct the code. sorry be late. This is
>>> new
>>> webrev for the code change. Please help to review it again.
>>>
>>> Description:
>>> 1. Convert shell script test to Java program test.
>>> 2. Using random server port by reusing Darryl Mocek's
>>> work(TestLibrary.getUnusedRandomPort) to replace fixed server port.
>>> 3. Because TestLibrary doesn't have package. Here is using
>>> reflection to call
>>> TestLibrary.getUnusedRandomPort. This is going to change when
>>> TestLibrary
>>> moves to named package.
>>> 4. Using java Process class to start client process. Client and
>>> server are
>>> sharing IO.
>>> 5. Also convert other shell script test runSerialBench.sh to java
>>> program test
>>> also.
>>> 6. ProblemList has been changed to get back
>>> java/rmi/reliability/benchmark/runRmiBench.sh test.
>>>
>>> http://cr.openjdk.java.net/~pzhang/Tristan/7190106/webrev/
>>>
>>> Thank you so much
>>> Tristan
>>>
>>>
>>> -----邮件原件-----
>>> 发件人: Stuart Marks
>>> 发送时间: Tuesday, November 12, 2013 11:38 PM
>>> 收件人: Tristan Yan
>>> 抄送: core-libs-dev at openjdk.java.net; Alexandre (Shura) Iline
>>> 主题: Re: RFR for JDK-7190106 RMI benchmark fails intermittently
>>> because of
>>> use of fixed port
>>>
>>> Unfortunately we can't use jdk.testlibrary.Utils.getFreePort() for
>>> the RMI
>>> tests, since RMI's TestLibrary.getUnusedRandomPort() respects a
>>> "reserved"
>>> port range that's used by some RMI tests that have to used fixed ports.
>>>
>>> s'marks
>>>
>>> On 11/11/13 2:39 PM, Tristan Yan wrote:
>>>> Hi Stuart
>>>> Also there is one more solution, which is there is one
>>>> jdk.testlibrary.Utils.getFreePort() method under test/lib. It's same
>>>> function as
>>>> TestLibrary.getUnusedRandomPort() and it has named package. Do you
>>>> mind I use this one?
>>>> Since these two functions provide same functionality. Maybe we should
>>>> think about to merge them at the same time.
>>>> Thank you
>>>> Tristan
>>>>
>>>> On 11/10/2013 11:19 AM, Tristan Yan wrote:
>>>>> Hi Stuart
>>>>> I tried your suggestion but unfortunately all the benchmarks have
>>>>> dependencies to Main class because they need get stub from server. I
>>>>> suggest we move the benchmark tests to unnamed package unless we do
>>>>> want to put TestLibrary into a named package right now.
>>>>> Please let me know if you have objection on this.
>>>>> Thank you
>>>>> Tristan
>>>>>
>>>>> On 11/09/2013 02:28 AM, Stuart Marks wrote:
>>>>>> Hi Tristan,
>>>>>>
>>>>>> Yes, it's kind of a problem that the RMI TestLibrary is in the
>>>>>> unnamed package. Classes in a named package cannot import classes
>>>>>> from the unnamed package. We've run into problems with this before.
>>>>>> Eventually, we should move TestLibrary a named package.
>>>>>>
>>>>>> I think it's possible to work around this without too much
>>>>>> difficulty. Note that classes in the unnamed package can import
>>>>>> classes
>>>>>> from named packages.
>>>>>> So, perhaps you can put the RmiBench main class in the unnamed
>>>>>> package so it has access to TestLibrary. Then have the benchmarks
>>>>>> themselves in the bench.rmi package. The config file already
>>>>>> references the benchmarks by fully qualified class name (e.g.,
>>>>>> "bench.rmi.NullCalls") so with a bit of tinkering you ought to be
>>>>>> able to
>>>>>> get this to work.
>>>>>>
>>>>>> s'marks
>>>>>>
>>>>>> On 11/8/13 3:00 AM, Tristan Yan wrote:
>>>>>>> Thank you, Stuart
>>>>>>> There is one review point I want to ask you opinion. Which is the
>>>>>>> reason that I moved from
>>>>>>> test/java/rmi/reliability/benchmark/bench/rmi to
>>>>>>> test/java/rmi/reliability/benchmark is Main.java need access class
>>>>>>> TestLibrary for supporting random port. TestLibrary is a unpackage
>>>>>>> class, I couldn't find a way to let a class which has Package to
>>>>>>> access
>>>>>>> the class without package. Do you have suggestion on that?
>>>>>>> Thank you so much.
>>>>>>> Tristan
>>>>>>>
>>>>>>> On 11/06/2013 09:50 AM, Stuart Marks wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 11/1/13 9:18 AM, Tristan Yan wrote:
>>>>>>>>> Hi Everyone
>>>>>>>>> http://cr.openjdk.java.net/~pzhang/Tristan/7190106/webrev/
>>>>>>>>>
>>>>>>>>> Description:
>>>>>>>>> 1. Convert shell script test to Java program test.
>>>>>>>>> 2. Using random server port by reusing Darryl Mocek's work to
>>>>>>>>> replace fixed server port.
>>>>>>>>> 3. Using java Process class to start client process.
>>>>>>>>> 4. Also convert other shell script test runSerialBench.sh to java
>>>>>>>>> program test also
>>>>>>>>
>>>>>>>> Hi Tristan,
>>>>>>>>
>>>>>>>> Several comments on this webrev.
>>>>>>>>
>>>>>>>>
>>>>>>>> ** The original arrangement within the
>>>>>>>> test/java/rmi/reliability/benchmark
>>>>>>>> directory had the main benchmark files (scripts) at the top, some
>>>>>>>> benchmark framework files in the "bench" subdirectory, and the
>>>>>>>> actual RMI and serialization benchmarks in bench/rmi and
>>>>>>>> bench/serial
>>>>>>>> subdirectories.
>>>>>>>>
>>>>>>>> The webrev moves all the RMI benchmarks to the top benchmark
>>>>>>>> directory but leaves the serial benchmarks in bench/serial. The
>>>>>>>> RMI benchmarks are now all cluttering the top directory, but the
>>>>>>>> main serial benchmark test is now buried in the bench/serial
>>>>>>>> directory. The nice organization that was there before is now
>>>>>>>> spoiled. Is this rearrangement necessary in order to convert
>>>>>>>> the scripts
>>>>>>>> to Java? I would prefer the original arrangement be left in place.
>>>>>>>>
>>>>>>>>
>>>>>>>> ** The RMI benchmark Main.java file has a @run tag of the form,
>>>>>>>>
>>>>>>>> @run main/othervm/policy=policy.all/timeout=1800 -server Main
>>>>>>>> -c config
>>>>>>>>
>>>>>>>> There is a subtle but serious problem here: the -server option is
>>>>>>>> passed to the >>JVM<< and not as an argument to the main() method.
>>>>>>>> The main() method gets neither a -server nor a -client argument,
>>>>>>>> so its default "run mode" as defined by the benchmark itself is
>>>>>>>> SAMEVM. This runs the client and server in the same JVM, which is
>>>>>>>> different from the shell script, which ran separate client and
>>>>>>>> server JVMs.
>>>>>>>>
>>>>>>>>
>>>>>>>> ** The logic to process the -server argument still expects to take
>>>>>>>> a port, even though the port is assigned automatically. So the
>>>>>>>> obvious fix to the above,
>>>>>>>>
>>>>>>>> @run main/othervm/policy=policy.all/timeout=1800 Main -server
>>>>>>>> -c config
>>>>>>>>
>>>>>>>> doesn't work, since a port is missing. The logic to increment the
>>>>>>>> argument index to collect the port argument should be removed.
>>>>>>>> Also, the -server line should be restored to the usage message,
>>>>>>>> but
>>>>>>>> without the port argument.
>>>>>>>>
>>>>>>>>
>>>>>>>> ** After this is done, the client's command line is constructed
>>>>>>>> improperly.
>>>>>>>> The command line ends up looking something like this:
>>>>>>>>
>>>>>>>> java client -cp <classpath> Main client localhost:58583 -c
>>>>>>>> config
>>>>>>>>
>>>>>>>> The word "client" appears twice, but what's really required is
>>>>>>>> "-client" to appear as an argument after Main.
>>>>>>>>
>>>>>>>>
>>>>>>>> ** The client is run using ProcessBuilder, which by default sends
>>>>>>>> stdout and stderr to pipes to be read by the parent. But the
>>>>>>>> parent never reads them, thus any messages from the client are
>>>>>>>> never seen. The client is the one that emits the benchmark report,
>>>>>>>> so its output needs to be seen. It might be sufficient to have the
>>>>>>>> client inherit the parent's stdout and stderr. This might intermix
>>>>>>>> the client's and server's output, but it's better than nothing.
>>>>>>>>
>>>>>>>>
>>>>>>>> ** The config file is checked with the following code:
>>>>>>>>
>>>>>>>> try {
>>>>>>>> confFile = args[i];
>>>>>>>> confstr = new
>>>>>>>> FileInputStream(System.getProperty("test.src")
>>>>>>>> + System.getProperty("file.separator") +
>>>>>>>> confFile);
>>>>>>>> } catch (IOException e) {
>>>>>>>> die("Error: unable to open \"" + args[i] + "\"");
>>>>>>>> }
>>>>>>>>
>>>>>>>> This is potentially misleading, as the message doesn't print the
>>>>>>>> actual filename that was attempted to be opened.
>>>>>>>>
>>>>>>>> This is important, as the test.src property doesn't exist in
>>>>>>>> the client JVM.
>>>>>>>>
>>>>>>>> Note that the original shell script passed full pathnames for the
>>>>>>>> config file to both the client and the server. The new @run tag
>>>>>>>> merely says "-c config" which redefines the config filename to be
>>>>>>>> relative to the test.src directory. You could pass -Dtest.src=...
>>>>>>>> to the client, but it seems like there should be something
>>>>>>>> better than
>>>>>>>> can be done.
>>>>>>>>
>>>>>>>>
>>>>>>>> ** The client needs to have its security policy set up. This is
>>>>>>>> missing from the construction of the client's command line.
>>>>>>>>
>>>>>>>>
>>>>>>>> ** ProcessBuilder takes a List<String> for its command; there is
>>>>>>>> no need to turn the list into an array.
>>>>>>>>
>>>>>>>>
>>>>>>>> ** In the benchmark main methods, code of the form,
>>>>>>>>
>>>>>>>> while (true) {
>>>>>>>> runBenchmarks();
>>>>>>>> if (exitRequested) {
>>>>>>>> System.exit();
>>>>>>>> }
>>>>>>>> }
>>>>>>>>
>>>>>>>> was replaced with
>>>>>>>>
>>>>>>>> while (!exitRequested) {
>>>>>>>> runBenchmarks();
>>>>>>>> }
>>>>>>>>
>>>>>>>> This is a subtle logic change, in that the former code always
>>>>>>>> executed the loop at least once. It seems unlikely, but it's
>>>>>>>> possible that a timer could set exitRequested before loop entry,
>>>>>>>> resulting in the benchmark running zero times. I guess, if you
>>>>>>>> really want to clean this up (we do need to avoid System.exit
>>>>>>>> in jtreg
>>>>>>>> tests), use a do-while loop instead.
>>>>>>>>
>>>>>>>>
>>>>>>>> ** Don't forget to remove the 7190106/runRmiBench.sh entry from
>>>>>>>> ProblemList.txt.
>>>>>>>>
>>>>>>>>
>>>>>>>> ** Remove the references to RMISecurityManager and just use
>>>>>>>> SecurityManager. This is just general cleanup. (I deprecated
>>>>>>>> RMISecurityManager last week.) :-)
>>>>>>>>
>>>>>>>>
>>>>>>>> It would be good if you could fix up these issues and post
>>>>>>>> another webrev.
>>>>>>>>
>>>>>>>> Thanks.
>>>>>>>>
>>>>>>>> s'marks
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> Thank you
>>>>>>>>> Tristan
>>>>>>>>>
>>>>>>>>> On 01/11/2013 23:58, Stuart Marks wrote:
>>>>>>>>>> On 10/31/13 10:22 PM, Tristan Yan wrote:
>>>>>>>>>>> I am working on bug
>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-7190106. Based on my
>>>>>>>>>>> research, it looks like the issue of fixed port was already
>>>>>>>>>>> addressed by Stuart Marks in other RMI tests which are Java
>>>>>>>>>>> based. I
>>>>>>>>>>> would like to reuse his solution, however it does not work
>>>>>>>>>>> for shell
>>>>>>>>>>> based tests.
>>>>>>>>>>
>>>>>>>>>> (Darryl Mocek did the unique port work for the RMI tests.)
>>>>>>>>>>
>>>>>>>>>> Was the patch attached to your message? If so, it didn't get
>>>>>>>>>> through. Most OpenJDK mailing lists strip off attachments before
>>>>>>>>>> forwarding Hi Stuart Also there is one more solution, which is
>>>>>>>>>> there is one
>>>>>>>>>> jdk.testlibrary.Utils.getFreePort() method under test/lib. It's
>>>>>>>>>> same function as TestLibrary.getUnusedRandomPort() and it has
>>>>>>>>>> named
>>>>>>>>>> package.
>>>>>>>>>> Do you mind I use this one?
>>>>>>>>>> Since these two function provide same functionality. Maybe we
>>>>>>>>>> should think about to merge them.
>>>>>>>>>> Thank you
>>>>>>>>>> Tristan
>>>>>>>>>>
>>>>>>>>>> On 11/10/2013 11:19 AM, Tristan Yan wrote:
>>>>>>>>>>> Hi Stuart
>>>>>>>>>>> I tried your suggestion but unfortunately all the benchmarks
>>>>>>>>>>> have dependencies to Main class because they need get stub from
>>>>>>>>>>> server. I suggest we move the benchmark tests to unnamed
>>>>>>>>>>> package unless we do want to put TestLibrary into a named
>>>>>>>>>>> package
>>>>>>>>>>> right now.
>>>>>>>>>>> Please let me know if you have objection on this.
>>>>>>>>>>> Thank you
>>>>>>>>>>> Tristan
>>>>>>>>>>>
>>>>>>>>>>> On 11/09/2013 02:28 AM, Stuart Marks wrote:
>>>>>>>>>>>> Hi Tristan,
>>>>>>>>>>>>
>>>>>>>>>>>> Yes, it's kind of a problem that the RMI TestLibrary is in the
>>>>>>>>>>>> unnamed package. Classes in a named package cannot import
>>>>>>>>>>>> classes from the unnamed package. We've run into problems with
>>>>>>>>>>>> this before. Eventually, we should move TestLibrary a named
>>>>>>>>>>>> package.
>>>>>>>>>>>>
>>>>>>>>>>>> I think it's possible to work around this without too much
>>>>>>>>>>>> difficulty.
>>>>>>>>>>>> Note that classes in the unnamed package can import classes
>>>>>>>>>>>> from named packages. So, perhaps you can put the RmiBench main
>>>>>>>>>>>> class in the unnamed package so it has access to TestLibrary.
>>>>>>>>>>>> Then have the benchmarks themselves in the bench.rmi package.
>>>>>>>>>>>> The config file already references the benchmarks by fully
>>>>>>>>>>>> qualified class name (e.g.,
>>>>>>>>>>>> "bench.rmi.NullCalls") so with a bit of tinkering you ought to
>>>>>>>>>>>> be able to get this to work.
>>>>>>>>>>>>
>>>>>>>>>>>> s'marks
>>>>>>>>>>>>
>>>>>>>>>>>> On 11/8/13 3:00 AM, Tristan Yan wrote:
>>>>>>>>>>>>> Thank you, Stuart
>>>>>>>>>>>>> There is one review point I want to ask you opinion. Which is
>>>>>>>>>>>>> the reason that I moved from
>>>>>>>>>>>>> test/java/rmi/reliability/benchmark/bench/rmi
>>>>>>>>>>>>> to test/java/rmi/reliability/benchmark is Main.java need
>>>>>>>>>>>>> access class TestLibrary for supporting random port.
>>>>>>>>>>>>> TestLibrary is a unpackage class, I couldn't find a way to
>>>>>>>>>>>>> let a class which has Package to access the class without
>>>>>>>>>>>>> package.
>>>>>>>>>>>>> Do you have suggestion on that?
>>>>>>>>>>>>> Thank you so much.
>>>>>>>>>>>>> Tristan
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 11/06/2013 09:50 AM, Stuart Marks wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 11/1/13 9:18 AM, Tristan Yan wrote:
>>>>>>>>>>>>>>> Hi Everyone
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~pzhang/Tristan/7190106/webrev/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Description:
>>>>>>>>>>>>>>> 1. Convert shell script test to Java program test.
>>>>>>>>>>>>>>> 2. Using random server port by reusing Darryl Mocek's work
>>>>>>>>>>>>>>> to replace fixed server port.
>>>>>>>>>>>>>>> 3. Using java Process class to start client process.
>>>>>>>>>>>>>>> 4. Also convert other shell script test runSerialBench.sh
>>>>>>>>>>>>>>> to java program test also
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Tristan,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Several comments on this webrev.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ** The original arrangement within the
>>>>>>>>>>>>>> test/java/rmi/reliability/benchmark directory had the main
>>>>>>>>>>>>>> benchmark files (scripts) at the top, some benchmark
>>>>>>>>>>>>>> framework files in the "bench" subdirectory, and the actual
>>>>>>>>>>>>>> RMI and serialization benchmarks in bench/rmi and
>>>>>>>>>>>>>> bench/serial
>>>>>>>>>>>>>> subdirectories.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The webrev moves all the RMI benchmarks to the top benchmark
>>>>>>>>>>>>>> directory but leaves the serial benchmarks in bench/serial.
>>>>>>>>>>>>>> The RMI benchmarks are now all cluttering the top directory,
>>>>>>>>>>>>>> but the main serial benchmark test is now buried in the
>>>>>>>>>>>>>> bench/serial directory.
>>>>>>>>>>>>>> The nice organization that was there before is now spoiled.
>>>>>>>>>>>>>> Is this rearrangement necessary in order to convert the
>>>>>>>>>>>>>> scripts to Java? I would prefer the original arrangement
>>>>>>>>>>>>>> be left in
>>>>>>>>>>>>>> place.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ** The RMI benchmark Main.java file has a @run tag of the
>>>>>>>>>>>>>> form,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> @run main/othervm/policy=policy.all/timeout=1800
>>>>>>>>>>>>>> -server
>>>>>>>>>>>>>> Main -c config
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> There is a subtle but serious problem here: the -server
>>>>>>>>>>>>>> option is passed to the >>JVM<< and not as an argument to
>>>>>>>>>>>>>> the
>>>>>>>>>>>>>> main() method.
>>>>>>>>>>>>>> The main() method gets neither a -server nor a -client
>>>>>>>>>>>>>> argument, so its default "run mode" as defined by the
>>>>>>>>>>>>>> benchmark
>>>>>>>>>>>>>> itself is SAMEVM.
>>>>>>>>>>>>>> This runs the client and server in the same JVM, which is
>>>>>>>>>>>>>> different from the shell script, which ran separate
>>>>>>>>>>>>>> client and
>>>>>>>>>>>>>> server JVMs.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ** The logic to process the -server argument still expects
>>>>>>>>>>>>>> to take a port, even though the port is assigned
>>>>>>>>>>>>>> automatically. So the obvious fix to the above,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> @run main/othervm/policy=policy.all/timeout=1800 Main
>>>>>>>>>>>>>> -server -c config
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> doesn't work, since a port is missing. The logic to
>>>>>>>>>>>>>> increment the argument index to collect the port argument
>>>>>>>>>>>>>> should be removed. Also, the -server line should be restored
>>>>>>>>>>>>>> to the usage message, but without the port argument.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ** After this is done, the client's command line is
>>>>>>>>>>>>>> constructed improperly. The command line ends up looking
>>>>>>>>>>>>>> something
>>>>>>>>>>>>>> like this:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> java client -cp <classpath> Main client localhost:58583
>>>>>>>>>>>>>> -c config
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The word "client" appears twice, but what's really required
>>>>>>>>>>>>>> is "-client" to appear as an argument after Main.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ** The client is run using ProcessBuilder, which by default
>>>>>>>>>>>>>> sends stdout and stderr to pipes to be read by the parent.
>>>>>>>>>>>>>> But the parent never reads them, thus any messages from
>>>>>>>>>>>>>> the client
>>>>>>>>>>>>>> are never seen.
>>>>>>>>>>>>>> The client is the one that emits the benchmark report, so
>>>>>>>>>>>>>> its output needs to be seen. It might be sufficient to have
>>>>>>>>>>>>>> the client inherit the parent's stdout and stderr. This
>>>>>>>>>>>>>> might intermix the client's and server's output, but it's
>>>>>>>>>>>>>> better
>>>>>>>>>>>>>> than nothing.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ** The config file is checked with the following code:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> try {
>>>>>>>>>>>>>> confFile = args[i];
>>>>>>>>>>>>>> confstr = new
>>>>>>>>>>>>>> FileInputStream(System.getProperty("test.src")
>>>>>>>>>>>>>> + System.getProperty("file.separator") +
>>>>>>>>>>>>>> confFile);
>>>>>>>>>>>>>> } catch (IOException e) {
>>>>>>>>>>>>>> die("Error: unable to open \"" + args[i] + "\"");
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This is potentially misleading, as the message doesn't print
>>>>>>>>>>>>>> the actual filename that was attempted to be opened.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This is important, as the test.src property doesn't exist in
>>>>>>>>>>>>>> the client JVM.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Note that the original shell script passed full pathnames
>>>>>>>>>>>>>> for the config file to both the client and the server. The
>>>>>>>>>>>>>> new @run tag merely says "-c config" which redefines the
>>>>>>>>>>>>>> config filename to be relative to the test.src directory.
>>>>>>>>>>>>>> You could pass -Dtest.src=... to the client, but it seems
>>>>>>>>>>>>>> like there should be something better than can be done.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ** The client needs to have its security policy set up. This
>>>>>>>>>>>>>> is missing from the construction of the client's command
>>>>>>>>>>>>>> line.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ** ProcessBuilder takes a List<String> for its command;
>>>>>>>>>>>>>> there is no need to turn the list into an array.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ** In the benchmark main methods, code of the form,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> while (true) {
>>>>>>>>>>>>>> runBenchmarks();
>>>>>>>>>>>>>> if (exitRequested) {
>>>>>>>>>>>>>> System.exit();
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> was replaced with
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> while (!exitRequested) {
>>>>>>>>>>>>>> runBenchmarks();
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This is a subtle logic change, in that the former code
>>>>>>>>>>>>>> always executed the loop at least once. It seems unlikely,
>>>>>>>>>>>>>> but it's possible that a timer could set exitRequested
>>>>>>>>>>>>>> before loop entry, resulting in the benchmark running zero
>>>>>>>>>>>>>> times. I guess, if you really want to clean this up (we do
>>>>>>>>>>>>>> need to avoid System.exit in jtreg tests), use a do-while
>>>>>>>>>>>>>> loop
>>>>>>>>>>>>>> instead.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ** Don't forget to remove the 7190106/runRmiBench.sh entry
>>>>>>>>>>>>>> from ProblemList.txt.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ** Remove the references to RMISecurityManager and just use
>>>>>>>>>>>>>> SecurityManager. This is just general cleanup. (I deprecated
>>>>>>>>>>>>>> RMISecurityManager last week.) :-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> It would be good if you could fix up these issues and post
>>>>>>>>>>>>>> another webrev.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> s'marks
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thank you
>>>>>>>>>>>>>>> Tristan
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 01/11/2013 23:58, Stuart Marks wrote:
>>>>>>>>>>>>>>>> On 10/31/13 10:22 PM, Tristan Yan wrote:
>>>>>>>>>>>>>>>>> I am working on bug
>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-7190106. Based
>>>>>>>>>>>>>>>>> on my research, it looks like the issue of fixed port was
>>>>>>>>>>>>>>>>> already addressed by Stuart Marks in other RMI tests
>>>>>>>>>>>>>>>>> which are Java based. I would like to reuse his solution,
>>>>>>>>>>>>>>>>> however it does not work for shell based tests.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> (Darryl Mocek did the unique port work for the RMI tests.)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Was the patch attached to your message? If so, it didn't
>>>>>>>>>>>>>>>> get through. Most OpenJDK mailing lists strip off
>>>>>>>>>>>>>>>> attachments before forwarding the message to the
>>>>>>>>>>>>>>>> recipients.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 2. My recommendation would be to convert this shell
>>>>>>>>>>>>>>>>> script test into Java based test and re-use the dynamic
>>>>>>>>>>>>>>>>> port allocation solution by Stuart Marks to address the
>>>>>>>>>>>>>>>>> issue
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 3. Also this test was written with server/client mode in
>>>>>>>>>>>>>>>>> shell script. In the past there have been sync issues
>>>>>>>>>>>>>>>>> between server/client which caused the test to fail. If
>>>>>>>>>>>>>>>>> we convert the shell script into Java based test, it
>>>>>>>>>>>>>>>>> would avoid using "sleep 10" mechanism to allow for
>>>>>>>>>>>>>>>>> server and client to start up and also give us better
>>>>>>>>>>>>>>>>> control in synchronizing server and client.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> (Background for interested readers.) In general, yes, it's
>>>>>>>>>>>>>>>> quite difficult to make reliable shell tests, especially
>>>>>>>>>>>>>>>> for multi-process tests like this one.
>>>>>>>>>>>>>>>> There is the unique port issue, and there is also the
>>>>>>>>>>>>>>>> issue of how long for the client to wait until the server
>>>>>>>>>>>>>>>> is ready. Error handling is also a problem, for example,
>>>>>>>>>>>>>>>> if one of the JVMs gets an unexpected exception, it's easy
>>>>>>>>>>>>>>>> for shell tests to mishandle this case. They might hang or
>>>>>>>>>>>>>>>> erroneously report success.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> If this is a rewrite, it's probably fairly large, so you
>>>>>>>>>>>>>>>> need to upload it somewhere (e.g., cr.openjdk.java.net)
>>>>>>>>>>>>>>>> and then post a link to it.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> s'marks
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> the message to
>>>>>>>>>> the recipients.
>>>>>>>>>>
>>>>>>>>>>> 2. My recommendation would be to convert this shell script test
>>>>>>>>>>> into Java based test and re-use the dynamic port allocation
>>>>>>>>>>> solution by Stuart Marks to address the issue
>>>>>>>>>>>
>>>>>>>>>>> 3. Also this test was written with server/client mode in
>>>>>>>>>>> shell script.
>>>>>>>>>>> In the
>>>>>>>>>>> past there have been sync issues between server/client which
>>>>>>>>>>> caused the test to fail. If we convert the shell script into
>>>>>>>>>>> Java based test, it would avoid using "sleep 10" mechanism to
>>>>>>>>>>> allow for server and client to start up and also give us better
>>>>>>>>>>> control in synchronizing server and client.
>>>>>>>>>>
>>>>>>>>>> (Background for interested readers.) In general, yes, it's quite
>>>>>>>>>> difficult to make reliable shell tests, especially for
>>>>>>>>>> multi-process tests like this one.
>>>>>>>>>> There is the unique port issue, and there is also the issue of
>>>>>>>>>> how long for the client to wait until the server is ready. Error
>>>>>>>>>> handling is also a problem, for example, if one of the JVMs gets
>>>>>>>>>> an unexpected exception, it's easy for shell tests to mishandle
>>>>>>>>>> this case. They might hang or erroneously report success.
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>>
>>>>>>>>>> If this is a rewrite, it's probably fairly large, so you need to
>>>>>>>>>> upload it somewhere (e.g., cr.openjdk.java.net) and then post
>>>>>>>>>> a link to
>>>>>>>>>> it.
>>>>>>>>>>
>>>>>>>>>> Thanks.
>>>>>>>>>>
>>>>>>>>>> s'marks
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
More information about the core-libs-dev
mailing list