RFR 9: 8077350 Process API Updates Implementation Review

Peter Levart peter.levart at gmail.com
Fri May 15 10:35:50 UTC 2015 

Hi Roger,

On 05/14/2015 03:44 PM, Roger Riggs wrote:
> Hi Peter,
>
> On 5/14/15 8:19 AM, Peter Levart wrote:
>> Hi Roger,
>>
>> The new API using Optional(s) looks fine. In particular for the 
>> ProcessHandle returning methods. They now either return 
>> Stream<ProcessHandle> or Optional<ProcessHandle>.
>>
>> At some point in the development of this API, the implementation 
>> introduced the AsyncExecutor to execute synchronous continuations of 
>> the onExit() returned CompletableFuture(s). What was the main 
>> motivation for this given that:
>> - previously, ForkJoinPoll.commonPool() was used instead that by 
>> default possesses some similar characteristics (Innocuous threads 
>> when SecurityManager is active)
> The AsyncExecutor also uses InnocuousThreads.

So that's not what is lost or gained by AsyncExecutor when comparing it 
with commonPool().

>
>> - this AsyncExecutor is only effective for 1st "wave" of synchronous 
>> continuations. Asynchronous continuations and synchronous 
>> continuations following them will still use ForkJoinPoll.commonPool()
> Unfortunately, the common ForkJoinPool assumes that tasks queued to it 
> complete relatively
> quickly and free the thread.  It does not grow the number of threads 
> and is not appropriate
> for tasks that block for indefinite periods as might be need to wait 
> for a Process to exit.

Ok, AsyncExecutor might be required for default implementation of 
Process.onExit(). But waiting on process exit in ProcessImpl and 
ProcessHandle is performed by internal 32K stack-sized reaper threads 
and what we did in ProcessImpl.onExit() was this (before the AsyncExecutor):

     @Override
     public CompletableFuture<Process> onExit() {
         return ProcessHandleImpl.completion(pid, false)
                 .handleAsync((exitStatus, unusedThrowable) -> this);
     }

Which means that FJP.commonPool() thread is employed after 
ProcessHandleImpl.completion() is completed. Meaning that just user code 
is run by commonPool() and that code does not wait for process to exit 
because it already did exit. User code might run for extended period of 
time, but any use of CompletableFuture is susceptible to that abuse. Are 
you afraid that users of Process API are not common CompletableFuture 
users and are not aware of commonPool() constraints?

>>
>> Would an alternative be to define two overloaded onExit() methods in 
>> the style of CompletableFuture itself?
>>
>>     CompletableFuture<ProcessHandle> onExit();
>>     CompletableFuture<ProcessHandle> onExit(Executor executor);
>>
>> ...and give the user a chance to supply it's own Executor if the 
>> default ForkJoinPoll.commonPool() does not fit?
> It is only one more method in PH and Process but that function is 
> available from CompletableFuture
> though perhaps not as conveniently.
> The onExit method returns a CompletableFuture that has the entire 
> complement of
> synchronous and async methods available to it.  The application can 
> control
> where subsequent computations are performed.

That's true for xxxxAsync methods. Synchronous continuations are 
executed by whichever thread executed the previous stage. We insert an 
asynchronous stage to shield the internal 32K threads from user code. 
What we do by executing that stage in AsyncExecutor might be desirable 
when the user attaches a synchronous continuation. But when he attaches 
an asynchronous one, the thread from AsyncExecutor is used just for 
mapping the result into Process instance and triggering the execution of 
next stage. Can we eliminate this unnecessary asynchronous stage?

What would be if we stoped pretending that user can execute a 
synchronous onExit() continuation when in fact that continuation is 
always attached to an asynchronous stage so it is actually asynchronous? 
What if CompletableFuture had a public superclass called 
AsyncCompletableFuture that only exposed .xxxxAsync() continuation 
methods which returned normal CompletableFuture(s)? Such class would by 
definition shield the thread that completes an AsyncCompletableFuture 
from user code that attaches continuations. And user code would have 
exclusive control on what type of thread executes it. Isn't this a 
desirable property that jsr166 could provide? It could be useful in 
other scenarios too.

The only problem with that approach is that we need a mapping stage that 
"attaches" the Proses[Handle] instance to 
AsyncCompletableFuture<Integer> (which is generic and returns an exit 
status) to get AsyncCompletableFuture<Process[Handle]>. Such operation 
could be provided by AsyncCompletableFuture as an "immediate" operation 
- not taking any lambda function which needs a thread to be executed, 
but a replacement object instead:

public class AsyncCompletableFuture ... {

     /**
      * Returns a new CompletionStage that, when this stage completes
      * normally, completes with the given replacementResult.
      *
      * @param replacementResult the successful completion value of the 
returned
      *               CompletionStage
      * @param <U> the value's type
      * @return the new CompletionStage
      */
     public <U> AsyncCompletableFuture<U> thenReplace(U 
replacementResult) { ...

I can ask on the concurrency-interest list about the feasibility of such 
[Async]CompletableFuture split. Would you be interested in using 
AsyncCompletableFuture if it was available?

>
> That convenience method could be added later when the use case and 
> frequency is clearer.

I saw it only as a workaround for needing AsyncExecutor. I don't like it 
either.

Regards, Peter

>>
>> Is there expectation that ForkJoinPoll.commonPool() will not fit in 
>> the common case?
> I don't have a sense of how onExit would be used or how often it would 
> be used.
> The common FJP seems like a good start for computations to be performed
> after the Process has exited.  I'd wait and see what needs can be 
> articulated.,
> hopefully within the JDK 9 timeframe.
>
> Thanks, Roger
>
>>
>> Regards, Peter
>>
>> On 05/13/2015 04:16 PM, Roger Riggs wrote:
>>> Hi,
>>>
>>> Are there any comments about the use of java.util.Optional in the 
>>> ProcessHandle API?
>>> Or a review of the changes?
>>>
>>> Thanks, Roger
>>>
>>>
>>> On 5/11/2015 11:49 AM, Roger Riggs 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