Stuart,
see inline...
On Fri 06 Jul 2012 11:47:54 AM PDT, Stuart Marks wrote:
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?
This was a port conflict, which is why the UnbindIdempotent test was
modified.
*** 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.
Changed.
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
Fixed in all places.
*** 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.
Added @run to this test.
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.
Changed.
*** 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).
Renamed.
*** UnregisterGroup.java
Similar to above, rename PORT.
Renamed.
*** 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.
I don't see a simple solution right now. I suggest we table this issue
and re-visit it after the commit. Another option is to not include the
fix for Bug #7161503 with this fix until this issue has been addressed.
*** AltSecurityManager.java
The registry and rmid fields probably should be final. Maybe all caps
too, except that RMID is the name of a class....
Made final, renamed.
In run() it should probably bomb if utilityToStart equals neither
registry nor rmid.
OK.
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.
Renamed.
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.
Made blank final.
*** 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);
This turned out to be an issue with calling
LocateRegitry.createRegistry(0), which occurs in
TestLibrary.createRegistryOnUnusedPort(). If
LocateRegitry.createRegistry(0) is called within the same VM multiple
times, after the first registry is created (and not destroyed), the
subsequent calls will fail. See the comment above creating the second
registry line:
// Need to get a random port for the second registry.
However, this really isn't the right solution, so I modified
TestLibrary.createRegistryOnUnusedPort to catch ExportException (which
is thrown if the above issue occurs), get a random port, and attempt to
create a registry on that port. See updated TestLibrary.
MultipleRegistries has been changed to what you have above as a result.
I'll post the changes to the comments here and the coming other half
after I address the other half as another webrev.
Darryl
-------
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
