Add a 30s timeout for all HTTP requests in Pub.

Add a 30s timeout for all HTTP requests in Pub.

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

[Bob Nystrom, 2012-09-06 22:52:05]

https://chromiumcodereview.appspot.com/10928041/diff/3001/utils/pub/io.dart
File utils/pub/io.dart (right):

https://chromiumcodereview.appspot.com/10928041/diff/3001/utils/pub/io.dart#n...
utils/pub/io.dart:346: .transform((bytes) => new String.fromCharCodes(bytes));
Indent +2

https://chromiumcodereview.appspot.com/10928041/diff/3001/utils/pub/io.dart#n...
utils/pub/io.dart:450: Future timeout(Future input, int milliSeconds, exception)
{
Can you create a TimeoutException class and then take a message string here? I'd
like to do something a little more structured that throwing a raw string, and
that lets us do better error-reporting/handling later at a higher level.

https://chromiumcodereview.appspot.com/10928041/diff/3001/utils/pub/io.dart#n...
utils/pub/io.dart:454: completer.completeException(exception);
I like the idea of timeout() being a separate orthogonal function, but the
problem here is that it doesn't actually do anything to try to cancel the
request that timed out.

It probably more or less works here because we explicitly exit() now, but if we
didn't, this would leave the app still running until dart:io's internal timeout
triggered. Can we do something better here?

[nweiz, 2012-09-06 23:33:34]

https://chromiumcodereview.appspot.com/10928041/diff/3001/utils/pub/io.dart
File utils/pub/io.dart (right):

https://chromiumcodereview.appspot.com/10928041/diff/3001/utils/pub/io.dart#n...
utils/pub/io.dart:346: .transform((bytes) => new String.fromCharCodes(bytes));
On 2012/09/06 22:52:05, Bob Nystrom wrote:
> Indent +2

Done.

https://chromiumcodereview.appspot.com/10928041/diff/3001/utils/pub/io.dart#n...
utils/pub/io.dart:450: Future timeout(Future input, int milliSeconds, exception)
{
On 2012/09/06 22:52:05, Bob Nystrom wrote:
> Can you create a TimeoutException class and then take a message string here?
I'd
> like to do something a little more structured that throwing a raw string, and
> that lets us do better error-reporting/handling later at a higher level.

Done.

https://chromiumcodereview.appspot.com/10928041/diff/3001/utils/pub/io.dart#n...
utils/pub/io.dart:454: completer.completeException(exception);
On 2012/09/06 22:52:05, Bob Nystrom wrote:
> I like the idea of timeout() being a separate orthogonal function, but the
> problem here is that it doesn't actually do anything to try to cancel the
> request that timed out.
> 
> It probably more or less works here because we explicitly exit() now, but if
we
> didn't, this would leave the app still running until dart:io's internal
timeout
> triggered. Can we do something better here?

Given that we do explicitly exit, I don't see anything wrong with letting the
original future dangle. I could change this to take a callback that could cancel
and then throw, but it feels like that just adds complexity without any
practical benefit.

[Bob Nystrom, 2012-09-07 00:15:34]

One comment, but LGTM!

https://chromiumcodereview.appspot.com/10928041/diff/3001/utils/pub/io.dart
File utils/pub/io.dart (right):

https://chromiumcodereview.appspot.com/10928041/diff/3001/utils/pub/io.dart#n...
utils/pub/io.dart:454: completer.completeException(exception);
On 2012/09/06 23:33:34, nweiz wrote:
> On 2012/09/06 22:52:05, Bob Nystrom wrote:
> > I like the idea of timeout() being a separate orthogonal function, but the
> > problem here is that it doesn't actually do anything to try to cancel the
> > request that timed out.
> > 
> > It probably more or less works here because we explicitly exit() now, but if
> we
> > didn't, this would leave the app still running until dart:io's internal
> timeout
> > triggered. Can we do something better here?
> 
> Given that we do explicitly exit, I don't see anything wrong with letting the
> original future dangle. I could change this to take a callback that could
cancel
> and then throw, but it feels like that just adds complexity without any
> practical benefit.

OK. Can you update the doc comment to note that this doesn't cancel the async
operation when a timeout occurs?