HTTP server for testing Google Drive.

chrome/browser/chromeos/gdata/test_servers/http_request.cc

Index: chrome/browser/chromeos/gdata/test_servers/http_request.cc
diff --git a/chrome/browser/chromeos/gdata/test_servers/http_request.cc b/chrome/browser/chromeos/gdata/test_servers/http_request.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c6e9fe1ce63e5341173120582e2c2991df339e6b
--- /dev/null
+++ b/chrome/browser/chromeos/gdata/test_servers/http_request.cc
@@ -0,0 +1,273 @@
+// 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 "chrome/browser/chromeos/gdata/test_servers/http_request.h"
+
+#include <algorithm>
+#include <map>
+#include <string>
+#include "base/basictypes.h"
+#include "base/logging.h"
+#include "googleurl/src/gurl.h"
+
+namespace gdata {
+namespace test_servers {
+
+namespace {
+int kRequestSizeLimit = 64 * 1024 * 1024;  // 64 mb.

[satorux1, 2012/10/16 03:12:18]

This server is used only for testing, so I think we don't need this.

[mtomasz, 2012/11/08 13:29:59]

Discussed offline, leaving as it is.

+}  // namespace
+
+HttpRequestParser::HttpRequestParser() : request_builder_(new HttpRequest()),
+                                         state_(REQUEST_LINE),
+                                         buffer_position_(0),
+                                         crlf_position_(-1),
+                                         crlf_checked_position_(0),
+                                         current_content_length_(0) {
+}
+
+HttpRequestParser::~HttpRequestParser() {
+}
+
+void HttpRequestParser::ProcessChunk(const char *data, int length) {
+  buffer_.append(data, length);
+  if (static_cast<int>(buffer_.length()) + length > kRequestSizeLimit) {
+    // Too big request. Treat it as a syntax error.
+    LOG(ERROR) << "The HTTP request is too large.";
+    state_ = SYNTAX_ERROR;
+    return;
+  }
+  if (crlf_position_ == -1 && state_ != DATA)
+    FindNextCrLf();

[satorux1, 2012/10/16 06:39:48]

I was confused about this. Why do we want to find CRLF here? 

I guess the answer is to ensure that we are ready to parse a header.

However, even if we find a CRLF here, it's not yet ready to parse a header as
values can be multi-line.

[mtomasz, 2012/11/08 13:29:59]

You were right. Parser rewritten and simplified. Done.

+}
+
+HttpRequestParser::STATE HttpRequestParser::ParseRequest() {

[satorux1, 2012/10/16 03:12:18]

The state machine code looks efficient, but for the test server, we'd prefer
simpler code than faster code. Would it be possible to simplify the code like:

0) assuming |buffer_| contains the entire HTTP request,
1) process the first line (method)
2) find the first blank line (the separator between the headers and the body)
3) go through headers and process each header (be wary of multi-line headers)
3.1) split key and value using SplitString(s, ':') and add it to map
4) copy the body

I think we don't need a state machine if we are to go with simpler code

[mtomasz, 2012/11/08 13:29:59]

Done. Please check the new parser.

