Add support for starting a detached process

Add support for starting a detached process

BUG=http://dartbug.com/20970
R=kustermann@google.com, lrn@google.com

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

[Søren Gjesse, 2015-01-15 09:36:13]

sgjesse@google.com changed reviewers:
+ kustermann@google.com, lrn@google.com

[Søren Gjesse, 2015-01-15 09:36:14]

Lasse: mainly look as the API surface.

Linux only implementation. The code in Process::Start is getting pretty long and
I will refactor it in a separate
CL.

A few questions:

Should we add a Process.startDetached which completes with
a Future<int> instead?

This implementation completes with a Process object where
most getters (e.g. stdout) return null. The problem with
Future<int> is that we currently don't have way of killing
a numbered PID, but we could add that.

Should we do a chdir to '/' as well?

[Lasse Reichstein Nielsen, 2015-01-15 09:55:40]

API lgtm with comments.

https://codereview.chromium.org/798743004/diff/20001/sdk/lib/io/process.dart
File sdk/lib/io/process.dart (right):

https://codereview.chromium.org/798743004/diff/20001/sdk/lib/io/process.dart#...
sdk/lib/io/process.dart:246: * [:%WINDIR%\system32\cmd.exe:] is used on Windows.
What practical difference does it make to run through a shell?

https://codereview.chromium.org/798743004/diff/20001/sdk/lib/io/process.dart#...
sdk/lib/io/process.dart:262: * detached process does have any connection to its
parent and can
does -> does not
or better:
does have -> has no
?

https://codereview.chromium.org/798743004/diff/20001/sdk/lib/io/process.dart#...
sdk/lib/io/process.dart:275: bool detached: false});
Other boolean argument names are imperative, this one is descriptive. Maybe
change to "detach" for consistency?

https://codereview.chromium.org/798743004/diff/20001/tests/standalone/io/proc...
File tests/standalone/io/process_detached_test.dart (right):

https://codereview.chromium.org/798743004/diff/20001/tests/standalone/io/proc...
tests/standalone/io/process_detached_test.dart:26: print(process.pid);
Expect.isNotNull(process.pid);
?

https://codereview.chromium.org/798743004/diff/20001/tests/standalone/io/proc...
tests/standalone/io/process_detached_test.dart:44: Expect.isTrue(e is
ProcessException);
Won't this catch the expect.fail from above (but then it'll fail, so I guess the
test is still correctly failing, just with a different error than expected).
Consider changing to:
   future.then((process) {
     Expect.fail('Starting process from invalid executable succeeded');
   }, onError: (e) {
     Expect.isTrue(e is ProcessException);
   }).whenComplete( ...

[kustermann, 2015-01-16 16:11:04]

lgtm, but:

* Not sure If the Future<Process> API is particularly good (since most fields
don't make sense)
* In particular the C++ code is really messy, I'd split it into 2 methods.

https://codereview.chromium.org/798743004/diff/20001/runtime/bin/process_linu...
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/798743004/diff/20001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:264: exec_control_fd, &pid, sizeof(pid));
doesn't this fit on the previous line?

https://codereview.chromium.org/798743004/diff/20001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:408: VOID_TEMP_FAILURE_RETRY(close(STDIN_FILENO));
That one should be included by the loop above.

https://codereview.chromium.org/798743004/diff/20001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:467: ExitCodeHandler::ProcessStarted();
For detached processes this is not necessary. We're not listening for the exit
code, are we?

https://codereview.chromium.org/798743004/diff/20001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:517: char* message =
static_cast<char*>(malloc(kMaxMessageSize));
The malloc() can potentially return NULL.

https://codereview.chromium.org/798743004/diff/20001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:530: ASSERT(sizeof(int) == sizeof(child_errno));
That is a tautology because child_errno is defined as 'int' on line 505. :-/

https://codereview.chromium.org/798743004/diff/20001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:540: char* message =
static_cast<char*>(malloc(kMaxMessageSize));
ditto.

https://codereview.chromium.org/798743004/diff/20001/sdk/lib/io/process.dart
File sdk/lib/io/process.dart (right):

https://codereview.chromium.org/798743004/diff/20001/sdk/lib/io/process.dart#...
sdk/lib/io/process.dart:275: bool detached: false});
Are you sure we should return a Process object here?


