Re: [boost] [http] Formal Review

Subject: Re: [boost] [http] Formal Review
From: Lee Clagett (forum_at_[hidden])
Date: 2015-08-12 18:27:54

On Wed, Aug 12, 2015 at 2:58 PM, Antony Polukhin <antoshkka_at_[hidden]>
wrote:

> 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.
>
>
This is one my complaints in my in-progress review. The documentation
doesn't explicitly state how the provided SequenceContainer is being used
during read/writes of the body. And SequenceContainer is a lax concept -
std::list models SequenceContainer, which means all read/writes to the body
require an internal copy to meet the concept correctly.

> 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).
>
> How is your http::stream version asynchronous? It appears it would be
blocking, unless there is some other magic occurring. It would be more
concerning on the reads (the generation could be pre-computed), because if
the underlying stream didn't have the data for .version(), etc., I think it
would need to block. Or it would have to read the entire header into one
larger buffer then parse? I guess you could use `async_read_until(..., ,
...) functionality and keep buffering (until a max of course), then throw
that to the parser?

Anyway - I was thinking along the same lines at various points. Having a
function that pre-generates HTTP messages is very useful IMO, and should
likely be included. Designing a good parsing concept would be a bit more
work I think, but probably worth it too. I'm not sure how the author
intends to swap out parsers in the current design. Having a fixed parser
seems acceptable, but the author almost seemed to suggest that it could be
selectable somehow.

> 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).
>
>
How much of a penalty do you think there is to this split design? It seems
likely that multiple handlers will have to be given to read_some (directly
or indirectly) for ASIO to read the header, etc. anyway.

>
> 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.
>
> I agree. More explicit memory management would differentiate this from
existing libraries, otherwise boost should likely just review one of the
other major libraries.

Lee