Add Process.runSync for running processe synchronously.

Add Process.runSync for running processes synchronously

BUG=http://dartbug.com/1707

R=whesse@google.com

Committed: https://code.google.com/p/dart/source/detail?r=26052

[Bill Hesse, 2013-08-09 15:32:23]

A first set of comments.

https://codereview.chromium.org/21816002/diff/8001/runtime/bin/io_buffer.cc
File runtime/bin/io_buffer.cc (right):

https://codereview.chromium.org/21816002/diff/8001/runtime/bin/io_buffer.cc#n...
runtime/bin/io_buffer.cc:27: 
I see why it is difficult to use Create in Allocate, to avoid duplicated code,
so it is OK not to.

https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_linux.cc
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_linux....
runtime/bin/process_linux.cc:629: if (TEMP_FAILURE_RETRY(poll(fds, alive, -1))
<= 0) {
Add comment - blocking call.  This implements a Dart synchronous function that
can block - the only one.

https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_linux....
runtime/bin/process_linux.cc:649: exit_code_data.bytes, 8));
shouldn't this be read(fds[i].fd, exit_code_data.bytes + bytes, 8)

(or 8 - bytes)?

https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_linux....
runtime/bin/process_linux.cc:666: if (i < alive) {
This guard is not really needed.  But it is OK, since it skips unneeded work.

https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_macos.cc
File runtime/bin/process_macos.cc (right):

https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_macos....
runtime/bin/process_macos.cc:651: exit_code_data.bytes, 8));
Ditto - + bytes

[Søren Gjesse, 2013-08-12 06:52:38]

https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_linux.cc
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_linux....
runtime/bin/process_linux.cc:629: if (TEMP_FAILURE_RETRY(poll(fds, alive, -1))
<= 0) {
On 2013/08/09 15:32:23, Bill Hesse wrote:
> Add comment - blocking call.  This implements a Dart synchronous function that
> can block - the only one.

Done.

https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_linux....
runtime/bin/process_linux.cc:649: exit_code_data.bytes, 8));
On 2013/08/09 15:32:23, Bill Hesse wrote:
> shouldn't this be read(fds[i].fd, exit_code_data.bytes + bytes, 8)
> 
> (or 8 - bytes)?

Removed the while, as we know that there are 8 bytes available.

https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_linux....
runtime/bin/process_linux.cc:666: if (i < alive) {
On 2013/08/09 15:32:23, Bill Hesse wrote:
> This guard is not really needed.  But it is OK, since it skips unneeded work.

This is needed as calling poll with closed file descriptors will fail with
invalid argument error.

https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_macos.cc
File runtime/bin/process_macos.cc (right):

https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_macos....
runtime/bin/process_macos.cc:651: exit_code_data.bytes, 8));
On 2013/08/09 15:32:23, Bill Hesse wrote:
> Ditto - + bytes

Removed the loop here as well.

[Søren Gjesse, 2013-08-12 12:29:28]

On 2013/08/12 06:52:38, Søren Gjesse wrote:
>
https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_linux.cc
> File runtime/bin/process_linux.cc (right):
> 
>
https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_linux....
> runtime/bin/process_linux.cc:629: if (TEMP_FAILURE_RETRY(poll(fds, alive, -1))
> <= 0) {
> On 2013/08/09 15:32:23, Bill Hesse wrote:
> > Add comment - blocking call.  This implements a Dart synchronous function
that
> > can block - the only one.
> 
> Done.
> 
>
https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_linux....
> runtime/bin/process_linux.cc:649: exit_code_data.bytes, 8));
> On 2013/08/09 15:32:23, Bill Hesse wrote:
> > shouldn't this be read(fds[i].fd, exit_code_data.bytes + bytes, 8)
> > 
> > (or 8 - bytes)?
> 
> Removed the while, as we know that there are 8 bytes available.
> 
>
https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_linux....
> runtime/bin/process_linux.cc:666: if (i < alive) {
> On 2013/08/09 15:32:23, Bill Hesse wrote:
> > This guard is not really needed.  But it is OK, since it skips unneeded
work.
> 
> This is needed as calling poll with closed file descriptors will fail with
> invalid argument error.
> 
>
https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_macos.cc
> File runtime/bin/process_macos.cc (right):
> 
>
https://codereview.chromium.org/21816002/diff/8001/runtime/bin/process_macos....
> runtime/bin/process_macos.cc:651: exit_code_data.bytes, 8));
> On 2013/08/09 15:32:23, Bill Hesse wrote:
> > Ditto - + bytes
> 
> Removed the loop here as well.