From:
class Proces {
  Future<int> exitCode;
  Stream<List<int>> get stdout;
  Stream<List<int>> get stderr;
  IOSink get stdin;
  int get pid;
  bool kill([ProcessSignal signal = ProcessSignal.SIGTERM]);
}

*only* pid/kill make sense.


Alternatives:

a)
class Process {
  static Future<int> startDetached(...);
  static bool kill(int pid);
}

b)
class Process {
  static Future<DetachedProcess> startDetached(...);
}
class DetachedProcess {
  int pid;
  bool kill();
}

https://codereview.chromium.org/798743004/diff/20001/tests/standalone/io/proc...
File tests/standalone/io/process_detached_test.dart (right):

https://codereview.chromium.org/798743004/diff/20001/tests/standalone/io/proc...
tests/standalone/io/process_detached_test.dart:10: //
VMOptions=--short_socket_read --short_socket_write
Since we don't do any I/O here, I don't think we need to make 5 different test
cases out of this one.

https://codereview.chromium.org/798743004/diff/20001/tools/gyp/configurations...
File tools/gyp/configurations_make.gypi (right):

https://codereview.chromium.org/798743004/diff/20001/tools/gyp/configurations...
tools/gyp/configurations_make.gypi:7: 'dart_debug_optimization_level%': '0',
What is this?

[nweiz, 2015-01-20 20:40:37]

nweiz@google.com changed reviewers:
+ nweiz@google.com

[nweiz, 2015-01-20 20:40:38]

Once again, please CC interested parties on CLs that define new APIs they've
requested.

