RFR of JDK-8030175: java/rmi/registry/altSecurityManager/AltSecurityManager.java fails due to timeout

[Roger Riggs]

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.


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