Enables clean VM shutdown.

Enables clean VM shutdown.

. Disables isolate spawning during Dart_Cleanup.
. Adds a static call Isolate::KillAllIsolates, which sends the
  OOB Kill message to all isolates when called from
  Dart_Cleanup.
. Modifies thread pool shutdown to block until all threads have
  exited.
. Fixes tests.


BUG=
R=turnidge@google.com

Committed: https://github.com/dart-lang/sdk/commit/174d552574f871d9f717e7014f0a685a1017dc52

[zra, 2015-06-20 04:57:35]

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

[zra, 2015-06-20 04:57:36]

I'll work on any changes that are needed the week of the 29th when I get back
from leave.

[zra, 2015-06-20 04:58:20]

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

[zra, 2015-06-20 04:58:20]

+turnidge for thread pool changes.

[turnidge, 2015-06-30 22:15:47]

https://codereview.chromium.org/1177153005/diff/120001/runtime/bin/dbg_messag...
File runtime/bin/dbg_message.cc (right):

https://codereview.chromium.org/1177153005/diff/120001/runtime/bin/dbg_messag...
runtime/bin/dbg_message.cc:1406: DebuggerConnectionHandler::WaitForConnection();
What led to the changes in this file?

https://codereview.chromium.org/1177153005/diff/120001/runtime/bin/dbg_messag...
runtime/bin/dbg_message.cc:1426: // If there is no recieve message queue, do not
wait for a connection, and
typo: receive

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

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1910: IsolateMessageHandler::kBeforeNextEventAction)));
We discusssed offline that it should be possible to kill an isolate without
forging this specific kill message.  Instead you would add a new kind of
interrupt bit.  But that approach has the drawback that the isolate, if idle,
will not notice that the interrupt bit has been set.  I am ok with this approach
for now but I am noting that it is (almost) possible to do this w/out all of
these messages/capabilities when killing from the VM itself.

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1943: void Isolate::KillAllIsolates(Isolate* isolate) {
Will the "isolate" param always be the current isolate?

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/message_han...
File runtime/vm/message_handler.cc (right):

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/message_han...
runtime/vm/message_handler.cc:88: running_ = true;
I think that "running_" is a bit confusing as a name.  I would be happier with a
name like "running_on_thread_pool_".   This would distinguish from those message
handlers that run, for example, in dartium's event loop.

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/message_han...
runtime/vm/message_handler.cc:205: // assigned to a thread pool.
Update comment.

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/thread_pool.cc
File runtime/vm/thread_pool.cc (right):

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:367: // worker is done_.
Note to self: figure out when this case happens and how it interacts with
shutdown.

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/thread_pool.h
File runtime/vm/thread_pool.h (right):

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/thread_pool...
runtime/vm/thread_pool.h:13: class ThreadPool : public AllStatic {
Is there a motivating reason to make this AllStatic?  I prefer to keep it
non-static.  This allows us to have mock thread pools, alternate/experimental
thread pools, or multiple/dedicated thread pools in the future.

I see statics as kind of a global variable that we use when necessary but I
don't see any reason to here.  I see that we save some parameters on some calls,
but that's true of any global.

[zra, 2015-07-20 22:23:39]

https://codereview.chromium.org/1177153005/diff/120001/runtime/bin/dbg_messag...
File runtime/bin/dbg_message.cc (right):

https://codereview.chromium.org/1177153005/diff/120001/runtime/bin/dbg_messag...
runtime/bin/dbg_message.cc:1406: DebuggerConnectionHandler::WaitForConnection();
On 2015/06/30 22:15:46, turnidge wrote:
> What led to the changes in this file?

The VM Isolate has no receive queue. Isolate::Shutdown tries to send it a
message anyway. Instead of putting a special case there, we just ignore the
message here if there is no receive queue.

A previous CL attempting to implement that policy neglected to also avoid
calling DebuggerConnectionHandler::WaitForConnection(). This change ensures that
if there is no receive queue, we do absolutely nothing.

https://codereview.chromium.org/1177153005/diff/120001/runtime/bin/dbg_messag...
runtime/bin/dbg_message.cc:1426: // If there is no recieve message queue, do not
wait for a connection, and
On 2015/06/30 22:15:46, turnidge wrote:
> typo: receive

Done.

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

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1910: IsolateMessageHandler::kBeforeNextEventAction)));
On 2015/06/30 22:15:46, turnidge wrote:
> We discusssed offline that it should be possible to kill an isolate without
> forging this specific kill message.  Instead you would add a new kind of
> interrupt bit.  But that approach has the drawback that the isolate, if idle,
> will not notice that the interrupt bit has been set.  I am ok with this
approach
> for now but I am noting that it is (almost) possible to do this w/out all of
> these messages/capabilities when killing from the VM itself.

