VM restart + shutdown fixes

VM restart + shutdown fixes

This change add the ability to restart the vm through the service
protocol.  All isolates are killed, and then the main isolate is
restarted cooperatively by the embedder.

This change also fixes the message handler to prevent it from
accidentally ignoring vm shutdown messages.

Previously, we would stop handling messages whenever we hit an error
(such as a compile error or an unhandled exception).  This would leave
shutdown requests sitting the oob queue, neglected.

We now process *all* oob requests, up to the first shutdown request.
When we hit a shutdown request, we clear the oob queue and process no
more messages.

To make all of this work, we had to change the return value of
HandleMessage from bool to a new enum type, allowing the message
handler to distinguish *normal* error cases from the more rarified
shutdown and restart cases.

R=johnmccutchan@google.com, zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/0d501ad53d1be5689934ceb8a38d9cde39399eb8

[turnidge, 2015-10-02 20:50:36]

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

[zra, 2015-10-05 16:56:47]

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:641: // Unwind errors are always fatal and don't notify
listeners.
Is this comment still correct?

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1321: static
It looks like we'd usually break the long line in the parameter list rather than
here.

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:771: void KillLocked(LibMsgId msg_id = kInternalKillMsg);
Maybe instead of making this public, make RestartVMIsolateVisitor a friend?
Otherwise if this should stay public, then IsolateKillerVisitor no longer needs
to be a friend. Also, I'd prefer just explicitly passing the LibMsgId
everywhere. There shouldn't be many call sites to fix.

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/message_hand...
File runtime/vm/message_handler.cc (right):

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/message_hand...
runtime/vm/message_handler.cc:357: ((status == kOK) ? "no live ports" :
"error"),
It might also be useful to know which error it was.

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/message_hand...
File runtime/vm/message_handler_test.cc (right):

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/message_hand...
runtime/vm/message_handler_test.cc:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
Would it be easy to have some unit tests of the new behavior here? E.g.
processing all OOB messages, and stopping processing OOB messages when we get
kShutdown.

[Cutch, 2015-10-05 18:10:47]

https://codereview.chromium.org/1371193005/diff/20002/runtime/observatory/tes...
File runtime/observatory/tests/service/vm_restart_test.dart (right):

https://codereview.chromium.org/1371193005/diff/20002/runtime/observatory/tes...
runtime/observatory/tests/service/vm_restart_test.dart:82: expect(event.isolate,
equals(isolate));
nit: flip order of print and expect

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:318: if (!error.IsUnwindError()) {
Based on the comment I would expect to see:

if (error.IsUnwindError()) {
  return;
}

followed by event sending case.

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:310: const String& msg = String::Handle(String::New(
Maybe a comment here that this path kills the current isolate.

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:771: void KillLocked(LibMsgId msg_id = kInternalKillMsg);
On 2015/10/05 16:56:47, zra wrote:
> Maybe instead of making this public, make RestartVMIsolateVisitor a friend?
> Otherwise if this should stay public, then IsolateKillerVisitor no longer
needs
> to be a friend. Also, I'd prefer just explicitly passing the LibMsgId
> everywhere. There shouldn't be many call sites to fix.

+1 to suggestion of dropping default msg_id value.

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/message_hand...
File runtime/vm/message_handler.cc (right):

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/message_hand...
runtime/vm/message_handler.cc:259: bool
ShouldPause(MessageHandler::MessageStatus status) {
static

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/message_hand...
runtime/vm/message_handler.cc:357: ((status == kOK) ? "no live ports" :
"error"),
On 2015/10/05 16:56:47, zra wrote:
> It might also be useful to know which error it was.

+1

[turnidge, 2015-10-05 22:18:24]

PTAL

https://codereview.chromium.org/1371193005/diff/20002/runtime/observatory/tes...
File runtime/observatory/tests/service/vm_restart_test.dart (right):

https://codereview.chromium.org/1371193005/diff/20002/runtime/observatory/tes...
runtime/observatory/tests/service/vm_restart_test.dart:82: expect(event.isolate,
equals(isolate));
On 2015/10/05 18:10:46, Cutch wrote:
> nit: flip order of print and expect

Done.

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:318: if (!error.IsUnwindError()) {
On 2015/10/05 18:10:46, Cutch wrote:
> Based on the comment I would expect to see:
> 
> if (error.IsUnwindError()) {
>   return;
> }
> 
> followed by event sending case.

Changed the sense of the comment.  For some reason a return bothers me here.

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:310: const String& msg = String::Handle(String::New(
On 2015/10/05 18:10:46, Cutch wrote:
> Maybe a comment here that this path kills the current isolate.

Done.

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:641: // Unwind errors are always fatal and don't notify
listeners.
On 2015/10/05 16:56:47, zra wrote:
> Is this comment still correct?

Updated.

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1321: static
On 2015/10/05 16:56:47, zra wrote:
> It looks like we'd usually break the long line in the parameter list rather
than
> here.

Done.

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:771: void KillLocked(LibMsgId msg_id = kInternalKillMsg);
On 2015/10/05 16:56:47, zra wrote:
> Maybe instead of making this public, make RestartVMIsolateVisitor a friend?
> Otherwise if this should stay public, then IsolateKillerVisitor no longer
needs
> to be a friend. Also, I'd prefer just explicitly passing the LibMsgId
> everywhere. There shouldn't be many call sites to fix.

I have made this private again.  I have added a msg_id parameter to
KillAllIsolates/KillIfExists and gotten rid of RestartVMIsolateVisitor.

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/message_hand...
File runtime/vm/message_handler.cc (right):

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/message_hand...
runtime/vm/message_handler.cc:259: bool
ShouldPause(MessageHandler::MessageStatus status) {
On 2015/10/05 18:10:46, Cutch wrote:
> static 

Done.

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/message_hand...
runtime/vm/message_handler.cc:357: ((status == kOK) ? "no live ports" :
"error"),
On 2015/10/05 16:56:47, zra wrote:
> It might also be useful to know which error it was.

Done.  But I was sad to have to import object.h -- it was novel to have a file
which didn't need it.

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/message_hand...
File runtime/vm/message_handler_test.cc (right):

https://codereview.chromium.org/1371193005/diff/20002/runtime/vm/message_hand...
runtime/vm/message_handler_test.cc:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
On 2015/10/05 16:56:47, zra wrote:
> Would it be easy to have some unit tests of the new behavior here? E.g.
> processing all OOB messages, and stopping processing OOB messages when we get
> kShutdown.

Good idea.  I added a couple of tests to cover these cases.  They turned up a
bug in HandleMessages.

[zra, 2015-10-05 22:59:47]

lgtm

[Cutch, 2015-10-05 23:02:52]

LGTM with one suggestion

https://codereview.chromium.org/1371193005/diff/70001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1371193005/diff/70001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:645: // Unwind don't notify error listeners and ignore
whether errors
Could this block be replaced with:

return StoreError(I, result);

[turnidge, 2015-10-06 18:19:07]

https://codereview.chromium.org/1371193005/diff/70001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1371193005/diff/70001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:645: // Unwind don't notify error listeners and ignore
whether errors
On 2015/10/05 23:02:51, Cutch wrote:
> Could this block be replaced with:
> 
> return StoreError(I, result);

Done.

[turnidge, 2015-10-06 18:27:30]

Committed patchset #6 (id:90001) manually as
0d501ad53d1be5689934ceb8a38d9cde39399eb8 (presubmit successful).