Code Review Request 7142596: RMI JPRT tests are failing

[Stuart Marks]

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


-------

*** ProblemList.txt


The comment for UnbindIdempotent says "Need to be marked othervm, or changed to 
be samevm safe" but the test isn't marked /othervm. Does the test need to be 
updated, or is this comment misleading, and the problem really is the port 
conflict?


*** ContextWithNullProperties.java


(Nitpick: import lines are out of order)

The comment says, "Create registry if one is not already running." This is 
mostly left over from the earlier version which also mentioned port 1099. But, 
aren't we changing things so that this tests always creates its own registry on 
a unique port? If so, I'd just remove the comment and remove the catch/ignore 
of RemoteException (which was suspect in the first place). If for some reason 
we can't create our own registry, we should just throw the exception out to the 
caller and let the test error out. If we ignored RemoteException we'd leave 
registryPort as -1 which would presumably cause some obscure failure later on.

We probably also don't need to declare/initialize registryPort separately 
anymore, so we can just replace the first 8 lines or so of main() with the 
following:

     Registry registry = TestLibrary.createRegistryOnUnusedPort();
     int registryPort = TestLibrary.getRegistryPort(registry);

This same pattern occurs in these other tests and so a similar fix should be 
applied to them as well:

  - UnbindIdempotent
  - LookupNameWithColon.java


*** LookupNameWithColon.java


This test is missing an @run tag, thus it won't actually get run! Since you've 
specified an @build tag, you have to specify a separate @run tag to run the 
test. It's possible to deduce this with a careful reading of the jtreg tag spec:

http://openjdk.java.net/jtreg/tag-spec.txt

(IMHO this is a terrible usability problem.) I've looked at the other tests in 
this webrev for similar issues, and I didn't see any, but I might have missed them.

Regarding the registry, this test is somewhat different from the others in that 
it was originally coded to use an existing registry if it couldn't create its 
own. If it were to find a registry running on 1099, it was probably created by 
some other test, and no assumptions can reliably be made about it. So, I'd just 
take out this logic and have the test unconditionally create its own registry.


*** StubClassesPermitted.java


(Nitpick.) It looks like REGISTRY_PORT is a constant, but it's a static field. 
It should be renamed to have a conventional field name (i.e., mixed case).


*** UnregisterGroup.java


Similar to above, rename PORT.


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


*** AltSecurityManager.java


The registry and rmid fields probably should be final. Maybe all caps too, 
except that RMID is the name of a class....

In run() it should probably bomb if utilityToStart equals neither registry nor 
rmid.

The ensureExit() method has a local variable port, which hides the field named 
port (well, not really, since ensureExit is static and port is an instance 
field) but still, this is kind of confusing.

If the port field is required to be initialized properly, make it a blank final 
(i.e., declare it final but without an initializer) and error-check it at the 
point where it's assigned in the constructor.


*** MultipleRegistries.java


Not really a big deal, but the way the second registry is created seems 
somewhat roundabout. It's not clear to me why the code can't just do this:

     Registry registryImpl1 = TestLibrary.createRegistryOnUnusedPort();
     Registry registryImpl2 = TestLibrary.createRegistryOnUnusedPort();
     int port1 = TestLibrary.getRegistryPort(registryImpl1);
     int port2 = TestLibrary.getRegistryPort(registryImpl2);


-------



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
>