Lee Clagett's review

[vinipsmaker]

• There is no built-in mechanism for writing out client headers, which means there is no client support.

  I would expect a boost::http library to be more feature complete before being accepted.

• Why aren't there higher-order functions for more common use cases?

• A std::ostringstream is used, when spirit::karma would be faster and have less memory allocations (minor).

• If a user want the write_header API, I'd be on top of current API as a separate abstraction (ResponseBuilder?)...

• update the requirements to better reflect the state...

• Provide an HTTP parser, as it's the only thing some users care about.

Specification/type-requirements

• "async_writeandasync_read_some` in the http::Socket concept requires an entire http::Message concept as an argument, when only the SequenceContainer concept should be necessary for this function" (TODO: decrease requirements and add a few helper functions/types).

• "It might be worth eventually relaxing the requirements for the key/values in the AssociativeMap used by the field parsing functions to a subset of string functions. begin(), end(), size(), empty(), push_back(), pop_back(), operator<(), a hash implementation, and contigous data. This way a quick and dirty small string optimization could be written for field handling."

• "The current async_write_response implementation assumes contiguous elements on the SequenceContainer concept, which is incorrect."

  The error on the implementation happens because the implementation converts the body to an asio::buffer. asio::buffer requires contiguous elements. Message concept only requires SequenceContainer.

  A solution could be requiring contiguous elements also within the Message concept.

• More considerations for design choices (or documentation wording) should be considered - All data reads / writes for the message body require a copy to be made.

• "How do the various write functions manipulate the headers? What fields are added, if any? This is partially mentioned at the bottom of the http::ServerSocket page, but I saw "content-length: 22" added to my message, and this was never explicitly stated (although obviously assumed). This should likely be explicitly specified in the concept itself, AND the location should be specified (i.e. these fields are explicitly appended). Should also document that some fields, such as content-length cannot be listed twice according to the specification, while comma de-limited fields can be listed multiple times as a valid form of appension. I.e. "content-encoding: gzip, sdch\r\n" and "content-encoding: gzip\r\ncontent-encoding: sdch\r\n" indicate equivalent content encodings applied to the data. Should probably mention that Transfer-Encoding is a comma delimited list too."

• "The key_type and mapped_type for the headers portion of the message::Concept indicate that they must meet the "String concept". I don't recall seeing this concept being defined in C++ or boost, where does it come from? If its from C++1z, it might be to merge the behavior of string_ref and basic_string, so that concept would be unlikely to require storage like a container. Might want to re-think the concept requirements here, or state that only a conforming std::basic_string implementation is acceptable for now."

pipelining and implicit writes

• I agree that documentation needs improvement here too.
• Documentation on http::socket being able to write "implicitly" (The ServerSocket concept gives guarantees about a producer to HTTP messages...).
• "http::SocketServer implementations manipulate the state of the http::Socket (and indirectly the asio::tcp::socket), making some actions unavailable after invoking those functions. For example, after invoking async_write_response, no writes on the http::Socket/http::ServerSocket or asio::ip::tcp::socket can occur until the callback is invoked because it calls asio::write on the asio::ip::tcp::socket directly. So unless I am incorrect, it is important for the users to know whats being manipulated (the effects)."
• "issuing an async_read_some command blocks both the send and receive channels of the underlying ASIO stream"
  • "This is not documented anywhere"
  • reason is "current async_read_some implementation has an error code path that can initiate a write"
  • Actually, it's already forbidden to issue a read while write hasn't finished, but user could try to use its own parser to read next request. So documentation must clarify this implementation detail. Something in the lines of "never do raw read/writes with the native handle". Pay attention to WebSocket (e.g. user should be able to borrow a "clean" handle during UPGRADE).