The fact that this provides no means of communication between the detached
process and the parent process limits its utility. A common pattern in other
languages is to start a subprocess normally, negotiate a communication channel
between it and the parent via stdin/stdout (e.g. "here's a URL I'm listening
on", "here's a socket file I'm using", etc), then detach the process. An API
along the lines of

class Process {
  // ...
  Future<DetachedProcess> detach();
}

would work better for that use case.

[Søren Gjesse, 2015-01-21 07:33:07]

On 2015/01/20 20:40:38, nweiz wrote:
> Once again, please CC interested parties on CLs that define new APIs they've
> requested.
> 
> The fact that this provides no means of communication between the detached
> process and the parent process limits its utility. A common pattern in other
> languages is to start a subprocess normally, negotiate a communication channel
> between it and the parent via stdin/stdout (e.g. "here's a URL I'm listening
> on", "here's a socket file I'm using", etc), then detach the process. An API
> along the lines of
> 
> class Process {
>   // ...
>   Future<DetachedProcess> detach();
> }
> 
> would work better for that use case.

Determining whether the process should be detached (deamonized) need to take
place when the process is started, and cannot be done when the process is
already running. However having an option of keeping stdin, stdout and stderr
open can be added. In that case the detached process can close its stdout and
stderr when initialization is done.

Instead of the detach flag it could be a mode enumeration (normal, detach,
detach_with_sdtio). I will look at that in a separate CL. Need to land this on
all platforms.

However in the case where several clients want to share the same daemon I am not
sure that this will work. If two clients start at the same time only one daemon
should end up running and they should both talk to the same. Both clients will
start a deamon, but one of then should detect that one is already running and
terminate. The one that "wins" should communicate that it is running to both
clients using e.g. a shared file.

[Søren Gjesse, 2015-01-21 12:32:35]

PTAL

Ported to Mac OS and Windows. I have checked that the Linux and Mac OS code is
exactly the same.

https://codereview.chromium.org/798743004/diff/20001/runtime/bin/process_linu...
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/798743004/diff/20001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:264: exec_control_fd, &pid, sizeof(pid));
On 2015/01/16 16:11:03, kustermann wrote:
> doesn't this fit on the previous line?

Done.

https://codereview.chromium.org/798743004/diff/20001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:408: VOID_TEMP_FAILURE_RETRY(close(STDIN_FILENO));
On 2015/01/16 16:11:03, kustermann wrote:
> That one should be included by the loop above.

Done, and added comment.

https://codereview.chromium.org/798743004/diff/20001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:467: ExitCodeHandler::ProcessStarted();
On 2015/01/16 16:11:03, kustermann wrote:
> For detached processes this is not necessary. We're not listening for the exit
> code, are we?

No, but we still need to call wait to avoid zombie processes when when
performing the first fork. The ExitCodeHandler is the only place where we call
wait.

https://codereview.chromium.org/798743004/diff/20001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:517: char* message =
static_cast<char*>(malloc(kMaxMessageSize));
On 2015/01/16 16:11:03, kustermann wrote:
> The malloc() can potentially return NULL.

Added check and return a fixed error if malloc fails.

https://codereview.chromium.org/798743004/diff/20001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:530: ASSERT(sizeof(int) == sizeof(child_errno));
On 2015/01/16 16:11:03, kustermann wrote:
> That is a tautology because child_errno is defined as 'int' on line 505. :-/

Yes, it is mainly there to document the checks for whether one or two times
sizeof(int) was read.

https://codereview.chromium.org/798743004/diff/20001/runtime/bin/process_linu...
runtime/bin/process_linux.cc:540: char* message =
static_cast<char*>(malloc(kMaxMessageSize));
On 2015/01/16 16:11:03, kustermann wrote:
> ditto.

ditto.

https://codereview.chromium.org/798743004/diff/20001/sdk/lib/io/process.dart
File sdk/lib/io/process.dart (right):

https://codereview.chromium.org/798743004/diff/20001/sdk/lib/io/process.dart#...
sdk/lib/io/process.dart:262: * detached process does have any connection to its
parent and can
On 2015/01/15 09:55:39, Lasse Reichstein Nielsen wrote:
> does -> does not
> or better:
> does have -> has no
> ?

Done.

https://codereview.chromium.org/798743004/diff/20001/sdk/lib/io/process.dart#...
sdk/lib/io/process.dart:275: bool detached: false});
On 2015/01/16 16:11:04, kustermann wrote:
> Are you sure we should return a Process object here?
> 
> 
> From:
> class Proces {
>   Future<int> exitCode;
>   Stream<List<int>> get stdout;
>   Stream<List<int>> get stderr;
>   IOSink get stdin;
>   int get pid;
>   bool kill([ProcessSignal signal = ProcessSignal.SIGTERM]);
> }
> 
> *only* pid/kill make sense.
> 
> 
> Alternatives:
> 
> a)
> class Process {
>   static Future<int> startDetached(...);
>   static bool kill(int pid);
> }
> 
> b)
> class Process {
>   static Future<DetachedProcess> startDetached(...);
> }
> class DetachedProcess {
>   int pid;
>   bool kill();
> }

Process.start is the Swiss army knife for starting processes, so I thing it make
sense to do it like this. We have Process.run for conveniently running a process
and not having to deal with stdout/stderr streams. We could add 

Regarding "static kill" on Process then we need to call it something else, as
kill is already taken, e.g. killPid, but that should also be fine.

https://codereview.chromium.org/798743004/diff/20001/sdk/lib/io/process.dart#...
sdk/lib/io/process.dart:275: bool detached: false});
On 2015/01/15 09:55:39, Lasse Reichstein Nielsen wrote:
> Other boolean argument names are imperative, this one is descriptive. Maybe
> change to "detach" for consistency?

Done.

https://codereview.chromium.org/798743004/diff/20001/tests/standalone/io/proc...
File tests/standalone/io/process_detached_test.dart (right):

https://codereview.chromium.org/798743004/diff/20001/tests/standalone/io/proc...
tests/standalone/io/process_detached_test.dart:10: //
VMOptions=--short_socket_read --short_socket_write
On 2015/01/16 16:11:04, kustermann wrote:
> Since we don't do any I/O here, I don't think we need to make 5 different test
> cases out of this one.

