On 12-10-17 06:47, Vinícius dos Santos Oliveira via Boost wrote:

What do you think about the following links?

What is the relevance of the links? They're extremely broad and general. If you are suggesting that the implementation of the parser interface should use parser combinators/generators, for sure. That is not necessarily an interface design concern. What _specific_ interface design concerns do you have in mind when linking these kind of general purpose libraries/approaches? Are you proposing a parser combinator library for Boost or the standard? (Would Spirit X3 fit the bill?). On 12-10-17 06:47, Vinícius dos Santos Oliveira via Boost 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/56e27d3b77d617ad1b4aea377f993592...

That is nice and tangible. Let's focus on concrete shortcomings, relevant to the library interface. Seth

On Wed, Oct 11, 2017 at 9:47 PM, Vinícius dos Santos Oliveira <vini.ipsmaker@gmail.com> 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/56e27d3b77d617ad1b4aea377f993592...

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 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, which you have done in your parser. The reason is that this unnecessary coupling pointlessly complicates the writing of the stream algorithm. 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); 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. 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. 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. 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/f09b2d3e1c9d383e5d0f57b1bf889568cf27c39f/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. 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. Thanks

On Fri, Oct 13, 2017 at 11:59 AM, Phil Endecott via Boost <boost@lists.boost.org> wrote:

Dear All, A "push" parser, which invokes client callbacks as tokens are processed, is easier to implement but harder to use as the client has to track its state between callbacks with e.g. an explicit FSM. On the other hand, a "pull parser" (possibly using an iterator interface) is easier for the client but instead now the parser may need the explicit state tracking.

