Add chunked uploads support to SPDY

net/http/http_stream_parser.cc

Index: net/http/http_stream_parser.cc
diff --git a/net/http/http_stream_parser.cc b/net/http/http_stream_parser.cc
index 2d00a26648c60bbb6a4fd13703c17ec58a497a69..a4f026f57dd180e8ed8c3ea8e63093b67fc58323 100644
--- a/net/http/http_stream_parser.cc
+++ b/net/http/http_stream_parser.cc
@@ -6,6 +6,7 @@
 
 #include "base/compiler_specific.h"
 #include "base/metrics/histogram.h"
+#include "base/string_util.h"
 #include "net/base/auth.h"
 #include "net/base/io_buffer.h"
 #include "net/base/ssl_cert_request_info.h"
@@ -39,7 +40,10 @@ HttpStreamParser::HttpStreamParser(ClientSocketHandle* connection,
       connection_(connection),
       net_log_(net_log),
       ALLOW_THIS_IN_INITIALIZER_LIST(
-          io_callback_(this, &HttpStreamParser::OnIOComplete)) {
+          io_callback_(this, &HttpStreamParser::OnIOComplete)),
+      chunk_length_(0),
+      chunk_length_without_encoding_(0),
+      sent_last_chunk_(false) {
   DCHECK_EQ(0, read_buffer->offset());
 }
 
@@ -70,8 +74,12 @@ int HttpStreamParser::SendRequest(const std::string& request_line,
   request_headers_ = new DrainableIOBuffer(headers_io_buf,
                                            headers_io_buf->size());
   request_body_.reset(request_body);
-  if (request_body_ != NULL && request_body_->is_chunked())
+  if (request_body_ != NULL && request_body_->is_chunked()) {
     request_body_->set_chunk_callback(this);
+    const int kChunkHeaderFooterSize = 12;  // 2 CRLFs + max of 8 hex chars.
+    chunk_buf_ = new IOBuffer(request_body_->GetMaxBufferSize() +
+                              kChunkHeaderFooterSize);
+  }
 
   io_state_ = STATE_SENDING_HEADERS;
   int result = DoLoop(OK);
@@ -266,17 +274,54 @@ int HttpStreamParser::DoSendHeaders(int result) {
 }
 
 int HttpStreamParser::DoSendBody(int result) {

[willchan no longer on Chromium, 2011/02/01 23:35:59]

Please add a bunch of assertions to validate this function works correctly. I'll
give some examples below.

I also feel like we have a lot of unnecessary
request_body_->MarkConsumedAndFillBuffer(0) calls. A stricter UploadDataStream
implementation should probably disallow this. Is there a way to rework this code
so we don't have to call it with 0?

[Satish, 2011/02/22 14:25:44]

In the newly added code MarkConsumedAndFillBuffer() is never called with zero.
Can you clarify which code flow you thought results in such calls?

[willchan no longer on Chromium, 2011/02/24 19:03:39]

I don't recall the control flow =/ I looked at don't see it, so maybe I was just
wrong before? I hope I didn't miss it on the re-read.

+  if (request_body_->is_chunked()) {
+    chunk_length_ -= result;
+    if (chunk_length_) {
+      memcpy(chunk_buf_->data(), chunk_buf_->data() + result, chunk_length_);

[willchan no longer on Chromium, 2011/02/01 23:35:59]

You need to use memmove instead of memcpy because you have overlapping memory
regions.

Rather than having to do copies, how about just incrementing an index pointer to
keep track of where you are in the buffer?

[Satish, 2011/02/22 14:25:44]

Done
The net::Socket::Write call in the next line only takes an IOBuffer and writes
the whole buffer out, so there is no easy way to use an index here.. unless I
change that to use a DrainableIOBuffer or something similar which is a larger
change.

[willchan no longer on Chromium, 2011/02/24 19:03:39]

DrainableIOBuffer is the right solution here. Of course, UploadDataStream is
creating the IOBuffer, rather than using a user-provided buffer. UGH.

+      return connection_->socket()->Write(chunk_buf_, chunk_length_,
+                                          &io_callback_);
+    }
+
+    if (sent_last_chunk_) {
+      io_state_ = STATE_REQUEST_SENT;
+      return OK;
+    }
+
+    request_body_->MarkConsumedAndFillBuffer(chunk_length_without_encoding_);
+    chunk_length_without_encoding_ = 0;
+    chunk_length_ = 0;
+
+    int buf_len = static_cast<int>(request_body_->buf_len());
+    if (request_body_->eof()) {
+      chunk_length_ = 5;
+      memcpy(chunk_buf_->data(), "0\r\n\r\n", chunk_length_);
+      sent_last_chunk_ = true;
+    } else if (buf_len) {
+      // Encode and send the buffer as 1 chunk.

[willchan no longer on Chromium, 2011/02/01 23:35:59]

Why don't you use a single sprintf() call? Something like:
sprintf("%X\r\n%*s\r\n", buf_len, buf_len, request_body_->buf()->data())? It's
annoying to have to read the destination pointers each time to make sure you
have the indices calculated correctly.

[Satish, 2011/02/22 14:25:44]

MSDN says that for %s 'characters are printed up to the first null character or
until the precision value is reached' - so if there was a zero byte in the
buffer only so many characters will be printed. Hence I'm doing the memcpy
myself

[willchan no longer on Chromium, 2011/02/24 19:03:39]

Doh, I forgot about the NUL. You're absolutely correct.

My style preference would be for you to chunk_ptr += length, after each memcpy,
but I don't feel strongly. What you have is ok too.

+      std::string chunk_header = StringPrintf("%X\r\n", buf_len);
+      char* chunk_ptr = chunk_buf_->data();
+      memcpy(chunk_ptr, chunk_header.data(), chunk_header.length());
+      memcpy(chunk_ptr + chunk_header.length(), request_body_->buf()->data(),
+             buf_len);
+      memcpy(chunk_ptr + chunk_header.length() + buf_len, "\r\n", 2);
+      chunk_length_without_encoding_ = buf_len;
+      chunk_length_ = chunk_header.length() + buf_len + 2;
+    }
+
+    if (!chunk_length_)  // More POST data is yet to come?
+      return ERR_IO_PENDING;
+
+    return connection_->socket()->Write(chunk_buf_, chunk_length_,
+                                        &io_callback_);
+  }
+
+  // Non-chunked request body.
   request_body_->MarkConsumedAndFillBuffer(result);
 
   if (!request_body_->eof()) {
     int buf_len = static_cast<int>(request_body_->buf_len());
-    if (buf_len) {
-      result = connection_->socket()->Write(request_body_->buf(), buf_len,
-                                            &io_callback_);
-    } else {
-      // More POST data is to come hence wait for the callback.
-      result = ERR_IO_PENDING;
-    }
+    result = connection_->socket()->Write(request_body_->buf(), buf_len,
+                                          &io_callback_);
   } else {
     io_state_ = STATE_REQUEST_SENT;
   }