RFR 9: 8077350 Process API Updates Implementation Review

[Roger Riggs]

Hi Paul,

a couple of followup responses...

On 5/18/2015 5:16 PM, Paul Sandoz wrote:
>
>>>
>>>   251     /**
>>>   252      * Kills the subprocess forcibly. The subprocess represented by this
>>>   253      * {@code Process} object is forcibly terminated.
>>>   254      * Forcible process destruction is defined as the immediate termination of a
>>>   255      * process, whereas normal termination allows a process to shut down cleanly.
>>>   256      * If the process is not alive, no action is taken.
>>>   257      * <p>
>>>   258      * The {@link java.util.concurrent.CompletableFuture} from {@link #onExit} is
>>>   259      * {@link java.util.concurrent.CompletableFuture#complete completed}
>>>   260      * when the process has terminated.
>>>   261      *
>> insert @implSpec
>>>   262      * <p>The default implementation of this method invokes {@link #destroy}
>>>   263      * and so may not forcibly terminate the process.
>> insert @implNote
>>>   264      * Concrete implementations of this class are strongly encouraged to override
>>>   265      * this method with a compliant implementation.
>> legacy spec  - move up before other @ tags
>>>   266      * Invoking this method on {@code Process} objects returned by
>>>   267      * {@link ProcessBuilder#start} and {@link Runtime#exec} forcibly terminate
>>>   268      * the process.
>>>   269      *
>>>
>>> Use @ImplNote?
>> Most of this is spec, not informational.
> Sorry i meant @implSpec, including for the "Concrete impls..." bit.
Perhaps I misunderstood the narrow scope of @implSpec.
The @implSpec is only for the specific implementation of the method 
being declared.
The concrete implementations are internal  subclasses are out of that scope.
...
>>>
>>>   361      * If the process is {@link #isAlive not alive} the {@link CompletableFuture}
>>>   362      * is immediately {@link java.util.concurrent.CompletableFuture#complete completed}.
>>>
>>> s/is immediately/is returned/ ?
>> The important action to be specified is that the CF is completed() immediately.
>> Potentially it could say it is completed before the CF is returned from onExit().
>>
> Yeah, that is what i was trying to get across, it returns with an already completed CF.
ok;
      * If the process is not alive the CompletableFuture returned has 
been completed.

>
>
>>>
>>>   406      * @return a {@code CompletableFuture<Process>} for the Process;
>>>   407      * a unique instance is returned for each call to {@code onExit}.
>>>   408      *
>>>
>>> Any need to mention about unique instances?
>> yes, since the hierarchy of the CF instances is visible to the app and it makes
>> a difference whether a unique CF is returned for each call or whether
>> the same CF is returned from each call.
> Perhaps say "Returns a new CF..." and "@return a new {@code ..." ?
ok
>
>>>
>>>   429     /**
>>>   430      * Returns a ProcessHandle for the Process.
>>>   431      *
>>>
>>> Some methods talk about "the subprocess" where as others talk about "the Process" and others "the process".
>> That variation of terminology predates this update.
> I think it's best to try and be consistent throughout the class e.g. keep with the existing term or change to consistently use a new term.
>
>
>>>
>>>   456     /**
>>>   457      * Returns a snapshot of information about the process.
>>>   458      *
>>>   459      * <p> An {@link ProcessHandle.Info} instance has various accessor methods
>>>   460      * that return information about the process, if the process is alive and
>>>   461      * the information is available, otherwise {@code null} is returned.
>>>   462      *
>>>   463      * @implSpec
>>>   464      * This implementation returns information about the process as:
>>>   465      * {@link #toHandle toHandle().info()}.
>>>   466      *
>>>   467      * @return a snapshot of information about the process; non-null
>>>
>>> Dangling "non-null". Do you mean it's can never be null or ", otherwise null if the process is not alive or such information is not available"? I suspect the former now all Info methods return Optional.
>> It is always non-null.
> Ok, so the first paragraph needs tweaking.
ok, the description of the methods of the info class are better left to 
the Info class. ...
>>>
>>>    86     /**
>>>    87      * Returns the native process ID of the process. The native process ID is an
>>>    88      * identification number that the operating system assigns to the process.
>>>    89      *
>>>    90      * @return the native process ID of the process
>>>    91      * @throws UnsupportedOperationException if the implementation
>>>    92      *         does not support this operation
>>>    93      */
>>>    94     long getPid();
>>>
>>> In what cases could this throw a USO if the static "of "method did not?
>> Its in an interface that might have arbitrary implementations. In a theoretical
>> Process implementation for remote processes their might not be a viable pid
>> or the access controls might be such that the PID is meaningless or restricted.
>>
> Ok, i think i get it: a call ProcessHandle.getPid obtained from ProcessHandle.of will never from a USO but other implementations of ProcessHandle might.
yes
>>>
>>>   326      * @return an {@code Optional<ProcessHandle>} for the process to be
>>>   327      *         forcibly destroyed;
>>>   328      *         the {@code Optional} has the value of the ProcessHandle
>>>   329      *         if the request to terminate was successful, otherwise it is empty
>>>   330      * @throws IllegalStateException if the process is the current process
>>>   331      */
>>>   332     Optional<ProcessHandle> destroyForcibly();
>>>
>>> Under what cases would an empty optional be returned? e.g. if native permissions are not granted?
>> yes, but Alan's and other remarks recommend changing the return type.
>> boolean will cover case to reflect that it was not possible to request the process
>> to be destroyed without creating a confusing state of an Optional.
> Yes, that seems a better signal. What could the caller do if false is returned? not much i guess except log something.
Correct, there is not much to log unless the OS specific error returns 
are exposed.

Thanks, Roger