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
