RFR of JDK-8172347: Refactoring src/java.rmi/share/classes/sun/rmi/registry/RegistryImpl.java to improve testability of rmiregistry
Hamlin Li
huaming.li at oracle.com
Mon Jan 9 19:24:16 UTC 2017
Hi Mark,
Thank you for reviewing, please check comments inline, and new webrev:
http://cr.openjdk.java.net/~mli/8172347/webrev.02/.
Thank you
-Hamlin
On 2017/1/9 6:36, Mark Sheppard wrote:
> Hi Hamlin,
>
> If I read the changes correctly, there would appear to be a side
> effect of your refactor extract method "run", such that the static
> member variable
> registry is no longer set in the main method, and is set in the run
> method ? Thus, invoking run multiple times (, whether that is intended
> or not,) will
> see the registry static member variable overwritten, for each call.
> While the current implementation that will occur once only within the
> main method.
> So, what's the desired semantics for multiple "run" calls?
modified, assign "registry" it in main.
>
> A "run" method is synonymous with Runnable interface, even though
> this run method has a different signature, it's better
> to help us unwary avoid confusion!
> Also the run method instantiates and returns a RegistryImpl, thus
> exhibiting factory method characteristics.
> As such, perhaps a refactor "rename" would be in order, such as
> create, execute, or launch ?
renamed to launch.
>
> It would be helpful to provide a current summary, or a recap, as to
> the motivation for the refactoring.
It's in the javadoc, I refined it again.
>
> Is this complimentary to the constructor RegistryImp(int port) or
> LocateRegistry.createRegistry( ..) ?
The new "launch" method is mainly used for tests to simulate rmiregistry
which is an executable released by jdk.
>
>
> regards
> Mark
>
>
>
> On 09/01/2017 07:56, Hamlin Li wrote:
>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the core-libs-dev
mailing list