RFR 9: 8077350 Process API Updates Implementation Review

Mon May 18 11:58:21 UTC 2015

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?

  92     /**
  93      * Default constructor for Process.
  94      */
  95     public Process() {}

Is that redundant?

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 * 
 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 *
 262 * The default implementation of this method invokes {@link #destroy}
 263 * and so may not forcibly terminate the process.
 264 * Concrete implementations of this class are strongly encouraged to override
 265 * this method with a compliant implementation.
 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?

270 * 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?

 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/ ?

 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 /

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?

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

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

 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.

480 * Note that processes are created and terminate asynchronously.
 481 * There is no guarantee that a process is {@link #isAlive alive}.
 482 *

s/terminate/terminated/

ProcessHandle
--

  79  * For example, EPERM on Linux.

I presume you are referring to native process-related operations that return an error code of EPERM?

  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?

167 /**
 168 * Returns a snapshot of all processes visible to the current process.
 169 * 
 170 * 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 *

Talks about "list".

 192      * @return a snapshot of information about the process; non-null

As for Process.info.

 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/

 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.

 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?

 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?

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