RFR 9: 8077350 Process API Updates Implementation Review
Alan.Bateman at oracle.com
Thu May 14 12:30:03 UTC 2015
On 13/05/2015 16:10, Roger Riggs wrote:
> Hi Alan,
> Yes, the destroy use looks a bit odd. In Process, destroyForcibly
> returns this so that the application can fluently wait for the
> termination to complete.
> Returning Optional<ProcessHandle> enables the case to fluently
> perform an action on the process when it does exit.
> ProcessHandle ph = ...;
> ph.destroy().ifPresent(p -> p.onExit( ...));
> Optional.isPresent() provides a boolean if that's needed.
Optional is good and the usages with parent(), etc. are nice update to
the API. Chaining methods invocations is good too.
It's just destroy that seems a bit much as it's too easy to mix up
"present" and "alive", e.g.: the process was present so we attempted to
destroy it, it may or may not be present now. In that context using
ifPresent in this example is confusing to read because it's really "was
present, on its way to not being present" (if you know what I mean).
That's the only one that might need to be looked at again to see if
there are better candidates. Related to this is that the destroy methods
don't have a way to communicate anything useful to log when they fail.
With Process no long implementing ProcessHandle then there is an
opportunity to look at that again.
More information about the core-libs-dev