Isolate.spawn{Uri} only reports errors asynchronously.

Isolate.spawn{Uri} only reports errors asynchronously.

It is generally considered bad style to throw synchronous and asynchronous errors. This CL catches all synchronous errors and reports them in the asynchronous future that is returned.

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

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

[floitsch, 2013-11-26 17:18:36]

Unfortunately the VM test grew, because the asynchronous error needs more
built-in functions (and needs the event-loop to run).
It also runs into the same problem as MakeHttpRequest (from the
scheduleImmediate CL): it needs a fake event to trigger the event loop.

Usually we try to provide one way (either synchronous or asynchronous) of
handling exceptions/errors, but if there are good arguments we are willing to
make errors synchronous.

In this particular case it wouldn't shock if the method throw a synchronous
error when the embedder didn't set up the isolate function.
I could change the isolate_patch file if I could easily detect that specific
error in the catch-clause (and you want me to).

[floitsch, 2013-11-26 17:56:00]

Moving Lasse from CC to Reviewer after the changes to dart2js libraries.

[Lasse Reichstein Nielsen, 2013-11-27 11:41:28]

lgtm

https://codereview.chromium.org/88783002/diff/20001/runtime/lib/isolate_patch...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/88783002/diff/20001/runtime/lib/isolate_patch...
runtime/lib/isolate_patch.dart:117: // TODO(floitsch): this relies on the fact
that any exception aborts the
Bugnumber for catching uncaught exceptions?

https://codereview.chromium.org/88783002/diff/20001/runtime/lib/isolate_patch...
runtime/lib/isolate_patch.dart:251: completer.completeError(e, st);
Could we add this to the head of the queue instead of at the end?

https://codereview.chromium.org/88783002/diff/20001/runtime/lib/schedule_imme...
File runtime/lib/schedule_immediate_patch.dart (right):

https://codereview.chromium.org/88783002/diff/20001/runtime/lib/schedule_imme...
runtime/lib/schedule_immediate_patch.dart:7: throw new
UnsupportedError("microtasks are not supported");
Capital M in Microtasks?

https://codereview.chromium.org/88783002/diff/20001/runtime/lib/schedule_micr...
File runtime/lib/schedule_microtask_patch.dart (right):

https://codereview.chromium.org/88783002/diff/20001/runtime/lib/schedule_micr...
runtime/lib/schedule_microtask_patch.dart:5: @deprecated
Why deprecate this and add another?
Can we just keep this one?

https://codereview.chromium.org/88783002/diff/20001/sdk/lib/_internal/lib/iso...
File sdk/lib/_internal/lib/isolate_patch.dart (right):

https://codereview.chromium.org/88783002/diff/20001/sdk/lib/_internal/lib/iso...
sdk/lib/_internal/lib/isolate_patch.dart:18: return new
Future<Isolate>.value(new Isolate._fromControlPort(controlPort));
Long line. Ditto on line 37.

[floitsch, 2013-11-27 13:57:06]

https://codereview.chromium.org/88783002/diff/20001/runtime/lib/isolate_patch...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/88783002/diff/20001/runtime/lib/isolate_patch...
runtime/lib/isolate_patch.dart:117: // TODO(floitsch): this relies on the fact
that any exception aborts the
On 2013/11/27 11:41:28, Lasse Reichstein Nielsen wrote:
> Bugnumber for catching uncaught exceptions?

I have added 8875.

https://codereview.chromium.org/88783002/diff/20001/runtime/lib/isolate_patch...
runtime/lib/isolate_patch.dart:251: completer.completeError(e, st);
On 2013/11/27 11:41:28, Lasse Reichstein Nielsen wrote:
> Could we add this to the head of the queue instead of at the end?

The current implementation is consistent with a non-sync completer.

https://codereview.chromium.org/88783002/diff/20001/runtime/lib/schedule_imme...
File runtime/lib/schedule_immediate_patch.dart (right):

https://codereview.chromium.org/88783002/diff/20001/runtime/lib/schedule_imme...
runtime/lib/schedule_immediate_patch.dart:7: throw new
UnsupportedError("microtasks are not supported");
On 2013/11/27 11:41:28, Lasse Reichstein Nielsen wrote:
> Capital M in Microtasks?

Done.

https://codereview.chromium.org/88783002/diff/20001/runtime/lib/schedule_micr...
File runtime/lib/schedule_microtask_patch.dart (right):

https://codereview.chromium.org/88783002/diff/20001/runtime/lib/schedule_micr...
runtime/lib/schedule_microtask_patch.dart:5: @deprecated
On 2013/11/27 11:41:28, Lasse Reichstein Nielsen wrote:
> Why deprecate this and add another?
> Can we just keep this one?

No.
This one is not in _collection-dev.
The only reason it is still here is, to have a smooth transition for the Dartium
bindings that set this closure instead of setting the one in _collection-dev.
I have already a patch for it, and we can then remove this one.
https://chromiumcodereview.appspot.com/85333004/

https://codereview.chromium.org/88783002/diff/20001/sdk/lib/_internal/lib/iso...
File sdk/lib/_internal/lib/isolate_patch.dart (right):

