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 * <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 * 262 * <p>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 * <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? 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 * <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/ 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 * <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". 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.ri...@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 > > >