Make Completers asynchronous.

Make Completers asynchronous.

BUG= http://dartbug.com/9990
R=ajohnsen@google.com, lrn@google.com

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

[floitsch, 2013-05-04 22:33:24]

@Anders: see below.
@Nathan: please check the pkg/scheduled_test changes.
@Peter: cced for mirror changes.
@Srdjan: cced for runtime changes

@Lasse: main reviewer. I'm not sure if we want to keep the Future.sync, but it's
not completely unreasonable and without it one VM tests fails because of an
isolate test that must not run asynchronously. The VM test would probably also
be fixed by implementing "runAsync" correctly.

@Anders:
http_server_early_client_close_test fails with testEarlyClose2b (currently not
running, see line 194).
There seems to be a dependency inside the http-library that Futures are executed
synchronously. I have traced it down to the server.close call in line 157.
My guess is, that we close the server and that a future (completed with a
Completer) is then supposed to shut down the input to the Socket. However the
future is not completed immediately anymore and we now try to push file-data to
a closed socket/stream.

[floitsch, 2013-05-04 22:37:37]

+srdjan.

[Lasse Reichstein Nielsen, 2013-05-06 06:49:11]

lgtm

https://codereview.chromium.org/14690009/diff/1/sdk/lib/async/future.dart
File sdk/lib/async/future.dart (right):

https://codereview.chromium.org/14690009/diff/1/sdk/lib/async/future.dart#new...
sdk/lib/async/future.dart:352: * known to be the final result of another
synchronous operation. If in doubt
synchronous -> asynchronous? (following the first example).

https://codereview.chromium.org/14690009/diff/1/sdk/lib/async/future_impl.dart
File sdk/lib/async/future_impl.dart (right):

https://codereview.chromium.org/14690009/diff/1/sdk/lib/async/future_impl.dar...
sdk/lib/async/future_impl.dart:11: final _FutureImpl<T> future;
_FutureImpl -> Future. No reason to change the type of a public field.
Cast it to _FutureImpl before using it, like the original code.

https://codereview.chromium.org/14690009/diff/1/sdk/lib/async/future_impl.dar...
sdk/lib/async/future_impl.dart:22: _FutureImpl future = this.future;
Move this cast to _setFutureValue.

https://codereview.chromium.org/14690009/diff/1/sdk/lib/async/future_impl.dar...
sdk/lib/async/future_impl.dart:33: _FutureImpl future = this.future;
And remove this one.