Acknowledged.

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1943: void Isolate::KillAllIsolates(Isolate* isolate) {
On 2015/06/30 22:15:46, turnidge wrote:
> Will the "isolate" param always be the current isolate?

No. It should always be the service isolate. I've added an assert.

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/message_han...
File runtime/vm/message_handler.cc (right):

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/message_han...
runtime/vm/message_handler.cc:88: running_ = true;
On 2015/06/30 22:15:46, turnidge wrote:
> I think that "running_" is a bit confusing as a name.  I would be happier with
a
> name like "running_on_thread_pool_".   This would distinguish from those
message
> handlers that run, for example, in dartium's event loop.

Done.

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/message_han...
runtime/vm/message_handler.cc:205: // assigned to a thread pool.
On 2015/06/30 22:15:46, turnidge wrote:
> Update comment.

Done.

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/thread_pool.cc
File runtime/vm/thread_pool.cc (right):

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:367: // worker is done_.
On 2015/06/30 22:15:46, turnidge wrote:
> Note to self: figure out when this case happens and how it interacts with
> shutdown.

Acknowledged.

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/thread_pool.h
File runtime/vm/thread_pool.h (right):

https://codereview.chromium.org/1177153005/diff/120001/runtime/vm/thread_pool...
runtime/vm/thread_pool.h:13: class ThreadPool : public AllStatic {
On 2015/06/30 22:15:47, turnidge wrote:
> Is there a motivating reason to make this AllStatic?  I prefer to keep it
> non-static.  This allows us to have mock thread pools, alternate/experimental
> thread pools, or multiple/dedicated thread pools in the future.
> 
> I see statics as kind of a global variable that we use when necessary but I
> don't see any reason to here.  I see that we save some parameters on some
calls,
> but that's true of any global.

I changed to AllStatic to enforce having only one thread pool because I was
worried that having multiple thread pools without a clear design of what to do
with them could be dangerous and/or confusing. Examples: Which thread pool do
you assign work to? What do you do when one thread pool has lots of idle
threads, but work keeps being assigned to some other thread pool? I think if we
don't have answers to questions like that, we should make sure we only have one.

It could make sense for testing, but isn't really necessary.

[zra, 2015-07-22 14:38:40]

ping

[turnidge, 2015-07-28 18:01:46]

On 2015/07/22 14:38:40, zra wrote:
> ping

Hi Zach,

I will get to this review again today.

Todd

[zra, 2015-08-01 03:13:09]

I have made the changes we discussed on Thursday. PTAL.

[zra, 2015-08-04 14:49:41]

ping

[turnidge, 2015-08-04 21:39:02]

lgtm w/ comments

https://codereview.chromium.org/1177153005/diff/360001/runtime/bin/vmservice/...
File runtime/bin/vmservice/vmservice_io.dart (right):

https://codereview.chromium.org/1177153005/diff/360001/runtime/bin/vmservice/...
runtime/bin/vmservice/vmservice_io.dart:94: // Enable this code, and remove the
call to Isoate::KillIsolate() from
protocal -> protocol, Isoate -> Isolate.

https://codereview.chromium.org/1177153005/diff/360001/runtime/include/dart_n...
File runtime/include/dart_native_api.h (right):

https://codereview.chromium.org/1177153005/diff/360001/runtime/include/dart_n...
runtime/include/dart_native_api.h:103: DART_EXPORT bool
Dart_PostOOBCObject(Dart_Port port_id, Dart_CObject* message);
Remove.

https://codereview.chromium.org/1177153005/diff/360001/runtime/tests/vm/dart/...
File runtime/tests/vm/dart/spawn_infinite_loop_test.dart (right):

https://codereview.chromium.org/1177153005/diff/360001/runtime/tests/vm/dart/...
runtime/tests/vm/dart/spawn_infinite_loop_test.dart:11: 
Comment to explain why we have this test.

https://codereview.chromium.org/1177153005/diff/360001/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (left):

https://codereview.chromium.org/1177153005/diff/360001/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1282: OS::PrintErr("Dart_Initialize: %s\n",
err_msg);
So the embedders that haven't been updated to deal with the new return value
will not print the error for a while, until they are updated.  Can you update
the Mojo embedder in a follow-up CL to handle this?

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

https://codereview.chromium.org/1177153005/diff/360001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:746: creation_enabled_ = true;
Consider calling EnableIsolateCreation here?

https://codereview.chromium.org/1177153005/diff/360001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1568: bool Isolate::creation_enabled_ = true;
Maybe we should start this as false.

https://codereview.chromium.org/1177153005/diff/360001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1949: // started going down, then it may not be in the
list.
I feel like there is a simpler version of this comment out there... maybe...

// If we are shutting down the VM, the isolate may not be in the list.

https://codereview.chromium.org/1177153005/diff/360001/runtime/vm/thread_pool.cc
File runtime/vm/thread_pool.cc (right):

https://codereview.chromium.org/1177153005/diff/360001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:231: if (shutting_down_workers_ == NULL) {
I believe this can be an ASSERT.

https://codereview.chromium.org/1177153005/diff/360001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:375: // id_ hasn't been set, yet, but the ThreadPool
is being shutdown.
Remove comma between "set" and "yet"?

[zra, 2015-08-05 06:23:07]

Thanks!

https://codereview.chromium.org/1177153005/diff/360001/runtime/bin/vmservice/...
File runtime/bin/vmservice/vmservice_io.dart (right):

https://codereview.chromium.org/1177153005/diff/360001/runtime/bin/vmservice/...
runtime/bin/vmservice/vmservice_io.dart:94: // Enable this code, and remove the
call to Isoate::KillIsolate() from
On 2015/08/04 21:39:02, turnidge wrote:
> protocal -> protocol, Isoate -> Isolate.

Done and added link to issue.

https://codereview.chromium.org/1177153005/diff/360001/runtime/include/dart_n...
File runtime/include/dart_native_api.h (right):

https://codereview.chromium.org/1177153005/diff/360001/runtime/include/dart_n...
runtime/include/dart_native_api.h:103: DART_EXPORT bool
Dart_PostOOBCObject(Dart_Port port_id, Dart_CObject* message);
On 2015/08/04 21:39:02, turnidge wrote:
> Remove.

Done.

https://codereview.chromium.org/1177153005/diff/360001/runtime/tests/vm/dart/...
File runtime/tests/vm/dart/spawn_infinite_loop_test.dart (right):

https://codereview.chromium.org/1177153005/diff/360001/runtime/tests/vm/dart/...
runtime/tests/vm/dart/spawn_infinite_loop_test.dart:11: 
On 2015/08/04 21:39:02, turnidge wrote:
> Comment to explain why we have this test.

Done.

https://codereview.chromium.org/1177153005/diff/360001/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (left):

https://codereview.chromium.org/1177153005/diff/360001/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1282: OS::PrintErr("Dart_Initialize: %s\n",
err_msg);
On 2015/08/04 21:39:02, turnidge wrote:
> So the embedders that haven't been updated to deal with the new return value
> will not print the error for a while, until they are updated.  Can you update
> the Mojo embedder in a follow-up CL to handle this?

