Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

isolate/illegal_msg_test fails on dart2js #6750

Closed
a-siva opened this issue Nov 15, 2012 · 18 comments
Closed

isolate/illegal_msg_test fails on dart2js #6750

a-siva opened this issue Nov 15, 2012 · 18 comments
Assignees
@lrhn
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-obsolete Closed as the reported issue is no longer relevant library-isolate

Comments

@a-siva
Copy link
Contributor

a-siva commented Nov 15, 2012

FAILED: dart2js-d8 release_ia32 isolate/illegal_msg_test
Expected: pass
Actual: fail

stdout:
test.dart: Compilation finished (step 1 of 2)

/mnt/data/b/build/slave/dart2js-linux-release-4-5/build/dart/out/ReleaseIA32/generated_tests/dart2js-d8/tests_isolate_illegal_msg_test/out.js:2216: Expect.isTrue(false) fails.
  throw jsError;
        ^
Expect.isTrue(false) fails.
    at $.$$throw (/mnt/data/b/build/slave/dart2js-linux-release-4-5/build/dart/out/ReleaseIA32/generated_tests/dart2js-d8/tests_isolate_illegal_msg_test/out.js:2211:13)
    at $.Expect__fail (/mnt/data/b/build/slave/dart2js-linux-release-4-5/build/dart/out/ReleaseIA32/generated_tests/dart2js-d8/tests_isolate_illegal_msg_test/out.js:2338:11)
    at $.Expect_isTrue (/mnt/data/b/build/slave/dart2js-linux-release-4-5/build/dart/out/ReleaseIA32/generated_tests/dart2js-d8/tests_isolate_illegal_msg_test/out.js:2324:5)
    at Function.$.main (/mnt/data/b/build/slave/dart2js-linux-release-4-5/build/dart/out/ReleaseIA32/generated_tests/dart2js-d8/tests_isolate_illegal_msg_test/out.js:3121:5)
    at _IsolateContext.$$._IsolateContext.eval$1 (/mnt/data/b/build/slave/dart2js-linux-release-4-5/build/dart/out/ReleaseIA32/generated_tests/dart2js-d8/tests_isolate_illegal_msg_test/out.js:1005:19)
    at $.startRootIsolate (/mnt/data/b/build/slave/dart2js-linux-release-4-5/build/dart/out/ReleaseIA32/generated_tests/dart2js-d8/tests_isolate_illegal_msg_test/out.js:3171:15)
    at /mnt/data/b/build/slave/dart2js-linux-release-4-5/build/dart/out/ReleaseIA32/generated_tests/dart2js-d8/tests_isolate_illegal_msg_test/out.js:3818:7
@peter-ahe-google
Copy link
Contributor

I'm not sure we have to fix this in M2, but it would be good to know what this is about. Nicolas, would you mind investigating?

Set owner to ngeoffray@google.com.
Added this to the M2 milestone.

@DartBot
Copy link

DartBot commented Nov 16, 2012

This comment was originally written by ngeoffray@google.com

The test is apparently checking that we get a synchronous exception when sending an invalid message to an SendPort. However I cannot see anywhere in the documentation that this is the intended behavior. What dart2js is asynchronously checking if the message is valid.

Siva, should the test be rewritten in an asynchronous way, or is there a specification for the send to throw synchronously?

cc @a-siva.
Added NeedsInfo label.

@a-siva
Copy link
Contributor Author

a-siva commented Nov 20, 2012

The current isolate API documents send as follows:

  void send(var message, [SendPort replyTo]);
  (the message is sent asynchronously and only certain type of objects can be sent in
   the message).

The API does not return a future to be able to re-write the test in
an asynchronous way. Considering that the function returns a void I would presume that we need to get synchronous exceptions when invalid objects are passed in.

I think when the API was designed we thought no errors would be reported as Kasper was always of the opinion that we asynchronously send a message and hope it reaches the other end. If it doesn't the program deals with it as part of it's logic.

But now we have this requirement that we need to throw ArgumentError when we have
bad arguments to the function. Either we change the spec of send to return a future or we have it throw synchronous ArgumentError.

@DartBot
Copy link

DartBot commented Nov 20, 2012

This comment was originally written by ngeoffray@google.com

Thanks for the explanation Siva. Adding Florian and Kasper to the discussion.

cc @kasperl.
cc @floitschG.

@floitschG
Copy link
Contributor

We discussed this here in the library team. We have a good plan on how to tackle this when we switch to isolate streams. We will probably require to serialize synchronously as otherwise error-handling just becomes too hard.

cc @lrhn.
cc @skabet.
Set owner to @floitschG.
Added Accepted label.

@iposva-google
Copy link
Contributor

In reply to comment 5: It is not the error handling that is too hard, but really the semantics need to be valid. Once you sent an object, you are free to modify it locally. If the state is not captured synchronously at the time of the send, you will end with lots of pain.

@floitschG
Copy link
Contributor

good point.

@kasperl
Copy link

kasperl commented Dec 12, 2012

Would this be better categorized as a library issue? Feel free to throw it back in the dart2js bucket if you feel that is better, Florian.

Removed Area-Dart2JS label.
Added Area-Library, Library-Isolates labels.

@anders-sandholm
Copy link
Contributor

Removed this from the M2 milestone.
Added this to the M3 milestone.

@dgrove
Copy link
Contributor

dgrove commented Jan 11, 2013

Added Library-Isolate label.

@dgrove
Copy link
Contributor

dgrove commented Jan 11, 2013

Removed Library-Isolates label.

@anders-sandholm
Copy link
Contributor

Removed this from the M3 milestone.
Added this to the M4 milestone.

@larsbak
Copy link

larsbak commented May 28, 2013

Removed this from the M4 milestone.
Added this to the M5 milestone.

@lrhn
Copy link
Member

lrhn commented Oct 24, 2013

The rewrite to streams is progressing. It may solve the problem.

Removed this from the M5 milestone.
Added this to the M8 milestone.

@floitschG
Copy link
Contributor

Set owner to @lrhn.

@kasperl
Copy link

kasperl commented Jun 4, 2014

Removed this from the M8 milestone.
Added this to the 1.6 milestone.

@kasperl
Copy link

kasperl commented Jul 10, 2014

Removed this from the 1.6 milestone.
Added Oldschool-Milestone-1.6 label.

@kasperl
Copy link

kasperl commented Aug 4, 2014

Removed Oldschool-Milestone-1.6 label.

@a-siva a-siva added Type-Defect area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-isolate closed-obsolete Closed as the reported issue is no longer relevant labels Aug 4, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lrhn lrhn

Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-obsolete Closed as the reported issue is no longer relevant library-isolate
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
10 participants
@dgrove @kasperl @anders-sandholm @lrhn @iposva-google @peter-ahe-google @larsbak @floitschG @a-siva @DartBot