Implement request_storage_base.

deanberris · deanberris · commit 29ec3037a541 · 2012-03-10T20:18:15.000+11:00
This commit implements an untested (but contains a building test)
request_storage_base that aims to hide the details of a segmented data
structure used to hold the request data. The intention is that the
storage base will be used if large amounts of data will be stored in an
efficient manner in memory.

This design also allows the request implementation to ignore the fact
that there's an underlying storage implementation and just use a
provided body chunk writer. This allows people to add a function that
pulls the data from whatever source and adds it to the request only when
necessary.

This is an evolutionary step forward moving from the inside out. The
intention is that we're able to build higher level logic to be able to
not rely on using the storage base in special circumstances.
diff --git a/boost/network/protocol/http/request/request.hpp b/boost/network/protocol/http/request/request.hpp
@@ -53,7 +53,7 @@ struct request : request_base {
   virtual void set_method(std::string const & method);
   virtual void set_status(std::string const & status);
   virtual void set_status_message(std::string const & status_message);
-  virtual void set_body_stream(shared_ptr<body_stream> stream);
+  virtual void set_body_writer(function<void(char*, size_t)> writer);
   virtual void set_uri(std::string const &uri);
   virtual void set_uri(network::uri::uri const &uri);
 
@@ -63,7 +63,8 @@ struct request : request_base {
   virtual void get_method(std::string & method) const;
   virtual void get_status(std::string & status) const;
   virtual void get_status_message(std::string & status_message) const;
-  virtual void get_body_stream(body_stream & output_stream) const;
+  virtual void get_body(function<void(char*, size_t)> chunk_reader) const;
+  virtual void get_body(std::string const & body) const;
 
   virtual ~request();
  private:
diff --git a/boost/network/protocol/http/request/request.ipp b/boost/network/protocol/http/request/request.ipp
@@ -21,7 +21,7 @@ struct request_pimpl {
 };
 
 request::~request() {
-  // Do nothing here.
+  // do nothing here
 }
 
 request::request(std::string const & url)
diff --git a/boost/network/protocol/http/request/request_base.hpp b/boost/network/protocol/http/request/request_base.hpp
@@ -13,7 +13,6 @@
 
 #include <boost/network/message/message_base.hpp>
 #include <boost/network/uri/uri.hpp>
-#include <boost/iostreams/categories.hpp>
 
 namespace boost { namespace network { namespace http {
 
@@ -22,29 +21,27 @@ struct body_source : iostreams::source {
   virtual ~body_source();
 };
 
+struct request_storage_base_pimpl;
+
 struct request_storage_base {
-  typedef iostreams::stream<body_source> body_stream;
  protected:
-  // TODO Implement a segmented storage base which efficiently supports
-  // efficient memory usage that makes sense for HTTP payload. The idea is
-  // to expose the internal (segmented) storage to the client implementation
-  // so that raw buffers of formatted HTTP request data (which may or may not
-  // support delegated streams from user input -- i.e. for the body contents)
-  // can be efficiently sent out to the wire. This also implies that all
-  // thread-safety guarantees are handled by the storage base as well.
-  request_storage_base(size_t hint = BOOST_NETWORK_BUFFER_CHUNK);
-  virtual void set_status_line(std::string const &status_line);
-  virtual void append_header(std::string const &name, std::string const &value);
+  request_storage_base(size_t chunk_size = BOOST_NETWORK_BUFFER_CHUNK);
+  virtual void append(char const *data, size_t size);
+  virtual size_t read(char *destination, size_t offset, size_t size);
+  virtual void flatten(std::string &destination);
+  virtual void clear();
   virtual ~request_storage_base();
+
+ private:
+  request_storage_base_pimpl *pimpl_;
 };
 
 struct request_base : message_base, request_storage_base {
-  using request_storage_base::body_stream;
   // Setters
   virtual void set_method(std::string const & method) = 0;
   virtual void set_status(std::string const & status) = 0;
   virtual void set_status_message(std::string const & status_message) = 0;
-  virtual void set_body_stream(shared_ptr<body_stream> stream) = 0;
+  virtual void set_body_writer(function<void(char*, size_t)> writer) = 0;
   virtual void set_uri(std::string const &uri) = 0;
   virtual void set_uri(network::uri::uri const &uri) = 0;
 
@@ -54,7 +51,8 @@ struct request_base : message_base, request_storage_base {
   virtual void get_method(std::string & method) const = 0;
   virtual void get_status(std::string & status) const = 0;
   virtual void get_status_message(std::string & status_message) const = 0;
-  virtual void get_body_stream(body_stream & output_stream) const = 0;
+  virtual void get_body(function<void(char*, size_t)> chunk_reader) const = 0;
+  virtual void get_body(std::string const & body) const = 0;
   virtual ~request_base() = 0;
 };
 
diff --git a/boost/network/protocol/http/request/request_base.ipp b/boost/network/protocol/http/request/request_base.ipp
@@ -8,17 +8,124 @@
 // http://www.boost.org/LICENSE_1_0.txt)
 
 #include <boost/network/protocol/http/request/request_base.hpp>
+#include <boost/thread/mutex.hpp>
+#include <cstring>
 
 namespace boost { namespace network { namespace http {
 
 request_base::~request_base() {
   // default implementation, only required for linking.
 }
 
+struct request_storage_base_pimpl {
+  request_storage_base_pimpl(size_t chunk_size);
+  void append(char const *data, size_t size);
+  size_t read(char *destination, size_t offset, size_t size);
+  void flatten(std::string &destination);
+  void clear();
+  request_storage_base_pimpl clone() const;
+  ~request_storage_base_pimpl();
+
+ private:
+  size_t chunk_size_;
+  typedef std::vector<std::pair<char *, size_t> > chunks_vector;
+  chunks_vector chunks_;
+  mutex chunk_mutex;
+};
+
+request_storage_base::request_storage_base(size_t chunk_size)
+: pimpl_(new (std::nothrow) request_storage_base_pimpl(chunk_size))
+{}
+
 request_storage_base::~request_storage_base() {
-  // default implementation, only required for linking.
+  delete pimpl_;
+}
+
+void request_storage_base::append(char const *data, size_t size) {
+  pimpl_->append(data, size);
+}
+
+size_t request_storage_base::read(char *destination, size_t offset, size_t size) {
+  return pimpl_->read(destination, offset, size);
+}
+
+void request_storage_base::flatten(std::string &destination) {
+  pimpl_->flatten(destination);
+}
+
+void request_storage_base::clear() {
+  pimpl_->clear();
+}
+
+request_storage_base_pimpl::request_storage_base_pimpl(size_t chunk_size)
+: chunk_size_(chunk_size)
+, chunks_()
+{
+  // do nothing here.
+}
+
+void request_storage_base_pimpl::append(char const *data, size_t size) {
+  if (chunks_.empty()) {
+    chunks_.push_back(std::make_pair(
+        new (std::nothrow) char[chunk_size_], 0));
+  }
+  std::pair<char *, size_t> *chunk = &chunks_.back();
+  size_t remaining = chunk_size_ - chunk->second;
+  while (remaining < size) {
+    std::memcpy(chunk->first + chunk->second, data, size - remaining);
+    chunk->second += size - remaining;
+    data += remaining;
+    size -= remaining;
+    chunks_.push_back(std::make_pair(
+        new (std::nothrow) char[chunk_size_], 0));
+    chunk = &chunks_.back();
+    remaining = chunk_size_ - chunk->second;
+  }
+  if (size > 0) {
+    std::memcpy(chunk->first + chunk->second, data, size);
+    chunk->second += size;
+  }
 }
-  
+
+size_t request_storage_base_pimpl::read(char *destination, size_t offset, size_t size) {
+  if (chunks_.empty()) return 0;
+  // First we find which chunk we're going to read from using the provided
+  // offset and some arithmetic to determine the correct one.
+  size_t chunk_index = offset / chunk_size_;
+
+  // Then we start copying up to size data either until we've reached the end
+  // or we're 
+  size_t chunks_count = chunks_.size();
+  size_t read_count = 0;
+  while (size > 0 && chunk_index < chunks_.size()) {
+    size_t bytes_to_read = std::min(chunks_[chunk_index].second, size);
+    std::memcpy(destination + read_count, chunks_[chunk_index].first, bytes_to_read);
+    read_count += bytes_to_read;
+    size -= bytes_to_read;
+    ++chunk_index;
+  }
+  return read_count;
+}
+
+void request_storage_base_pimpl::flatten(std::string &destination) {
+  chunks_vector::const_iterator chunk_iterator = chunks_.begin();
+  for (; chunk_iterator != chunks_.end(); ++chunk_iterator) {
+    destination.append(chunk_iterator->first, chunk_iterator->second);
+  }
+}
+
+void request_storage_base_pimpl::clear() {
+  chunks_vector::const_iterator chunk_iterator = chunks_.begin();
+  for (; chunk_iterator != chunks_.end(); ++chunk_iterator) {
+    delete [] chunk_iterator->first;
+  }
+  chunks_vector().swap(chunks_);
+}
+
+request_storage_base_pimpl::~request_storage_base_pimpl() {
+  clear();
+}
+
 } /* http */
 
 } /* network */
diff --git a/libs/network/test/http/CMakeLists.txt b/libs/network/test/http/CMakeLists.txt
@@ -12,6 +12,15 @@ if (OPENSSL_FOUND)
 endif()
 
 if (Boost_FOUND)
+    # These are the internal (simple) tests.
+    add_executable(cpp-netlib-http-request_base_test request_base_test.cpp)
+    target_link_libraries(cpp-netlib-http-request_base_test
+        ${Boost_LIBRARIES}
+        cppnetlib-message
+        cppnetlib-http-message)
+    add_test(cpp-netlib-http-request_base_test
+        ${CPP-NETLIB_BINARY_DIR}/tests/cpp-netlib-http-request_base_test)
+
     set ( TESTS
         client_constructor_test
         client_get_test
@@ -25,15 +34,6 @@ if (Boost_FOUND)
                 PROPERTIES COMPILE_FLAGS "-Wall")
         endif()
         add_executable(cpp-netlib-http-${test} ${test}.cpp)
-        add_dependencies(cpp-netlib-http-${test}
-            cppnetlib-constants
-            cppnetlib-uri
-            cppnetlib-message
-            cppnetlib-message-wrappers
-            cppnetlib-message-directives
-            cppnetlib-http-message
-            cppnetlib-http-client
-            cppnetlib-http-client-connections)
         target_link_libraries(cpp-netlib-http-${test}
             ${Boost_LIBRARIES}
             ${CMAKE_THREAD_LIBS_INIT}
diff --git a/libs/network/test/http/request_base_test.cpp b/libs/network/test/http/request_base_test.cpp
@@ -0,0 +1,49 @@
+// Copyright 2012 Dean Michael Berris <dberris@google.com>.
+// Copyright 2012 Google, Inc.
+// Distributed under the Boost Software License, Version 1.0.
+// (See accompanying file LICENSE_1_0.txt or copy at
+// http://www.boost.org/LICENSE_1_0.txt)
+
+#define BOOST_TEST_MODULE HTTP Request Storage Base Test
+#include <boost/network/protocol/http/request/request_base.hpp>
+#include <boost/test/unit_test.hpp>
+
+namespace http = boost::network::http;
+
+// In this test we make sure that the implementation of the default request
+// storage base actually doesn't have bugs and works as advertised. Although we
+// don't intend to expose this interface to users, we use the test as a sanity
+// check on the internals of the implementation.
+struct request_test : http::request_storage_base {
+  typedef http::request_storage_base base_type;
+  
+  // Expose the protected functions so that we can test them.
+  using base_type::append;
+  using base_type::read;
+  using base_type::flatten;
+  using base_type::clear;
+
+  request_test(size_t chunk_size)
+  : base_type(chunk_size)
+  {}
+
+  ~request_test() {
+    // do nothing here.
+  }
+};
+
+BOOST_AUTO_TEST_CASE(request_storage_flow) {
+  // Use a few byte chunks just to make it manageable.
+  request_test simple(64);
+  static char data[] =
+  "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Vivamus vitae ante sed nunc dapibus convallis in at neque. Vestibulum sed congue nunc. Sed tempus lorem non dui ultrices porttitor porta ligula venenatis. Sed a orci gravida tellus condimentum laoreet. Vivamus pulvinar, tortor eu adipiscing tempus, dolor urna tincidunt enim, id pretium eros ante quis dui. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. In hac habitasse platea dictumst. Maecenas mattis metus.";
+  simple.append(data, sizeof(data));
+  char output[sizeof(data)];
+  size_t bytes_read = simple.read(output, 0, sizeof(data));
+  BOOST_CHECK_EQUAL(bytes_read, sizeof(data));
+  std::string flattened;
+  simple.flatten(flattened);
+  BOOST_CHECK_EQUAL(flattened, std::string(output, sizeof(data)));
+  BOOST_CHECK_EQUAL(std::string(data, sizeof(data)), std::string(output, sizeof(data)));
+  simple.clear();
+}

Original file line number	Diff line number	Diff line change
`@@ -21,7 +21,7 @@ struct request_pimpl {`
`21`	`21`	`};`
`22`	`22`
`23`	`23`	`request::~request() {`
`24`		`- // Do nothing here.`
	`24`	`+ // do nothing here`
`25`	`25`	`}`
`26`	`26`
`27`	`27`	`request::request(std::string const & url)`