Thanks for your review Lee, your comments are surely helpful for me, as I'm
going to do several improvements based on them.
2015-08-17 2:01 GMT-03:00 Lee Clagett
So why is `async_write_response_continue` mandated in the http::ServerSocket concept when a free function can be implemented, that uses the other concept functions? If the reasoning is performance (which appears to be the reason) - why isn't a more performant method (probably some form of pre-generation) of sending messages added? This would prevent the socket concept from continually absorbing additional special case messages as higher order functions are developed, and allow for clients to pre-generate error responses.
`async_write_response_continue` is a different concept, it cannot be composed in terms of the other operations. It has a different semantic. It changes the read_state status. - `async_read_request`, and `async_write_response_metadata` in the
http::ServerSocket concept require an entire http::Message concept as an argument, when only the AssociativeContainer concept should be necessary for this function.
- `async_write` and `async_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.
Just passing the message object is less error-prone (e.g. should I pass headers() or trailers()?). But yes, I could do better (decrease requirements and add a few helper functions/types). - All data reads / writes for the message body require a copy to be made,
because memory from a SequenceContainer concept cannot be given to ASIO/OS directly (std::liststd::uint8_t is a SequenceContainer). The message body should likely be restricted to the C++1z ContiguousContainer concept instead. I think users would prefer speed here more than flexibility. Its also easier to relax requirements after release than the opposite.
std::vector is used by default. Not sure how helpful it is to restrict the user if he is willing to pay. A parser embedded in the message object would have trouble implementing the ContiguousContainer concept when it comes to chunked entities. I agree that's easier to relax requirements after release than the opposite. - Ideally the read / write functions for the payload would have the same
requirements as an asio buffer. Requiring a container prevents memory locations from other sources (such as directly from a streambuf), and prevents scatter gather i/o. I'd say this is less critical, as its something most users are unlikely to use.
It may kill some use cases. If some type is more performant for the user, it should be used. The default should satisfy most of the users wish. - `async_read_trailers` and `async_writer_trailers` in the http::Socket
concept require an entire http::Message concept as an argument, when only the AssociativeContainer concept should be necessary.
Just passing the message object is less error-prone (e.g. should I pass headers() or trailers()?). But yes, I could do better (decrease requirements and add a few helper functions/types). - I mentioned this before; client and server specific header
writing/reading is a shame due to the overwhelming similarities. My earlier suggestions were criticized for being stringly typed (or having more states), but I think this was a somewhat weak criticism since the type system can be used more effectively. `async_write_header(http::response{200, "OK"}, ...)` or `async_write_header(http::request{http::request::GET, "/"}, ...)`.
This design would have much more implicit behaviour and it'd lead to more surprises. A ResponseBuilder or alike could be done, separately, in case you don't mind the surprises. - What is the purpose of the standalone `async_write_response` and
`async_write_response_metadata` functions? They only forward to functions with the same name in the http::ServerSocket concept, is this cruft?
They fill the reason phrase for you based on the status code. - The current `async_read_some` implementation has an error code path that
can initiate a write. This is not documented anywhere, and effectively issuing an `async_read_some` command blocks both the send and receive channels of the underlying ASIO stream. This problem is likely to occur in a few other spots, but I didn't check.
The ServerSocket concept gives guarantees about a producer to HTTP messages (and you, as the user, writes the HTTP consumer). A channel to exchange HTTP messages somehow. There were no guarantees about exposing the implementation detail errors to the user. Imagine a ZeroMQ-based ServerSocket that cannot send "pings" because it was not documented in the general concept. The general ServerSocket concept doesn't restrict the freedom from HTTP "providers" too much. Anyway, this is a serious documentation error within the library and I need to fix it. -- VinÃcius dos Santos Oliveira https://about.me/vinipsmaker