On May 18, 2015, at 10:49 PM, Roger Riggs <roger.ri...@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.