That is generally true, and especially true for XML and other languages that have a similar structure. Specifically, that there are opening and closing tags which determine the validity of subsequent grammar, and have a recursive structure (like HTML). But this is not the case for HTTP. There are no opening and closing tags. There is no need to keep a "stack" of "open tags". It is quite straightforward. Therefore, when designing an HTTP parser we can place less emphasis on the style of parser and instead focus those energies to other considerations (as I described in my previous post, regarding the separation of concerns for stream algorithms and parser consumers). If you look at the Beast parser derived class, you can see that the state is quite minimal: template<bool isRequest, class Body, class Allocator> class parser : public basic_parser<isRequest, parser<isRequest, Body, Allocator>> { message<isRequest, Body, basic_fields<Allocator>> m_; typename Body::writer wr_; bool wr_inited_ = false; std::function<...> cb_h_; // for manual chunking std::function<...> cb_b_; // for manual chunking ... <https://github.com/boostorg/beast/blob/f09b2d3e1c9d383e5d0f57b1bf889568cf27c39f/include/boost/beast/http/parser.hpp#L45> Callbacks don't need to store state used by subsequent callbacks to interpret the incoming structured HTTP data, because HTTP is simple compared to XML or HTML.

Here's a very very rough sketch of what I have in mind, for the case of HTTP header parsing; note that I don't even have a compiler that supports coroutines yet so this is far from real code:

I think it is great that you're providing an example but you have chosen the most simple, regular part of HTTP which is the headers. I suspect that if you try to use the iterator model for the start-line (which is different for requests and responses) and then try to express the message body using iterators you will run into considerable difficulty coming up with a design that is elegant and feature-rich. Especially when you consider the need to transform the chunk-encoding while providing the metadata to the caller. I know this because I went through many iterations before settling on what is in Beast currently. Thanks

On Sat, Oct 14, 2017 at 12:03 PM, Phil Endecott via Boost <boost@lists.boost.org> wrote:

The issue of generator<T> providing only input iterators is the most significant issue I've spotted so far. This is in some way related to the whole ASIO "buffer sequence" thing; the code I posted before read into contiguous buffers, but that was lost before the downstream code saw it, so it couldn't hope to optimise with e.g. word-sized copies or compares.

Buffer sequences are not the problem, it is that parsed HTTP data types are heterogeneous. For example, the series of types generated when parsing a request looks like this: 1. std::pair<verb, string>: verb enum (if known) and method string 2. string: request-target string 3. integer (HTTP-version) 4. vector<tuple<field, string, string>>: field name enum (if known), name, value 5. vector<string>: body data OR 5. vector<string, string>: body data plus chunk-extension An interface which presents parsed data through a function return value (for example, an iterator's operator*) is only capable of yielding one type. The only way to use the same control flow and produce different types is to do two things: inform the caller of the type of the next incoming object, and then provide a set of functions from which the caller chooses the correct one with the proper matching return type for receiving the next value. You can see this in the Boost.Http parser calling code: do { request_reader.next(); switch (request_reader.code()) { case code::skip: // do nothing break; case code::method: method = request_reader.value<token::method>(); break; case code::request_target: request_target = request_reader.value<token::request_target>(); break; case code::version: version = request_reader.value<token::version>(); break; case code::field_name: last_header = request_reader.value<token::field_name>(); } } while(request_reader.code() != code::end_of_message); A viable alternative, which does not preserve the same structure of calling code, is to use a type of "visitor". The parser calls a user defined function specific to the next anticipated token, whose argument list has the correct types. This is the approach used in Beast. The parser calls a particular member function of the derived class depending on what structured element was parsed. The arguments to the member function have the correct high level types. For example, when Beast parses the request-line it invokes a member function with this signature in the derived class: /// Called after receiving the request-line (isRequest == true). void on_request_impl( verb method, // The method verb, verb::unknown if no match string_view method_str, // The method as a string string_view target, // The request-target int version, // The HTTP-version error_code& ec); // The error returned to the caller, if any Note the rich variety of types: `verb` is an enumeration of known HTTP methods: <http://www.boost.org/doc/libs/master/libs/beast/doc/html/beast/ref/boost__beast__http__verb.html> `method_str` is the exact method string extracted by the parser. This is needed when the method does not match one of the method strings known to the library, indicated by the enumeration value `verb::unknown`. `target` is a straightforward string, while `version` is conveyed as an integer. Since the parser owns the control flow at the time the member function is called, the `ec` output parameter allows the callee to indicate that it wishes to break out of the parser's loop and return control to the calling function. After the request-line comes zero or more calls to a member function with field name/value pairs. That member function signature looks like this: /// Called after receiving a header field. void on_field_impl( field f, // The known-field enumeration constant string_view name, // The field name string. string_view value, // The field value error_code& ec); // The error returned to the caller, if any Note how the collection of types presented for a header field is different from the request-line. Expressing this irregular stream of different types through an iterator interface is going to be very clumsy. Furthermore, there is metadata generated during the parse which is not easily reflected in an iterator interface. For example, after the HTTP headers have been parsed, Beast calculates the "keep-alive" semantic as well as the disposition of the Content-Length, which may be in three states: body-to-eof, chunked, or known. The keep-alive semantics are communicated to the caller of the parser through a member function `basic_parser::is_keep_alive`: <http://www.boost.org/doc/libs/master/libs/beast/doc/html/beast/ref/boost__beast__http__basic_parser/is_keep_alive.html> I described in a previous post how Beast's parser exposes two interfaces. The public interface is consumed by stream algorithms (e.g. read_some, async_read_some) while the derived class interface is used to store structure HTTP elements. The function `is_keep_alive` is exposed through the public interface of the parser because it is primarily of interest to the stream algorithm, since the stream algorithm concerns itself with the connection and whether or not it should be closed afterwards. Meanwhile, the Content-Length disposition is exposed to the derived class since it is a piece of metadata of interest to the algorithm which stores the body in the message container. It is communicated by the parser through a call to this derived class member: /// Called just before processing the body, if a body exists. void on_body_init_impl( boost::optional< std::uint64_t> const& content_length, // Content length if known, else `boost::none` error_code& ec); // The error returned to the caller, if any There is so much type irregularity in the information presented during the parse that I feel an iterator based approach would be, to use informal terms, "quite ugly." Thanks

2017-10-13 16:24 GMT-03:00 Vinnie Falco via Boost <boost@lists.boost.org>:

<https://github.com/boostorg/beast/blob/f09b2d3e1c9d383e5d0f 57b1bf889568cf27c39f/include/boost/beast/http/parser.hpp#L45>

Callbacks don't need to store state used by subsequent callbacks to interpret the incoming structured HTTP data, because HTTP is simple compared to XML or HTML.

Half-truth. Indeed, there is no need to store state to interpret incoming structured HTTP data, but we still may need to store state between one call and another. This was just the case in the previous example I gave you. Given the nature of the project Tufão using Qt, I have this thing called "safe signals": http://vinipsmaker.github.io/tufao/ref/1.x/safe-signal.html This safe signal of mine forbid me to access the object after I emit a signal. So I have to parse the whole received data before emitting any signal. What happens is... in the example I gave earlier, Boost.Beast parser will force me to have state inside the object itself: https://github.com/vinipsmaker/tufao/blob/56e27d3b77d617ad1b4aea377f9935 92bc2c0d77/src/httpserverrequest.cpp#L134 If you compare to the usage of Boost.Http parser, it's different: https://github.com/vinipsmaker/tufao/blob/1d7a943e4f6aae2284045f94e2b821 4a142dea9a/src/httpserverrequest.cpp#L135 (a local variable that only exists when it needs to) You should be more humble[1] about the user needs and wants or you'll limit the potential usefulness of your library. Therefore, when designing an HTTP parser we can place

less emphasis on the style of parser and instead focus those energies to other considerations

My project Tufão couldn't care less about the other stuff you grab from ASIO that you're trying to convert into the focus of the discussion. It's a Qt project and Qt networking is used. That's just the motivation for your separate submission (as users requested abstractions to parse HTTP without ASIO from the review comments). And I still find disappointing that I still need to show specific/concrete cases. [1] https://en.wikipedia.org/wiki/There_are_known_knowns -- Vinícius dos Santos Oliveira https://vinipsmaker.github.io/

On Sat, Oct 14, 2017 at 8:03 AM, Seth via Boost <boost@lists.boost.org> wrote:

¹ coroutines are not zero cost

That depends. I've done some investigation into the Coroutines TS described in n4134. For coroutines whose scope is strictly limited to the calling function, they can be implemented with zero cost (no dynamic allocation and comparable assembly output). The expository code that Phil provided certainly falls into that category. Thanks

On 14-10-17 20:04, Phil Endecott via Boost wrote:

In some cases they can have negative cost. See Gor Nishanov's CppCon 2015 presentation, "C++ Coroutines - a negative overhead abstraction".

I'm sorry I assumed from experience with Boost Coroutine only. This is indeed fantastic stuff and I had seen that particular vid. Thanks for correcting my memory, Seth

2017-10-12 9:45 GMT-03:00 Vinnie Falco via Boost <boost@lists.boost.org>:

On Wed, Oct 11, 2017 at 9:47 PM, Vinícius dos Santos Oliveira <vini.ipsmaker@gmail.com> 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/5ea65c64467e689cc9b67b8e66da6... 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/9908fe06d4b2364ce18ea9b416264... 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/AsyncRea... 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/