JEP 102 Process Review

[Roger Riggs]

Hi Chris,

Thanks for the review and comments.

On 3/24/2015 10:42 AM, Chris Hegarty wrote:
> Roger,
>
> I think the API is looking much better now ( just a few comments below on small specific issues ), so I’ve taken a pass over the implementation changes in the sandbox. Here are some comments:
>
> 1) Some operations on ProcessHandle require RuntimePermission
>      "manageProcess", but I don't see this specified in
>      java.lang.RuntimePermission ( there is a table of target runtime
>      permission names ). I think you just need to add it.
Yes will add
>
> 2) Can ProcessImpl.waitForProcessExit and its native counterpart be
>      removed? ( since its function is now performed through ProcessHandleImpl )
I'll look at that again; the two behaviors:
1) waiting for and reaping the exit value used by Process passes a 
pid/windows handle is one case,
2) waiting for a pid and not touching the exit value passing a pid is 
the other used by ProcessHandle.
It currently passes a flag argument but may be better as two different 
native methods,
considering that Process no longer extends ProcessHandle.
>
> 3) "process reaper”, maybe
>      “Process-Reaper-“ + monotonically increasing ID ( similar to
>      ForkJoin worker threads, or other ). But I do accept that this
>      is the existing name for the reaper threads.
Makes sense;  most thread names use mixed case;
I notice many thread names use spaces in the names instead of '-'s.
>
> 4) Should Info.toString() print something if all fields are null,
>      or -1.
ok, but  localization of any words would be an issue.
I'd propose to put '[]' brackets around the whole string so it will 
empty '[]' if none.
>
> 5) Could the fields for Info be private final, and use a separate
>      private holder for retrieving the information from native?
>      Seems desirable for them to be final.
The implementation class is package private and only exposes the Info 
interface.
What is the concern?  adding another class doesn't seem worth the overhead
or complexity.
>
> 6) Process has s number of @implXXX tags, they typically appear
>      before the @return in usage I’ve seen in the JDK.
ok
>
> 7) Should ProcessHandleImpl.toString include Info, and/or other
>      information about the handle?
The toString is just the PID, retrieving the rest of the info is 
relatively slow,
involving a native call and creating objects.  I think simple is best.

>
> Subjective/Minor:
>
> 1) There are number of commented out fprintfs in native code
>      that should be removed.
Yes, still cleaning the sandbox
>
> 2) You may need to take a pass over the copyright year range.
>      I understand that some of these files were created in 2014, but
>      I suspect that they should all at least include 2015.
ok
>
> 3) OnExitTest & InfoTest include the “Classpath” exception, when
>      they should not.
ok
> 4) There is some duplication of javadoc in implementation
>      that could be removed, e.g:
>         /**
>          * Returns the process ID as a decimal String.
>          * @return the process ID as a decimal String
>          */
>
>      My preference is to not repeat the public API doc in
>      implementation classes, but I accept that this is sometimes
>      done for in-place readability.
ok, will cleanup the comments
> 5) There are number of lines that are greater than 80
>      characters. These can be difficult for future maintenance
>      and reviews ( webrev side-by-side diffs ).
Good point, I'll wrap them.
>
> I need to spend a little more time on the native code, but the tests pass so that is a good start!
Thanks, Roger