Add a test for timers in child isolates.

Add a test for timers in child isolates.

And fix some subtle problems.

R=podivilov@chromium.org,vsm@google.com

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

[vsm, 2013-03-29 14:37:39]

https://codereview.chromium.org/13308002/diff/3001/tests/html/async_test.dart
File tests/html/async_test.dart (right):

https://codereview.chromium.org/13308002/diff/3001/tests/html/async_test.dart...
tests/html/async_test.dart:43: fail('Should never be invoked');
Will a 'fail' on a background isolate be caught be the test framework?  Might
need to send a message back.

https://codereview.chromium.org/13308002/diff/3001/tests/html/async_test.dart...
tests/html/async_test.dart:70:
expect(spawnFunction(oneshotTimerIsolate).call('START'),
oneshotTimerIsolate -> periodicTimerIsolate

https://codereview.chromium.org/13308002/diff/3001/tools/dom/src/Timer.dart
File tools/dom/src/Timer.dart (right):

https://codereview.chromium.org/13308002/diff/3001/tools/dom/src/Timer.dart#n...
tools/dom/src/Timer.dart:57: // Therefore, let's resort to then only if really
necessary.
I'm confused by this.  What was the problem you were hitting in the old code? 
Why wasn't the new time registering?

It doesn't seem like this change should fix anyway (beyond reducing a potential
delay).

[Anton Muhin, 2013-03-29 16:09:15]

https://codereview.chromium.org/13308002/diff/3001/tests/html/async_test.dart
File tests/html/async_test.dart (right):

https://codereview.chromium.org/13308002/diff/3001/tests/html/async_test.dart...
tests/html/async_test.dart:43: fail('Should never be invoked');
On 2013/03/29 14:37:39, vsm wrote:
> Will a 'fail' on a background isolate be caught be the test framework?  Might
> need to send a message back.

That worked for me when I ran a single test from local server.

There are overall some issues with tests which I was going to discuss w/ Emily

Hopefully, even if fail is swollen, test should either fail or timeout anyway,
correct?

https://codereview.chromium.org/13308002/diff/3001/tests/html/async_test.dart...
tests/html/async_test.dart:70:
expect(spawnFunction(oneshotTimerIsolate).call('START'),
On 2013/03/29 14:37:39, vsm wrote:
> oneshotTimerIsolate -> periodicTimerIsolate

Thanks!  Do'h.

https://codereview.chromium.org/13308002/diff/3001/tools/dom/src/Timer.dart
File tools/dom/src/Timer.dart (right):

https://codereview.chromium.org/13308002/diff/3001/tools/dom/src/Timer.dart#n...
tools/dom/src/Timer.dart:57: // Therefore, let's resort to then only if really
necessary.
(whatever outcome of our discuss be, I need to modify cancel code to behave
similarly).

There was an interesting corner case I found.

Imagine you spawn a timer which pings you every 10 ms and from the handler of
this timer you'd like to spawn another timer.  Alas, timer creation requires
.then which invoke Timer.run which invokes new Timer.

So I'd like to by pass timer creation while I am trying to create a new timer.

Does that sound reasonable?

I've also updates the text.

On 2013/03/29 14:37:39, vsm wrote:
> I'm confused by this.  What was the problem you were hitting in the old code? 
> Why wasn't the new time registering?
> 
> It doesn't seem like this change should fix anyway (beyond reducing a
potential
> delay).

[vsm, 2013-03-29 17:34:36]

https://codereview.chromium.org/13308002/diff/3001/tests/html/async_test.dart
File tests/html/async_test.dart (right):

https://codereview.chromium.org/13308002/diff/3001/tests/html/async_test.dart...
tests/html/async_test.dart:43: fail('Should never be invoked');
In this case, it looks like oneshot could fire/fail (silently in the background)
and periodic would still send DONE to the main thread.

If you're confident fail is doing the right thing and reporting properly, I'd
leave this as is.  I think you can test by setting oneshot to fire in 5 ms
instead of 30.  If it fails as expected, we're probably good.

If not, you could make shot (below) an int instead of a bool, and increment it
each time oneshot or periodic is fired.  And then, replace the replyTo message
with:
shot == 1 ? 'DONE' : 'FAIL'


On 2013/03/29 16:09:15, Anton Muhin wrote:
> On 2013/03/29 14:37:39, vsm wrote:
> > Will a 'fail' on a background isolate be caught be the test framework? 
Might
> > need to send a message back.
> 
> That worked for me when I ran a single test from local server.
> 
> There are overall some issues with tests which I was going to discuss w/ Emily
> 
> Hopefully, even if fail is swollen, test should either fail or timeout anyway,
> correct?

