RFR 9: 8077350 Process API Updates Implementation Review

[Paul Sandoz]

On May 18, 2015, at 10:49 PM, Roger Riggs <Roger.Riggs at Oracle.com> wrote:

> Hi Paul,
> 
> Thanks for the comments.
> 
> On 5/18/2015 7:58 AM, Paul Sandoz wrote:
>> Ho Roger,
>> 
>> I mostly focused on the specification.
>> 
>> Paul.
>> 
>> 
>> Process
>> --
>> 
>>  35  * {@code Process} provides control of native processes started by
>>  36  * ProcessBuilder.start and Runtime.exec.
>> 
>> Link to those methods?
> Links are not preferred in the first sentence used in the class summary.
> They are linked in the paragraph that follows

Ok, seems odd to me but oh well..


>> 
>> 
>>   92     /**
>>   93      * Default constructor for Process.
>>   94      */
>>   95     public Process() {}
>> 
>> Is that redundant?
> It seemed good form to document the constructor exists.
> The implicit public zero-arg constructor can't have javadoc.

But there is nothing to say :-)


>> 
>> 
>>  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.


>> 
>> 
>>  270      * <p>Note: The subprocess may not terminate immediately.
>>  271      * i.e. {@code isAlive()} may return true for a brief period
>>  272      * after {@code destroyForcibly()} is called. This method
>>  273      * may be chained to {@code waitFor()} if needed.
>>  274      *
>> 
>> Use @apiNote?
> seems reasonable
> But the resulting javadoc breaks up the flow of information. the API note that a developer
> would want to know about is far from the rest of the method description.

Ok.


>> 
>> 
>>  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.


>> 
>> 
>>  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 ..." ?


>> 
>> 
>>  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.


>> 
>> 
>>  480      * <em>Note that processes are created and terminate asynchronously.
>>  481      * There is no guarantee that a process is {@link #isAlive alive}.
>>  482      * </em>
>> 
>> s/terminate/terminated/
> Well sometimes the terminate themselves, 'terminated' sounds like it is an
> external actor performing the termination.

Ok.


>> 
>> 
>> ProcessHandle
>> --
>> 
>>   79  * For example, EPERM on Linux.
>> 
>> I presume you are referring to native process-related operations that return an error code of EPERM?
> Yes, but the example does not add much and I'll remove it.

Ok.


>> 
>> 
>>   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.


>> 
>> 
>>  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.

Paul.

> Unless there is sufficient need for information about why the request was
> denied. That would suggest an exception be thrown with a os specific message.