Update pkg/http to use the new async APIs.

Update pkg/http to use the new async APIs.

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

[Bob Nystrom, 2013-01-08 23:50:49]

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/base_request...
File pkg/http/lib/src/base_request.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/base_request...
pkg/http/lib/src/base_request.dart:96: Stream<List<int>> finalize() {
I think I understand why it's chunked, but I wonder if it would be cleaner if it
was just a flat list of bytes. Thoughts?

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/base_request...
pkg/http/lib/src/base_request.dart:124: throw e;
Might want to mention dartbug.com/7781 here since this will be affected by it.

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/byte_stream....
File pkg/http/lib/src/byte_stream.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/byte_stream....
pkg/http/lib/src/byte_stream.dart:13: /// A stream of bytes representing a
single chunked piece of data.
Clarify that it's a stream of chunks of bytes, and not a single chunk.

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/byte_stream....
pkg/http/lib/src/byte_stream.dart:16: var id = _id++;
Debug code?

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart
File pkg/http/lib/src/utils.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:137: /// [ByteStream].
How about making this a (potentially factory) constructor on ByteStream? That
way any code using that type has easy access to the method?

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:153: /// data.
Is this just for compatibility with the existing dart:io code? If so, add a TODO
to that effect?

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:160: stream.onError = (e) =>
controller.signalError(new AsyncError(e));
Having to manually create the AsyncError here is lame. Maybe file a bug to
change this?

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:200: // TODO(nweiz): remove this when it's added to
the Stream API.
Is there a bug# for this?

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:210: completer.complete(null);
The arg is optional here now. Just do:

completer.complete();

(Too bad the param in the then() callback isn't optional...)

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:222: onDone: () => completer.complete(null));
complete()

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:244: Pair<Stream, Stream> tee(Stream stream) {
If single-subscriber streams are going to stick around, it seems like this
should be in corelib then. File a bug?

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:282: });
Use onError:

