Add chunked uploads support to SPDY

net/http/http_stream_parser.cc

 // Copyright (c) 2010 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/http/http_stream_parser.h"
 
 #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"
 #include "net/http/http_net_log_params.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_request_info.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_util.h"
 #include "net/socket/ssl_client_socket.h"
 #include "net/socket/client_socket_handle.h"
@@ skipping 13 matching lines @@
       response_header_start_offset_(-1),
       response_body_length_(-1),
       response_body_read_(0),
       chunked_decoder_(NULL),
       user_read_buf_(NULL),
       user_read_buf_len_(0),
       user_callback_(NULL),
       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());
 }
 
 HttpStreamParser::~HttpStreamParser() {
   if (request_body_ != NULL && request_body_->is_chunked())
     request_body_->set_chunk_callback(NULL);
 }
 
 int HttpStreamParser::SendRequest(const std::string& request_line,
                                   const HttpRequestHeaders& headers,
@@ skipping 10 matching lines @@
         NetLog::TYPE_HTTP_TRANSACTION_SEND_REQUEST_HEADERS,
         make_scoped_refptr(new NetLogHttpRequestParameter(
             request_line, headers)));
   }
   response_ = response;
   std::string request = request_line + headers.ToString();
   scoped_refptr<StringIOBuffer> headers_io_buf(new StringIOBuffer(request));
   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);
   if (result == ERR_IO_PENDING)
     user_callback_ = callback;
 
   return result > 0 ? OK : result;
 }
 
 int HttpStreamParser::ReadResponseHeaders(CompletionCallback* callback) {
@@ skipping 173 matching lines @@
   } else if (request_body_ != NULL &&
              (request_body_->is_chunked() || request_body_->size())) {
     io_state_ = STATE_SENDING_BODY;
     result = OK;
   } else {
     io_state_ = STATE_REQUEST_SENT;
   }
   return 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;
   }
   return result;
 }
 
 int HttpStreamParser::DoReadHeaders() {
   io_state_ = STATE_READ_HEADERS_COMPLETE;
 
   // Grow the read buffer if necessary.
@@ skipping 364 matching lines @@
 void HttpStreamParser::GetSSLCertRequestInfo(
     SSLCertRequestInfo* cert_request_info) {
   if (request_->url.SchemeIs("https") && connection_->socket()) {
     SSLClientSocket* ssl_socket =
         static_cast<SSLClientSocket*>(connection_->socket());
     ssl_socket->GetSSLCertRequestInfo(cert_request_info);
   }
 }
 
 }  // namespace net