- Make sure we schedule another wakeup when handling a timeout

- Make sure we schedule another wakeup when handling a timeout
  from the event handler.

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

[Ivan Posva, 2015-03-03 05:02:12]

iposva@google.com changed reviewers:
+ koda@google.com, zra@google.com

[Ivan Posva, 2015-03-03 05:02:12]

With this change we clearly mark that we do have consumed the last scheduled
wakeup.

-Ivan

[koda, 2015-03-03 06:49:20]

It's late, so I might have missed something. Let's talk tomorrow in any case.

https://codereview.chromium.org/971193002/diff/1/runtime/lib/timer_impl.dart
File runtime/lib/timer_impl.dart (right):

https://codereview.chromium.org/971193002/diff/1/runtime/lib/timer_impl.dart#...
runtime/lib/timer_impl.dart:144: bool _repeating;  // Indicates periodic timers.
final

https://codereview.chromium.org/971193002/diff/1/runtime/lib/timer_impl.dart#...
runtime/lib/timer_impl.dart:146: int _id;  // Incrementing id to enable sorting
of timers with same expiry.
final

https://codereview.chromium.org/971193002/diff/1/runtime/lib/timer_impl.dart#...
runtime/lib/timer_impl.dart:301: VMLibraryHooks.eventHandlerSendData(null,
_sendPort, _NO_TIMER);
Shouldn't you also assign null to _scheduledWakeupTime here, to maintain the
representation invariant?

As the code is now, a zero-timer's callback could cancel all the non-zero timers
without resetting _scheduledWakeupTime, and then if the user adds a zero timer
that in turn adds a non-zero-timer with a wakeup that happens to match
_scheduledWakeupTime, the test below would be incorrect, and that non-zero timer
would be missed, no?

[koda, 2015-03-03 14:08:41]

On 2015/03/03 06:49:20, koda wrote:
> It's late, so I might have missed something. Let's talk tomorrow in any case.
> 
> https://codereview.chromium.org/971193002/diff/1/runtime/lib/timer_impl.dart
> File runtime/lib/timer_impl.dart (right):
> 
>
https://codereview.chromium.org/971193002/diff/1/runtime/lib/timer_impl.dart#...
> runtime/lib/timer_impl.dart:144: bool _repeating;  // Indicates periodic
timers.
> final
> 
>
https://codereview.chromium.org/971193002/diff/1/runtime/lib/timer_impl.dart#...
> runtime/lib/timer_impl.dart:146: int _id;  // Incrementing id to enable
sorting
> of timers with same expiry.
> final
> 
>
https://codereview.chromium.org/971193002/diff/1/runtime/lib/timer_impl.dart#...
> runtime/lib/timer_impl.dart:301: VMLibraryHooks.eventHandlerSendData(null,
> _sendPort, _NO_TIMER);
> Shouldn't you also assign null to _scheduledWakeupTime here, to maintain the
> representation invariant?
> 
> As the code is now, a zero-timer's callback could cancel all the non-zero
timers
> without resetting _scheduledWakeupTime, and then if the user adds a zero timer
> that in turn adds a non-zero-timer with a wakeup that happens to match
> _scheduledWakeupTime, the test below would be incorrect, and that non-zero
timer
> would be missed, no?

The following code demonstrates the bug I described. Both before and after your
CL, it sometimes hangs, and you can increase "countdown" to make it arbitrarily
likely to hang.

import 'dart:async';
import 'dart:math';

int countdown = 4;
var rng = new Random(1234);

void test(int delay, int delta) {
  var t0 = new Timer(new Duration(milliseconds: delay + delta),
                     () => print("should have been cancelled by now"));
  new Timer(Duration.ZERO, () => t0.cancel());
  new Timer(Duration.ZERO,
            () => new Timer(new Duration(milliseconds: delay),
                            () {
                              if (--countdown == 0) {
                                print("done");
                              } else {
                                test(delay, max(0, delta + rng.nextInt(2) - 1));
                              }
                            }));
}

void main() {
  test(100, 2);
}

koda@koda-linux:~/timer/dart$ out/ReleaseIA32/dart test.dart 
^C
koda@koda-linux:~/timer/dart$ out/ReleaseIA32/dart test.dart 
done
koda@koda-linux:~/timer/dart$ out/ReleaseIA32/dart test.dart 
^C
koda@koda-linux:~/timer/dart$ out/ReleaseIA32/dart test.dart 
^C
koda@koda-linux:~/timer/dart$ out/ReleaseIA32/dart test.dart 
done
koda@koda-linux:~/timer/dart$ out/ReleaseIA32/dart test.dart 
^C

[Ivan Posva, 2015-03-03 18:53:23]

Now with more fixes and Daniel's test.

-Ivan

[koda, 2015-03-03 18:58:51]

LGTMwC

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

https://codereview.chromium.org/971193002/diff/20001/runtime/lib/timer_impl.d...
runtime/lib/timer_impl.dart:418: // Tell the event handle to wake this isolate
at a specific time.
handle -> handler

https://codereview.chromium.org/971193002/diff/20001/runtime/lib/timer_impl.d...
runtime/lib/timer_impl.dart:421: // Create a receive port and register a message
handler for the timer
Move this comment to the actual method.

https://codereview.chromium.org/971193002/diff/20001/runtime/lib/timer_impl.d...
runtime/lib/timer_impl.dart:435: 
Remove blank line.

https://codereview.chromium.org/971193002/diff/20001/tests/lib/async/timer_re...
File tests/lib/async/timer_regress22626_test.dart (right):

https://codereview.chromium.org/971193002/diff/20001/tests/lib/async/timer_re...
tests/lib/async/timer_regress22626_test.dart:1: // Copyright (c) 2011, the Dart
project authors.  Please see the AUTHORS file
2011 -> 2015

[Ivan Posva, 2015-03-03 19:14:18]

Thanks,
-Ivan

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

https://codereview.chromium.org/971193002/diff/20001/runtime/lib/timer_impl.d...
runtime/lib/timer_impl.dart:418: // Tell the event handle to wake this isolate
at a specific time.
On 2015/03/03 18:58:51, koda wrote:
> handle -> handler

Acknowledged.

https://codereview.chromium.org/971193002/diff/20001/runtime/lib/timer_impl.d...
runtime/lib/timer_impl.dart:421: // Create a receive port and register a message
handler for the timer
On 2015/03/03 18:58:51, koda wrote:
> Move this comment to the actual method.

Done.

https://codereview.chromium.org/971193002/diff/20001/runtime/lib/timer_impl.d...
runtime/lib/timer_impl.dart:435: 
On 2015/03/03 18:58:51, koda wrote:
> Remove blank line.

Done.

https://codereview.chromium.org/971193002/diff/20001/tests/lib/async/timer_re...
File tests/lib/async/timer_regress22626_test.dart (right):

https://codereview.chromium.org/971193002/diff/20001/tests/lib/async/timer_re...
tests/lib/async/timer_regress22626_test.dart:1: // Copyright (c) 2011, the Dart
project authors.  Please see the AUTHORS file
On 2015/03/03 18:58:51, koda wrote:
> 2011 -> 2015

Done.

[Ivan Posva, 2015-03-03 19:14:36]

Committed patchset #3 (id:30001) manually as r44191 (presubmit successful).