RFR 9: 8132394 : (process) ProcessBuilder support for a pipeline of processes
Roger Riggs
Roger.Riggs at Oracle.com
Mon Nov 9 18:07:33 UTC 2015
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
>>
>>
>>
>>
>>
More information about the core-libs-dev
mailing list