Hi Hamlin,
Its time to use 2017 for copyright dates.
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.
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
