Code Review Request (7161503 subcase) 7142596: RMI JPRT tests are failing

[Olivier Lagneau]

Hi Stuart,

I am in vacation for 2 days, but I think I need reply anyway now.

First thanks for reviewing and giving your time on this.

Now in the 7161503 SetChilEnv case:
>
> *** SetChildEnv.java
>
>
> The testing of the message string from the IOException causes me great 
> concern. This message is defined all the way over in 
> java.io.PipedInputStream, and while it's not localized, it does seem 
> like a pretty fragile dependency. I mean, changing some exception 
> message in java.io might cause an RMI Activation test to fail??!? 
> (Sorry.)
>
> Certainly we want to ignore spurious errors, and it sounds from the 
> comment like normal termination of rmid causes these exceptions to be 
> thrown. But I'm wondering if rmid crashes, won't we get the same 
> exception and ignore it, improperly?
>
> I don't know what the right thing to do is here. It seems like there 
> ought to be a more definitive way to distinguish between normal 
> termination and pipe closure from an error.
>
The fact is that the part of the fix we are discussing here is 
cleaning-up of the test. It is not related to 7142596.
As as said in other mails, this test should be written differently and 
in addition brings a systematic "Pipe Broken exception"
each time runwith() is called. I mean in the current JDK state, not this 
fixed one. an ugly thing. That was considered to be ok for a test 
success however.

I surely have gone too far in that fix, and may not have tried to clean 
it up.
Btw, none of the other rmi java test fixed here does try to do any cleanup.

Thus, and because it is important to remain consistant in bug fixes, 
since this one is for 7142596 only,
I think we should just revert to the existing code regarding the 
DebugExecWatcher and related exception cleanup fix.
We must then accept to keep this exception raised each time runwith() is 
called, since this is the current state of the code.
We should also then include in bug description  a note stating the 
problem and how we can fix it.
Such a fix would then be part of recent 7168267.

a full cleanup  fix of DebugExecWatcher code  is simple but involves 
testLibrary framework:
- DebugExecWatcher is a consumer of redirected output of distant rmid: 
it needs to check that "DebugExec" string is found in that stream;
- Thus a way of dealing with consumer of redirected output is needed for it.
- We define in testlibrary an OutputConsumer interface with only one 
method processInput() that must do the needed work.
- By default, no such consumer exist and StreamPipe of testLibrary does 
the work as usual.
- Otherwise, at rmid creation such a consumer is passed and its 
processInput method will be call from streamPipe as needed.

Thanks,
Olivier.










Le 06/07/2012 20:47, Stuart Marks a écrit :
> Hi,
>
> I've reviewed the first half of the files, thru 
> test/java/rmi/registry/reexport/Reexport.java. Basically things look 
> good and make sense, but there are some details to be ironed out and 
> some cleanups to be done. Nothing major, I think, with the exception 
> of SetChildEnv. See discussion below.
>
> Second half to follow later.
>
> s'marks
>
>
> -------
>
>
>
> On 7/5/12 2:22 PM, Darryl Mocek wrote:
>> Hello core-libs. Please review this webrev to fix Bugs #7142596 and 
>> 7161503.
>> Webrev can be found here: 
>> http://cr.openjdk.java.net/~dmocek/7142596/webrev.02.
>> This commit fixes concurrency issues with the RMI tests.
>>
>> - Added TestLibrary.createRegistryOnUnusedPort method. This creates an
>> RMIRegistry on an unused port. It will try up to 10 times before 
>> giving up.
>> - Added a TestLibrary.getRegistryPort(Registry) method to get the 
>> port number
>> of the registry.
>> - Changed almost all tests from using hard port numbers to using 
>> random port
>> numbers for running the RMI Registry and RMID.
>> - Removed othervm from those tests which don't need it.
>> - Added parameters for tests which spawn a separate VM to pass RMI 
>> Registry and
>> RMID ports in cases where needed.
>> - Added PropertyPermission to security policy files where needed.
>> - Removed java/rmi and sun/rmi from tests which cannot be run 
>> concurrently.
>> - Added java/rmi/Naming to list of tests which cannot be run 
>> concurrently.
>>
>> Thanks,
>> Darryl
>>