HTTP server for testing Google Drive.

chrome/browser/chromeos/drive/test_servers/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 "chrome/browser/chromeos/drive/test_servers/http_request.h"
+
+#include <algorithm>
+#include <map>
+#include <string>
+#include "base/basictypes.h"
+#include "base/logging.h"
+#include "base/string_util.h"
+#include "googleurl/src/gurl.h"
+
+
+namespace drive {
+namespace test_servers {
+
+namespace {
+size_t kRequestSizeLimit = 64 * 1024 * 1024;  // 64 mb.
+}  // namespace
+
+HttpRequest::HttpRequest() : method(UNKNOWN) {
+}
+
+HttpRequest::~HttpRequest() {
+}
+
+HttpRequestParser::HttpRequestParser() : http_request_(new HttpRequest()),
+                                         state_(STATE_HEADERS),
+                                         buffer_position_(0),
+                                         current_content_length_(0) {

[satorux1, 2012/11/12 06:07:00]

Maybe:

HttpRequestParser::HttpRequestParser()
    : http_request_(new HttpRequest()),
      .... {
}

[mtomasz, 2012/11/12 12:17:44]

Done.

+}
+
+HttpRequestParser::~HttpRequestParser() {
+}
+
+void HttpRequestParser::ProcessChunk(const char *data, size_t length) {
+  buffer_.append(data, length);

[satorux1, 2012/11/12 06:07:00]

If you use StringPiece for buffer_position_, it should also be updated here:

buffer_position_.set(buffer_position_.data(),
                     buffer_position_.size() + length,
                     

[mtomasz, 2012/11/12 12:17:44]

I am not sure if I understand correctly. So, we will have two variables:
std::string buffer_ and StringPiece buffer_position_ (an iterator on buffer_)?
Isn't it then bug prone? Eg. when we do buffer_ = "", then working on
buffer_position_ would sigsegv. Also, after appending to buffer_, its internal
array may be reallocated causing a sigsegv in buffer_position_. Please correct
me if I am wrong.

[satorux1, 2012/11/13 05:13:18]

Yes, but it's no worse than now. We already have two.
This is also unsafe with the current code. buffer_position_ should be set to 0
in this case, right?
That's a good point, but this is the only place where data is appended to
buffer_, so I think it's not very error-prone.

StringPiece carries two states: the current position and the remaining length.
Here, the remaining space is extended, so we should update StringPiece here.

On the other hand, size_t carries only a single state, so you need to compute
the remaining length, which I think is tricky. Overall, I think StringPiece
allows us to write cleaner code. What do you think?

[mtomasz, 2012/11/13 12:23:07]

I am still not convinced. To append, the code will look very tricky.

void HttpRequestParser::ProcessChunk(const base::StringPiece& data) {
  // Pointer arithmetic.
  size_t last_index = buffer_.data() - buffer_position_.data();
  // (or tricky last_index = buffer_.size() - buffer_position_.size())

  data.AppendToString(buffer_);

  // Recreate buffer_position_.
  buffer_position_.set(buffer_.data() + last_index,
                       buffer_position.size() + data.size());
}

Also, if we accidentally call any non-const method on the buffer_ member, or
data() or c_str() anywhere outside outside ProcessChunk(), then we may get a
segfault.

If you see any safer way to implement it with StringPiece then I'll change it.

How about writing a small HttpRequestBuffer class, which would support appending
and fetching lines, or chunk of bytes?

[satorux1, 2012/11/14 01:54:31]

You are right that this is tricky. Let's keep buffer_postion_ as size_t.

However, we can change the parameter to StringPiece. The first line will be

- buffer_.append(data, length);
+ buffer_.append(data.data(), data.size());

+  DCHECK(buffer_.length() + length <= kRequestSizeLimit) <<

[satorux1, 2012/11/12 06:07:00]

DCHECK_LE

[mtomasz, 2012/11/12 12:17:44]

Why is it better? Done.

[satorux1, 2012/11/13 05:13:18]

The failure message is better. You'll see the two values.

[mtomasz, 2012/11/13 12:23:07]

Got it.

+      "The HTTP request is too large.";
+}
+
+std::string HttpRequestParser::ShiftLine() {
+  size_t eoln_position = buffer_.find("\r\n", buffer_position_);
+  if (eoln_position == std::string::npos)
+    return "";
+  const int line_length = eoln_position - buffer_position_;
+  std::string result = buffer_.substr(buffer_position_, line_length);
+  buffer_position_ += line_length + 2;
+  return result;

[satorux1, 2012/11/12 06:07:00]

I think it'd be simpler to use StringPiece for buffer_position_. The code will
look like:

size_t line_length =  buffer_position_.find("\r\n");
if (line_length == std::string::npos)
  return "";
std::string result = buffer_position_.substr(line_length).as_string();
buffer_position_.remove_prefix(line_length + 2_;
return result;

[mtomasz, 2012/11/12 12:17:44]

See above. This would be great if we could easily append.

+}
+
+HttpRequestParser::ParseResult HttpRequestParser::ParseRequest() {
+  DCHECK(state_ != STATE_ACCEPTED);

[satorux1, 2012/11/12 06:07:00]

DCHECK_NE

[mtomasz, 2012/11/12 12:17:44]

Done.

+  // Parse the request from beginning. However, entire request may not be
+  // available in the buffer.
+  if (state_ == STATE_HEADERS) {
+    if (ParseHeaders() == ACCEPTED)
+      return ACCEPTED;
+  }
+  if (state_ == STATE_CONTENT) {
+    if (ParseContent() == ACCEPTED)
+      return ACCEPTED;
+  }
+  return WAITING;
+}
+
+HttpRequestParser::ParseResult HttpRequestParser::ParseHeaders() {
+  // Check if the whole request input is available.

[satorux1, 2012/11/12 06:07:00]

request input is -> request headers are 

[mtomasz, 2012/11/12 12:17:44]

Done.

+  if (buffer_.find("\r\n\r\n", buffer_position_) == std::string::npos)

[satorux1, 2012/11/12 06:07:00]

if you use StringPiece: buffer_position_.find("\r\n\r\n")

[mtomasz, 2012/11/12 12:17:44]

Ack. Let's discuss offline.

+    return WAITING;
+
+  // Parse request's the first header line.
+  // Request main main header, eg. GET /foobar.html HTTP/1.1
+  {
+    const std::string header_line = ShiftLine();
+    std::vector<std::string> header_line_tokens;
+    Tokenize(header_line, " ", &header_line_tokens);

[satorux1, 2012/11/12 06:07:00]

Can we use SplitString() instead? SplitString() is more commonly used, and the
name is more obvious about what it does. :)

[mtomasz, 2012/11/12 12:17:44]

Done!

+    DCHECK(header_line_tokens.size() == 3);
+    // Method.
+    http_request_->method = GetMethodType(StringToLowerASCII(
+        header_line_tokens[0]));
+    // Address.
+    const GURL host = GURL("http://localhost/");
+    http_request_->uri = host.Resolve(header_line_tokens[1]);
+    // Protocol.
+    const std::string protocol = StringToLowerASCII(header_line_tokens[2]);
+    CHECK(protocol == "http/1.0" || protocol == "http/1.1") <<
+        "Protocol not supported: " << protocol;
+  }
+
+  // Parse further headers.
+  {
+    std::string header_line = ShiftLine();
+    std::string header_name;
+    while (header_line != "") {

[satorux1, 2012/11/12 06:07:00]

!header_line.empty()

[mtomasz, 2012/11/12 12:17:44]

Done.

+      if (header_line[0] == ' ') {

[satorux1, 2012/11/12 06:07:00]

|| header_line[0] == '\t'

[mtomasz, 2012/11/12 12:17:44]

Done.

+        // Continuation of the previous multi-line header.
+        http_request_->headers[header_name] += "\n" +

[satorux1, 2012/11/12 06:07:00]

IIRC, I think white spaces should be interpreted as one space,

[mtomasz, 2012/11/12 12:17:44]

You're right. Done.

+            header_line.substr(1, header_line.length() - 1);

[satorux1, 2012/11/12 06:07:00]

Please use size(). I think size() is more widely used in Chrome code base.

[mtomasz, 2012/11/12 12:17:44]

Done.

+      } else {
+        // New header.
+        size_t delimiter_pos = header_line.find(":");
+        DCHECK(delimiter_pos != std::string::npos) << "Syntax error.";
+        header_name = header_line.substr(0, delimiter_pos);
+        size_t header_value_pos = delimiter_pos + 1;
+        // Skip spaces (if any) after the colon.
+        while (header_value_pos < header_line.length() &&
+            header_line[header_value_pos] == ' ') {
+          header_value_pos++;
+        }
+        http_request_->headers[header_name] =
+            header_line.substr(header_value_pos,
+                               header_line.length() - header_value_pos);
+      }
+      header_line = ShiftLine();

[satorux1, 2012/11/12 06:07:00]

Maybe the loop is easier to read with:

while (true) {
  header_line = ShiftLine();
  if (header_line.empty())
    break;
  ...
}

so ShiftLine is called only at one place.

[mtomasz, 2012/11/12 12:17:44]

Done.

+    }
+  }
+
+  // Headers done. Is any content data attached to the request?
+  if (http_request_->headers.find("Content-Length") !=

[satorux1, 2012/11/12 06:07:00]

more like a matter of taste, but the following is shorter:

http_request_->headers.count("Content-Length") > 0

[mtomasz, 2012/11/12 12:17:44]

Done.

+      http_request_->headers.end()) {
+    current_content_length_ =
+        atoi(http_request_->headers["Content-Length"].c_str());

[satorux1, 2012/11/12 06:07:00]

Please use StringToSizeT in base/string_number_conversions.h 

[mtomasz, 2012/11/12 12:17:44]

Done.

+    if (!current_content_length_)

[satorux1, 2012/11/12 06:07:00]

current_content_length_ == 0

[mtomasz, 2012/11/12 12:17:44]

Done.

+      return ACCEPTED;
+  }

[satorux1, 2012/11/12 06:07:00]

If Content-Length: is not present, I think we can assume that the content data
is not present, hence:

} else {
  return ACCEPTED;
}

[mtomasz, 2012/11/12 12:17:44]

Right! Done.

+
+  // The request has not yet been parsed, content data is still to be parsed.
+  state_ = STATE_CONTENT;
+  return WAITING;
+}
+
+HttpRequestParser::ParseResult HttpRequestParser::ParseContent() {
+  const size_t available_bytes = buffer_.length() - buffer_position_;
+  const size_t fetch_bytes = std::min(available_bytes,
+                                      current_content_length_);

[satorux1, 2012/11/12 06:07:00]

Are you trying to limit the size of |http_request_->content| to be at most
|current_content_length_| bytes? If so, did you mean:

std::min(available_bytes,
         current_content_length - http_request_->content.size())

However, I don't know if we want to handle this case. Maybe a DCHECK would
suffice:

http_request_->content.append(...);
DCHECK_LE(http_request_->content.size(), current_content_length);

[mtomasz, 2012/11/12 12:17:44]

Fixed std::min(). Done.

As for the DCHECK, I think it may not work since it is quite possible that we
get two requests in one chunk.

+  http_request_->content.append(buffer_.data() + buffer_position_,
+                                fetch_bytes);
+  buffer_position_ += fetch_bytes;

[satorux1, 2012/11/12 06:07:00]

If you use StringPiece for buffer_position_, the code will look like:

http_request_->content.append(buffer_position_.data(),
                              buffer_position_.size());
DCHECK_LE(http_request_->content.size(), current_content_length);
buffer_position_.clear();

[mtomasz, 2012/11/12 12:17:44]

Ack. Let's discuss.

+
+  if (current_content_length_ == http_request_->content.length()) {
+    state_ = STATE_ACCEPTED;
+    return ACCEPTED;
+  }
+
+  return WAITING;
+}
+
+scoped_ptr<HttpRequest> HttpRequestParser::GetRequest() {
+  DCHECK(state_ == STATE_ACCEPTED);
+  scoped_ptr<HttpRequest> result = http_request_.Pass();
+
+  // Prepare for parsing a new request.
+  state_ = STATE_HEADERS;
+  http_request_.reset(new HttpRequest());
+  buffer_ = buffer_.substr(buffer_position_,

[satorux1, 2012/11/12 06:07:00]

Why not just buffer_.clear()?

At this moment, All the content in buffer_ should be consumed, right?

If so, we can add a DCHECK like:

DCHECK_EQ(buffer_.size(), buffer_position_);

or 

DCHECK(buffer_position_.empty())

if you use StringPiece.

[mtomasz, 2012/11/12 12:17:44]

This is in case, we get two (or more) requests in one chunk.

[satorux1, 2012/11/13 05:13:18]

I think we don't need to support the case. The server does not support
Keep-alive, so the client won't reuse the same connection.

[mtomasz, 2012/11/13 12:23:07]

I've just checked the rfc. In http/1.1 all connections are considered persistent
unless declared otherwise. However, server 
can close the connection after the first request and ignore further request in
the chunk.

So, we can not implement handling multiple requests, but we have to assume that
there may be more than one request in a data chunk. However, in both cases the
amount of code is the same. So is it worth removing support for it? What do you
think?

[satorux1, 2012/11/14 01:34:32]

Let's remove the support for the multiple requests case. Less things to worry
about is a good thing (ex. less things to test). Besides, we can add this at a
later time if needed. 

[mtomasz, 2012/11/14 03:23:35]

Done.

+                           buffer_.length() - buffer_position_);
+  buffer_position_ = 0;
+  current_content_length_ = 0;
+
+  return result.Pass();
+}
+
+HttpMethod HttpRequestParser::GetMethodType(const std::string& token) const {
+  if (token == "get") {
+    return GET;
+  } else if (token == "head") {
+    return HEAD;
+  } else if (token == "post") {
+    return POST;
+  } else if (token == "put") {
+    return PUT;
+  } else if (token == "delete") {
+    return DELETE;
+  }
+  NOTREACHED() << "Method not implemented: " << token;
+  return UNKNOWN;
+}
+
+}  // namespace test_servers
+}  // namespace drive