https://codereview.chromium.org/88783002/diff/20001/sdk/lib/_internal/lib/iso...
sdk/lib/_internal/lib/isolate_patch.dart:18: return new
Future<Isolate>.value(new Isolate._fromControlPort(controlPort));
On 2013/11/27 11:41:28, Lasse Reichstein Nielsen wrote:
> Long line. Ditto on line 37.

Done.

[floitsch, 2013-12-06 18:16:32]

ping.

[floitsch, 2013-12-10 10:38:48]

On 2013/12/06 18:16:32, floitsch wrote:
> ping.

ping

[Ivan Posva, 2013-12-12 12:11:49]

LGTM once the code necessary for tests has been moved to the test script.

Adding the comment explaining the use of the closure in print_patch.dart is a
nice to have.

Thanks,
-Ivan

https://codereview.chromium.org/88783002/diff/60001/runtime/lib/isolate_patch...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/88783002/diff/60001/runtime/lib/isolate_patch...
runtime/lib/isolate_patch.dart:245: // yet have the time to install an error
handler.
time -> opportunity

https://codereview.chromium.org/88783002/diff/60001/runtime/lib/isolate_patch...
runtime/lib/isolate_patch.dart:268: // yet have the time to install an error
handler.
ditto

https://codereview.chromium.org/88783002/diff/60001/runtime/lib/print_patch.dart
File runtime/lib/print_patch.dart (right):

https://codereview.chromium.org/88783002/diff/60001/runtime/lib/print_patch.d...
runtime/lib/print_patch.dart:8: _printClosure(line);
Types mismatch between parameter and print closure.

