RFR 9: 8077350 Process API Updates Implementation Review

[Roger Riggs]

Hi Peter,

On 5/15/2015 6:35 AM, Peter Levart wrote:
> 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().
Are there other good attributes of commonPool that would be missed by 
using a separate pool?
>
>>
>>> - 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?
The earlier spec specifically used the common pool but from a 
specification point of view
it allows more flexibility in the implementation not to bind onExit to 
the commonPool.
Its not clear that the attributes of threads used to handle output or 
managing Processes
are the same as those attributed to the commonPool.

The default sizing of the commonPool is based on the number of processors
and does not grow (except via the ManagedBlockers) mechanism.
The number of threads waiting for processes is unrelated to the number 
of CPUs.
It is simpler to maintain to use a separate pool and not depend on 
consistent
assumptions between Process handling and the commonPool.

>
>>>
>>> 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?
One alternative is for onExit to return a CF subclass that maps 
synchronous methods
to the corresponding async methods.  Each call to onExit would return a 
new proxy
that would delegate to a new CF created from the ExitCompletion.handle(...).
For all requests that involved calling app code it would delegate to the 
corresponding
async method.  For example,

         CompletableFuture<ProcessHandle> cf =
                 ProcessHandleImpl.completion(getPid(), false)
                     .handle((exitStatus, unusedThrowable) -> this);
         return new CompletionProxy(cf);

The CompletionProxy can ensure that all CFs that call application code 
are Async.
It would override the syncronous methods and redirect to the 
corresponding Async method.

         public <U> CompletableFuture<U> thenApply(Function<? super 
ProcessHandle, ? extends U> fn) {
             return thenApplyAsync(fn);
         }

The CompletionProxy can also supply the Executor as needed unless 
supplied by the application

It adds a simple delegation but removed the extra task dispatch except 
where needed.

Does that work as you would expect?

Thanks, Roger




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