RFR 9: 8077350 Process API Updates Implementation Review
Roger Riggs
Roger.Riggs at Oracle.com
Mon May 18 20:49:10 UTC 2015
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
>
>
> 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.
>
>
> 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.
>
>
> 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.
>
>
> 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().
>
>
> 390 * When the {@link #waitFor()} returns successfully the CompletableFuture is
> 391 * {@link java.util.concurrent.CompletableFuture#complete completed} regardless
> 392 * of the exit status of the process.
>
> s/When the/When /
fixed
>
>
> 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.
>
>
> 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.
>
>
> 437 * @implSpec
> 438 * This implementation throws an instance of
> 439 * {@link java.lang.UnsupportedOperationException} and performs no other action.
> 440 * Sub-types should override this method to provide a ProcessHandle for the
> 441 * process. The methods {@link #getPid}, {@link #info}, {@link #children},
> 442 * and {@link #allChildren}, unless overridden, operate on the ProcessHandle.
>
> IIRC i incorrectly suggested Sub-types rather than Subclasses. There are other places with similar requirements where we can use the same language (e.g. destroyForcibly and onExit) "Subclasses should override this method...."
Fixed
>
>
> 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.
>
>
> 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.
>
>
> 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.
>
>
> 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.
>
>
> 167 /**
> 168 * Returns a snapshot of all processes visible to the current process.
> 169 * <p>
> 170 * <em>Note that processes are created and terminate asynchronously. There
> 171 * is no guarantee that a process in the list is alive or that no other
> 172 * processes may have been created since the inception of the snapshot.
> 173 * </em>
>
> Talks about "list".
thx, will fix.
>
>
> 192 * @return a snapshot of information about the process; non-null
>
> As for Process.info.
fixed
>
>
> 196 /**
> 197 * Information snapshot about a process.
> 198 * The attributes of a process vary by operating system and not available
> 199 * in all implementations. Information about processes is limited
>
> s/and not/and are not/
fixed
>
>
> 260 * If the process is {@link #isAlive not alive} the {@link CompletableFuture}
> 261 * is immediately {@link java.util.concurrent.CompletableFuture#complete completed}.
>
> As for Process.onExit.
same answer
>
>
> 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.
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.
>
>
> 345 * Compares this ProcessHandle with the specified object for order.
>
> s/with the specification object for order./with another process handle./ ?
>
>
> ProcessHandleImpl
> --
>
> 317 @Override
> 318 public Optional<ProcessHandle> destroyForcibly() {
> 319 if (this.equals(current)) {
> 320 throw new IllegalStateException("destroy of current process not allowed");
> 321 }
> 322 return (destroy0(getPid(), false))
> 323 ? Optional.of(this) : Optional.empty();
> 324 }
>
> Should pass "true" to destroy0?
fixed.
Thanks, Roger
>
> Paul.
>
>
> On May 11, 2015, at 5:49 PM, Roger Riggs <Roger.Riggs at Oracle.com> wrote:
>
>> Please review clarifications and updates to the proposed Precess API.
>>
>> A few loose ends in the ProcessHandle API were identified.
>>
>> 1) The ProcessHandle.parent() method currently returns null if the parent cannot
>> be determined and the ProcessHandle.of(PID) method returns null if the PID does not exist.
>> It has been suggested to return an Optional<ProcessHandle> to make
>> these methods more flexible and allow a fluent style and work better with streams.
>>
>> 2) The behavior of Processhandle.destroy and destroyForcibly are different
>> than Process.destroy and destroyForcibly. Those functions always succeed because
>> they are children of the spawning process.
>>
>> In contrast, ProcessHandle.destroy and destroyForcible are requests to
>> destroy the process and may not succeed due to operating system restrictions such
>> as the process not being a child or not having enough privilege.
>> The description of the methods needs to be clarified that it is a request to destroy
>> and it may not succeed, In that case the destroy and destroyForcibly methods
>> should indicate that the request was not successful. In particular, the caller
>> may not want to wait for the process to terminate (its not going to).
>>
>> The proposed update is to return an Optional<ProcessHandle> .
>> It can be streamed and can take advantage of the conditional operations on the Optional.
>>
>> 3) Using Optional is also attractive for the return values of the information
>> about a ProcessHandles, since not all values are available from every OS.
>> The returns values of Info.command, arguments, startInstant, totalDuration, and user
>> are proposed to be updated to return Optional<x>.
>> It allows for more compact code and fewer explicit checks for null.
>>
>> Please review and comment:
>>
>> Webrev:
>> http://cr.openjdk.java.net/~rriggs/webrev-ph/
>>
>> javadoc:
>> http://cr.openjdk.java.net/~rriggs/ph-apidraft/
>>
>> Diffs of the spec/javadoc from previous draft:
>> http://cr.openjdk.java.net/~rriggs/ph-diffs-2015-05-11/overview-summary.html
>>
>> Thanks, Roger
>>
>>
>>
More information about the core-libs-dev
mailing list