Added chunked upload support for embedded_test_server

net/test/embedded_test_server/http_request.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/test/embedded_test_server/http_request.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 #include "base/strings/string_util.h"
@@ skipping 116 matching lines @@
         std::string header_value = Trim(header_line.substr(
             delimiter_pos + 1,
             header_line.size() - delimiter_pos - 1));
         http_request_->headers[header_name] = header_value;
       }
     }
   }
 
   // Headers done. Is any content data attached to the request?
   declared_content_length_ = 0;
-  if (http_request_->headers.count("Content-Length") > 0) {
+  if (http_request_->headers.count("Transfer-Encoding") > 0) {

[mmenke, 2015/02/12 15:34:30]

Suggest flipping the order - ignoring a Content-Length tag when there's a
tranfer-encoding other than chunked seems a little weird (Admittedly, the server
isn't doing what it should in that case, anyways)

[xunjieli, 2015/02/12 15:58:40]

Done.

+    if (http_request_->headers["Transfer-Encoding"] == "chunked") {
+      http_request_->has_content = true;
+      chunked_decoder_.reset(new HttpChunkedDecoder());
+      state_ = STATE_CONTENT;
+      return WAITING;
+    }
+  } else if (http_request_->headers.count("Content-Length") > 0) {
     http_request_->has_content = true;
     const bool success = base::StringToSizeT(
         http_request_->headers["Content-Length"],
         &declared_content_length_);
     DCHECK(success) << "Malformed Content-Length header's value.";
   }
   if (declared_content_length_ == 0) {
     // No content data, so parsing is finished.
     state_ = STATE_ACCEPTED;
     return ACCEPTED;
   }
 
   // The request has not yet been parsed yet, content data is still to be
   // processed.
   state_ = STATE_CONTENT;
   return WAITING;
 }
 
 HttpRequestParser::ParseResult HttpRequestParser::ParseContent() {
   const size_t available_bytes = buffer_.size() - buffer_position_;
+  if (chunked_decoder_.get()) {

[mmenke, 2015/02/12 15:34:30]

nit:  The ".get()" isn't needed

[xunjieli, 2015/02/12 15:58:40]

Done.

+    std::string result;
+    result.append(buffer_.data() + buffer_position_, available_bytes);
+    int bytes_written = chunked_decoder_->FilterBuf(
+        &result[0], static_cast<int>(result.size()));

[mmenke, 2015/02/12 15:34:30]

Why do we need result?  Can't we just use:

bytes_written = chunked_decoder_->FilterBuf(buffer_.data() + buffer_position_,
available_bytes);?
http_request_->content.append(buffer_.data() + buffer_position_, bytes_written);
buffer_position_ += bytes_written;

(Note that this is odd code - we can either not apparent each read until the
end, or we can just always read from the start of the buffer, but fixing that is
beyond the scope of this CL)

[xunjieli, 2015/02/12 15:58:40]

When I tried "chunked_decoder_->FilterBuf(buffer_.data() + buffer_position_,
available_bytes)", it complains that buffer_.data() + buffer_position_ is a
const char*, but it expects a char*.

I tried to do "buffer_position += bytes_written" every time, but it seems like
FilterBuf can only parse data if the data is prefixed with size, so we have to
do FilterBuf from the start of the buffer..

[mmenke, 2015/02/12 16:17:44]

That's because buffer_ is a string (I had assumed it was an io_buffer_).  I
suggest switching it to a std::vector<char>, though I haven't looked into all
the implications of doing so.

+    if (bytes_written > 0 && chunked_decoder_->reached_eof()) {
+      buffer_position_ += bytes_written;

[mmenke, 2015/02/12 15:34:30]

This should be outside of the if block.  The fact tests didn't catch that means
we should probably have another test - slowly append upload data on a timer? 
Normally not a big fan of timers, but appending a 1 character chunk ever 10
milliseconds a couple times doesn't seem to bad.

[xunjieli, 2015/02/12 15:58:40]

It looks like FilterBuf can only parse chunked data if it is prefixed with
chunked size, so we need to parse from the start of the buffer. Therefore, I
incremented buffer_position, only when we reached eof.

[mmenke, 2015/02/12 16:17:44]

You're misreading the code.  FilterBuf is a streaming function, and buffers data
if it doesn't have a complete chunked.

If you call it on a partial chunk header...FilterBuf("100"), it will fill the
buffer with the partial data...And then if you call it again with "100\r\nBlah",
it will incorrectly interpret that data as the continuation of the partial chunk
you already passed it - i.e., it will think you've received "100100\r\nBlah". 
Note that once it's parsed the chunk length, it will just stream the data to you
(in the earlier case, you'd call FilterBuf("100"), it would read nothing, then
you'd call FilterBuf("\r\nBlah"), and it would set the buffer to "Blah" and
return 4, so it never needs more space than the number of bytes you're passing
in to it)

It's meant to be used incrementally...If we're receiving a 15 GB chunked
download, we can't cache the entire file in memory.

+      http_request_->content.append(result.data(), bytes_written);
+      state_ = STATE_ACCEPTED;
+      return ACCEPTED;
+    }
+    state_ = STATE_CONTENT;
+    return WAITING;
+  }
+
   const size_t fetch_bytes = std::min(
       available_bytes,
       declared_content_length_ - http_request_->content.size());
   http_request_->content.append(buffer_.data() + buffer_position_,
                                 fetch_bytes);
   buffer_position_ += fetch_bytes;
 
   if (declared_content_length_ == http_request_->content.size()) {
     state_ = STATE_ACCEPTED;
     return ACCEPTED;
@@ skipping 30 matching lines @@
     return METHOD_DELETE;
   } else if (token == "patch") {
     return METHOD_PATCH;
   }
   NOTREACHED() << "Method not implemented: " << token;
   return METHOD_UNKNOWN;
 }
 
 }  // namespace test_server
 }  // namespace net