From b07647c8a8da71b36dc0274939aed35e73da0b3f Mon Sep 17 00:00:00 2001
From: "rnystrom@google.com" <rnystrom@google.com>
Date: Mon, 12 Aug 2013 21:32:26 +0000
Subject: [PATCH] Handle missing files more gracefully in pub serve.
BUG=
R=nweiz@google.com
Review URL: https://codereview.chromium.org//22878002
git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge@26036 260f80e4-7a28-3924-810f-c04153c831b5
---
lib/src/command/serve.dart | 35 +++++++++++++++++++-------
lib/src/utils.dart | 40 ++++++++++++++++++++++++++++++
test/serve/missing_asset_test.dart | 28 +++++++++++++++++++++
test/serve/missing_file_test.dart | 32 ++++++++++++++++++++++--
4 files changed, 124 insertions(+), 11 deletions(-)
create mode 100644 test/serve/missing_asset_test.dart
diff --git a/lib/src/command/serve.dart b/lib/src/command/serve.dart
index ce8db31d..69d92a1f 100644
--- a/lib/src/command/serve.dart
+++ b/lib/src/command/serve.dart
@@ -78,14 +78,14 @@ class ServeCommand extends PubCommand {
// are added or modified.
HttpServer.bind("localhost", port).then((server) {
- log.message("Serving ${entrypoint.root.name} "
- "on http://localhost:${server.port}");
-
// Add all of the visible files.
for (var package in provider.packages) {
barback.updateSources(provider.listAssets(package));
}
+ log.message("Serving ${entrypoint.root.name} "
+ "on http://localhost:${server.port}");
+
server.listen((request) {
var id = getIdFromUri(request.uri);
if (id == null) {
@@ -93,14 +93,31 @@ class ServeCommand extends PubCommand {
}
barback.getAssetById(id).then((asset) {
- log.message(
- "$_green${request.method}$_none ${request.uri} -> $asset");
- // TODO(rnystrom): Set content-type based on asset type.
- return request.response.addStream(asset.read()).then((_) {
+ return validateStream(asset.read()).then((stream) {
+ log.message(
+ "$_green${request.method}$_none ${request.uri} -> $asset");
+ // TODO(rnystrom): Set content-type based on asset type.
+ return request.response.addStream(stream).then((_) {
+ request.response.close();
+ });
+ }).catchError((error) {
+ log.error("$_red${request.method}$_none "
+ "${request.uri} -> $error");
+
+ // If we couldn't read the asset, handle the error gracefully.
+ if (error is FileException) {
+ // Assume this means the asset was a file-backed source asset
+ // and we couldn't read it, so treat it like a missing asset.
+ notFound(request, error);
+ return;
+ }
+
+ // Otherwise, it's some internal error.
+ request.response.statusCode = 500;
+ request.response.reasonPhrase = "Internal Error";
+ request.response.write(error);
request.response.close();
});
- // TODO(rnystrom): Serve up a 500 if we get an error reading the
- // asset.
}).catchError((error) {
log.error("$_red${request.method}$_none ${request.uri} -> $error");
if (error is! AssetNotFoundException) {
diff --git a/lib/src/utils.dart b/lib/src/utils.dart
index 6a048cf1..da8f640e 100644
--- a/lib/src/utils.dart
+++ b/lib/src/utils.dart
@@ -168,6 +168,46 @@ void chainToCompleter(Future future, Completer completer) {
onError: (e) => completer.completeError(e));
}
+/// Ensures that [stream] can emit at least one value successfully (or close
+/// without any values).
+///
+/// For example, reading asynchronously from a non-existent file will return a
+/// stream that fails on the first chunk. In order to handle that more
+/// gracefully, you may want to check that the stream looks like it's working
+/// before you pipe the stream to something else.
+///
+/// This lets you do that. It returns a [Future] that completes to a [Stream]
+/// emitting the same values and errors as [stream], but only if at least one
+/// value can be read successfully. If an error occurs before any values are
+/// emitted, the returned Future completes to that error.
+Future<Stream> validateStream(Stream stream) {
+ var completer = new Completer<Stream>();
+ var controller = new StreamController(sync: true);
+
+ StreamSubscription subscription;
+ subscription = stream.listen((value) {
+ // We got a value, so the stream is valid.
+ if (!completer.isCompleted) completer.complete(controller.stream);
+ controller.add(value);
+ }, onError: (error) {
+ // If the error came after values, it's OK.
+ if (completer.isCompleted) controller.addError(error);
+
+ // Otherwise, the error came first and the stream is invalid.
+ completer.completeError(error);
+
+ // We don't be returning the stream at all in this case, so unsubscribe
+ // and swallow the error.
+ subscription.cancel();
+ }, onDone: () {
+ // It closed with no errors, so the stream is valid.
+ if (!completer.isCompleted) completer.complete(controller.stream);
+ controller.close();
+ });
+
+ return completer.future;
+}
+
// TODO(nweiz): remove this when issue 7964 is fixed.
/// Returns a [Future] that will complete to the first element of [stream].
/// Unlike [Stream.first], this is safe to use with single-subscription streams.
diff --git a/test/serve/missing_asset_test.dart b/test/serve/missing_asset_test.dart
new file mode 100644
index 00000000..1f69b6c4
--- /dev/null
+++ b/test/serve/missing_asset_test.dart
@@ -0,0 +1,28 @@
+// Copyright (c) 2013, the Dart project authors. Please see the AUTHORS d.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.
+
+library pub_tests;
+
+import 'dart:io';
+
+import '../descriptor.dart' as d;
+import '../test_pub.dart';
+import 'utils.dart';
+
+main() {
+ initConfig();
+ integration("responds with a 404 for missing assets", () {
+ d.dir(appPath, [
+ d.appPubspec()
+ ]).create();
+
+ startPubServe();
+ requestShould404("index.html");
+ requestShould404("packages/myapp/nope.dart");
+ requestShould404("assets/myapp/nope.png");
+ requestShould404("dir/packages/myapp/nope.dart");
+ requestShould404("dir/assets/myapp/nope.png");
+ endPubServe();
+ });
+}
diff --git a/test/serve/missing_file_test.dart b/test/serve/missing_file_test.dart
index cfd7d077..693ae553 100644
--- a/test/serve/missing_file_test.dart
+++ b/test/serve/missing_file_test.dart
@@ -4,8 +4,10 @@
library pub_tests;
-import 'dart:io';
+import 'package:path/path.dart' as path;
+import 'package:scheduled_test/scheduled_test.dart';
+import '../../lib/src/io.dart';
import '../descriptor.dart' as d;
import '../test_pub.dart';
import 'utils.dart';
@@ -14,10 +16,36 @@ main() {
initConfig();
integration("responds with a 404 for missing files", () {
d.dir(appPath, [
- d.appPubspec()
+ d.appPubspec(),
+ d.dir("asset", [
+ d.file("nope.png", "nope")
+ ]),
+ d.dir("lib", [
+ d.file("nope.dart", "nope")
+ ]),
+ d.dir("web", [
+ d.file("index.html", "<body>"),
+ ])
]).create();
+ // Start the server with the files present so that it creates barback
+ // assets for them.
startPubServe();
+
+ // TODO(rnystrom): When pub serve supports file watching, we'll have to do
+ // something here to specifically disable that so that we can get barback
+ // into the inconsistent state of thinking there is an asset but where the
+ // underlying file does not exist. One option would be configure barback
+ // with an insanely long delay between polling to ensure a poll doesn't
+ // happen.
+
+ // Now delete them.
+ schedule(() {
+ deleteEntry(path.join(sandboxDir, appPath, "asset", "nope.png"));
+ deleteEntry(path.join(sandboxDir, appPath, "lib", "nope.dart"));
+ deleteEntry(path.join(sandboxDir, appPath, "web", "index.html"));
+ }, "delete files");
+
requestShould404("index.html");
requestShould404("packages/myapp/nope.dart");
requestShould404("assets/myapp/nope.png");
--
GitLab