Fix a synchrony bug in barback.

Fix a synchrony bug in barback.

Barback was assuming that certain Future operations were synchronous when in
fact they were not (issue 14323). This caused behavioral bugs in situations with
high input churn. Specifically, barback would attempt to use the synchronous
Future operations to guarantee that no changes happened between a condition
being checked and that condition being reported to the user. Since the Future
operations were not in fact synchronous, such changes were possible.

This change works around the issue by using a manual callback-passing style in
places where synchrony is important and Futures would otherwise be used.

R=alanknight@google.com

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

[Alan Knight, 2013-10-23 16:30:18]

lgtm

[nweiz, 2013-10-23 17:55:07]

Committed patchset #1 manually as r29103 (presubmit successful).

[Bob Nystrom, 2013-10-28 16:51:30]

I don't understand this change. We almost always prefer futures over callbacks,
so why refactor from the former to the latter? Are sync Futures not actually
sync?

https://codereview.chromium.org/36463002/diff/1/pkg/barback/test/package_grap...
File pkg/barback/test/package_graph/repetition_test.dart (right):

https://codereview.chromium.org/36463002/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/repetition_test.dart:12: main() {
Can you add some documentation here explaining what this is stress testing and
why?

https://codereview.chromium.org/36463002/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/repetition_test.dart:48: for (var i = 0; i < 1;
i++) {
That's not very many times.

https://codereview.chromium.org/36463002/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/repetition_test.dart:61: for (var i = 0; i < 1;
i++) {
Ditto.

[nweiz, 2013-10-28 23:54:49]

It turns out there was never such a thing as a "synchronous future".
[Future.sync] only runs the callback synchronously; the behavior of the returned
future is identical to any other future. [Completer.sync] only synchronously
fires events that are registered prior to [Completer.complete] being called.
There's no way to create a future that will synchronously call a [then] callback
when it's registered. The dart:async folks don't seem interested in changing
this; see issue 14323.

I'd like to use Futures here; I even tried to implement a SyncFuture class to
get the behavior we intended and assumed. It didn't work, so I came to the
conclusion that the only clean way to do this is to accept that the only way to
cleanly handle potentially-synchronous callbacks is to manually dispatch them
the old-fashioned way.

https://codereview.chromium.org/36463002/diff/1/pkg/barback/test/package_grap...
File pkg/barback/test/package_graph/repetition_test.dart (right):

https://codereview.chromium.org/36463002/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/repetition_test.dart:12: main() {
On 2013/10/28 16:51:30, Bob Nystrom wrote:
> Can you add some documentation here explaining what this is stress testing and
> why?

Done.

https://codereview.chromium.org/36463002/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/repetition_test.dart:48: for (var i = 0; i < 1;
i++) {
On 2013/10/28 16:51:30, Bob Nystrom wrote:
> That's not very many times.

Oops, left over from debuggnig. Fixed.

https://codereview.chromium.org/36463002/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/repetition_test.dart:61: for (var i = 0; i < 1;
i++) {
On 2013/10/28 16:51:30, Bob Nystrom wrote:
> Ditto.

Done.