https://codereview.chromium.org/13308002/diff/3001/tools/dom/src/Timer.dart
File tools/dom/src/Timer.dart (right):

https://codereview.chromium.org/13308002/diff/3001/tools/dom/src/Timer.dart#n...
tools/dom/src/Timer.dart:57: // Therefore, let's resort to then only if really
necessary.
I think I'm starting to get this - was the old code hitting an infinite loop on
_HELPER_ISOLATE_PORT.then?

Not entirely clear to me whether this is a bug fix or an optimization from your
comment.

On 2013/03/29 16:09:15, Anton Muhin wrote:
> (whatever outcome of our discuss be, I need to modify cancel code to behave
> similarly).
> 
> There was an interesting corner case I found.
> 
> Imagine you spawn a timer which pings you every 10 ms and from the handler of
> this timer you'd like to spawn another timer.  Alas, timer creation requires
> .then which invoke Timer.run which invokes new Timer.
> 
> So I'd like to by pass timer creation while I am trying to create a new timer.
> 
> Does that sound reasonable?
> 
> I've also updates the text.
> 
> On 2013/03/29 14:37:39, vsm wrote:
> > I'm confused by this.  What was the problem you were hitting in the old
code? 
> > Why wasn't the new time registering?
> > 
> > It doesn't seem like this change should fix anyway (beyond reducing a
> potential
> > delay).
>

[Anton Muhin, 2013-04-01 14:52:57]

https://codereview.chromium.org/13308002/diff/3001/tools/dom/src/Timer.dart
File tools/dom/src/Timer.dart (right):

https://codereview.chromium.org/13308002/diff/3001/tools/dom/src/Timer.dart#n...
tools/dom/src/Timer.dart:57: // Therefore, let's resort to then only if really
necessary.
I've updated the comment.  May you, please, tell me if it's sufficient enough?

On 2013/03/29 17:34:36, vsm wrote:
> I think I'm starting to get this - was the old code hitting an infinite loop
on
> _HELPER_ISOLATE_PORT.then?
> 
> Not entirely clear to me whether this is a bug fix or an optimization from
your
> comment.
> 
> On 2013/03/29 16:09:15, Anton Muhin wrote:
> > (whatever outcome of our discuss be, I need to modify cancel code to behave
> > similarly).
> > 
> > There was an interesting corner case I found.
> > 
> > Imagine you spawn a timer which pings you every 10 ms and from the handler
of
> > this timer you'd like to spawn another timer.  Alas, timer creation requires
> > .then which invoke Timer.run which invokes new Timer.
> > 
> > So I'd like to by pass timer creation while I am trying to create a new
timer.
> > 
> > Does that sound reasonable?
> > 
> > I've also updates the text.
> > 
> > On 2013/03/29 14:37:39, vsm wrote:
> > > I'm confused by this.  What was the problem you were hitting in the old
> code? 
> > > Why wasn't the new time registering?
> > > 
> > > It doesn't seem like this change should fix anyway (beyond reducing a
> > potential
> > > delay).
> > 
>

[vsm, 2013-04-01 15:27:32]

https://codereview.chromium.org/13308002/diff/18001/tests/html/async_test.dart
File tests/html/async_test.dart (right):

https://codereview.chromium.org/13308002/diff/18001/tests/html/async_test.dar...
tests/html/async_test.dart:70:
expect(spawnFunction(oneshotTimerIsolate).call('START'),
Looks like you lost this fix (should be periodicTimerIsolate) / changes on this
file.

https://codereview.chromium.org/13308002/diff/18001/tools/dom/src/Timer.dart
File tools/dom/src/Timer.dart (right):

https://codereview.chromium.org/13308002/diff/18001/tools/dom/src/Timer.dart#...
tools/dom/src/Timer.dart:64: // in Timer code.
Thanks, this looks good.

[Anton Muhin, 2013-04-02 11:51:22]

https://codereview.chromium.org/13308002/diff/18001/tests/html/async_test.dart
File tests/html/async_test.dart (right):

https://codereview.chromium.org/13308002/diff/18001/tests/html/async_test.dar...
tests/html/async_test.dart:70:
expect(spawnFunction(oneshotTimerIsolate).call('START'),
On 2013/04/01 15:27:32, vsm wrote:
> Looks like you lost this fix (should be periodicTimerIsolate) / changes on
this
> file.

Thank you, bad merge.

[Anton Muhin, 2013-04-02 11:52:17]

Vijay, turning into TBR as people start to face problems.

Would be glad to address additional comments in a separate CL.

[Anton Muhin, 2013-04-02 12:29:47]

Committed patchset #5 manually as r20773 (presubmit successful).