RFR for JDK-7190106 RMI benchmark fails intermittently because of use of fixed port

[Tristan Yan]

/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
>>> /
>> /
>> /
/
/