Hi Roger,
Thank you for reviewing, please check comments inline.
This is a bug separated from JDK-8030175
bug: https://bugs.openjdk.java.net/browse/JDK-8172314
webrev: http://cr.openjdk.java.net/~mli/8172314/webrev.00/
On 2017/1/5 4:18, Roger Riggs wrote:
Hi Hamlin,
The original issue with timeout may be due to heavily loaded systems
and short timeouts.
15 sec is not enough on an overloaded system to wait for a process to
start and then die.
There is no indication in this issue about port-in-use; that would be
a different issue.
Common comments to both A and B.
I'll need more time to look at B; it would be cleaner to use A if it
can address the issue.
The alternative is to duplicate the code in run() in the TestLibrary
and not modify the RegistryImpl.
JavaVM:
- Document the new methods.
Line 232: Document the exception that may be thrown from exitValue.
Fixed.
RMID:
Line 204, 222: when adding new functions to the test library please
add documentation for the methods.
There are now many variations of the methods that differ only by the
number arguments.
It would be better if the name included some hint as to the additional
functionality.
typo: "additionalOptions" -> "add*i*tionalOptions"
Fixed.
REGISTRY:
- Document the new methods.
- The name should be more indicative of its function and should NOT
be all caps; RMID is an acronym where the caps make sense.
above fixed.
- line 105: use JavaVM.waitFor(timeout) and avoid duplicating code
to wait for the subprocess.
Not quite get your point.
- If the subprocesses are in an unknown state it would be useful to
print diagnostic info from the subprocess before terminating.
Line 106:
Not quite get your point. if subprocess vm (either RegistryRunner or
RMIRegistryRunner) exit before get the port number from its output, that
means vm exit exceptionally ( for example rmiregistry in
AltSecurityManager.java is to exit before port number is output ). if
there are any output generated by vm (rmiregistry), it should have
already been caught and printed by JavaVM.
- Line 124:
- I think I would have promoted the shutdown method to JavaVM instead
of creating a new cleanup method
to keep the code simpler.
** The cleanup method never calls super.cleanup() so the process is
never destroyed!.
RMID.java already has both cleanup and destroy, RMID.cleanup is already
used by lots of tests, it means destroy + clean up log files, so I add
cleanup in JavaVM, cleanup can be used for both RMID and RegistryVM as a
JavaVM instance. To avoid misunderstanding, I added some comments for
cleanup in JavaVM.java. and removed RegistryVM.shutdown, which is
useless and misleading.
AltSecurityManager.java:
- Line 61: the empty constructor can be removed entirely.
- Line 80: change the message to say the exception did not occur.
As written it implies it may have occurred but was not caught.
- Line 86: typo "a unexpected" -> "an unexpected"
- Line 90: remove the printStackTrace; it is not useful and is a red
flag in .jtr files
above fixed.
- Line 125: I don't see that cleanup is better than destroy; If there
are doubts about destroy
then destroy should be fixed not avoided.
Same reason as above for your comment in REGISTRY(now RegistryVM).
Thank you
-Hamlin
Roger
On 12/26/2016 3:51 AM, Hamlin Li wrote:
Would you please review the below patches?
bug: https://bugs.openjdk.java.net/browse/JDK-8030175
webrev (version A): http://cr.openjdk.java.net/~mli/8030175/webrev.00/
webrev (version B):
http://cr.openjdk.java.net/~mli/8030175/webrev.sun.rmi.registry.RegistryImpl.00/
There are 2 versions to be reviewed.
In version A, just use RegistryRunner to replace rmiregistry.
In version B, refactor sun.rmi.registry.RegistryImpl to improve the
testability of RMI code, and create/use RMIRegistryRunner to simulate
rmiregistry.
Thank you
-Hamlin
