On 07/12/2012 05:13 PM, Stuart Marks wrote:
OK, I took a look at the revised webrev. Most stuff is good. A
couple changes
are necessary though.
*** MultipleRegistries.java
Not a big deal, but the comment at lines 65-67 is no longer necessary.
Comment removed.
*** TestLibrary.java
I think the reserved port range stuff still needs to be fixed up.
The comment at lines 81-82 talks about a range 64000-64100 which
isn't really
present anywhere. The comment should instead say that PORT_MIN and
PORT_MAX
should be kept adjusted so that they specify the range of ports
defined by
the constants following. Make sure to do this adjustment; PORT_MAX
is 64002
here.
I reserved ports 64000-64100 (even though only 64001-64005 are
currently being
used) for fixed port tests (for tests which require fixed ports) so
any future
tests which require fixed ports can be assured there are ports
available within
a range for their use. In fixing the tests, I saw that those who
wrote tests
and used fixed ports grabbed random port numbers all over the place.
Providing
a reserved range allows them to just grab the next number and to keep
the fixed
port numbers in a group, even when new tests are added. If the ports are
defined in a central place, like TestLibrary, then hopefully
developers will
come to TestLibrary to reserve the ports rather then define them in
the tests
themselves, like was done previously. I chose 100 ports for
"future-proofing"...I don't think 100 is too many, but I've reduced
it to 10
and lowered the max to 64010. I was also thinking of looking for
tests which
used fixed ports outside of RMI tests and standardizing those ports
here, but
that's possible future work and I'm not sure it's necessary.
The previous webrev had PORT_MIN and PORT_MAX specify a range of
1024-64000
as the *allowed* range for ports returned by getUnusedRandomPort().
But as I
described previously, this is difficult to enforce given the varying
port
allocation schemes on different OSes.
Instead I think we need to invert the sense of the range
PORT_MIN..PORT_MAX
and make it be the range of ports reserved by tests that require
fixed ports
(64001 through 64005). Then, getUnusedRandomPort() needs to be
changed to
*disallow* ports in this range, that is, to retry if the port is
within this
range. Thus the condition on the while-loop has to be inverted,
something like
while (... unusedRandomPort >= PORT_MIN && unusedRandomPort <=
PORT_MAX)
Yes, this is correct and it has been changed.
So, how could getUnusedRandomPort() have worked in its present
state? Well I
think it ends up retrying 10 times, then at the end of the loop
unusedRandomPort is actually some legal port number -- albeit
possibly one
within the disallowed range -- and almost certainly not -1, and so
the method
returns it and the caller uses it. So, the success-testing logic
here will
also have to change so that it retests whether the port is in the
disallowed
range.
Correct...done.
The comment on getUnusedRandomPort() also needs to be updated to
reflect its
new policy on the reserved range, as well as throwing an exception on
failure. Also (nitpick) it should say "less than" not "less then".
Yes, I always do this. :(
Looking at getUnusedRandomPort() again more closely [sorry] I think
the catch
of Exception that does nothing is suspicious. I'm not sure why
getting a
ServerSocket would fail, but if it does (maybe the system is out of
ports??)
we should just throw out to the caller. Perhaps ideally we'd just
have this
method throw IOException, but if that requires all callers to be
updated, it
might be easier just to catch IOException and throw a
RuntimeException that
wraps the caught IOException.
I see no harm in trying again if it fails. If we have run out of
ports, then
possibly one or more used ports will be closed and freed and a
subsequent try
will succeed. I don't know all of the cases where a failure might occur.
Anyway, if this is the prevailing way of handling this, I'll change it.
Similar comments apply to the catch(Exception)/do-nothing code in
the other
utility methods.
Now throwing RuntimeException in getRegistryPort.
Certainly getRegistryPort() should just throw (or possibly wrap and
rethrow)
any exception caught.
For createRegistryOnUnusedPort(), the catching of ExportException is
handled
properly. The second catch clause of the outer try-block, and the
catch of
the inner try-block, both ignore exceptions. The code will then end up
retrying. Is it reasonable that retrying in these cases will result
in a
different outcome? My guess is that it's more likely that something is
seriously wrong that will cause all the retries to fail, in which
case this
method will discard the exceptions it has just caught and throw a *new*
RemoteException instance.
This was a holdover from webrev.00 which should have been removed. I
agree with
Alan and you here that multiple tries of
LocateRegistry.createRegistry(0) is
unnecessary. I've modified this to make a single attempt at using
LocateRegistry.createRegistry(0) and if it fails, make a single
attempt at
getting an unused random port and creating a registry on this port, then
failing with a RuntimeException.
I'm particularly sensitive to this; as you might recall a couple
weeks ago I
was having a wrestling match with the jstatd tests (I finished the
match, but
I'm not sure I won). The primary problem was that several layers of
code
would catch and discard exceptions, which made diagnosing the problem
incredibly difficult. So, I'd recommend removing the "do nothing"
catch clauses.
Thinking further about createRegistryOnUnusedPort(), I'm not sure that
retrying 10 (or some other number of times) actually makes sense. It
does for
getUnusedRandomPort(), which has to retry in order to get a port
outside the
disallowed range. But for creating a registry, createRegistry(0)
will usually
work the first time. If it throws ExportException, it does so for a
specific
reason, so we should retry once on an unused random port. But if
this still
fails, don't think retrying repeatedly makes sense.
See above.
Changes pass all tests. Updated webrev at
http://cr.openjdk.java.net/~dmocek/7142596/webrev.04
Darryl
s'marks
On 7/10/12 2:14 PM, Darryl Mocek wrote:
On 07/09/2012 04:41 PM, Stuart Marks wrote:
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.
Changed to a private registryPort (see next issue).
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.
Rather then going the "fixed port" route, which is what we're
trying to get
away from, I've changed the implementation of both AppletUserImpl's
and
ApplicationServer so ApplicationServer requires a port and
AppleUserImpl
supplies the port on construction of ApplicationServer. I thought
of modifying
ApplicationServer's constructor to create a port using
TestLibrary.getUnusedRandomPort, but decided requiring a port is
better as
ApplicationServer's job is to look for already exported AppleUser
objects.
*** 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
Integer.parseInt returns a primitive (which is what the return is
assigned to)
and it appears Integer.parseInt is "faster" then creating a new
Integer.
Changed to Integer.parseInt in all places referenced.
*** 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()?
Looks like extra code left over from the change from using
TestLibrary.getUnusedRandomPort/LocateRegistry.createRegistry(randomPort)
to
TestLibrary.createRegistryOnUnusedPort...removed.
*** 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.
I'll try setting the range this narrow, but I don't know how many
sequential
tests will be run at a time and I'm concerned 5 is too few. The
-concurrency
option on jtreg allows you to specify how many concurrent tests
will be run. We
should have enough test ports reserved to satisfy any concurrency
request. I've
run the tests with -concurrency=8 (I have a dual-core system
showing 4 CPU's).
I tried reducing the port range to 64001/64002 and concurrency=4
and all passed
fine, so maybe we're OK with just 5.
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.
I already modified createRegistryOnUnusedPort to throw an exception
as part of
the MultipleRegistries change. I'm now throwing a RuntimeException for
getRegistryPort and getUnusedRandomPort if they fail.
See updated webrev:
http://cr.openjdk.java.net/~dmocek/7142596/webrev.03
Darryl
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