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

utils/pub/io.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 /**
  * Helper functionality to make working with IO easier.
  */
 #library('io');
 
 #import('dart:io');
+#import('dart:isolate');
 #import('dart:uri');
 
 /** Gets the current working directory. */
 String get workingDir => new File('.').fullPathSync();
 
 /**
  * Prints the given string to `stderr` on its own line.
  */
 void printError(value) {
   stderr.writeString(value.toString());
@@ skipping 257 matching lines @@
   });
 }
 
 /**
  * Given [entry] which may be a [String], [File], or [Directory] relative to
  * the current working directory, returns its full canonicalized path.
  */
 // TODO(rnystrom): Should this be async?
 String getFullPath(entry) => new File(_getPath(entry)).fullPathSync();
 
+// TODO(nweiz): make this configurable
+/**
+ * The amount of time in milliseconds to allow HTTP requests before assuming
+ * they've failed.
+ */
+final HTTP_TIMEOUT = 30 * 1000;
+
 /**
  * Opens an input stream for a HTTP GET request to [uri], which may be a
  * [String] or [Uri].
+ *
+ * Callers should be sure to use [timeout] to make sure that the HTTP request
+ * doesn't last indefinitely
  */
-InputStream httpGet(uri) {
-  var resultStream = new ListInputStream();
-  var client = new HttpClient();
-  var connection = client.getUrl(_getUri(uri));
-
-  // TODO(nweiz): propagate this error to the return value. See issue 3657.
-  connection.onError = (e) { throw e; };
-  connection.onResponse = (response) {
-    if (response.statusCode >= 400) {
-      // TODO(nweiz): propagate this error to the return value. See issue 3657.
-      throw new Exception(
-          "HTTP request for $uri failed with status ${response.statusCode}");
-    }
-
-    pipeInputToInput(response.inputStream, resultStream, client.shutdown);
-  };
-
-  return resultStream;
-}
-
-/**
- * Opens an input stream for a HTTP GET request to [uri], which may be a
- * [String] or [Uri]. Completes with the result of the request as a String.
- */
-Future<String> httpGetString(uri) {
-  // TODO(rnystrom): This code is very similar to httpGet() and
-  // consumeInputStream() (but this one propagates errors). Merge them when
-  // #3657 is fixed.
+Future<InputStream> httpGet(uri) {
+  // TODO(nweiz): This could return an InputStream synchronously if issue 3657
+  // were fixed and errors could be propagated through it. Then we could also
+  // automatically attach a timeout to that stream.
   uri = _getUri(uri);
 
-  var completer = new Completer<String>();
+  var completer = new Completer<InputStream>();
   var client = new HttpClient();
   var connection = client.getUrl(uri);
 
   connection.onError = (e) {
     // Show a friendly error if the URL couldn't be resolved.
     if (e is SocketIOException &&
         (e.osError.errorCode == 8 ||
          e.osError.errorCode == -2 ||
          e.osError.errorCode == -5)) {
       e = 'Could not resolve URL "${uri.origin}".';
     }
 
     client.shutdown();
     completer.completeException(e);
   };
 
   connection.onResponse = (response) {
     if (response.statusCode >= 400) {
       client.shutdown();
       completer.completeException(
           new HttpException(response.statusCode, response.reasonPhrase));
       return;
     }
 
-    var resultStream = new ListInputStream();
-    var buffer = <int>[];
-
-    response.inputStream.onClosed = () {
-      client.shutdown();
-      completer.complete(new String.fromCharCodes(buffer));
-    };
-
-    response.inputStream.onData = () {
-      buffer.addAll(response.inputStream.read());
-    };
-
-    response.inputStream.onError = (e) {
-      client.shutdown();
-      completer.completeException(e);
-    };
+    completer.complete(response.inputStream);
   };
 
   return completer.future;
 }
 
 /**
+ * Opens an input stream for a HTTP GET request to [uri], which may be a
+ * [String] or [Uri]. Completes with the result of the request as a String.
+ */
+Future<String> httpGetString(uri) {
+  var future = httpGet(uri).chain((stream) => consumeInputStream(stream))
+    .transform((bytes) => new String.fromCharCodes(bytes));

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

Indent +2

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

Done.

+  return timeout(future, HTTP_TIMEOUT, 'Timed out while fetching URL "$uri".');
+}
+
+/**
  * Takes all input from [source] and writes it to [sink].
  *
  * [onClosed] is called when [source] is closed.
  */
 void pipeInputToInput(InputStream source, ListInputStream sink,
     [void onClosed()]) {
   source.onClosed = () {
     sink.markEndOfStream();
     if (onClosed != null) onClosed();
   };
@@ skipping 74 matching lines @@
     exitCode = actualExitCode;
     checkComplete();
   };
 
   process.onError = (error) => completer.completeException(error);
 
   return completer.future;
 }
 
 /**
+ * Wraps [input] to provide a timeout. If [input] completes before
+ * [milliSeconds] have passed, then the return value completes in the same way.
+ * However, if [milliSeconds] pass before [input] has completed, it completes
+ * with [exception].
+ */
+Future timeout(Future input, int milliSeconds, exception) {

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

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.

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

Done.

+  var completer = new Completer();
+  var timer = new Timer(milliSeconds, (_) {
+    if (completer.future.isComplete) return;
+    completer.completeException(exception);

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

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]

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]

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

+  });
+  input.handleException((e) {
+    if (completer.future.isComplete) return false;
+    timer.cancel();
+    completer.completeException(e);
+    return true;
+  });
+  input.then((value) {
+    if (completer.future.isComplete) return;
+    timer.cancel();
+    completer.complete(value);
+  });
+  return completer.future;
+}
+
+/**
  * Tests whether or not the git command-line app is available for use.
  */
 Future<bool> get isGitInstalled {
   // TODO(rnystrom): We could cache this after the first check. We aren't right
   // now because Future.immediate() will invoke its callback synchronously.
   // That does bad things in cases where the caller expects futures to always
   // be async. In particular, withGit() in the pub tests which calls
   // expectAsync() will fail horribly if the test isn't actually async.
 
   var completer = new Completer<bool>();
@@ skipping 78 matching lines @@
   return new Directory(entry);
 }
 
 /**
  * Gets a [Uri] for [uri], which can either already be one, or be a [String].
  */
 Uri _getUri(uri) {
   if (uri is Uri) return uri;
   return new Uri.fromString(uri);
 }