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
