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 22:31:22 UTC 2017
On 2017/1/9 13:55, Roger Riggs wrote:
> Hi Hamlin,
>
> Its time to use 2017 for copyright dates.
Hi Roger,
Yes, the new year! :-)
>
> The method name launch does not reflect is really happening.
> Launch implies an active process or thread is being started.
> But in this case there is no active element; it just creates and
> exports a remote object.
> Otherwise, looks fine.
modified as createRegistry.
the code is pushed.
Thank you
-Hamlin
>
> Thanks, Roger
>
>
> On 1/9/2017 4:49 PM, Hamlin Li wrote:
>> Hi Roger,
>>
>> Thank you for reviewing, please check the comments inline, and new
>> webrev: http://cr.openjdk.java.net/~mli/8172347/webrev.03/
>>
>> Thank you
>> -Hamlin
>>
>> On 2017/1/9 13:04, Roger Riggs wrote:
>>> Hi Hamlin,
>>>
>>> In addition to Mark's comments:
>>>
>>> How about this for the method comment:
>>>
>>> /**
>>> * Return a new RegistryImpl on the requested port and export it to
>>> serve registry requests.
>>> * A classloader is initialized from the system property
>>> "env.class.path" and
>>> * a security manager is set unless one is already set.
>>> * <p>
>>> * The returned Registry is fully functional within the current
>>> process and is usable for
>>> * internal and testing purposes.
>>> *
>>> * @param regPort port on which the rmiregistry accepts requests;
>>> * if 0, an implementation specific port
>>> is assigned
>>> * @return a RegistryImpl instance
>>> * @exception RemoteException If remote operation failed.
>>> * @since 9
>>> */
>> Your java doc looks more clear, modified.
>>> public static RegistryImpl run(int regPort) throws
>>> RemoteException {...}
>>>
>>> The method should throw RemoteException. Throws clauses should
>>> always be as specific as possible.
>> modified throws.
>>>
>>> On 1/9/2017 9:36 AM, 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?
>>> The static registry field is never used and can/should be removed.
>>> It can be handled with a local in the method.
>> handled in a local in the method, and assign it to registry in man.
>>>>
>>>> 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 ?
>>> +1, perhaps createRegistry(int port)
>>>
>> use launch, createRegistry makes me think of it as the same of
>> LocateRegistry.createRegistry.
>>>
>>>>
>>>> It would be helpful to provide a current summary, or a recap, as to
>>>> the motivation for the refactoring.
>>> That can be in the test or the bug report; the implementation code
>>> itself should describe the functionality
>>> and its use.
>> will do it in test.
>>>
>>> $.02, Roger
>>>
>>>>
>>>> Is this complimentary to the constructor RegistryImp(int port) or
>>>> LocateRegistry.createRegistry( ..) ?
>>>>
>>>>
>>>> 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