[boost] [http] Formal Review

Subject: [boost] [http] Formal Review
From: Antony Polukhin (antoshkka_at_[hidden])
Date: 2015-08-12 14:58:55

Hi,

This is my formal review for Boost.HTTP library.

1. Should Boost.Http be accepted into Boost?

No. It has some big architectural issues and a lot of issues in code.

2. What is your evaluation of the design?

That's the fatal problem of the library. It seems like the library attempts
to get best from ASIO and cpp-netlib and unfortunately fails. Now let's get
into details:

* ASIO is a low level abstraction over protocol that does not take care
about memory management of buffers (in most cases it's the responsibility
of user to allocate and keep alive the required buffers).
* cpp-netlib takes care of memory management, hides low-level stuff.

Boost.Http is stuck somewhere in the middle: sometimes it allocates and
extends buffers, sometimes not. Because of that the design does not seem
solid. Any attempt to hide all the memory management inside the library
will end in an another implementation of cpp-netlib. So it seems right to
remove all the memory management from the library internals and make the
library closer to ASIO.

Here are my recommendations for separation of flies and dishes:

* Stick to the idea that http socket must ONLY manage connection without
extending the ASIO's send/receive function signatures. That means that
http1.0 and http1.1 sockets must be almost a typedef for tcp/ssl socket.
Http2.0 socket must be very close to SSL socket. No additional methods
(like async_read_trailers or async_write_response) must be provided.

* It must be a user's responsibility to provide buffers for messages. Never
extend buffers in socket-related methods.

* Provide helper classes that parse and generate http

* Provide out-of-the-box completion conditions for async_read operation:
http::completions::full, http::completions::headers,
http::completions::body...

Those bullets will allow to write following code (based on
http://www.boost.org/doc/libs/1_58_0/doc/html/boost_asio/tutorial/tutdaytime3/src.html
with minimal modifications):

#include <ctime>
#include <iostream>
#include <string>
#include <boost/bind.hpp>
#include <boost/shared_ptr.hpp>
#include <boost/enable_shared_from_this.hpp>
#include <boost/asio.hpp>

std::string make_daytime_string() {
  using namespace std; // For time_t, time and ctime;
  time_t now = time(0);
  return ctime(&now);
}

class http_connection : public
boost::enable_shared_from_this<http_connection> {
public:
  typedef boost::shared_ptr<http_connection> pointer;

  static pointer create(boost::asio::io_service& io_service) {
    return pointer(new http_connection(io_service));
  }

  http::socket& socket() {
    return socket_;
  }

  void start() {
    message_.resize(4 * 1024);

    boost::asio::async_read(socket_, boost::asio::buffer(message_),
        http::completions::full(message_), // read until all the whole
request is in `message_`
        boost::bind(&http_connection::handle_read, shared_from_this(),
          boost::asio::placeholders::error,
          boost::asio::placeholders::bytes_transferred));
  }

private:
  http_connection(boost::asio::io_service& io_service)
    : socket_(io_service)
  {}

  void handle_write(const boost::system::error_code& /*error*/,
      size_t /*bytes_transferred*/)
  {}

  void handle_read(const boost::system::error_code& error, size_t
bytes_transferred) {
    assert(!error); // for shortness
    // `message_` contains the whole request, including headers and body
    // because http::completions::full was provided

    {
      boost::system::error_code e;
      http::view v(message_, e);      // represent buffer as http message
      assert(!e);                     // no error occured during parsing
      assert(v.read_state() == http::read_state::empty);
      std::cerr << v.version();       // "HTTP1.1"
      std::cerr << v["Content-type"]; // "plain/text"
      std::cerr << v.body();          // string_ref("Hello! This is a body
content. What time is it?")
      message_.clear();               // `v` is now invalid to use!
    }

    {
      http::generator resp(message_);         // use `message_` for an
output
      resp << make_daytime_string();          // append daytime to body
      resp["Content-Type"] = "plain/text";    // set some headers
      resp.version(http::version::HTTP_1_1);  // explicitly set HTTP version
      resp.code(200);                         // "200 OK"
      resp.flush();                           // explicitly flush to
buffer. Done automatically in destructor
    }

    boost::asio::async_write(socket_, boost::asio::buffer(message_),
        boost::bind(&http_connection::handle_write, shared_from_this(),
          boost::asio::placeholders::error,
          boost::asio::placeholders::bytes_transferred));
  }

  http::socket socket_; // manages the connection only
  std::string message_;
};

