RFR 9: 8077350 Process API Updates Implementation Review (original) (raw)

Paul Sandoz paul.sandoz at oracle.com
Mon May 18 21:16:00 UTC 2015

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

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 *

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 *

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

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 * Note that processes are created and terminate asynchronously. 481 * There is no guarantee that a process is {@link #isAlive alive}. 482 * 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} 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 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.

More information about the core-libs-dev mailing list