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

[Tristan Yan]

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