Update Transfer-Encoding logic. (dart-lang#66)

This should be the last CL in this unfortunate series. The adapter
requirements have been updated to consistently ensure that a Message's
body is in the chunked format if and only if its headers indicate that
it should be.

Closes dart-lang#64

CHANGELOG.md

@@ -1,3 +1,16 @@
+## 0.6.7+2
+
+* Go back to auto-generating a `Content-Length` header when the length is known
+  ahead-of-time *and* the user hasn't explicitly specified a `Transfer-Encoding:
+  chunked`.
+
+* Clarify adapter requirements around transfer codings.
+
+* Make `shelf_io` consistent with the clarified adapter requirements. In
+  particular, it removes the `Transfer-Encoding` header from chunked requests,
+  and it doesn't apply additional chunking to responses that already declare
+  `Transfer-Encoding: chunked`.
+
 ## 0.6.7+1
 
 * Never auto-generate a `Content-Length` header.

README.md

@@ -98,16 +98,7 @@ or it might pipe requests directly from an HTTP client to a Shelf handler.
 
 [shelf_io.serve]: http://www.dartdocs.org/documentation/shelf/latest/index.html#shelf/shelf-io@id_serve
 
-When implementing an adapter, some rules must be followed. The adapter must not
-pass the `url` or `handlerPath` parameters to [new shelf.Request][]; it should
-only pass `requestedUri`. If it passes the `context` parameter, all keys must
-begin with the adapter's package name followed by a period. If multiple headers
-with the same name are received, the adapter must collapse them into a single
-header separated by commas as per [RFC 2616 section 4.2][].
-
-[new shelf.Request]: http://www.dartdocs.org/documentation/shelf/latest/index.html#shelf/shelf.Request@id_Request-
-
-[RFC 2616 section 4.2]: http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html
+### API Requirements
 
 An adapter must handle all errors from the handler, including the handler
 returning a `null` response. It should print each error to the console if
@@ -118,14 +109,6 @@ don't result in exposing internal information in production by default; if the
 user wants to return detailed error descriptions, they should explicitly include
 middleware to do so.
 
-An adapter should include information about itself in the Server header of the
-response by default. If the handler returns a response with the Server header
-set, that must take precedence over the adapter's default header.
-
-An adapter should include the Date header with the time the handler returns a
-response. If the handler returns a response with the Date header set, that must
-take precedence.
-
 An adapter should ensure that asynchronous errors thrown by the handler don't
 cause the application to crash, even if they aren't reported by the future
 chain. Specifically, these errors shouldn't be passed to the root zone's error
@@ -153,6 +136,55 @@ An adapter that knows its own URL should provide an implementation of the
 
 [server]: http://www.dartdocs.org/documentation/shelf/latest/index.html#shelf/shelf@id_Server
 
+### Request Requirements
+
+When implementing an adapter, some rules must be followed. The adapter must not
+pass the `url` or `handlerPath` parameters to [new shelf.Request][]; it should
+only pass `requestedUri`. If it passes the `context` parameter, all keys must
+begin with the adapter's package name followed by a period. If multiple headers
+with the same name are received, the adapter must collapse them into a single
+header separated by commas as per [RFC 2616 section 4.2][].
+
+[new shelf.Request]: http://www.dartdocs.org/documentation/shelf/latest/index.html#shelf/shelf.Request@id_Request-
+
+[RFC 2616 section 4.2]: http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html
+
+If the underlying request uses a chunked transfer coding, the adapter must
+decode the body before passing it to [new shelf.Request][] and should remove the
+`Transfer-Encoding` header. This ensures that message bodies are chunked if and
+only if the headers declare that they are.
+
+### Response Requirements
+
+An adapter must not add or modify any [entity headers][] for a response.
+
+[entity headers]: https://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html#sec7.1
+
+If *none* of the following conditions are true, the adapter must apply
+[chunked transfer coding][] to a response's body and set its Transfer-Encoding header to `chunked`:
+
+* The status code is less than 200, or equal to 204 or 304.
+* A Content-Length header is provided.
+* The Content-Type header indicates the MIME type `multipart/byteranges`.
+* The Transfer-Encoding header is set to anything other than `identity`.
+
+[chunked transfer coding]: https://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.6.1
+
+Adapters may find the [`addChunkedEncoding()`][addChunkedEncoding] middleware
+useful for implementing this behavior, if the underlying server doesn't
+implement it manually.
+
+When responding to a HEAD request, the adapter must not emit an entity body.
+Otherwise, it shouldn't modify the entity body in any way.
+
+An adapter should include information about itself in the Server header of the
+response by default. If the handler returns a response with the Server header
+set, that must take precedence over the adapter's default header.
+
+An adapter should include the Date header with the time the handler returns a
+response. If the handler returns a response with the Date header set, that must
+take precedence.
+
 ## Inspiration
 
 * [Connect](http://www.senchalabs.org/connect/) for NodeJS.

lib/shelf.dart

@@ -4,7 +4,8 @@
 
 export 'src/cascade.dart';
 export 'src/handler.dart';
-export 'src/handlers/logger.dart';
+export 'src/middleware/add_chunked_encoding.dart';
+export 'src/middleware/logger.dart';
 export 'src/hijack_exception.dart';
 export 'src/middleware.dart';
 export 'src/pipeline.dart';

lib/shelf_io.dart

@@ -18,6 +18,8 @@
 import 'dart:async';
 import 'dart:io';
 
+import 'package:collection/collection.dart';
+import 'package:http_parser/http_parser.dart';
 import 'package:stack_trace/stack_trace.dart';
 
 import 'shelf.dart';
@@ -116,6 +118,9 @@ Request _fromHttpRequest(HttpRequest request) {
     headers[k] = v.join(',');
   });
 
+  // Remove the Transfer-Encoding header per the adapter requirements.
+  headers.remove(HttpHeaders.TRANSFER_ENCODING);
+
   void onHijack(callback) {
     request.response
         .detachSocket(writeHeaders: false)
@@ -136,11 +141,36 @@ Future _writeResponse(Response response, HttpResponse httpResponse) {
 
   httpResponse.statusCode = response.statusCode;
 
+  // An adapter must not add or modify the `Transfer-Encoding` parameter, but
+  // the Dart SDK sets it by default. Set this before we fill in
+  // [response.headers] so that the user or Shelf can explicitly override it if
+  // necessary.
+  httpResponse.headers.chunkedTransferEncoding = false;
+
   response.headers.forEach((header, value) {
     if (value == null) return;
     httpResponse.headers.set(header, value);
   });
 
+  var coding = response.headers['transfer-encoding'];
+  if (coding != null && !equalsIgnoreAsciiCase(coding, 'identity')) {
+    // If the response is already in a chunked encoding, de-chunk it because
+    // otherwise `dart:io` will try to add another layer of chunking.
+    //
+    // TODO(nweiz): Do this more cleanly when sdk#27886 is fixed.
+    response = response.change(
+        body: chunkedCoding.decoder.bind(response.read()));
+    httpResponse.headers.set(HttpHeaders.TRANSFER_ENCODING, 'chunked');
+  } else if (response.statusCode >= 200 &&
+      response.statusCode != 204 &&
+      response.statusCode != 304 &&
+      response.contentLength == null &&
+      response.mimeType != 'multipart/byteranges') {
+    // If the response isn't chunked yet and there's no other way to tell its
+    // length, enable `dart:io`'s chunked encoding.
+    httpResponse.headers.set(HttpHeaders.TRANSFER_ENCODING, 'chunked');
+  }
+
   if (!response.headers.containsKey(HttpHeaders.SERVER)) {
     httpResponse.headers.set(HttpHeaders.SERVER, 'dart:io with Shelf');
   }
@@ -149,9 +179,6 @@ Future _writeResponse(Response response, HttpResponse httpResponse) {
     httpResponse.headers.date = new DateTime.now().toUtc();
   }
 
-  // Work around sdk#27660.
-  if (response.isEmpty) httpResponse.headers.chunkedTransferEncoding = false;
-
   return httpResponse
       .addStream(response.read())
       .then((_) => httpResponse.close());

lib/src/body.dart

@@ -23,12 +23,11 @@ class Body {
   /// encoding was used.
   final Encoding encoding;
 
-  /// If `true`, the stream returned by [read] will be empty.
-  ///
-  /// This may have false negatives, but it won't have false positives.
-  final bool isEmpty;
+  /// The length of the stream returned by [read], or `null` if that can't be
+  /// determined efficiently.
+  final int contentLength;
 
-  Body._(this._stream, this.encoding, this.isEmpty);
+  Body._(this._stream, this.encoding, this.contentLength);
 
   /// Converts [body] to a byte stream and wraps it in a [Body].
   ///
@@ -39,23 +38,25 @@ class Body {
     if (body is Body) return body;
 
     Stream<List<int>> stream;
-    var isEmpty = false;
+    int contentLength;
     if (body == null) {
-      isEmpty = true;
+      contentLength = 0;
       stream = new Stream.fromIterable([]);
     } else if (body is String) {
-      isEmpty = body.isEmpty;
       if (encoding == null) {
         var encoded = UTF8.encode(body);
         // If the text is plain ASCII, don't modify the encoding. This means
-        // that an encoding of "text/plain" without a charset will stay put.
+        // that an encoding of "text/plain" will stay put.
         if (!_isPlainAscii(encoded, body.length)) encoding = UTF8;
+        contentLength = encoded.length;
         stream = new Stream.fromIterable([encoded]);
       } else {
-        stream = new Stream.fromIterable([encoding.encode(body)]);
+        var encoded = encoding.encode(body);
+        contentLength = encoded.length;
+        stream = new Stream.fromIterable([encoded]);
       }
     } else if (body is List) {
-      isEmpty = body.isEmpty;
+      contentLength = body.length;
       stream = new Stream.fromIterable([DelegatingList.typed(body)]);
     } else if (body is Stream) {
       stream = DelegatingStream.typed(body);
@@ -64,7 +65,7 @@ class Body {
           'Stream.');
     }
 
-    return new Body._(stream, encoding, isEmpty);
+    return new Body._(stream, encoding, contentLength);
   }
 
   /// Returns whether [bytes] is plain ASCII.

lib/src/message.dart

@@ -5,6 +5,7 @@
 import 'dart:async';
 import 'dart:convert';
 
+import 'package:collection/collection.dart';
 import 'package:http_parser/http_parser.dart';
 
 import 'body.dart';
@@ -13,6 +14,11 @@ import 'util.dart';
 
 Body getBody(Message message) => message._body;
 
+/// The default set of headers for a message created with no body and no
+/// explicit headers.
+final _defaultHeaders = new ShelfUnmodifiableMap<String>(
+    {"content-length": "0"}, ignoreKeyCase: true);
+
 /// Represents logic shared between [Request] and [Response].
 abstract class Message {
   /// The HTTP headers.
@@ -41,7 +47,7 @@ abstract class Message {
   /// If `true`, the stream returned by [read] won't emit any bytes.
   ///
   /// This may have false negatives, but it won't have false positives.
-  bool get isEmpty => _body.isEmpty;
+  bool get isEmpty => _body.contentLength == 0;
 
   /// Creates a new [Message].
   ///
@@ -144,20 +150,39 @@ abstract class Message {
 Map<String, String> _adjustHeaders(
     Map<String, String> headers, Body body) {
   var sameEncoding = _sameEncoding(headers, body);
-  if (sameEncoding) return headers ?? const ShelfUnmodifiableMap.empty();
+  if (sameEncoding) {
+    if (body.contentLength == null ||
+        getHeader(headers, 'content-length') ==
+            body.contentLength.toString()) {
+      return headers ?? const ShelfUnmodifiableMap.empty();
+    } else if (body.contentLength == 0 &&
+        (headers == null || headers.isEmpty)) {
+      return _defaultHeaders;
+    }
+  }
 
   var newHeaders = headers == null
       ? new CaseInsensitiveMap<String>()
       : new CaseInsensitiveMap<String>.from(headers);
 
-  if (newHeaders['content-type'] == null) {
-    newHeaders['content-type'] =
-        'application/octet-stream; charset=${body.encoding.name}';
-  } else {
-    var contentType = new MediaType.parse(newHeaders['content-type'])
-        .change(parameters: {'charset': body.encoding.name});
-    newHeaders['content-type'] = contentType.toString();
+  if (!sameEncoding) {
+    if (newHeaders['content-type'] == null) {
+      newHeaders['content-type'] =
+          'application/octet-stream; charset=${body.encoding.name}';
+    } else {
+      var contentType = new MediaType.parse(newHeaders['content-type'])
+          .change(parameters: {'charset': body.encoding.name});
+      newHeaders['content-type'] = contentType.toString();
+    }
   }
+
+  if (body.contentLength != null) {
+    var coding = newHeaders['transfer-encoding'];
+    if (coding == null || equalsIgnoreAsciiCase(coding, 'identity')) {
+      newHeaders['content-length'] = body.contentLength.toString();
+    }
+  }
+
   return newHeaders;
 }
 

lib/src/middleware/add_chunked_encoding.dart

@@ -0,0 +1,39 @@
+// Copyright (c) 2016, 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.
+
+import 'package:collection/collection.dart';
+import 'package:http_parser/http_parser.dart';
+
+import '../middleware.dart';
+
+/// Middleware that adds [chunked transfer coding][] to responses if none of the
+/// following conditions are true:
+///
+/// [chunked transfer coding]: https://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.6.1
+///
+/// * A Content-Length header is provided.
+/// * The Content-Type header indicates the MIME type `multipart/byteranges`.
+/// * The Transfer-Encoding header already includes the `chunked` coding.
+///
+/// This is intended for use by [Shelf adapters][] rather than end-users.
+///
+/// [Shelf adapters]: https://github.com/dart-lang/shelf#adapters
+final addChunkedEncoding = createMiddleware(responseHandler: (response) {
+  if (response.contentLength != null) return response;
+  if (response.statusCode < 200) return response;
+  if (response.statusCode == 204) return response;
+  if (response.statusCode == 304) return response;
+  if (response.mimeType == 'multipart/byteranges') return response;
+
+  // We only check the last coding here because HTTP requires that the chunked
+  // encoding be listed last.
+  var coding = response.headers['transfer-encoding'];
+  if (coding != null && !equalsIgnoreAsciiCase(coding, 'identity')) {
+    return response;
+  }
+
+  return response.change(
+      headers: {'transfer-encoding': 'chunked'},
+      body: chunkedCoding.encoder.bind(response.read()));
+});