class http_server {
public:
  http_server(boost::asio::io_service& io_service)
    : acceptor_(io_service, http::endpoint(http::all_versions, tcp::v4(),
80))
  {
    start_accept();
  }

private:
  void start_accept() {
    http_connection::pointer new_connection =
      http_connection::create(acceptor_.get_io_service());

    acceptor_.async_accept(new_connection->socket(),
        boost::bind(&http_server::handle_accept, this, new_connection,
          boost::asio::placeholders::error));
  }

  void handle_accept(http_connection::pointer new_connection,
      const boost::system::error_code& error)
  {
    if (!error) {
      if (new_connection->socket().version() == http::version::HTTP_2_0) {
        // do some more stuff
      }

      new_connection->start();
    }

    start_accept();
  }

  http::acceptor acceptor_;
};

int main() {
  try {
    boost::asio::io_service io_service;
    http_server server(io_service);
    io_service.run();
  } catch (std::exception& e) {
    std::cerr << e.what() << std::endl;
  }

  return 0;
}

This will also untie Boost.HTTP from ASIO. Now users could use
http::generator/http::view to read/generate HTTP data. This significantly
extends the use cases of your library (for example on a previous work we
had our own network transfer implementation, but we'd gladly use a http
parser and generator).

Such code is also less intrusive: only minimal chages were required to
change a tcp example into the http example. Users will appreciate that,
especially in cases when ASIO is already used for HTTP.

BTW, after that you could easily implement http::stream (that must be close
to tcp::stream by functionality). http::stream would be a killer feature
that would be widely used by utility programs (as I remember current
Boost's regression testing requires it, but uses tcp::stream with a bunch
of workarounds).

Please, implement the HTTP2.0 protocol. There are some open source
solutions that are licensed under the Boost license and are very close to
full implementation of HTTP2.0. Just reuse the code and chat with the
authors for better understanding of the problems.

3. What is your evaluation of the implementation?

Not perfect.

For example
https://github.com/BoostGSoC14/boost.http/blob/master/include/boost/http/socket-inl.hpp#L352
It seems that `std::vector<asio::const_buffer> buffers;
buffers.reserve(nbuffer_pieces);` is meant to be here. There are more such
places...

It seems that some useful features (like custom allocators) are not
supported by the library. It's better to be fixed.

Some of the implementation issues are because of the library design. For
example requesting multiple async operations
(async_read_request+async_read_some+async_read_trailers) is performance
hit. It's better to read all the data at once (just like in the example
from above).

4. What is your evaluation of the documentation?

First code snippet in tutorial contains mostly ASIO's stuff and almost no
HTTP stuff. That's wired... If bullets from above would be implemented,
then I'd rather start with parsing and generating simple http messages.
Then an example with sockets could be provided.

Also please do not use coroutines at least in first example. coroutines is
a new feature in ASIO, many users use old Boost versions that have no
coroutines and some users are not familiar with them. So if you start with
coroutines user will have to go to ASIO docs and dig into coroutines to
understand the very first example. Example without coroutines will be more
user friendly.

5. What is your evaluation of the potential usefulness of the library?

Library is very useful and required. But in current state it must not be
accepted into Boost.

6. Did you try to use the library? With what compiler? Did you have
 any problems?

Have not tryed.

7. How much effort did you put into your evaluation? A glance? A quick
 reading? In-depth study?

A few hours digging into docs, about 3 hours looking into the sources,
about 3 hours formulising the issues and recommending solutions.

8. Are you knowledgeable about the problem domain?

Not an expert, but used ASIO a lot along with cpp-netlib and some other
libraries close to the proposed one.

8.3333(3) Review

Please, reserve a bigger window when such complex library is proposed for
review. It's hard to review it in 10 days.

8.6666(6) For Reviewers

I urge other reviewers to take a look at the cpp-netlib. If you treat
Boost.Http as an out-of-box solution, then cpp-netlib is more mature,
user-friendly and solid. If you treat Boost.Http as a basic building block
for HTTP (just like ASIO is a basic building block for networking), then
it's better to stick to the ASIO's design and do not *partially* manage
memory!

P.S.: I hope that this library would be changed according to my
recommendations and submitted for a new review.

-- 
Best regards,
Antony Polukhin