RFR 9: 8132394 : (process) ProcessBuilder support for a pipeline of processes

[Roger Riggs]

Hi Paul,

Thanks for the review,

On 11/9/2015 4:32 AM, Paul Sandoz wrote:
> Hi Roger,
>
> ProcessBuilder
> —
>
>   711      * The FileDescriptor is used as the standard input of the next Process
>   712      * begin started.
>
> s/begin/to be started
> ?
ok
>
>   714     static class RedirectPipeImpl extends Redirect {
>   715         final FileDescriptor fd;
>   716
>   717         RedirectPipeImpl() {
>   718             this.fd = new FileDescriptor();
>   719         }
>   720         public Type type() { return Type.PIPE; }
>   721         public String toString() { return type().toString();}
>   722         FileDescriptor getFd() { return fd; }
>   723     }
>
> Can you add @Override to appropriate methods?
ok
>
> Why do you need to use a FileDescriptor rather than an int? AFAICT FileDescriptor is used as a box to the underlying descriptor that is accessed via shared secrets.
It is a cleaner encapsulation for the file descriptor.  On Windows, it 
is a handle, not a fd.
>
>
> 1140      * All input and output streams between the intermediate processes are
> 1141      * not accessible, those redirects should be {@link Redirect#PIPE Redirect.PIPE}.
>
> This was a little unclear to me until i read the documentation for throwing an ISE*(IAE)*  e.g. we need to talk about the process builder here.
ok,  added a description of the redirects to the the previous paragraph 
that is talking about first and last.
      * The redirects for standard
      * input of the first process and standard output of the last 
process are
      * initialized using the redirect settings of the respective 
ProcessBuilder.
      * All other {@code ProcessBuilder} redirects should be
      * {@link Redirect#PIPE Redirect.PIPE}.
>
> 1153      * @apiNote
> 1154      * For example to count the  unique imports for all the files in a file hierarchy:
> 1155      *
> 1156      * <pre>{@code
> 1157      * String directory = "/home/duke/src";
> 1158      * List<Process> processes = ProcessBuilder.startPipeline(
> 1159      *         new ProcessBuilder("find", directory,  "-type", "f"),
> 1160      *         new ProcessBuilder("xargs", "grep", "-h", "^import "),
> 1161      *         new ProcessBuilder("awk", "{print $2;}"),
> 1162      *         new ProcessBuilder("sort", "-u"));
> 1163      * Process last = processes.get(processes.size()-1);
> 1164      * try (InputStream is = last.getInputStream();
> 1165      *         Reader isr = new InputStreamReader(is);
> 1166      *         BufferedReader r = new BufferedReader(isr)) {
> 1167      *     long count = r.lines().count();
> 1168      * }
> 1169      * }</pre>
>
> Perhaps you might want to qualify that example saying something like “on a UNIX compatible platform”.
>
+1
> 1214             processes.forEach(p -> p.destroyForcibly());
>
> If you feel in anyway inclined you can use a method reference here.
ok
>
>
> PipelineTest
> —
>
> Can you make the test method names more meaningful?
ok

Thanks, Roger

>
> Paul.
>
>> On 5 Nov 2015, at 22:56, Roger Riggs <Roger.Riggs at Oracle.com> wrote:
>>
>> Please review the new ProcessBuilder.startPipeline API, implementation, and tests.
>>
>> The new method starts a Process for each ProcessBuilder, creating a pipeline of
>> processes linked by their standard output and standard input streams.
>> Each builder can use redirectErrorsream to coalesce error output with standard output.
>> But otherwise standard error streams are not modified.
>>
>> The API is the same as discussed on the earlier core-libs thread [1] and addresses
>> the comments.
>>
>> webrev:
>>    http://cr.openjdk.java.net/~rriggs/webrev-pipeline-8132394/
>>
>> javadoc of ProcessBuilder:  only startPipeline is new:
>>    http://cr.openjdk.java.net/~rriggs/pipedoc/
>>
>> Thanks, Roger
>>
>> p.s. The PIPE_CHANNEL redirection proposed by Peter Levert is complementary
>> and still has a bug or two to work out.
>>
>> [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-July/034634.html
>>
>>
>>
>>
>>