RFR of JDK-8172347: Refactoring src/java.rmi/share/classes/sun/rmi/registry/RegistryImpl.java to improve testability of rmiregistry

[Hamlin Li]

On 2017/1/6 6:15, Roger Riggs wrote:
> Hi Hamlin,
>
> There are too many issues being mixed together...
>
> Comments on B) RegistryImpl:
>
> Refactoring of RegistryImpl Main should be clean and clearly separated.
Hi Roger,

Thank you for reviewing.
Not sure if I understood you correctly, I created a new bug to track 
refactoring of RegistryImpl, I will send out separate reviews for 
AltSecurityManager in JDK-8172314, JDK-8030175. Please let me know if 
you did not mean it.

bug: https://bugs.openjdk.java.net/browse/JDK-8172347
webrev: http://cr.openjdk.java.net/~mli/8172347/webrev.00/, fix all the 
below points.

Thank you
-Hamlin
>
> 365: static void RegistryImpl run():
>
> -  The signature of run should be run(int port) and documented as 
> needing to be run in its
>    own thread since it changes Thread context classloader and that it 
> sets a securityManager.
>    Leave it to main to parse and choose a port number.
>
> -  The comments should be functional as to the purpose of the code and 
> not referencing a particular bug.
>    (Regardless of prior example).
>
>  - The comment about getting the exact port is out of place because 
> the port number cannot be
>    retrieved from the returned RegistryImpl.  IF that's why this 
> refactoring is needed, then
>    perhaps there should be a getPort method that extracts it from the 
> created UnicastServerRef.
>
> 423: static void main(String[] args):
>
>  - Parsing of args should be left in main(); avoiding the question 
> about why NumberFormat is handled.
>
>  - Either main or run should set a security manager; but not both.
>
> Roger
>
>
>
>
> 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.
>>>
>>>
>>> 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
>>>
>>
>