- Implement Isolate.kill.

- Implement Isolate.kill.

R=asiva@google.com

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

[Ivan Posva, 2014-12-12 23:19:07]

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

[siva, 2014-12-13 00:24:06]

lgtm

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

https://codereview.chromium.org/800713002/diff/1/runtime/vm/code_generator.cc...
runtime/vm/code_generator.cc:1197: const String& msg =
String::Handle(String::New("unwind"));
why not "isolate terminated" instead of "unwind" string.

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

https://codereview.chromium.org/800713002/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:118: bool HandleLibMessage(const Array& message);
Not for this CL but it might be more readable to have this return
enum {
  kContinueMessageProcessing = 0,
  kTerminateIsolate = 1
};

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

https://codereview.chromium.org/800713002/diff/1/runtime/vm/isolate.h#newcode368
runtime/vm/isolate.h:368: bool VerifyTerminateCapability(const Object&
capability) const;
Should there also be a VerifyPingCapability ?

[Ivan Posva, 2014-12-13 00:36:48]

Thanks!

-Ivan

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

https://codereview.chromium.org/800713002/diff/1/runtime/vm/code_generator.cc...
runtime/vm/code_generator.cc:1197: const String& msg =
String::Handle(String::New("unwind"));
On 2014/12/13 00:24:05, siva wrote:
> why not "isolate terminated" instead of "unwind" string.

Done and added a comment.

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

https://codereview.chromium.org/800713002/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:118: bool HandleLibMessage(const Array& message);
On 2014/12/13 00:24:05, siva wrote:
> Not for this CL but it might be more readable to have this return
> enum {
>   kContinueMessageProcessing = 0,
>   kTerminateIsolate = 1
> };

Seems like a plan.

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

https://codereview.chromium.org/800713002/diff/1/runtime/vm/isolate.h#newcode368
runtime/vm/isolate.h:368: bool VerifyTerminateCapability(const Object&
capability) const;
On 2014/12/13 00:24:05, siva wrote:
> Should there also be a VerifyPingCapability ?

Having a handle to the isolate is equivalent to having ping capability. If you'd
like to change the API please talk to the libraries team.

[Ivan Posva, 2014-12-13 00:39:20]

Committed patchset #1 (id:1) manually as r42355 (presubmit successful).

[Lasse Reichstein Nielsen, 2014-12-13 02:04:45]

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

[Lasse Reichstein Nielsen, 2014-12-13 02:04:46]

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

https://codereview.chromium.org/800713002/diff/1/runtime/vm/code_generator.cc...
runtime/vm/code_generator.cc:1199: Exceptions::PropagateError(error);
If an UnwindError terminates the isolate without running any further user code,
then this looks fine. It shouldn't trigger catch or finally blocks.

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

https://codereview.chromium.org/800713002/diff/1/runtime/vm/isolate.h#newcode368
runtime/vm/isolate.h:368: bool VerifyTerminateCapability(const Object&
capability) const;
Ping does not require any capability, anyone with the command port can do it
(or, alternatively, the command port is effectively the only capability you
need). The same goes for inspecting uncaught errors or isolate exit.

[turnidge, 2014-12-13 02:14:49]

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

[turnidge, 2014-12-13 02:14:50]

DBC

We haven't actually used UnwindError for real before.  I'm sure there are some
places in the lib/bin code that forget to call Dart_PropagateError.  Just
saying.

We might want to make an automated test at some point that kills lots of dart
code at random points in time to try to shake some of this out.  Maybe also an
audit of lib.

[Ivan Posva, 2014-12-15 22:25:52]

On 2014/12/13 02:14:50, turnidge wrote:
> DBC
> 
> We haven't actually used UnwindError for real before.  I'm sure there are some
> places in the lib/bin code that forget to call Dart_PropagateError.  Just
> saying.
> 
> We might want to make an automated test at some point that kills lots of dart
> code at random points in time to try to shake some of this out.  Maybe also an
> audit of lib.

Library code that does not properly handle errors, such as not propagating
Dart_PropagateError further would fail in a similar way for compilation errors.

-Ivan

[Ivan Posva, 2014-12-15 22:25:59]

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

https://codereview.chromium.org/800713002/diff/1/runtime/vm/code_generator.cc...
runtime/vm/code_generator.cc:1199: Exceptions::PropagateError(error);
On 2014/12/13 02:04:46, Lasse Reichstein Nielsen wrote:
> If an UnwindError terminates the isolate without running any further user
code,
> then this looks fine. It shouldn't trigger catch or finally blocks.

PropagateError does propagate the error to the next C frame on the stack so that
native  C/C++ code can properly release resources before propagating the error
further. This is the same mechanism that is used to propagate compilation errors
(AKA LanguageError).