future.then((v) => completer.complete(v), onError: (e) {
  completer.completeError(e.error, e.stackTrace);
});

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/client_test.dart
File pkg/http/test/client_test.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/client_test.dar...
pkg/http/test/client_test.dart:28: }).whenComplete(client.close),
completion(parse(equals({
then() has an optional named onError parameter. Use that instead of chaining
here.

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/http_test.dart
File pkg/http/test/http_test.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/http_test.dart#...
pkg/http/test/http_test.dart:29: }).then((response) {
I think you'll need guardAsync here and below, right?

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/request_test.dart
File pkg/http/test/request_test.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/request_test.da...
pkg/http/test/request_test.dart:21: expect(request.send().then((response) {
I think you'll need guardAsync here.

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/request_test.da...
pkg/http/test/request_test.dart:24: }).whenComplete(stopServer),
completion(parse(equals({
And maybe even here around stopServer.

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/utils.dart
File pkg/http/test/utils.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/utils.dart#newc...
pkg/http/test/utils.dart:108: /// Buffers all input from an InputStream and
returns it as a future.
Is this a dart:io compatibility thing? If so, document it as such and add a TODO
to get rid of it when we can.

[nweiz, 2013-01-09 00:52:11]

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/base_request...
File pkg/http/lib/src/base_request.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/base_request...
pkg/http/lib/src/base_request.dart:96: Stream<List<int>> finalize() {
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> I think I understand why it's chunked, but I wonder if it would be cleaner if
it
> was just a flat list of bytes. Thoughts?

I haven't actually run benchmarks, but I'm pretty sure emitting an individual
event for every byte would be really really bad for performance. ByteStream
makes the chunked mode just about as usable as unchunked, while being much more
efficient.

Speaking of which, this annotation should be ByteStream.

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/base_request...
pkg/http/lib/src/base_request.dart:124: throw e;
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> Might want to mention dartbug.com/7781 here since this will be affected by it.

I don't think mentioning that will do any good, since the code won't change
after the bug is fixed.

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/byte_stream....
File pkg/http/lib/src/byte_stream.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/byte_stream....
pkg/http/lib/src/byte_stream.dart:13: /// A stream of bytes representing a
single chunked piece of data.
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> Clarify that it's a stream of chunks of bytes, and not a single chunk.

Done.

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/byte_stream....
pkg/http/lib/src/byte_stream.dart:16: var id = _id++;
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> Debug code?

Oops, removed.

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart
File pkg/http/lib/src/utils.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:137: /// [ByteStream].
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> How about making this a (potentially factory) constructor on ByteStream? That
> way any code using that type has easy access to the method?

It feels too similar to the default ByteStream constructor. If I could make the
default constructor this without hosing the ability to extend ByteStream, I
would, but since I can't I think it's better to leave it here. I doubt most code
will need to make use of it.

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:153: /// data.
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> Is this just for compatibility with the existing dart:io code? If so, add a
TODO
> to that effect?

Done.

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:160: stream.onError = (e) =>
controller.signalError(new AsyncError(e));
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> Having to manually create the AsyncError here is lame. Maybe file a bug to
> change this?

I'm not sure what a better API would be.

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:200: // TODO(nweiz): remove this when it's added to
the Stream API.
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> Is there a bug# for this?

Filed one.

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:210: completer.complete(null);
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> The arg is optional here now. Just do:
> 
> completer.complete();
> 
> (Too bad the param in the then() callback isn't optional...)

Done.

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:222: onDone: () => completer.complete(null));
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> complete()

Done.

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:244: Pair<Stream, Stream> tee(Stream stream) {
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> If single-subscriber streams are going to stick around, it seems like this
> should be in corelib then. File a bug?

Done.

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:282: });
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> Use onError:
> 
> future.then((v) => completer.complete(v), onError: (e) {
>   completer.completeError(e.error, e.stackTrace);
> });

Why? The documentation says (and I agree) that catchError is more readable than
onError, and Future.complete doesn't synchronously throw errors any more.

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/client_test.dart
File pkg/http/test/client_test.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/client_test.dar...
pkg/http/test/client_test.dart:28: }).whenComplete(client.close),
completion(parse(equals({
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> then() has an optional named onError parameter. Use that instead of chaining
> here.

See previous comment. Also, the semantics of onError are very different than
those of whenComplete.

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/http_test.dart
File pkg/http/test/http_test.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/http_test.dart#...
pkg/http/test/http_test.dart:29: }).then((response) {
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> I think you'll need guardAsync here and below, right?

No, expect/completes handles all of that for us.

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/request_test.dart
File pkg/http/test/request_test.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/request_test.da...
pkg/http/test/request_test.dart:21: expect(request.send().then((response) {
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> I think you'll need guardAsync here.

See previous comment.

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/request_test.da...
pkg/http/test/request_test.dart:24: }).whenComplete(stopServer),
completion(parse(equals({
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> And maybe even here around stopServer.

See previous² comment.

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/utils.dart
File pkg/http/test/utils.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/utils.dart#newc...
pkg/http/test/utils.dart:108: /// Buffers all input from an InputStream and
returns it as a future.
On 2013/01/08 23:50:49, Bob Nystrom wrote:
> Is this a dart:io compatibility thing? If so, document it as such and add a
TODO
> to get rid of it when we can.

Done.

[Bob Nystrom, 2013-01-09 03:00:43]

LGTM!

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart
File pkg/http/lib/src/utils.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:282: });
On 2013/01/09 00:52:11, nweiz wrote:
> On 2013/01/08 23:50:49, Bob Nystrom wrote:
> > Use onError:
> > 
> > future.then((v) => completer.complete(v), onError: (e) {
> >   completer.completeError(e.error, e.stackTrace);
> > });
> 
> Why? The documentation says (and I agree) that catchError is more readable
than
> onError, and Future.complete doesn't synchronously throw errors any more.

<shrug>

I figured one method call with two args was simpler than chaining (and, for what
it's worth, it creates one fewer future), but I don't have a strong preference.
I do wish "catchError" had a shorter name.

We should pick a consistent style, though, since there's little reason to mix
them.

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/client_test.dart
File pkg/http/test/client_test.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/test/client_test.dar...
pkg/http/test/client_test.dart:28: }).whenComplete(client.close),
completion(parse(equals({
On 2013/01/09 00:52:11, nweiz wrote:
> On 2013/01/08 23:50:49, Bob Nystrom wrote:
> > then() has an optional named onError parameter. Use that instead of chaining
> > here.
> 
> See previous comment. Also, the semantics of onError are very different than
> those of whenComplete.

Sorry, read this code wrong. You're exactly right.

[nweiz, 2013-01-09 03:07:40]

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart
File pkg/http/lib/src/utils.dart (right):

https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:282: });
On 2013/01/09 03:00:43, Bob Nystrom wrote:
> On 2013/01/09 00:52:11, nweiz wrote:
> > On 2013/01/08 23:50:49, Bob Nystrom wrote:
> > > Use onError:
> > > 
> > > future.then((v) => completer.complete(v), onError: (e) {
> > >   completer.completeError(e.error, e.stackTrace);
> > > });
> > 
> > Why? The documentation says (and I agree) that catchError is more readable
> than
> > onError, and Future.complete doesn't synchronously throw errors any more.
> 
> <shrug>
> 
> I figured one method call with two args was simpler than chaining (and, for
what
> it's worth, it creates one fewer future), but I don't have a strong
preference.
> I do wish "catchError" had a shorter name.
> 
> We should pick a consistent style, though, since there's little reason to mix
> them.

I vote chaining.

[Bob Nystrom, 2013-01-09 16:05:37]

On 2013/01/09 03:07:40, nweiz wrote:
> https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart
> File pkg/http/lib/src/utils.dart (right):
> 
>
https://codereview.chromium.org/11825010/diff/1/pkg/http/lib/src/utils.dart#n...
> pkg/http/lib/src/utils.dart:282: });
> On 2013/01/09 03:00:43, Bob Nystrom wrote:
> > On 2013/01/09 00:52:11, nweiz wrote:
> > > On 2013/01/08 23:50:49, Bob Nystrom wrote:
> > > > Use onError:
> > > > 
> > > > future.then((v) => completer.complete(v), onError: (e) {
> > > >   completer.completeError(e.error, e.stackTrace);
> > > > });
> > > 
> > > Why? The documentation says (and I agree) that catchError is more readable
> > than
> > > onError, and Future.complete doesn't synchronously throw errors any more.
> > 
> > <shrug>
> > 
> > I figured one method call with two args was simpler than chaining (and, for
> what
> > it's worth, it creates one fewer future), but I don't have a strong
> preference.
> > I do wish "catchError" had a shorter name.
> > 
> > We should pick a consistent style, though, since there's little reason to
mix
> > them.
> 
> I vote chaining.

Works for me, especially since the docs explicitly encourage that. (Though that
does raise the question of why even have the optional arg in then()...)

[nweiz, 2013-01-09 20:34:30]

On 2013/01/09 16:05:37, Bob Nystrom wrote:
> Works for me, especially since the docs explicitly encourage that. (Though
that
> does raise the question of why even have the optional arg in then()...)

It's useful in the specific case where you need to catch an error in the future
but pass on errors from the then() handler, similar to the begin/rescue/else
pattern in Ruby.