Re: [boost] Boost.HTTPKit, a new library from the makers of Beast!

Subject: Re: [boost] Boost.HTTPKit, a new library from the makers of Beast!
From: Vinícius dos Santos Oliveira (vini.ipsmaker_at_[hidden])
Date: 2018-01-01 22:03:04

2017-10-12 9:45 GMT-03:00 Vinnie Falco via Boost <boost_at_[hidden]>:

> On Wed, Oct 11, 2017 at 9:47 PM, Vinícius dos Santos Oliveira
> <vini.ipsmaker_at_[hidden]> wrote:
> > I took the liberty to convert the Tufão project that you've
> > mentioned to use the Boost.Beast parser:
> > https://github.com/vinipsmaker/tufao/commit/
> 56e27d3b77d617ad1b4aea377f993592bc2c0d77
> >
> > Would you say you like the new result better?
>
> It seems pretty reasonable to me.
>
> > Would you say that I've misused your parser to favour my approach?
> > How would you have done it in this case?
>
> Misused? I don't think so. The only meaningful change I would make is
> that I would have simply called basic_parser::is_keep_alive() instead
> of re-implementing the logic for interpreting the Connection header.
>
> > Would you go beyond and accept the idea that the spaghetti effect
> > is inherent to the callback-based approach of push parsers?
>
> This is where we are venturing into the world of opinion. It seems
> like you have a general aversion to callbacks. But there is a reason
> Beast's parser is written this way. Recognize that there are two
> primary consumers of the parser:
>
> 1. Stream algorithms such as beast::http::read_some
>

Such as async_read_some, which are useless if you want to use other
networking APIs (which is _the_ reason for the desire of separate parser
API to start with).

2. Consumers of structured HTTP elements (e.g. fields)
>
> The Beast design separates these concerns. Public member functions of
> `basic_parser` provide the interface needed for stream algorithms,
> while calls to the derived class provide the structured HTTP elements.
> I don't think it is a good idea to combine these into one interface
>

Point 1 is _useless_ if you are providing a _parser_ API. You don't know
which networking API the user will use. It's that simple. There is no point
1.

which you have done in your parser. The reason is that this
> unnecessary coupling pointlessly complicates the writing of the stream
> algorithm.

Not coupling. Point 1 is not even there. Why would you design a parser with
a specific networking API in mind? Message consuming/producing is the work
of a higher layer.

Anyone who wants to write an algorithm to feed the parser
> from some source of incoming bytes now has to care about tokens. This
> is evident from your documentation:
>
> <http://boostgsoc14.github.io/boost.http/#parsing_tutorial1>
>
> In your example you declare a class `my_socket_consumer`. It has a
> single function `on_socket_callback` which is called repeatedly with
> incoming data. Not shown in your example is the stream algorithm (the
> function which interacts with the actual socket to retrieve the data).
> However, we know that this stream algorithm must be aware of the
> concrete type `my_socket_consumer` and that it needs to call
> `on_socket_callback` with an `asio::buffer`. A signature for this
> stream algorithm might look like this:
>
>     template<class SyncReadStream>
>     void read(SyncReadStream& stream, my_socket_consumer& consumer);
>

The stream algorithms you provide were of no use to convert Tufão to use
your parser. And I don't recall writing stream algorithms to use the parser.

Observe that this stream algorithm can only ever work with that
> specific consumer type. In your example, `my_socket_consumer` handles
> HTTP requests. Therefore, this stream algorithm can now only handle
> HTTP requests.

Same function to handle request and responses:
https://github.com/BoostGSoC14/boost.http/blob/5ea65c64467e689cc9b67b8e66da65c43304dc38/include/boost/http/socket.ipp#L1131

There is a little of TMP in the lines above, but so do you have to use
templates if you want to generalize your algorithm over the `isRequest`
template parameter.

In order to receive a response, a new stream algorithm
> must be written. Compare this to the equivalent signature of a Beast
> styled stream algorithm:
>
>     template<class SyncReadStream, bool isRequest, class Derived>
>     void read(SyncReadStream& stream, basic_parser<isRequest, Derived>&
> parser);
>
> This allows an author to create a stream algorithm which works not
> just for requests which store their data as data members in a class
> (`my_socket_consumer`) but for any parser, thanks to the CRTP design.
>

What can your stream algorithms do besides feed data to the parser +
consumer? Just a few messages ago I've showed concrete examples of how
powerful the other model is. In the other model, you could even inject new
tokens (in a virtual way, without the need to read the stream twice and
inject new HTTP data at the appropriate points).

For example, if I create a parser by subclassing
> `beast::http::basic_parser` with an implementation that discards
> headers I don't care about, then it will work with the stream
> algorithm described above without requiring modification to that
> algorithm.

And if you change your networking API, the stream algorithm becomes pretty
useless.

This element that I've coded, in the other side, will work okay in any
"stream model" that you wish:
https://github.com/BoostGSoC14/boost.http/blob/9908fe06d4b2364ce18ea9b4162640b38013c699/include/boost/http/detail/atomic_field_parser.hpp#L46

It is interesting to note that your `my_socket_consumer` is
> roughly equivalent to the beast::http::parser class (which is derived
> from beast::http::basic_parser):
>
> <https://github.com/boostorg/beast/blob/f09b2d3e1c9d383e5d0f57b1bf8895
> 68cf27c39f/include/boost/beast/http/parser.hpp#L45>
>
> Both of these classes store incoming structured HTTP elements in a
> container for holding HTTP message data. However note that unlike
> `beast::http::parser`, `my_socket_consumer` also has to know about
> buffers:
>
>     void on_socket_callback(asio::buffer data)
>     {
>         ....
>         buffer.push_back(data);
>         request_reader.set_buffer(buffer);
>
> It might not be evident to casual readers but the implementation of
> `my_socket_consumer` has to know that the parser needs the serialized
> version of the message to be entirely contained in a single contiguous
> buffer.

Nope. Go read again.

In my opinion this is a design flaw because it does not
> enforce a separation of concerns. The handling of structured HTTP
> elements should not concern itself with the need to assemble the
> incoming message into a single contiguous buffer; that responsibility
> lies with the stream algorithm.
>
> The design decision in Beast is to keep the interfaces used by stream
> algorithms separate from the interface used by consumers of HTTP
> tokens. Furthermore the design creates a standard interface so that
> stream algorithms can work with any instance of `basic_parser`,
> including both requests and responses, and for any user-defined
> derived class.

http://www.boost.org/doc/libs/develop/doc/html/boost_asio/reference/AsyncReadStream.html

Stream concepts that borrow ASIO concepts... that's so lame. You think the
users want access to a parser to implement an ASIO API? They will just use
Boost.Beast instead using a parser directly.

-- 
Vinícius dos Santos Oliveira
https://vinipsmaker.github.io/