Re: [boost] [asio-users] [http] Formal review of Boost.Http

Subject: Re: [boost] [asio-users] [http] Formal review of Boost.Http
From: Vinícius dos Santos Oliveira (vini.ipsmaker_at_[hidden])
Date: 2015-08-25 01:43:02

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 <forum_at_[hidden]>:

> 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::list<std::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