Complete with error if Worker fails.

Complete with error if Worker fails.

R=floitsch@google.com, lrn@google.com

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

[ahe, 2014-05-22 13:32:03]

The VM passes the test already.

[floitsch, 2014-05-22 14:17:38]

LGTM once the spawning from workers is fixed.

https://codereview.chromium.org/292743019/diff/1/dart/sdk/lib/_internal/lib/i...
File dart/sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/292743019/diff/1/dart/sdk/lib/_internal/lib/i...
dart/sdk/lib/_internal/lib/isolate_helper.dart:1012: if (completer == null)
throw 'no completer';
This doesn't make sense. You have a call to _spawnWorker in line 808 where you
explicitly pass in `null`.

Why do you need this explicit check? is this an assert?

https://codereview.chromium.org/292743019/diff/1/dart/sdk/lib/_internal/lib/i...
dart/sdk/lib/_internal/lib/isolate_helper.dart:1015: final onerrorTrampoline =
JS(
Why do you need the trampoline?
Can't you just write the closure in Dart?

https://codereview.chromium.org/292743019/diff/1/dart/sdk/lib/_internal/lib/i...
dart/sdk/lib/_internal/lib/isolate_helper.dart:1052: var message =
JS('String|Null', '#.message', event);
String message

https://codereview.chromium.org/292743019/diff/1/dart/sdk/lib/_internal/lib/i...
dart/sdk/lib/_internal/lib/isolate_helper.dart:1054: // Some browsers, including
Chrome, fails to provide a proper error
fail to

https://codereview.chromium.org/292743019/diff/1/dart/sdk/lib/_internal/lib/i...
dart/sdk/lib/_internal/lib/isolate_helper.dart:1063:
completer.completeError(message);
This assumes that we will only receive one message from the worker:
either a
- _SPAWNED_SIGNAL
- an error.

If there can be a spawned-signal and an error (in sequence), or multiple errors,
we will throw here, since a future may not be completed twice.

@Lasse: wdyt?

[Lasse Reichstein Nielsen, 2014-05-23 06:55:41]

https://codereview.chromium.org/292743019/diff/1/dart/sdk/lib/_internal/lib/i...
File dart/sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/292743019/diff/1/dart/sdk/lib/_internal/lib/i...
dart/sdk/lib/_internal/lib/isolate_helper.dart:1012: if (completer == null)
throw 'no completer';
Agree, make it an assert if anything.

https://codereview.chromium.org/292743019/diff/1/dart/sdk/lib/_internal/lib/i...
dart/sdk/lib/_internal/lib/isolate_helper.dart:1015: final onerrorTrampoline =
JS(
Some documentation here would be great. Reading JS like this is only slightly
easier than reading assembler.

[ahe, 2014-05-26 12:03:15]

Lasse, Florian, thank you for taking a look.

Please take another look :-)

https://codereview.chromium.org/292743019/diff/1/dart/sdk/lib/_internal/lib/i...
File dart/sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/292743019/diff/1/dart/sdk/lib/_internal/lib/i...
dart/sdk/lib/_internal/lib/isolate_helper.dart:1012: if (completer == null)
throw 'no completer';
On 2014/05/22 14:17:38, floitsch wrote:
> This doesn't make sense. You have a call to _spawnWorker in line 808 where you
> explicitly pass in `null`.
> 
> Why do you need this explicit check? is this an assert?

Debug code, removed.

https://codereview.chromium.org/292743019/diff/1/dart/sdk/lib/_internal/lib/i...
dart/sdk/lib/_internal/lib/isolate_helper.dart:1015: final onerrorTrampoline =
JS(
On 2014/05/22 14:17:38, floitsch wrote:
> Why do you need the trampoline?
> Can't you just write the closure in Dart?

I've explained it a comment.

https://codereview.chromium.org/292743019/diff/1/dart/sdk/lib/_internal/lib/i...
dart/sdk/lib/_internal/lib/isolate_helper.dart:1015: final onerrorTrampoline =
JS(
On 2014/05/23 06:55:42, Lasse Reichstein Nielsen wrote:
> Some documentation here would be great. Reading JS like this is only slightly
> easier than reading assembler.

Done.

https://codereview.chromium.org/292743019/diff/1/dart/sdk/lib/_internal/lib/i...
dart/sdk/lib/_internal/lib/isolate_helper.dart:1052: var message =
JS('String|Null', '#.message', event);
On 2014/05/22 14:17:38, floitsch wrote:
> String message

Done.

https://codereview.chromium.org/292743019/diff/1/dart/sdk/lib/_internal/lib/i...
dart/sdk/lib/_internal/lib/isolate_helper.dart:1054: // Some browsers, including
Chrome, fails to provide a proper error
On 2014/05/22 14:17:38, floitsch wrote:
> fail to

Done.

https://codereview.chromium.org/292743019/diff/1/dart/sdk/lib/_internal/lib/i...
dart/sdk/lib/_internal/lib/isolate_helper.dart:1063:
completer.completeError(message);
On 2014/05/22 14:17:38, floitsch wrote:
> This assumes that we will only receive one message from the worker:
> either a
> - _SPAWNED_SIGNAL
> - an error.
> 
> If there can be a spawned-signal and an error (in sequence), or multiple
errors,
> we will throw here, since a future may not be completed twice.
> 
> @Lasse: wdyt?

Fixed this by having Florian standing next to me and tell me what to type in :-)

[floitsch, 2014-05-26 12:34:35]

LGTM.

https://codereview.chromium.org/292743019/diff/40001/dart/sdk/lib/_internal/l...
File dart/sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/292743019/diff/40001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/isolate_helper.dart:928: signalReply, (String
message) => completer.completeError(message));
Just put completer.completeError here:
signalReply, completer.completeError);

https://codereview.chromium.org/292743019/diff/40001/dart/tests/isolate/spawn...
File dart/tests/isolate/spawn_uri_missing_from_isolate_test.dart (right):

https://codereview.chromium.org/292743019/diff/40001/dart/tests/isolate/spawn...
dart/tests/isolate/spawn_uri_missing_from_isolate_test.dart:19: import
'spawn_uri_missing_test.dart';
I would just copy the `doTest` here.

Unless there is lots of code that can be shared I prefer to make tests
self-contained.

[Lasse Reichstein Nielsen, 2014-05-26 12:51:03]

lgtm

lgtm

[Lasse Reichstein Nielsen, 2014-05-26 12:51:03]

lgtm

[ahe, 2014-05-27 09:33:02]

Committed patchset #3 manually as r36657 (presubmit successful).