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

[Hamlin Li]

Hi Roger,

Thank you for reviewing, please check new webrev: 
http://cr.openjdk.java.net/~mli/8172347/webrev.01/

Thank you
-Hamlin

On 2017/1/6 12:56, Roger Riggs wrote:
> Hi Hamlin,
>
> Yes, that looks better.
>
> On the comments, use the normal javadoc comment conventions for any 
> public API.
> @param @return, @throw, etc.
>
> I think comments should be direct about what the function does. It 
> does not need
> to explain why so much.  Or if so, later and in a separate paragraph;  
> when reading
> the most important information should be first.
>
> Thanks, Roger
>
>
> On 1/6/2017 4:59 AM, Hamlin Li wrote:
>>
>> 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
>>>>>
>>>>
>>>
>>
>