Good point. Removed.

https://codereview.chromium.org/798743004/diff/20001/tests/standalone/io/proc...
tests/standalone/io/process_detached_test.dart:26: print(process.pid);
On 2015/01/15 09:55:40, Lasse Reichstein Nielsen wrote:
> Expect.isNotNull(process.pid);
> ?

Done added

Expect.isTrue(process.pid is int);

as well.

https://codereview.chromium.org/798743004/diff/20001/tests/standalone/io/proc...
tests/standalone/io/process_detached_test.dart:44: Expect.isTrue(e is
ProcessException);
On 2015/01/15 09:55:40, Lasse Reichstein Nielsen wrote:
> Won't this catch the expect.fail from above (but then it'll fail, so I guess
the
> test is still correctly failing, just with a different error than expected).
> Consider changing to:
>    future.then((process) {
>      Expect.fail('Starting process from invalid executable succeeded');
>    }, onError: (e) {
>      Expect.isTrue(e is ProcessException);
>    }).whenComplete( ...

Good catch, thanks!

https://codereview.chromium.org/798743004/diff/20001/tools/gyp/configurations...
File tools/gyp/configurations_make.gypi (right):

https://codereview.chromium.org/798743004/diff/20001/tools/gyp/configurations...
tools/gyp/configurations_make.gypi:7: 'dart_debug_optimization_level%': '0',
On 2015/01/16 16:11:04, kustermann wrote:
> What is this?

Setting -O0 for debug builds. Only for debugging. Reverted.

[nweiz, 2015-01-21 21:48:32]

On 2015/01/21 07:33:07, Søren Gjesse wrote:
> On 2015/01/20 20:40:38, nweiz wrote:
> > Once again, please CC interested parties on CLs that define new APIs they've
> > requested.
> > 
> > The fact that this provides no means of communication between the detached
> > process and the parent process limits its utility. A common pattern in other
> > languages is to start a subprocess normally, negotiate a communication
channel
> > between it and the parent via stdin/stdout (e.g. "here's a URL I'm listening
> > on", "here's a socket file I'm using", etc), then detach the process. An API
> > along the lines of
> > 
> > class Process {
> >   // ...
> >   Future<DetachedProcess> detach();
> > }
> > 
> > would work better for that use case.
> 
> Determining whether the process should be detached (deamonized) need to take
> place when the process is started, and cannot be done when the process is
> already running. However having an option of keeping stdin, stdout and stderr
> open can be added. In that case the detached process can close its stdout and
> stderr when initialization is done.
> 
> Instead of the detach flag it could be a mode enumeration (normal, detach,
> detach_with_sdtio). I will look at that in a separate CL. Need to land this on
> all platforms.

This would work.

> However in the case where several clients want to share the same daemon I am
not
> sure that this will work. If two clients start at the same time only one
daemon
> should end up running and they should both talk to the same. Both clients will
> start a deamon, but one of then should detect that one is already running and
> terminate. The one that "wins" should communicate that it is running to both
> clients using e.g. a shared file.

[kustermann, 2015-01-22 09:33:03]

lgtm

https://codereview.chromium.org/798743004/diff/160001/runtime/bin/process_lin...
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/798743004/diff/160001/runtime/bin/process_lin...
runtime/bin/process_linux.cc:270: static const int kMaxMessageSize = 256;
why is the static needed?

https://codereview.chromium.org/798743004/diff/160001/runtime/bin/process_mac...
File runtime/bin/process_macos.cc (right):

https://codereview.chromium.org/798743004/diff/160001/runtime/bin/process_mac...
runtime/bin/process_macos.cc:261: static void ReportPid(int exec_control_fd, int
pid) {
I assume this file is doing the exact same thing as process_linux.cc, so I'm not
going to look at it in detail.

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

https://codereview.chromium.org/798743004/diff/160001/runtime/bin/process_win...
runtime/bin/process_win.cc:478: }
Do we necessarily need to open "NULL" 3 times in READ/WRITE?
Can't we just reuse one handle 3 times?

[then you also need the error checking code only once.

https://codereview.chromium.org/798743004/diff/160001/runtime/bin/process_win...
runtime/bin/process_win.cc:905: return result ? true : false;
I'm not really familiar with this code. Will we get a signal once the process is
dead (like on linux) or how do we remove it from the ProcessInfoList again?

https://codereview.chromium.org/798743004/diff/160001/tests/standalone/io/pro...
File tests/standalone/io/process_detached_test.dart (right):

https://codereview.chromium.org/798743004/diff/160001/tests/standalone/io/pro...
tests/standalone/io/process_detached_test.dart:31: });
Regarding this test: Assuming we would actually not detach the process but
rather just make stderr/stdin/stdout/exitCode null, how would this test catch
this? I.e. how do we test for the actual detaching?

Maybe we would need to put a process in the middle which will be killed and it's
parent could check for the existence of the grand child & kill it then.
Something like that.

[Søren Gjesse, 2015-01-22 11:47:25]

Thanks for the review.

https://codereview.chromium.org/798743004/diff/160001/runtime/bin/process_lin...
File runtime/bin/process_linux.cc (right):

https://codereview.chromium.org/798743004/diff/160001/runtime/bin/process_lin...
runtime/bin/process_linux.cc:270: static const int kMaxMessageSize = 256;
On 2015/01/22 09:33:02, kustermann wrote:
> why is the static needed?

It's not - removed.

https://codereview.chromium.org/798743004/diff/160001/runtime/bin/process_mac...
File runtime/bin/process_macos.cc (right):

https://codereview.chromium.org/798743004/diff/160001/runtime/bin/process_mac...
runtime/bin/process_macos.cc:261: static void ReportPid(int exec_control_fd, int
pid) {
On 2015/01/22 09:33:02, kustermann wrote:
> I assume this file is doing the exact same thing as process_linux.cc, so I'm
not
> going to look at it in detail.

It is, I have compared the diffs line-by-line.

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

https://codereview.chromium.org/798743004/diff/160001/runtime/bin/process_win...
runtime/bin/process_win.cc:478: }
On 2015/01/22 09:33:02, kustermann wrote:
> Do we necessarily need to open "NULL" 3 times in READ/WRITE?
> Can't we just reuse one handle 3 times?
> 
> [then you also need the error checking code only once.

It is, as the new process might close one of these, which should not result in
all being closed. I also looked at using DuplicateHandle, but just opening
seemed simpler as the Windows DuplicateHandle has just as many arguments as
CreateFile.

https://codereview.chromium.org/798743004/diff/160001/runtime/bin/process_win...
runtime/bin/process_win.cc:905: return result ? true : false;
On 2015/01/22 09:33:02, kustermann wrote:
> I'm not really familiar with this code. Will we get a signal once the process
is
> dead (like on linux) or how do we remove it from the ProcessInfoList again?

On Windows I don't put a detached process in to this list, as there are no
requirements for waiting on dead processes. On Windows you just wait on the
process handle as any other waitable object to get notified when it terminates.

https://codereview.chromium.org/798743004/diff/160001/tests/standalone/io/pro...
File tests/standalone/io/process_detached_test.dart (right):

https://codereview.chromium.org/798743004/diff/160001/tests/standalone/io/pro...
tests/standalone/io/process_detached_test.dart:31: });
On 2015/01/22 09:33:02, kustermann wrote:
> Regarding this test: Assuming we would actually not detach the process but
> rather just make stderr/stdin/stdout/exitCode null, how would this test catch
> this? I.e. how do we test for the actual detaching?
> 
> Maybe we would need to put a process in the middle which will be killed and
it's
> parent could check for the existence of the grand child & kill it then.
> Something like that.

Sounds like a good idea. I will think of something in a separate CL. That would
need support for killing a specific PID as well.

[Søren Gjesse, 2015-01-22 11:49:50]

Committed patchset #11 (id:200001) manually as 43070 (presubmit successful).