On 2017/1/6 6:15, Roger Riggs wrote:
On 1/4/2017 10:21 PM, Hamlin Li wrote:
Hi Roger,
Thank you for reviewing, please check comments inline.
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.
Agree, I put 2 fixes in one patch together as there is no port in use
issue reported, but by reviewing the code, potential port in use
issue could happen some time in the future.
Best to keep 1-1 to avoid complicating the discussion and increasing
code complexity.
Hi Roger,
Got it, the new webrev for timeout issue is at:
http://cr.openjdk.java.net/~mli/8030175/webrev.01/
I just created another bug
https://bugs.openjdk.java.net/browse/JDK-8172314 to track the "port in
use" bug, will send out separate webrev in another thread later.
Thank you
-Hamlin
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.
I prefer B, because
1. Although A looks cleaner but B is simulating more like rmiregistry.
2. There are some other issue for example JDK-7146543, JDK-8030950,
JDK-8038772, fix based on version B works well, but fix based on
version A fails.
3. Impact of RegistryImpl modification is minimal. ( May we could
make Registry run(String args[]) private and access it in test by
reflection? )
4. Although it's simple to duplicate the code in run() in the
TestLibrary, but seems it's not a good design choice. (let's call it
version C.)
5. For JDK-8170728, the fix will need to modify
sun.rmi.registry.RegistryImpl anyway.
Thank you for detailed review comments below.
As we have several options, I will wait for your further comments on
choice of version A/B/C, then send out new webrev, hope I only need
to send out one version :-).
Thank you
-Hamlin
JavaVM:
- Document the new methods.
Line 232: Document the exception that may be thrown from exitValue.
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"
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.
- line 105: use JavaVM.waitFor(timeout) and avoid duplicating code
to wait for the subprocess.
- If the subprocesses are in an unknown state it would be useful
to print diagnostic info from the subprocess before terminating.
Line 106:
- 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!.
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
- 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.
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
