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
