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 21:49:09 UTC 2017
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