+  while (state_ != READY &&
+         state_ != SYNTAX_ERROR &&
+         ShouldParseBuffer()) {
+    // StringPiece is converted to String, since a copy is needed in most cases
+    // to perform upper/lower case conversion.
+    std::string token;
+    switch (state_) {
+      case REQUEST_LINE:
+        // Parse a method, eg. GET.
+        token = ShiftToken(IDENTIFIER).as_string();
+        if (token.empty()) {
+          state_ = SYNTAX_ERROR;
+          break;
+        }
+        std::transform(token.begin(), token.end(), token.begin(), ::toupper);

[satorux1, 2012/10/16 06:39:48]

StringToUpperASCII()

[mtomasz, 2012/11/08 13:29:59]

Done.

+        if (token == "GET") {
+          request_builder_->method = GET;
+        } else if (token == "HEAD") {
+          request_builder_->method = HEAD;
+        } else if (token == "POST") {
+          request_builder_->method = POST;
+        } else if (token == "PUT") {
+          request_builder_->method = PUT;
+        } else if (token == "DELETE") {
+          request_builder_->method = DELETE;
+        } else {
+          // TODO(mtomasz): Implement other methods.

[satorux1, 2012/10/16 06:39:48]

You can do

NOTREACHED() << "Unsupported method: " << token

[mtomasz, 2012/11/08 13:29:59]

Done.

+          request_builder_->method = CUSTOM;
+        }

[satorux1, 2012/10/16 06:39:48]

Please create a function to convert string to method type like

request_builder_->method = GetMethodType(token);

[mtomasz, 2012/11/08 13:29:59]

Done.

+
+        // Parse an URL, eg. index.cgi.
+        token = ShiftToken(IDENTIFIER).as_string();
+        if (token.empty()) {
+          LOG(ERROR) << "The url token must not be empty.";
+          state_ = SYNTAX_ERROR;
+          break;
+        }
+        request_builder_->raw_url = token;

[satorux1, 2012/10/16 06:39:48]

raw_url seems to be a misnomer. This is path + query, right? path_and_query may
be more descriptive. 

[mtomasz, 2012/11/08 13:29:59]

Path_and_query looks to be quite long. How about just uri?

+
+        // Parse a protocol, eg. HTTP/1.1.
+        // Currently we support only HTTP/1.1, since HTTP/1.0 is quite obsolete.
+        token = ShiftToken(LINE).as_string();
+        std::transform(token.begin(), token.end(), token.begin(), ::tolower);
+        if (token.empty() || token != "http/1.1") {
+          LOG(ERROR) << "This protocol is not supported: [" << token << "]";
+          state_ = SYNTAX_ERROR;
+          break;
+        }
+        request_builder_->protocol = token;
+        state_ = HEADER_LINE;
+        break;
+      case HEADER_LINE:
+        // If we have an empty line, then finish parsing headers and validate
+        // the request.
+        if (ShiftCrLf()) {
+          if (request_builder_->headers.find("host") ==
+              request_builder_->headers.end()) {
+            LOG(ERROR) << "Host header is required for HTTP/1.1";
+            state_ = SYNTAX_ERROR;

[satorux1, 2012/10/16 06:39:48]

I think you don't have to check this. We can assume our clients do this right
(i.e. we are not interested in testing the low-level networking stack).

[mtomasz, 2012/11/08 13:29:59]

Done.

+            break;
+          } else {
+            // TODO(mtomasz): We add http:// protocol, but how about https://?
+            // Do we want to support it?
+            GURL host = GURL("http://" + request_builder_->headers["host"]);

[satorux1, 2012/10/16 06:39:48]

Let's just use http://localhost for now

[mtomasz, 2012/11/08 13:29:59]

Done.

+            request_builder_->url = host.Resolve(request_builder_->raw_url);
+          }
+          if (request_builder_->headers.find("content-length") !=
+              request_builder_->headers.end()) {
+            current_content_length_ =
+                atoi(request_builder_->headers["content-length"].c_str());
+          }
+          if (current_content_length_ > 0) {
+            state_ = DATA;
+          } else {
+            state_ = READY;

[satorux1, 2012/10/16 06:39:48]

There is something called "chunked encoding" but we don't need to support it.
Please add some comment saying

// Chunked encoding is not supported.

[mtomasz, 2012/11/08 13:29:59]

Done in the header file.

+          }
+          break;
+        }
+        {
+          // Parse a header name, eg. Content-length.
+          std::string header_name = ShiftToken(HEADER_KEY).as_string();

[satorux1, 2012/10/16 06:39:48]

This assumes that |buffer_| contains the key name entirely. 

Would it be possible that |buffer_| ends in the middle of a header key?

[mtomasz, 2012/11/08 13:29:59]

I don't think so. We were parsing headers line by line.

+          std::transform(header_name.begin(),
+                         header_name.end(),
+                         header_name.begin(),
+                         ::tolower);

[satorux1, 2012/10/16 06:39:48]

StringToLowerASCII

[mtomasz, 2012/11/08 13:29:59]

Done.

+          if (header_name.empty()) {
+            LOG(ERROR) << "Header key must not be empty.";
+            state_ = SYNTAX_ERROR;
+            break;
+          }
+
+          // Parse a header value, eg. 100 (for Content-Length).
+          StringPiece header_value = ShiftToken(LINE);

[satorux1, 2012/10/16 06:39:48]

I think the multi-line values are not supported yet, right?

multi-line headers are fairly common so not supporting it is bad.

[mtomasz, 2012/11/08 13:29:59]

Rewritten parser. Not it is supported. Done.

+          request_builder_->headers[header_name] = header_value.as_string();
+          break;
+        }
+      case DATA:
+        if (current_content_length_ ==
+            static_cast<int>(request_builder_->content.length())) {
+          // Entire data received.
+          state_ = READY;
+        } else {
+          int to_read = current_content_length_ -
+              request_builder_->content.length();
+          if (to_read > static_cast<int>(request_builder_->content.length()) -
+              buffer_position_) {

[satorux1, 2012/10/16 06:39:48]

I'm confused. What does the following value mean?

  request_builder_->content.length() - buffer_position_


I thought you wanted to compute the remaining data in buffer_. If so, don't you
want to do this instead:

  const int available_bytes = buffer_.size() - buffer_position_;
  to_read = std::min(to_read, available_bytes);

If you switch from 'int' to StringPiece, the code can be as simple as:

   to_read = std::min(to_read, buffer_position_.size());

[mtomasz, 2012/11/08 13:29:59]

You were right. Removed. Done.

+            to_read = request_builder_->content.length() - buffer_position_;
+          }
+          StringPiece chunk = ShiftData(to_read);
+          DCHECK(chunk.length() > 0);
+          request_builder_->content.append(chunk.data(), chunk.length());
+        }
+        break;
+      case SYNTAX_ERROR:
+        break;
+      default:
+        state_ = SYNTAX_ERROR;
+        break;
+    }
+  }
+  return state_;
+}
+
+scoped_ptr<HttpRequest> HttpRequestParser::GetRequest() {
+  DCHECK(state_ == READY);
+  scoped_ptr<HttpRequest> result = request_builder_.Pass();
+  request_builder_.reset(new HttpRequest());
+
+  // Reset the parser state.
+  state_ = REQUEST_LINE;
+  buffer_position_ = 0;
+  buffer_.clear();
+  crlf_position_ = -1;
+  crlf_checked_position_ = 0;
+  current_content_length_ = 0;

[satorux1, 2012/10/16 06:39:48]

Please add Reset() function that resets everything.

[mtomasz, 2012/11/08 13:29:59]

Do we need it? Now there is less stuff there. Also, such Reset method would
never be called in any other situation.

+
+  return result.Pass();
+}
+
+const StringPiece HttpRequestParser::ShiftToken(SHIFT_TOKEN_TYPE token_type) {
+  SkipSpaces();  // Ignore any spaces from the beginning.
+  size_t found_position;

[satorux1, 2012/10/16 06:39:48]

please remove this here.

[mtomasz, 2012/11/08 13:29:59]

Done.

+  int token_start_position = buffer_position_;
+
+  switch (token_type) {
+    case IDENTIFIER:
+      found_position = buffer_.find(' ', buffer_position_);

[satorux1, 2012/10/16 06:39:48]

const size_t found_position = ..

[mtomasz, 2012/11/08 13:29:59]

Done.

+      if (found_position != std::string::npos) {
+        buffer_position_ = static_cast<int>(found_position) + 1;
+        return StringPiece(buffer_.data() + token_start_position,
+                           found_position - token_start_position);
+      }
+      break;
+    case HEADER_KEY:
+      found_position = buffer_.find(':', buffer_position_);
+      if (found_position != std::string::npos) {
+        buffer_position_ = static_cast<int>(found_position) + 1;
+        return StringPiece(buffer_.data() + token_start_position,
+                           found_position - token_start_position);
+      }
+      break;
+    case LINE:
+      DCHECK(crlf_position_ != -1) << "CRLF not found, but expected.";
+      buffer_position_ = crlf_position_ + 2;
+      int token_length = crlf_position_ - token_start_position;

[satorux1, 2012/10/16 06:39:48]

I'm confused about this. Why don't we find the CRLF here?

If we don't need to special case the LINE, we can generalize this function like

const StringPiece HttpRequestParser::ConsumeUntil(const std::string& separator)

and the client code will look like

std::string header_name = ConsumeUntil(":").as_string();

[mtomasz, 2012/11/08 13:29:59]

This was to keep the parser's complexity O(n). Now it is O(n^2) without such
tricks.

+      FindNextCrLf();
+      return StringPiece(buffer_.data() + token_start_position, token_length);
+      break;
+  }
+
+  return StringPiece();
+}
+
+bool HttpRequestParser::ShiftCrLf() {
+  if (buffer_position_ == crlf_position_) {
+    buffer_position_ += 2;
+    FindNextCrLf();
+    return true;
+  }
+
+  return false;
+}
+
+const StringPiece HttpRequestParser::ShiftData(int length) {
+  if (length > static_cast<int>(request_builder_->content.length()) -
+      buffer_position_) {
+    return StringPiece();
+  }
+
+  const StringPiece result(buffer_.data() + buffer_position_, length);
+  buffer_position_ += length;
+
+  return result;
+}
+
+void HttpRequestParser::SkipSpaces() {
+  StringPiece buffer_data(buffer_.data() + buffer_position_,
+                          buffer_.length() - buffer_position_);
+
+  // Ignore any spaces from the beginning.
+  while (buffer_data.length() && buffer_data[0] == ' ') {
+    buffer_data.remove_prefix(1);
+  }
+}
+
+bool HttpRequestParser::ShouldParseBuffer() {
+  return buffer_position_ < static_cast<int>(buffer_.length()) &&
+      (crlf_position_ != -1 || state_ == DATA);
+}
+
+bool HttpRequestParser::FindNextCrLf() {
+  if (!buffer_.length()) {
+    return false;
+  }
+
+  size_t found_position = buffer_.find("\r\n", crlf_checked_position_);
+  if (found_position == std::string::npos) {
+    crlf_position_ = -1;
+    crlf_checked_position_ = buffer_.length() - 1;
+    return false;
+  }
+
+  crlf_position_ = static_cast<int>(found_position);
+  crlf_checked_position_ = crlf_position_ + 2;
+  return true;
+}
+
+}  // namespace test_servers
+}  // namespace gdata