Shutdown the service isolate when shutting down the VM

Shutdown the service isolate when shutting down the VM

- Add a service control message to initiate service isolate shutdown.
- Provide a callback for the embedder to do any service isolate cleanup.
- Dart_Cleanup will now block until the service isolate shuts down.

R=zra@google.com

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

[Cutch, 2015-03-24 21:02:58]

johnmccutchan@google.com changed reviewers:
+ zra@google.com

[zra, 2015-03-24 21:53:59]

https://codereview.chromium.org/1030013002/diff/1/runtime/bin/vmservice/serve...
File runtime/bin/vmservice/server.dart (right):

https://codereview.chromium.org/1030013002/diff/1/runtime/bin/vmservice/serve...
runtime/bin/vmservice/server.dart:183: close(bool force) {
Return type

https://codereview.chromium.org/1030013002/diff/1/runtime/bin/vmservice/vmser...
File runtime/bin/vmservice/vmservice_io.dart (right):

https://codereview.chromium.org/1030013002/diff/1/runtime/bin/vmservice/vmser...
runtime/bin/vmservice/vmservice_io.dart:33: server.close(true).catchError((e,
st) => null);
Maybe the catchError should assert() so that we at least see the error in
shutdown in checked mode.

https://codereview.chromium.org/1030013002/diff/1/runtime/bin/vmservice/vmser...
runtime/bin/vmservice/vmservice_io.dart:37: _bootServer() {
Why drop the return types?

https://codereview.chromium.org/1030013002/diff/1/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/1030013002/diff/1/runtime/vm/dart.cc#newcode187
runtime/vm/dart.cc:187: ServiceIsolate::Shutdown();
Make a note that this needs to happen before we try to delete the thread_pool_.

https://codereview.chromium.org/1030013002/diff/1/runtime/vm/service_isolate.cc
File runtime/vm/service_isolate.cc (right):

https://codereview.chromium.org/1030013002/diff/1/runtime/vm/service_isolate....
runtime/vm/service_isolate.cc:467: if (!IsRunning()) {
Should this be an ASSERT()?

https://codereview.chromium.org/1030013002/diff/1/runtime/vm/service_isolate....
runtime/vm/service_isolate.cc:470: if ((exit_message_ == NULL) ||
(exit_message_length_ == 0)) {
ditto

https://codereview.chromium.org/1030013002/diff/1/runtime/vm/service_isolate....
runtime/vm/service_isolate.cc:719: if (!IsRunning()) {
ditto

https://codereview.chromium.org/1030013002/diff/1/runtime/vm/service_isolate....
runtime/vm/service_isolate.cc:729: while (shutting_down_ && (port_ !=
ILLEGAL_PORT)) {
Why would port_ be illegal?

[Cutch, 2015-03-24 23:39:08]

https://codereview.chromium.org/1030013002/diff/1/runtime/bin/vmservice/serve...
File runtime/bin/vmservice/server.dart (right):

https://codereview.chromium.org/1030013002/diff/1/runtime/bin/vmservice/serve...
runtime/bin/vmservice/server.dart:183: close(bool force) {
On 2015/03/24 21:53:59, zra wrote:
> Return type

I'm omitting 'void' return types in Dart code.

https://codereview.chromium.org/1030013002/diff/1/runtime/bin/vmservice/vmser...
File runtime/bin/vmservice/vmservice_io.dart (right):

https://codereview.chromium.org/1030013002/diff/1/runtime/bin/vmservice/vmser...
runtime/bin/vmservice/vmservice_io.dart:33: server.close(true).catchError((e,
st) => null);
On 2015/03/24 21:53:59, zra wrote:
> Maybe the catchError should assert() so that we at least see the error in
> shutdown in checked mode.

Done.

https://codereview.chromium.org/1030013002/diff/1/runtime/bin/vmservice/vmser...
runtime/bin/vmservice/vmservice_io.dart:37: _bootServer() {
On 2015/03/24 21:53:59, zra wrote:
> Why drop the return types?

I'm omitting 'void' return types in Dart code.

https://codereview.chromium.org/1030013002/diff/1/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/1030013002/diff/1/runtime/vm/dart.cc#newcode187
runtime/vm/dart.cc:187: ServiceIsolate::Shutdown();
On 2015/03/24 21:53:59, zra wrote:
> Make a note that this needs to happen before we try to delete the
thread_pool_.

I've moved it up and left a note.

https://codereview.chromium.org/1030013002/diff/1/runtime/vm/service_isolate.cc
File runtime/vm/service_isolate.cc (right):

https://codereview.chromium.org/1030013002/diff/1/runtime/vm/service_isolate....
runtime/vm/service_isolate.cc:467: if (!IsRunning()) {
On 2015/03/24 21:53:59, zra wrote:
> Should this be an ASSERT()?

No, we may have failed to startup the service isolate or the service isolate
could have already exited.

https://codereview.chromium.org/1030013002/diff/1/runtime/vm/service_isolate....
runtime/vm/service_isolate.cc:470: if ((exit_message_ == NULL) ||
(exit_message_length_ == 0)) {
On 2015/03/24 21:53:59, zra wrote:
> ditto

Similarly.

https://codereview.chromium.org/1030013002/diff/1/runtime/vm/service_isolate....
runtime/vm/service_isolate.cc:719: if (!IsRunning()) {
On 2015/03/24 21:53:59, zra wrote:
> ditto

ditto.

https://codereview.chromium.org/1030013002/diff/1/runtime/vm/service_isolate....
runtime/vm/service_isolate.cc:729: while (shutting_down_ && (port_ !=
ILLEGAL_PORT)) {
On 2015/03/24 21:53:59, zra wrote:
> Why would port_ be illegal?

Before the service isolate starts up and after it shuts down the port_ is
ILLEGAL_PORT (the canonical value for an uninitialized Dart_Port).

[zra, 2015-03-25 15:07:11]

lgtm

[Cutch, 2015-03-25 15:50:34]

Committed patchset #2 (id:20001) manually as 44685 (presubmit successful).

[kasperl, 2015-03-27 07:50:54]

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

[kasperl, 2015-03-27 07:50:56]

https://codereview.chromium.org/1030013002/diff/20001/runtime/bin/vmservice/v...
File runtime/bin/vmservice/vmservice_io.dart (right):

https://codereview.chromium.org/1030013002/diff/20001/runtime/bin/vmservice/v...
runtime/bin/vmservice/vmservice_io.dart:34: server.close(true).catchError((e,
st) => assert(e));
This is illegal syntax (assert isn't an expression) and it just broke our dev
channel build. I filed https://code.google.com/p/dart/issues/detail?id=23013 as
a request for doing static analysis on this code on every commit to catch such
errors a bit earlier.