https://codereview.chromium.org/88783002/diff/60001/runtime/lib/print_patch.d...
runtime/lib/print_patch.dart:11: void _unsupportedPrint(String line) {
This does not match the _PrintClosure type.

This whole file could use a comment explaining exactly what the print closure is
supposed to do. Does it get a String passed to it and the toString conversion is
done in the caller or is the closure responsible for that part. Asked
differently is the closure only supposed to get the characters to the user or is
it implementing the functionality of print.

https://codereview.chromium.org/88783002/diff/60001/runtime/lib/print_patch.d...
runtime/lib/print_patch.dart:16: _PrintClosure _nullPrintClosure = (String line)
{
ditto (types don't match)

https://codereview.chromium.org/88783002/diff/60001/runtime/vm/isolate_test.cc
File runtime/vm/isolate_test.cc (right):

https://codereview.chromium.org/88783002/diff/60001/runtime/vm/isolate_test.c...
runtime/vm/isolate_test.cc:44: NewString("_nullPrintClosure"));
I think it would be better to not pollute the internal library with the
_nullPrintClosure and instead put the _nullPrintClosure into the test script
being loaded here.

[floitsch, 2013-12-12 21:08:37]

Thanks Ivan.
@Lasse: could you PTAL for the status-file changes.

https://codereview.chromium.org/88783002/diff/60001/runtime/lib/isolate_patch...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/88783002/diff/60001/runtime/lib/isolate_patch...
runtime/lib/isolate_patch.dart:245: // yet have the time to install an error
handler.
On 2013/12/12 12:11:49, Ivan Posva wrote:
> time -> opportunity

Done.

https://codereview.chromium.org/88783002/diff/60001/runtime/lib/isolate_patch...
runtime/lib/isolate_patch.dart:268: // yet have the time to install an error
handler.
On 2013/12/12 12:11:49, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/88783002/diff/60001/runtime/lib/print_patch.dart
File runtime/lib/print_patch.dart (right):

https://codereview.chromium.org/88783002/diff/60001/runtime/lib/print_patch.d...
runtime/lib/print_patch.dart:8: _printClosure(line);
On 2013/12/12 12:11:49, Ivan Posva wrote:
> Types mismatch between parameter and print closure.

Updated the typedef.

https://codereview.chromium.org/88783002/diff/60001/runtime/lib/print_patch.d...
runtime/lib/print_patch.dart:11: void _unsupportedPrint(String line) {
On 2013/12/12 12:11:49, Ivan Posva wrote:
> This does not match the _PrintClosure type.
> 
> This whole file could use a comment explaining exactly what the print closure
is
> supposed to do. Does it get a String passed to it and the toString conversion
is
> done in the caller or is the closure responsible for that part. Asked
> differently is the closure only supposed to get the characters to the user or
is
> it implementing the functionality of print.

Added comment.
It is supposed to get a string.

https://codereview.chromium.org/88783002/diff/60001/runtime/lib/print_patch.d...
runtime/lib/print_patch.dart:16: _PrintClosure _nullPrintClosure = (String line)
{
On 2013/12/12 12:11:49, Ivan Posva wrote:
> ditto (types don't match)

moved to test file.

https://codereview.chromium.org/88783002/diff/60001/runtime/vm/isolate_test.cc
File runtime/vm/isolate_test.cc (right):

https://codereview.chromium.org/88783002/diff/60001/runtime/vm/isolate_test.c...
runtime/vm/isolate_test.cc:44: NewString("_nullPrintClosure"));
On 2013/12/12 12:11:49, Ivan Posva wrote:
> I think it would be better to not pollute the internal library with the
> _nullPrintClosure and instead put the _nullPrintClosure into the test script
> being loaded here.

Done.

[Lasse Reichstein Nielsen, 2013-12-13 11:16:58]

Still LGTM.

https://codereview.chromium.org/88783002/diff/100001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/88783002/diff/100001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:248: });
How about doing the same as in the JS patch:
 try { 
    ...
    Completer completer = new ...
    readyPort.handler = ...
    return completer.future;
 } catch (e, st) {
    return new Future<Isolate>.error(e, st);
 }

Add space after "catch" here and below.

https://codereview.chromium.org/88783002/diff/100001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/dart.dart (right):

https://codereview.chromium.org/88783002/diff/100001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/dart.dart:130:
Isolate.spawnUri(Uri.parse(message['uri']), [], message['message'])
I still wouldn't guarantee that there are no synchronous errors.
The current code wraps everything in a try/catch, but we could choose to only
wrap the parts that are related to the remote isolate.

If you pass arguments of the wrong type (in checked mode or in unchecked mode),
any statement using the value can fail with a noSuchMethod error. I don't want
to promise that we catch all such errors. This code is an internal hack that
should be invisible to the user, so it should handle all errors, even checked
mode type errors, just as if the remote end had done it.

https://codereview.chromium.org/88783002/diff/100001/tests/co19/co19-runtime....
File tests/co19/co19-runtime.status (right):

https://codereview.chromium.org/88783002/diff/100001/tests/co19/co19-runtime....
tests/co19/co19-runtime.status:36: LibTest/isolate/Isolate/spawnUri_A02_t01:
RuntimeError # Dart issue 15617
This one always fails to throw on the null uri, even in checked mode, right?

https://codereview.chromium.org/88783002/diff/100001/tests/isolate/static_fun...
File tests/isolate/static_function_test.dart (right):

https://codereview.chromium.org/88783002/diff/100001/tests/isolate/static_fun...
tests/isolate/static_function_test.dart:102: throwsTest("static closure",
staticClosure);
Rename to failTest, perhaps even functionFailTest.

[floitsch, 2013-12-13 12:13:37]

https://codereview.chromium.org/88783002/diff/100001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/88783002/diff/100001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:248: });
On 2013/12/13 11:16:58, Lasse Reichstein Nielsen wrote:
> How about doing the same as in the JS patch:
>  try { 
>     ...
>     Completer completer = new ...
>     readyPort.handler = ...
>     return completer.future;
>  } catch (e, st) {
>     return new Future<Isolate>.error(e, st);
>  }
> 
> Add space after "catch" here and below.

Done.

https://codereview.chromium.org/88783002/diff/100001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/dart.dart (right):

https://codereview.chromium.org/88783002/diff/100001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/dart.dart:130:
Isolate.spawnUri(Uri.parse(message['uri']), [], message['message'])
On 2013/12/13 11:16:58, Lasse Reichstein Nielsen wrote:
> I still wouldn't guarantee that there are no synchronous errors.
> The current code wraps everything in a try/catch, but we could choose to only
> wrap the parts that are related to the remote isolate.
> 
> If you pass arguments of the wrong type (in checked mode or in unchecked
mode),
> any statement using the value can fail with a noSuchMethod error. I don't want
> to promise that we catch all such errors. This code is an internal hack that
> should be invisible to the user, so it should handle all errors, even checked
> mode type errors, just as if the remote end had done it.

I think that the pub-team assumes that it doesn't do any Programming Error, so
that all errors should be exceptions that are returned in the future.

https://codereview.chromium.org/88783002/diff/100001/tests/co19/co19-runtime....
File tests/co19/co19-runtime.status (right):

https://codereview.chromium.org/88783002/diff/100001/tests/co19/co19-runtime....
tests/co19/co19-runtime.status:36: LibTest/isolate/Isolate/spawnUri_A02_t01:
RuntimeError # Dart issue 15617
On 2013/12/13 11:16:58, Lasse Reichstein Nielsen wrote:
> This one always fails to throw on the null uri, even in checked mode, right?

yes.

https://codereview.chromium.org/88783002/diff/100001/tests/isolate/static_fun...
File tests/isolate/static_function_test.dart (right):

https://codereview.chromium.org/88783002/diff/100001/tests/isolate/static_fun...
tests/isolate/static_function_test.dart:102: throwsTest("static closure",
staticClosure);
On 2013/12/13 11:16:58, Lasse Reichstein Nielsen wrote:
> Rename to failTest, perhaps even functionFailTest.

Done.

[Lasse Reichstein Nielsen, 2013-12-13 12:16:30]

https://codereview.chromium.org/88783002/diff/100001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/dart.dart (right):

https://codereview.chromium.org/88783002/diff/100001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/dart.dart:130:
Isolate.spawnUri(Uri.parse(message['uri']), [], message['message'])
That is reasonable.

[Lasse Reichstein Nielsen, 2013-12-13 12:19:24]

lgtm

[floitsch, 2013-12-13 13:05:38]

Committed patchset #7 manually as r31131 (presubmit successful).