Acknowledged.

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

https://codereview.chromium.org/1177153005/diff/360001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:746: creation_enabled_ = true;
On 2015/08/04 21:39:02, turnidge wrote:
> Consider calling EnableIsolateCreation here?

Done.

https://codereview.chromium.org/1177153005/diff/360001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1568: bool Isolate::creation_enabled_ = true;
On 2015/08/04 21:39:02, turnidge wrote:
> Maybe we should start this as false.

Done.

https://codereview.chromium.org/1177153005/diff/360001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1949: // started going down, then it may not be in the
list.
On 2015/08/04 21:39:02, turnidge wrote:
> I feel like there is a simpler version of this comment out there... maybe...
> 
> // If we are shutting down the VM, the isolate may not be in the list.

Done.

https://codereview.chromium.org/1177153005/diff/360001/runtime/vm/thread_pool.cc
File runtime/vm/thread_pool.cc (right):

https://codereview.chromium.org/1177153005/diff/360001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:231: if (shutting_down_workers_ == NULL) {
On 2015/08/04 21:39:02, turnidge wrote:
> I believe this can be an ASSERT.

Done.

https://codereview.chromium.org/1177153005/diff/360001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:375: // id_ hasn't been set, yet, but the ThreadPool
is being shutdown.
On 2015/08/04 21:39:02, turnidge wrote:
> Remove comma between "set" and "yet"?

Done.

[zra, 2015-08-05 06:29:30]

Committed patchset #20 (id:380001) manually as
174d552574f871d9f717e7014f0a685a1017dc52 (presubmit successful).