https://codereview.chromium.org/14690009/diff/1/sdk/lib/async/future_impl.dar...
sdk/lib/async/future_impl.dart:40: class _CompleterImpl<T> extends _Completer<T>
{
I prefer _AsyncCompleter (and _SyncCompleter below).
The "Impl" suffix is really only for when you have only one implementation with
no distinguishing characteristic (and you are suffering from lack of
inspiration).

https://codereview.chromium.org/14690009/diff/1/tests/standalone/io/http_serv...
File tests/standalone/io/http_server_early_client_close_test.dart (right):

https://codereview.chromium.org/14690009/diff/1/tests/standalone/io/http_serv...
tests/standalone/io/http_server_early_client_close_test.dart:194: //
testEarlyClose2b();
Commented code?

[Anders Johnsen, 2013-05-06 07:44:42]

LGTM

https://codereview.chromium.org/14690009/diff/1/tests/standalone/io/http_pars...
File tests/standalone/io/http_parser_test.dart (right):

https://codereview.chromium.org/14690009/diff/1/tests/standalone/io/http_pars...
tests/standalone/io/http_parser_test.dart:278: void testWrite(List<int>
requestData, [int chunkSize = -1]) {
Nice cleanup, this is exactly how it should be!

https://codereview.chromium.org/14690009/diff/1/tests/standalone/io/http_serv...
File tests/standalone/io/http_server_early_client_close_test.dart (right):

https://codereview.chromium.org/14690009/diff/1/tests/standalone/io/http_serv...
tests/standalone/io/http_server_early_client_close_test.dart:137:
testEarlyClose2b() {
Florian,

This test is already flaky. We have it tracked in a bug, with a potential fix
underway. Can you add a TODO at the bottom? Thanks!

[floitsch, 2013-05-06 15:14:45]

https://codereview.chromium.org/14690009/diff/1/sdk/lib/async/future.dart
File sdk/lib/async/future.dart (right):

https://codereview.chromium.org/14690009/diff/1/sdk/lib/async/future.dart#new...
sdk/lib/async/future.dart:352: * known to be the final result of another
synchronous operation. If in doubt
On 2013/05/06 06:49:11, Lasse Reichstein Nielsen wrote:
> synchronous -> asynchronous? (following the first example).
> 

Done.

https://codereview.chromium.org/14690009/diff/1/sdk/lib/async/future_impl.dart
File sdk/lib/async/future_impl.dart (right):

https://codereview.chromium.org/14690009/diff/1/sdk/lib/async/future_impl.dar...
sdk/lib/async/future_impl.dart:11: final _FutureImpl<T> future;
On 2013/05/06 06:49:11, Lasse Reichstein Nielsen wrote:
> _FutureImpl -> Future. No reason to change the type of a public field.
> Cast it to _FutureImpl before using it, like the original code.

Done.

https://codereview.chromium.org/14690009/diff/1/sdk/lib/async/future_impl.dar...
sdk/lib/async/future_impl.dart:22: _FutureImpl future = this.future;
On 2013/05/06 06:49:11, Lasse Reichstein Nielsen wrote:
> Move this cast to _setFutureValue.

Done.

https://codereview.chromium.org/14690009/diff/1/sdk/lib/async/future_impl.dar...
sdk/lib/async/future_impl.dart:33: _FutureImpl future = this.future;
On 2013/05/06 06:49:11, Lasse Reichstein Nielsen wrote:
> And remove this one.

Done.

https://codereview.chromium.org/14690009/diff/1/sdk/lib/async/future_impl.dar...
sdk/lib/async/future_impl.dart:40: class _CompleterImpl<T> extends _Completer<T>
{
On 2013/05/06 06:49:11, Lasse Reichstein Nielsen wrote:
> I prefer _AsyncCompleter (and _SyncCompleter below).
> The "Impl" suffix is really only for when you have only one implementation
with
> no distinguishing characteristic (and you are suffering from lack of
> inspiration).

Done.

https://codereview.chromium.org/14690009/diff/1/tests/standalone/io/http_serv...
File tests/standalone/io/http_server_early_client_close_test.dart (right):

https://codereview.chromium.org/14690009/diff/1/tests/standalone/io/http_serv...
tests/standalone/io/http_server_early_client_close_test.dart:137:
testEarlyClose2b() {
On 2013/05/06 07:44:42, Anders Johnsen wrote:
> Florian,
> 
> This test is already flaky. We have it tracked in a bug, with a potential fix
> underway. Can you add a TODO at the bottom? Thanks!

Extracted it into a separate file and marked it as failing in the status file
(with the bug-number).

https://codereview.chromium.org/14690009/diff/1/tests/standalone/io/http_serv...
tests/standalone/io/http_server_early_client_close_test.dart:194: //
testEarlyClose2b();
On 2013/05/06 06:49:11, Lasse Reichstein Nielsen wrote:
> Commented code?

I asked Anders to look into it. This test started failing.

[floitsch, 2013-05-06 15:15:32]

Committed patchset #2 manually as r22417 (presubmit successful).

[srdjan, 2013-05-06 16:03:21]

lgtm

[nweiz, 2013-05-07 00:11:52]

https://codereview.chromium.org/14690009/diff/9001/pkg/scheduled_test/test/su...
File pkg/scheduled_test/test/substitute_future_test.dart (right):

https://codereview.chromium.org/14690009/diff/9001/pkg/scheduled_test/test/su...
pkg/scheduled_test/test/substitute_future_test.dart:154: }));
[expectAsync*] is usually less expressive than [expect]/[completes]. In this
case, you could write:

    expect(future.then((_) => future.substitute(new Future.value())),
        throwsStateError);

since [throwsA] is Future-aware by default.

Also, add a comment here explaining why this needs to be async.

https://codereview.chromium.org/14690009/diff/9001/pkg/scheduled_test/test/va...
File pkg/scheduled_test/test/value_future_test.dart (right):

https://codereview.chromium.org/14690009/diff/9001/pkg/scheduled_test/test/va...
pkg/scheduled_test/test/value_future_test.dart:103: }));
These can be

    expect(future.then((_) => future.value), completion(equals(12)));

etc

[floitsch, 2013-05-13 11:19:25]

https://codereview.chromium.org/14690009/diff/9001/pkg/scheduled_test/test/su...
File pkg/scheduled_test/test/substitute_future_test.dart (right):

https://codereview.chromium.org/14690009/diff/9001/pkg/scheduled_test/test/su...
pkg/scheduled_test/test/substitute_future_test.dart:154: }));
On 2013/05/07 00:11:52, nweiz wrote:
> [expectAsync*] is usually less expressive than [expect]/[completes]. In this
> case, you could write:
> 
>     expect(future.then((_) => future.substitute(new Future.value())),
>         throwsStateError);
> 
> since [throwsA] is Future-aware by default.
> 
> Also, add a comment here explaining why this needs to be async.

done in CL 15128002.

https://codereview.chromium.org/14690009/diff/9001/pkg/scheduled_test/test/va...
File pkg/scheduled_test/test/value_future_test.dart (right):

https://codereview.chromium.org/14690009/diff/9001/pkg/scheduled_test/test/va...
pkg/scheduled_test/test/value_future_test.dart:103: }));
On 2013/05/07 00:11:52, nweiz wrote:
> These can be
> 
>     expect(future.then((_) => future.value), completion(equals(12)));
> 
> etc

done in CL 15128002.