On Wed, Aug 12, 2015 at 2:58 PM, Antony Polukhin
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/tutdaytime... with minimal modifications):
#include <ctime> #include <iostream> #include <string> #include
#include #include #include 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
{ public: typedef boost::shared_ptr 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/soc... It seems that `std::vectorasio::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