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

Subject: Re: [boost] [asio-users] [http] Formal review of Boost.Http
From: Lee Clagett (forum_at_[hidden])
Date: 2015-08-08 15:36:20

On Sat, Aug 8, 2015 at 5:16 AM, Vinícius dos Santos Oliveira <
vinipsmaker_at_[hidden]> wrote:

> 2015-08-08 0:10 GMT-03:00 Lee Clagett <forum_at_[hidden]>:
>
>> FWIW it should be possible to do client and server side pipelining with
>> this design right now. The reads and writes of the HTTP Socket concept
>> should be (and currently are by `basic_socket`) handled independently.
>> Since the notification for a write completion is based on the lower layer
>> write completion, and NOT whether a HTTP response was received, multiple
>> (pipelined) requests should be possible by a client. Similarly, this design
>> should allow servers to handle pipelined requests in parrallel. Providing a
>> framework for client or server pipelining will require more work, and
>> should probably not be a part of the Socket concept. In fact -
>>
>
> The current design **does** support pipelining. Some HTTP clients disable
> pipelining because some buggy servers will get confuse. This won't happen
> with Boost.Http (there are tests to ensure pipelning is working).
>
> What the current design does **not** support is handling pipelined
> requests concurrently. When you starting read a request, the write_state[1]
> will change to finished until the fully request is received, so you can't
> get multiple request while you don't issue the replies. This behaviour is
> explained in the ServerSocket concept[2].
>
> There are just too many ways to allow HTTP concurrent pipeline that would
> be transparent for the user, but all of them are heavier. If I was to allow
> handling multiple concurrent HTTP pipelined requests, I'd expose user
> cooperation, so the abstraction doesn't become so heavy. The user would
> need to be aware if the reply for some request can already be delivered or
> not. Of course this means the socket will need to remember order and attach
> some id to the request messages. It can be problematic in alternative
> backends because the id could be of a different type and makes it difficult
> to use the same message type with different HTTP backends. What do you
> think? The design would just be more complex for not much gain. HTTP/2.0
> allow real multiplexing and doesn't show this problem.
>

I agree that providing any pipelining framework in the library is probably
unnecessary right now. However, the design should not inhibit such
functionality from being added (which I do not think is a problem with this
design).

> I think the ServerSocket concept should be removed entirely. The
>> `http::basic_socket<...>` models both concepts, so theres lots of server
>> specific code in it, which seems confusing to me (is this really a "basic"
>> socket?).
>>
>
> basic_socket uses basic_ prefix just like basic_string.
>

I was asking whether basic_socket should NOT model the http::ServerSocket
concept. I understood the naming convention, and the decision to provide
the implementation as a configurable template. The problem is the lack of
composability. If a different http::Socket concept is desired (someone not
using ASIO, etc.), then the http::ServerSocket concept has to be
re-implemented also since there is no other implementation. The obvious way
is to achieve composability is to use inheritance with:
`http::basic_server_socket<Socket>` where `Socket` is a http::Socket
concept. Inheritance in this situation has its drawbacks, for sure (a
virtual destructor should be considered).

> I think standalone functions for typical client and server operations can
>> be implemented as "composed" operations similar to `asio::async_write`. The
>> documentation will have to be explicit on what the composed function is
>> doing so that users know whether the Socket concept has outstanding reads
>> or writes (and therefore cannot do certain operations until the handler is
>> invoked). In fact, if there is a server operation that _cannot_ be done
>> with the Socket concept, then the concept probably needs to be re-designed.
>>
>
> About "users know whether..." seems like a bad idea if I want to deliver
> multiple backends. Some implementation details should just be abstracted
> away.
>
> 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).

I don't understand what you mean by "if there is a server operation that
> cannot be done with the Socket concept [...]". Boost.Http has two concepts,
> ServerSocket and Socket. Socket concept will be useful to client-side HTTP
> and that's the reason why there are two concepts.
>
>
The http::ServerSocket concept requires a http::Socket to write its
messages, but does the http::ServerSocket concept need to be a refinement
of the http::Socket concept? This implies a strong relationship. The
current specification looks like a http::ServerTraits concept - its
specifying how a http::Socket is being used in situations specific to
servers. There doesn't appear to be any additional state tracking needed
for the http::ServerSocket functions, which is why I suggested standalone
functions (I didn't see the FileServer section). In fact implementation can
be inverted; make all of the current implementations for http::ServerSocket
standalone functions, and then have the default http::ServerTraits
implementation call the standalone versions of the functions. The
http::ServerTraits should then take a http::Socket as an argument for each
of the functions. This complete de-coupling would allow someone to
independently change the behavior of any related server functions, even
while using the same http::Socket object.

[1] https://boostgsoc14.github.io/boost.http/reference/write_state.html
> [2] "The ServerSocket object MUST prevent the user from issuing new
> replies while [...]",
> https://boostgsoc14.github.io/boost.http/reference/server_socket_concept.html
>
>

Lee