PTAL

Don't care about the in-efficient way to stdout and stderr data is collected. I
have a followup change i https://codereview.chromium.org/22827002/ to change
that.

[Søren Gjesse, 2013-08-12 14:50:15]

Patch Set #8 now includes the changes in 
https://codereview.chromium.org/22827002/.

[Bill Hesse, 2013-08-13 08:34:48]

LGTM, with comments.

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/eventhandler_...
File runtime/bin/eventhandler_win.h (right):

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.h:37: // An IOBuffer encapsulates the OVERLAPPED
structure and the
OverlappedBuffer

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process.cc
File runtime/bin/process.cc (right):

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process.cc#ne...
runtime/bin/process.cc:196: Process::Kill(pid, 9);
Grab the OS error before Process::Kill?

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process.h
File runtime/bin/process.h (right):

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process.h#new...
runtime/bin/process.h:177: }
Something seems missing in this for loop.

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process_macos.cc
File runtime/bin/process_macos.cc (right):

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process_macos...
runtime/bin/process_macos.cc:581: reinterpret_cast<void*>(tail_->data_ +
(kBufferSize - free_size_)),
Maybe put the pointer to buffer free space in a method on BufferListBase?

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process_win.cc
File runtime/bin/process_win.cc (right):

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process_win.c...
runtime/bin/process_win.cc:649: void DataRead(intptr_t size) {
Maybe DataIsRead, to distinguish between the present tense of read and past
participle read?

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process_win.c...
runtime/bin/process_win.cc:672: return head_->data_;
ASSERT(head_->next == NULL)?

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process_win.c...
runtime/bin/process_win.cc:697: // Get the data read as a rasult of a completed
overlapped operation.
result

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process_win.c...
runtime/bin/process_win.cc:763: // All pipes created to the sub-process supports
overlapped IO.
support

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process_win.c...
runtime/bin/process_win.cc:824: memcpy(&exit, oh[2].GetDataBuffer(),
sizeof(exit));
I would call this "GetFirstBufferSegment" or "GetFirstData" or "GetTinyData", to
be clear what this is, and also put an assert that the buffer has only one chunk
in the implementation.

https://codereview.chromium.org/21816002/diff/34001/tests/standalone/io/proce...
File tests/standalone/io/process_sync_script.dart (right):

https://codereview.chromium.org/21816002/diff/34001/tests/standalone/io/proce...
tests/standalone/io/process_sync_script.dart:23: exitCode =
int.parse(options.arguments[3]);
Why not call exit(exitCode) here?

https://codereview.chromium.org/21816002/diff/34001/tests/standalone/io/proce...
File tests/standalone/io/process_sync_test.dart (right):

https://codereview.chromium.org/21816002/diff/34001/tests/standalone/io/proce...
tests/standalone/io/process_sync_test.dart:13: // Get the Dart script file which
generated output.
that generates

https://codereview.chromium.org/21816002/diff/34001/tests/standalone/io/proce...
tests/standalone/io/process_sync_test.dart:14: var scriptFile =
Why not use Platform.script, and path package, to find process_sync_script?  I
thought our tests could actually run from any directory.  This will be a problem
if we copy the command from test.py, and run it manually when in a different
directory.

https://codereview.chromium.org/21816002/diff/34001/tests/standalone/io/proce...
tests/standalone/io/process_sync_test.dart:51: test(1, 100000, 100000, 0);
Maybe buffer blocksize and buffer blocksize - 1 here?

[Søren Gjesse, 2013-08-13 13:00:38]

Landing when tested on Mac OS and Windows once more

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/eventhandler_...
File runtime/bin/eventhandler_win.h (right):

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.h:37: // An IOBuffer encapsulates the OVERLAPPED
structure and the
On 2013/08/13 08:34:48, Bill Hesse wrote:
> OverlappedBuffer

Done.

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process.cc
File runtime/bin/process.cc (right):

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process.cc#ne...
runtime/bin/process.cc:196: Process::Kill(pid, 9);
On 2013/08/13 08:34:48, Bill Hesse wrote:
> Grab the OS error before Process::Kill?

Done.

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process.h
File runtime/bin/process.h (right):

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process.h#new...
runtime/bin/process.h:177: }
On 2013/08/13 08:34:48, Bill Hesse wrote:
> Something seems missing in this for loop.

Added

