Code Review Request 7142596: RMI JPRT tests are failing

[Stuart Marks]

OK, here's the review for the second half of the files in the webrev. I saw 
your reply to the first half (to which I'll reply separately), and I don't 
think there's anything here that's affected by them.


*** AppleUserImpl.java
*** ApplicationServer.java


REGISTRY_PORT should be a local variable; also rename to use mixed case.

Eh, whoops, after looking at ApplicationServer.java I see that it accesses the 
REGISTRY_PORT field directly. This is why direct field access is a bad idea. 
:-) Now the question is, has REGISTRY_PORT been initialized before 
ApplicationServer needs it? It turns out that it has been -- but only in some 
cases.

It seems like the test is trying to support two modes, one that runs in two 
threads in the same JVM, and the other that runs in two separate JVMs. If they 
are in separate JVMs, things will no longer work because in the JVM that runs 
ApplicationServer.main(), AppleUserImpl.REGISTRY_PORT will be -1. I suspect 
that our test environment doesn't support the separate JVM mode, but it seems 
unwise to break it.

I'd suggest that in two-JVM mode the classes fall back to using a "well-known" 
default registry port number, which in this case seems like 2006.

In single-JVM mode, AppleUserImpl creates an instance of ApplicationServer, so 
I'd suggest adding a method to ApplicationServer that allows AppleUserImpl to 
store the randomly-assigned registry port number into it, overriding the 
default value.

This seems like this is the simplest way to preserve the two modes of operation 
but to support the random port selection model we're trying to achieve.


*** activatable/EchoImpl.java


     int registryPort = new Integer(System.getProperty("rmi.registry.port"));

I'd suggest using Integer.parseInt() instead of new Integer(). Not a huge deal, 
but it's probably more conventional to use parseInt() and it avoids boxing.

One could probably do Integer.getInteger("rmi.registry.port") but this is seems 
pretty obscure to me even though it's more succinct.

The same also applies to the following:
  - HelloImpl.java
  - unicast/EchoImpl.java
  - ShutdownImpl.java
  - SelfTerminator.java
  - CheckFQDNClient.java
  - LeaseLeakClient.java
  - dgcDeadLock/TestImpl.java


*** FiniteGCLatency.java


The pattern here is a bit odd, as the test creates the registry, throws away 
the returned reference, and then calls getRegistry() to get another Registry 
reference. It *seems* like they're identical references, but in fact the first 
is apparently a reference to the actual Registry implementation, whereas the 
second is a remote stub.

The tests seem to do all the actual work using the remote stub, which seems proper.

This is confusing, though, as it looks like there's a redundant Registry 
reference now. This might lead someone in the future to "simplify" the test by 
not getting the remote stub, which in turn might invalidate some tests. (In 
fact I was going to suggest this but I decided to investigate further first.)

At the very least, I'd suggest renaming the variable that holds the newly 
created Registry to something like "registryImpl" to make it clear that it's 
different from the thing returned by getRegistry(), even though they have a the 
same time.

Another possibility is to rearrange the TestLibrary API so that there is a 
single utility method that combines createRegistryOnUnusedPort() and 
getRegistryPort(). That is, it creates a new registry and simply returns the 
port on which it was created, not a reference to the registry implementation.
I don't think the registry implementation is actually ever used by the tests, 
and it might simplify things a bit as well.

Possibly similar issues with:
  - UnreferencedContext.java
  - NoConsoleOutput.java


*** HttpSocketTest.java


Unnecessary call to TestLibrary.getUnusedRandomPort()?


*** TestLibrary.java

Mostly pretty straightforward, but I do have some concerns about the random 
port selection and a potential clash with the "reserved port range" as defined 
in this test library.

The getUnusedRandomPort() method attempts to get a socket within the range 
(1024,64000) and will retry 10 times if it can't. Unfortunately, MacOS 
allocates ports more-or-less sequentially in the range [49152, 65536) which 
means that when the kernel's internal counter gets to 64000, 
getUnusedRandomPort()'s retries will fail, causing tests to fail until the 
counter wraps around.

Other systems behave differently; Linux seems to allocate them randomly in the 
range [32768,65536) and Windows XP SP3 allocates them sequentially in the range 
(1024,5000]. So it's probably not a problem for them.

I think the thing to do is to check only for "reserved ports" that are actually 
used by tests here. These are in the range [64001,64005]. In 
getUnusedRandomPort(), it should only need to retry if the returned port is 
within this narrow, reserved range. If it's anything else it should be OK.

On another topic, the three utility methods here:
  - createRegistryOnUnusedPort
  - getRegistryPort
  - getUnusedRandomPort

all catch exceptions and then return illegal values (null or -1), sometimes 
after printing some diagnostic information. The problem is that the caller will 
attempt to soldier on with the illegal return value and will stumble over 
something later, such as NullPointerException or IllegalArgumentException. This 
will probably be obvious but it's equally likely to be confusing.

Since these utilities are all called from test code, and the tests are relying 
on them to return valid results, I'd suggest just throwing exceptions from the 
utility methods if they fail. This will (should) cause the test to error out, 
but that's OK, as it never could have succeeded anyway if the utility call had 
failed.

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
>