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

[Roger Riggs]

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
>>>>
>>>
>>
>