delete current;

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process_macos.cc
File runtime/bin/process_macos.cc (right):

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process_macos...
runtime/bin/process_macos.cc:581: reinterpret_cast<void*>(tail_->data_ +
(kBufferSize - free_size_)),
On 2013/08/13 08:34:48, Bill Hesse wrote:
> Maybe put the pointer to buffer free space in a method on BufferListBase?

Done.

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process_win.cc
File runtime/bin/process_win.cc (right):

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process_win.c...
runtime/bin/process_win.cc:649: void DataRead(intptr_t size) {
On 2013/08/13 08:34:48, Bill Hesse wrote:
> Maybe DataIsRead, to distinguish between the present tense of read and past
> participle read?

Done. Added a comment as well.

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process_win.c...
runtime/bin/process_win.cc:672: return head_->data_;
On 2013/08/13 08:34:48, Bill Hesse wrote:
> ASSERT(head_->next == NULL)?

Done.

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process_win.c...
runtime/bin/process_win.cc:697: // Get the data read as a rasult of a completed
overlapped operation.
On 2013/08/13 08:34:48, Bill Hesse wrote:
> result

Done.

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process_win.c...
runtime/bin/process_win.cc:763: // All pipes created to the sub-process supports
overlapped IO.
On 2013/08/13 08:34:48, Bill Hesse wrote:
> support

Done.

https://codereview.chromium.org/21816002/diff/34001/runtime/bin/process_win.c...
runtime/bin/process_win.cc:824: memcpy(&exit, oh[2].GetDataBuffer(),
sizeof(exit));
On 2013/08/13 08:34:48, Bill Hesse wrote:
> I would call this "GetFirstBufferSegment" or "GetFirstData" or "GetTinyData",
to
> be clear what this is, and also put an assert that the buffer has only one
chunk
> in the implementation.

Done.

https://codereview.chromium.org/21816002/diff/34001/tests/standalone/io/proce...
File tests/standalone/io/process_sync_script.dart (right):

https://codereview.chromium.org/21816002/diff/34001/tests/standalone/io/proce...
tests/standalone/io/process_sync_script.dart:23: exitCode =
int.parse(options.arguments[3]);
On 2013/08/13 08:34:48, Bill Hesse wrote:
> Why not call exit(exitCode) here?

Done.

https://codereview.chromium.org/21816002/diff/34001/tests/standalone/io/proce...
File tests/standalone/io/process_sync_test.dart (right):

https://codereview.chromium.org/21816002/diff/34001/tests/standalone/io/proce...
tests/standalone/io/process_sync_test.dart:13: // Get the Dart script file which
generated output.
On 2013/08/13 08:34:48, Bill Hesse wrote:
> that generates

Done.

https://codereview.chromium.org/21816002/diff/34001/tests/standalone/io/proce...
tests/standalone/io/process_sync_test.dart:14: var scriptFile =
On 2013/08/13 08:34:48, Bill Hesse wrote:
> Why not use Platform.script, and path package, to find process_sync_script?  I
> thought our tests could actually run from any directory.  This will be a
problem
> if we copy the command from test.py, and run it manually when in a different
> directory.

This is some old code I copied. Changed to

  var scriptFile = new File(join(dirname(Platform.script),
                                 "process_sync_script.dart"));

https://codereview.chromium.org/21816002/diff/34001/tests/standalone/io/proce...
tests/standalone/io/process_sync_test.dart:51: test(1, 100000, 100000, 0);
On 2013/08/13 08:34:48, Bill Hesse wrote:
> Maybe buffer blocksize and buffer blocksize - 1 here?

Done.

[Søren Gjesse, 2013-08-13 14:12:50]

Committed patchset #12 manually as r26052 (presubmit successful).

[Anders Johnsen, 2013-08-19 05:27:59]

DBC

https://codereview.chromium.org/21816002/diff/52001/runtime/bin/process_linux.cc
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/21816002/diff/52001/runtime/bin/process_linux...
runtime/bin/process_linux.cc:646: return CloseProcessBuffers(fds);
If we return here (or any other place after start, and before successfully
close), we'll not be calling BufferList::Free(). I think we should either

1) Always call Free on CloseProcessBuffers.
2) Always call Free on ~BufferList (I prefer this - double call should be fine).

https://codereview.chromium.org/21816002/diff/52001/runtime/bin/process_win.cc
File runtime/bin/process_win.cc (right):

https://codereview.chromium.org/21816002/diff/52001/runtime/bin/process_win.c...
runtime/bin/process_win.cc:786: OverlappedHandle oh[kHandles];
Best variable name ever! :)