On Sat, Jul 1, 2017 at 7:46 AM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

+ This library is far better quality than when I last looked at it which was actually probably a long time ago. Congrats to Vinnie on that. It ain't easy getting it to this far.

Thanks! A lot of work went into it in the last couple of months, and the users chipped in with bug reports and helpful suggestions.

- For this collection of HTTP-related library routines...

It seems that WebSocket is the red-headed stepchild that no one talks about. Of course, I understand that the HTTP portions of Beast are far more interesting (controversial?) so I guess we'll be talking about that for most of the review.

...which is what this library actually is

I think the author of Beast might have some idea of "what this library actually is", especially when it is stated in the documentation: <http://vinniefalco.github.io/beast/beast/using_http.html>

I do not see why any awareness of ASIO is needed at all.

Also stated in the documentation: "...using the consistent asynchronous model of Boost.Asio." <http://vinniefalco.github.io/beast/beast/introduction.html>

I don't get why it's needed,

Boost.Asio is effectively what will become the standard way to do C++ networking (i.e. N4588). Beast recognizes that C++ will soon have a common, cross-platform networking interface and builds on that interface exclusively.

and moreover, I think the library would be far better off removing all awareness of a networking implementation entirely.

Beast stream algorithms are not dependent on any networking implementation. It only depends on a networking interface. Specifically, Beast stream algorithms require the following concepts: SyncReadStream SyncWriteStream AsyncReadStream AsyncWriteStream ConstBufferSequence MutableBufferSequence DynamicBuffer It is entirely possible to create your own objects which meet these requirements that do not use Asio sockets. For example, a libuv socket can be wrapped in a manner that makes it usable with Beast. None of the concepts above require a particular networking implementation, subject to the following caveat: The Boost.Asio requirements for the stream and buffer concepts listed above DO require Asio. Specifically they require the following concrete types (plus a few more related to asynchronous initiating function customization points which I won't bore readers with): boost::asio::io_service boost::asio::const_buffer boost::asio::mutable_buffer At the moment, Beast is tied to Boost.Asio because of the aforementioned types. However, N4588 introduces significant improvements to the networking interface. The concrete io_service class is replaced with a new concept called an Executor. The buffer requirements are relaxed, instead of requiring a concrete type such as const_buffer it is possible to use any type that offers a data() and size() member. Thus, N4588 removes the last vestiges of implementation dependence on code written for it. My plan for Beast is to port it to N4588 as soon as it becomes available in an official capacity in the standard libraries used by clang, gcc, or msvc. So to answer your question, Boost.Asio is needed today because N4588 is not part of the standard. When it becomes standardized, Beast will be dependent on the C++ Standard Library networking interface (interface, not implementation as you stated).

Most of the routines Beast supplies that I examined would be more reusable, easier to use, and less templatey if they made zero assumptions about what networking implementation is in use.

beast::http::serializer and beast::http::basic_parser make no assumptions about what networking implementation is in use. Perhaps you can demonstrate a sample program using those Beast classes which makes zero assumptions about which networking implementation is in use, that works with at least two different implementations; say, Boost.Asio and libUV? Personally, I don't see a point to investing resources into supporting networking implementations which will not become part of the C++ Standard Library.

WG21 has much superior vocabulary types for doing buffer sequences than ASIO's which are needlessly complex, over engineered, and over wraught.

I intend to make sure Beast is compatible with the standard so whatever buffer sequences make it in to the standard, is what Beast will use. If you believe that the buffer sequence concepts in N4588 are deficient, I strongly advise you to open a LEWG issue before the committee makes a terrible mistake! There's still time.

A new library should not repeat the design mistakes of ASIO unless it has to, and I see no compelling reason given the limited implementation of HTTP offered by Beast to have any dependency on ASIO at all.

It is only Beast's stream algorithms which use Boost.Asio's stream concepts.

The ASIO design model should be replaced with a more generic reusable design instead, one which works with any arbitrary socket API - or indeed, non-socket API.

You can always write your own stream algorithm using beast::http::basic_parser and beast::http::serializer which do not work with streams at all and do not require Boost.Asio, subject to the following caveat: These Beast classes use Boost.Asio's buffer concepts which are tied to a couple of concrete types. This dependence will disappear in the port to N4588, which does not mandate concrete types.

With Beast's current design, trying to use Beast with the UDT API is going to be unnatural and awkward. If Beast were instead a reusable, generic, HTTP related utility library, it would be a much better library for it.

I'm not sure I agree, you can always implement your own stream algorithm which uses UDT API and use beast::http::basic_parser and beast::http::serializer with it. The documentation even provides examples of how you can use the serializer and parser with std::ostream and std::istream, bypassing Asio streams entirely: http://vinniefalco.github.io/beast/beast/using_http/buffer_oriented_serializ... http://vinniefalco.github.io/beast/beast/using_http/buffer_oriented_parsing....

The choice to use STL allocators I feel is a code smell.

Beast intends to use standard concepts. Allocator and AllocatorAwareContainer are concepts defined by the C++ standard, and the universally understood models for allocator customization points. If you feel that "STL allocators" are a "code smell" then I urge you to submit a paper to WG21 with actionable advice (ideally, proposed wording) on how to banish the stench.

I think a correct HTTP primitive library would never allocate memory, and thus never need to call a STL allocator.

beast::http::serializer never allocates memory. beast::http::basic_parser never allocates memory when its input is a single buffer. An optimized allocator is provided in the http-server-fast example which makes basic_fields never allocate from the heap: https://github.com/vinniefalco/Beast/blob/78a065ba39836d91d7e70d93de7f9140f5... Beast also comes with an extensive set of tools to empower users to avoid memory allocations: http://vinniefalco.github.io/beast/beast/ref/beast__http__basic_dynamic_body... http://vinniefalco.github.io/beast/beast/ref/beast__http__string_view_body.h... http://vinniefalco.github.io/beast/beast/ref/beast__static_buffer.html http://vinniefalco.github.io/beast/beast/ref/beast__static_buffer_n.html http://vinniefalco.github.io/beast/beast/ref/beast__static_string.html Here is a declaration for a message which performs no dynamic allocations, which allows a a header of up to 4096 bytes and a body of up to 16384 bytes: #include <beast/core.hpp> #include <beast/http.hpp> #include <example/http-server-fast/fields_alloc.hpp> fields_alloc fa{4096}; beast::http::response< beast::http::basic_dynamic_body<beast::static_buffer_n<16384>, beast::http::basic_fields<fields_alloc>> res{ std::piecewise_construct, std::make_tuple(), std::make_tuple(fa)};

- I appreciate that this my final negative will not be widely agreed with on boost-dev, but personally speaking I think Beast should work well with C++ exceptions disabled, and therefore make no use of exception throws at all.

The DynamicBuffer concept prescribes throwing std::length_error if the buffer maximum size is exceeded: <http://vinniefalco.github.io/beast/beast/concept/DynamicBuffer.html> This is a N4588 concept and will eventually become part of the C++ standard. If you feel that N4588 is deficient because it has a concept which mandates exceptions, I urge you to open a LEWG issue before the committee makes a terrible mistake! There's still time.

There are lots of end users who would like to speak HTTP without enabling C++ exceptions for cultural or safety validation reasons: embedded devices, IoT, games, medical devices etc. Beast is not useful to any of those use cases if it insists on ASIO and throws exceptions, neither of which are necessary nor make for a better HTTP utilities library.

beast::http::serializer and beast::http::basic_parser do not throw exceptions. Thanks

On Sat, Jul 1, 2017 at 11:08 AM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

Beast WebSocket belongs in another, separate library. That library could bring in ASIO as a dependency.

Beast is provided as a coherent package of components that work well together, with integrated tests and documentation. It is presented as a single library.

1. Message containers 2. Stream reading 3. Stream writing 4. Serialisation 5. Parsing You have told me no reason why any of these needs a hard dependency on any networking implementation, or awareness of any specific networking design pattern.

(repeating myself) Beast's message containers, serialization, and parsing do not depend on any specific networking interface. I do not know of a way to write an algorithm which works with an unspecified stream concept, so I had to choose which concepts I wanted to work with. I chose these Boost.Asio concepts because they are the closest thing to becoming a standard: SyncReadStream SyncWriteStream AsyncReadStream AsyncWriteStream DynamicBuffer Perhaps you can demonstrate how a network algorithm may be written which works with an unspecified stream concept? How about a simple, synchronous function that writes a string, I'll start you off with a function signature: template<class Stream> void write(Stream& stream, std::string_view string); Please implement this function for us and demonstrate how it works with Boost.Asio, Networking-TS, POSIX sockets, WinSock, UDT, or libUV without modification.

**HTTP has nothing to do with networking**.

Beast offers algorithms to serialize and parse HTTP messages on Boost.Asio streams. If you think that is not part of "HTTP" that's fine, the label is unimportant. What is important is that Beast offers this functionality.

It therefore seems to me that reading ought to be calculated views of underlying byte data, and writing ought to be compositing a sequence of byte spans to gather into a send.

Correct. You have described the operations performed by beast::http::serializer and beast::http::basic_parser, which do not require any networking interface.

WG21 has much superior vocabulary types for doing buffer sequences than ASIO's which are needlessly complex, over engineered, and over wraught

Please specify the WG21 vocabulary types you are referring to. Thanks

On Sat, Jul 1, 2017 at 1:38 PM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

At the very minimum, I think Beast needs to become two, separate libraries: (i) the HTTP utility library (ii) WebSocket.

Beast is proposed as-is.

The natural split point for Beast into multiple, focused libraries is between the code which only concerns itself with structured data, and everything else.

Beast is proposed as-is.

So why are you forcing end users to drag in ASIO?

Beast will be part of Boost, whose distributions include Asio. To use serializer or basic_parser, the header <boost/asio/buffer.hpp> must be included, which is a pretty self-contained file. This is a temporary situation until Boost.Asio is updated to N4588.

The reason why according to you is for the buffer infrastructure. But as I've already told you, that's a relic from a decade ago. New code neither ought to nor needs to use that. We have far better available today.

That "relic from a decade ago" is in the latest Boost 1.65.0. As I have targeted Boost.Asio specifically, you will naturally understand that Beast uses that buffer infrastructure for better or for worse. When a version of Boost.Asio appears which is "far better", then Beast will be ported to it. The stand-alone Asio is already a bit ahead of Boost.Asio in its buffer technologies; since you feel that Boost.Asio is so far behind perhaps you can take on the task of porting stand-alone Asio's changes to Boost?

And if the really reusable parts of Beast, the ones not dependent on ASIO except for some buffer adapters, can be broken off and be made free of ASIO, that's a big value add to end users who don't need WebSocket and just want a HTTP utilities library.

Eliminating the dependence on Asio's buffers is something that is on my horizon, but it is not a feature for the library being proposed. There hasn't been much demand for using the parser or serializer without <boost/asio/buffer.hpp>. But if that were to change, I would likely reprioritize the feature. A port to N4588 will automatically solve the problem.

...

...

WG21 has much superior vocabulary types for doing buffer sequences than ASIO's which are needlessly complex, over engineered, and over wraught

Please specify the WG21 vocabulary types you are referring to. The Ranges TS is an obvious place to start from as a source of Concepts

Until you file the LEWG issue where you describe N4588 buffer sequence concepts as "needlessly complex, over engineered, and over wraught[sic]" and provide an alternative in the form of proposed language, there is nothing actionable in your statement.

Very little in your library has, or ought to have, anything to do with i/o. .. Stop thinking in terms of i/o. HTTP is just structured data. So treat it as such. Parse as structured data, generate as structured data.

Is my understanding correct that you say Beast should not provide functions to send and receiving HTTP messages using Asio streams? Thanks

On Sun, Jul 2, 2017 at 8:55 AM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

...

Very little in your library has, or ought to have, anything to do with i/o.

Is my understanding correct that you say Beast should not provide functions to send and receive HTTP messages using Asio streams? Thanks

On Sun, Jul 2, 2017 at 8:55 AM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

...

Is my understanding correct that you say Beast should not provide functions to send and receiving HTTP messages using Asio streams?

Absolutely correct.

This opinion is so far out of touch with the feedback I have gotten from users about what they want, that it raises the question whether your screeds have any value.

On Sun, Jul 2, 2017 at 9:07 AM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

Ok, Vinnie. Enough of the aggression. ... If you don't want my feedback because of some grudge against me, say so

There's no grudge but this is what I'm hearing: "Beast should not perform socket I/O." I don't know if its your style of speaking, or word choice but this feedback is patently absurd on the face of it. So you think users shouldn't be able to implement a web server or HTTP client using Beast? How does one even begin to respond to that? I'm going to push back considerably on statements of the form "your library shouldn't do what it does" such as yours. I want to give you the benefit of the doubt, maybe you're trying to say something else such as, the library should be presented as two separate libraries? But that's not how it sounds. Thanks

On Sun, Jul 2, 2017 at 5:00 PM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

So free your library of the ASIO dependency.

1. Beast stream operations are built for Boost.Asio. 2. Beast's dynamic buffers and buffer adapters are built for <boost/asio/buffer.hpp> This is not going to change. When I port Beast to Net-TS, I will make similar statements: 1. Beast stream operations are built for Net-TS 2. Beast's dynamic buffers and buffer adapters are built for <experimental/network/buffers> (or whatever the header is called) I have no desire to enlarge the scope of Beast beyond that which is suitable for standardization. Like it or not, Boost.Asio is the closest we have to something standard, and soon it will be Net-TS which is practically identical to Asio except for cosmetic difference in buffer types. If you feel that Beast should work with non-Asio I/O models then the venue for that fight is in the LEWG. Beast will track the standard or as close to it as possible. So if you convince the working group there is room in the standard for networking I/O models different from Net-TS, Beast will adopt them as a matter of policy. If you want to fork Beast and show me how its done you have my blessing. If your fork became popular I would definitely reconsider.

...your justifications of design choice to me, and persuasion that your choices are not showstoppers, are what gets a library accepted.

I can't imagine any argument more persuasive than "it tracks standards."

I think your library should...Do one thing really well

On this we can agree. That "one thing" is "HTTP operations on Boost.Asio stream concepts." ...and WebSockets too! Why do they neglect the websockets? Its like the kid who always gets picked last for the team at P.E. Thanks

On Sun, Jul 2, 2017 at 5:17 PM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

(note to this reply: on his request, Vinnie and I have replayed an offlist conversation we had back onto boost-dev in order to get feedback.

Thank you. The conversation was of sufficient importance to merit an on-list discussion.

I really think you need to tell the Boost peer review that, because I don't think anyone else realises that either.

Did anyone not realize that Beast's roadmap includes a standardization proposal? If so then the documentation and my mailing list comments have done a poor job...as Prego say's "It's in there!"

...if you want Beast into the C++ standard, you really ought to say that's your plan now as an announcement. You'll get a much more useful to you review

See item 5: <http://vinniefalco.github.io/stage/beast/review/beast/design_choices/faq.html>

WG21...see ASIO as the interim Networking v1, with a strong expectation that a v2 will replace it once they've done iostreams v2

I don't have the same experience and involvement with the C++ luminaries that you have, so I lack an inside track on what will become part of C++2023 or C++2026. Meanwhile, Beast users want HTTP solutions today so in the absence of reliable tea leaves I can only design for what is standard (or close to it). Thanks

On Sun, Jul 2, 2017 at 6:14 PM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

Do remember I suggested you design for a different part of the standard, not away from the standard.

Is there another networking proposal hiding somewhere that I am not privy to? Thanks

On Sun, Jul 2, 2017 at 7:44 PM, Edward Diener via Boost < boost@lists.boost.org> wrote:

On 7/2/2017 8:17 PM, Niall Douglas via Boost wrote:

...

(note to this reply: on his request, Vinnie and I have replayed an offlist conversation we had back onto boost-dev in order to get feedback. Hence the tone adopted is a bit weird and I'm talking about boost-dev as a third party, which makes no sense if I were writing this to here. Be aware I have edited this message somewhat over the original as this goes on the public record, it is not identical to the original)

I have no desire to enlarge the scope of Beast beyond that which is

...

suitable for standardization.

I would be absolutely amazed if HTTP hard coded for the Networking TS could be standardised.

There are tons of big C++ users out there who would rightly veto such a thing. Qt provides an extensive networking implementation which does not work well with ASIO. So does Apple, Google and Microsoft, all of whom have substantial proprietary networking implementations, and whose WG21 reps would veto such a proposal in my opinion.

If you are aiming for Beast to be standardised, I think you are absolutely dead in the water with the current design. I had no idea until now that you intended that, and I really think you need to tell the Boost peer review that, because I don't think anyone else realises that either.

No, this sort of thing is irrelevant. Whether any Boost library would like to be part of the C++ standard or not should not come into consideration when reviewers look at a library to be added to Boost. In reality nearly all Boost authors of libraries probably would like their libraries to be added as a C++ standard library, but this has nothing to do with the quality of the library itself.

+1 In C and C++ it is possible to design libraries that are both portable and low overhead compared to pretty much any other language. While Python may need all kinds of libraries to do basic things, in C and C++ many times there is no need for a library to be standardized, there would be little gain. Consider for example something like libpng or Boost -- they're available, often preinstalled on pretty much any operating system under the sun already. Further, I think it is a bad idea to standardize a library before it has been deployed for many years outside of the standard, no matter how good it may be. If it is that good, people will reach and use it anyway, and if not the standard is better for not including it. It is especially evil to attempt to standardize a library as means to force adoption -- again, if the library is better than any other alternative, people will use it. If not, it has no place in the standard.

On Mon, Jul 3, 2017 at 9:37 AM, Phil Endecott via Boost <boost@lists.boost.org> wrote:

I think this is an important point - on what platforms is Beast actually useful in practice?

Wherever Boost.Asio is useful.

How much of Beast is useful without ASIO?

Not very much, although you can use beast::http::serializer and beast::http::basic_parser using only <boost/asio/buffer.hpp> which is pretty small, self contained, and knows nothing about sockets. There is a point of contention here, one reviewer thinks that buffer.hpp should not be a dependency. However that discussion is stalled due to non-technical reasons. Realistically speaking if you are trying to use Beast without Asio (modulo buffer.hpp as stated above) you aren't going to get very far at all.

On Mon, Jul 3, 2017 at 11:54 AM, Emil Dotchevski via Boost < boost@lists.boost.org> wrote:

On Mon, Jul 3, 2017 at 9:40 AM, Vinnie Falco via Boost < boost@lists.boost.org> wrote:

...

...

How much of Beast is useful without ASIO?

Realistically speaking if you are trying to use Beast without Asio (modulo buffer.hpp as stated above) you aren't going to get very far at all.

In my opinion, arguing that coupling is necessary should begin with defining an interface which Beast can use, which can be trivially implemented in terms of Asio. In designing this interface the only concern should be avoiding the coupling; specifically, performance considerations should be completely ignored. Only then we can have a reasonable discussion is it worth it.

This is a very good way to think about this.

Regardless, in my experience this kind of exercise always improves the design of a library.

This is definitely true. I think those two points are (mostly?) what Niall was trying to convey. Zach

On Sun, Jul 2, 2017 at 5:00 PM, Niall Douglas via Boost < boost@lists.boost.org> wrote:

On 02/07/2017 18:25, Vinnie Falco wrote:

...

On Sun, Jul 2, 2017 at 9:07 AM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

...

Ok, Vinnie. Enough of the aggression. ... If you don't want my feedback because of some grudge against me, say so

There's no grudge but this is what I'm hearing:

"Beast should not perform socket I/O."

I don't know if its your style of speaking, or word choice but this feedback is patently absurd on the face of it.

I don't see the absurdity at all.

Let me give you some background.

I've implemented basic HTTP perhaps six times now in my career to date. Four times in C/C++, twice in Python. I don't have the depth of knowledge of full fat HTTP like say Bjorn does, seeing as he contributed extensively to curl, but I've implemented this stuff lots of times now.

I think Vinnie is right that it is "your style". "I've done HTTP six times in my career" is not an argument, the subtext is "you're inexperienced and, this library is a nice try, but if you want to play with the big boys you have to do it differently." Further, I know many smart people who have done something six times in their career, and never got close to a good design. I'm not an expert so I can't comment on the Beast (other than to say that IMO it is a poor choice of name), but it would be helpful (including to people like me) to add structure to your criticisms; e.g. this is what the library does now, but this is a bad idea for this and that reason, and it would be better to do it this way, rather than "dude, this is so 10 years out of date!" :)

Niall Douglas wrote:

What I didn't do was write essays of detail. It would take too long, it's not my place, and I'm pretty sure Vinnie knows ASIO internals as well as I do, so I am assuming he understands everything I'm writing.

I understand everything you're writing, but the complete picture still eludes me. Specific examples: http::request_parser<http::string_body> parser; parser.header_limit( 8192 ); parser.body_limit( 8192 ); http::read( socket_, buffer, parser, ec ); /*...*/ do_request( parser.get(), ec ); How would this look like in the blobs in, blobs out model? Same question for the reverse one: std::vector<unsigned char> data; /* ... fill data... */ beast::string_view sv( (char*)data.data(), data.size() ); http::response<http::string_view_body> res( sv ); res.result( http::status::ok ); res.set( http::field::server, BEAST_VERSION_STRING ); res.set( http::field::content_type, "image/bmp" ); res.set( http::field::content_length, data.size() ); http::write( socket_, res, ec ); How does the ASIO-decoupled code look?

On Sun, Jul 2, 2017 at 6:43 PM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

char buffer[16384]; // no need to explicitly limit header + body

// Create a HTTP request parsing view of char[16384] http::request_parser<http::string_body> parser(buffer);

// Read enough data to parse the request char *p = buffer; do { p += asio::read(socket, asio::buffer(p, buffer + sizeof(buffer) - p)); } while(!parser.has_body(p - buffer)); do_request(parser.get(), ec);

// Use any spill at the end memcpy(buffer, buffer + parser.size_bytes(), p - (buffer + parser.size_bytes())); p = buffer;

You're asking for beast::http::parser to not use <boost/asio/buffer.hpp>

// These prepend additional gather buffers for the headers res.result( http::status::ok ); res.set( http::field::server, BEAST_VERSION_STRING ); res.set( http::field::content_type, "image/bmp" ); res.set( http::field::content_length, data.size() );

// Tell ASIO to send the response by gathering all the buffers asio::write(socket, boost::make_iterator_range(res.begin(), res.end()));

This is in no way equivalent to what Beast does now, I suggest you study BodyReader and http::serializer. Still, it sounds like again you are simply asking that http::serializer not use <boost/asio/buffer.hpp> I will solicit help (not from you) to see if this problem can be solved in a fashion that remains Asio friendly and also preserves Beast's features (your examples don't, but you are unfamiliar with the library so that is understandable). Thanks

On 03/07/2017 03:07, Vinnie Falco via Boost wrote:

On Sun, Jul 2, 2017 at 6:43 PM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

...

It's nearly 3am here...

I looked once again at http::basic_parser and http::serializer. It might be possible to refactor http::basic_parser into a separate class and with some mess in http::parser support two versions. One which allows ConstBufferSequence and the other which allows just pair<void*, std::size>.

I've always felt concern about pair<void*, size_t>. AFIO v2 uses it, but I am uncomfortable with it. In your case you can't use it because if you declare it as a buffer type to ASIO and another piece of code does the same, you get an ODR violation. So probably a struct beast::buffer { void *data; size_t length; }; or something is safer.

http::serializer is another story entirely. It contains and is tied to adapters which contain Asio buffer types. And it uses Asio buffer algorithms.

You've taken the position that this is an easy matter, that you have extensive knowledge and experience in the domain, so I would kindly ask that you provide a working prototype of http::serializer which does not use Asio buffer types and yet remains compatible with the rest of Beast.

Your http::serializer claims it is for: 1. Send the header first, and the body later. 2. Set chunk extensions or trailers using a chunk decorator. 3. Send a message incrementally: bounded work in each I/O cycle. 4. Use a series of caller-provided buffers to represent the body. The way I've always implemented HTTP serialisers in the recent past is as a collection of open ranges of contiguous byte buffers for ASIO to gather send from. Generally I've kept a headers() collection and a body() collection. Iterators for the whole serialiser iterate all the headers first, then the body. But if the end user wanted to send just the headers first, they simply by hand feed the headers range to ASIO first, then the body range. So that's Item 1 taken care of. Item 2, as I mentioned before I've never needed more than basic HTTP in my career to date, so never needed to implement chunked encoding. But I can tell you what my first instinct is: body() is a FIFO drain, so as the user appends new body buffers, they get drained and sent. So, the end iterator returned by body() is able to stop pointing after the last item in body() if new items are added to body(). Item 3 is very straightforward because the end user is the person creating the iterator range to hand to ASIO. So they simply choose a range which is a subset of the total instead of the total. Item 4 is even easier, because this entire design already is a series of caller provided buffers from start to finish. Beast might provide convenience functions for generating those buffers, but essentially the caller supplies the buffers. Finally, trailers I have never had need to implement either, but I'd suggest a trailers() collection for those as well. Exactly the same semantics as earlier. I appreciate Vinnie that you envisioned Beast as doing all this stuff for the end user and hiding all this complexity by wrapping up ASIO with an i/o API which hides how ASIO is being used underneath. I think that makes sense in a high level HTTP library. For a low level HTTP library I think it a design mistake, much simpler is much better. Less is more. I've used the above serialiser design on a number of occasions now, it's very efficient, composable, and flexible. It *does* push understanding of HTTP onto the end user, but then if the end user doesn't understand HTTP, they wouldn't be able to use a low level library anyway. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/

...

Generally I've kept a headers() collection and a body() collection.

Words are cheap. Please provide a serializer which compiles and runs, with tests, and operates on Beast messages and does not use <boost/asio/buffer.hpp>. I've provided something that works, time for you to do the same.

That's not how Boost reviews work Vinnie. We each provide observations, comments, questions and proposals of alternatives. We discuss. We may each provide formal reviews. The review manager considers all of the discussion, and provides a judgement. So I don't need to convince *you* of anything. I need to convince everybody else on boost-dev, and thence Michael. It's a bonus to convince you too, but it's not a requirement. I think there is a great library lurking in Beast, and you've done a great job bringing us a HTTP library for C++ that is far better than any I've seen yet. But I think some more chiselling is yet needed to hew it out. In particular, I think it's not consistently low level enough yet, and I think a View + Ranges API design for the bottom layer makes a lot more sense, with additional higher level ASIO-dependent layers laid on top. If they others disagree with this opinion, they'll (usually silently) ignore it. If they don't understand it, they'll ask for clarification. If they agree, they'll formally vote the same way or similarly. That's how reviews work. (It's also very possible that another reviewer blows a big hole in my alternative design as having a showstopper design fault, and I thus change *my* opinion. You might consider taking this dialectical approach of proving why my ideas can't work rather than challenging people to write alternative implementations. If I had the spare time to do that, I'd have written a HTTP library a long time ago and brought it here for review. So would many others on here who have far more experience in HTTP than either you or I, and if properly resourced, could have made the definitive implementation bar none) Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/

2017-07-03 14:57 GMT+02:00 Vinnie Falco via Boost <boost@lists.boost.org>:

...

Generally I've kept a headers() collection and a body() collection.

Words are cheap. Please provide a serializer which compiles and runs, with tests, and operates on Beast messages and does not use <boost/asio/buffer.hpp>. I've provided something that works, time for you to do the same.

This is one of the most ridiculous things one can say during a Boost review. It's like freeing yourself from any critique. Your vote only counts if you've implemented a library that can do this, this and that. So I'll implement one feature that nobody uses today and then nobody can criticize my library. What's the purpose of the review then? If the review pleases you, it's one more approval. If the review displeases you, you just need to run to “you aren't Beast author nor any library close to that so your opinion don't count”. I'd **never** reply such nonsense in reviews done to libraries of mine. -- Vinícius dos Santos Oliveira https://vinipsmaker.github.io/

...

I think that makes sense in a high level HTTP library. For a low level HTTP library I think it a design mistake, much simpler is much better. Less is more. I've used the above serialiser design on a number of occasions now, it's very efficient, composable, and flexible. It *does* push understanding of HTTP onto the end user, but then if the end user doesn't understand HTTP, they wouldn't be able to use a low level library anyway.

If BEAST is accepted, is there any reason why such an even--lower-level library could not (or should not) be written (and also accepted)?

That's a great question. It could be that I am over egging the importance of there being a lower layer independent of networking implementation. But then my past HTTP implementations have tended to go over some really weird non-socket transport layers, so I'm going on what I know. Hence I delay my review until others have given theirs. I know that's a cop out, but it's also recognition that my experience of "normal" HTTP is limited, and I could be making unreasonable demands. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/

On 03/07/2017 16:27, Vinnie Falco via Boost wrote:

On Mon, Jul 3, 2017 at 8:19 AM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

...

That's a great question. It could be that I am over egging the importance of there being a lower layer independent of networking implementation.

Is your issue with beast::http::basic_parser and beast::serializer that it depends on <boost/asio/buffer.hpp>? Would your needs be satisfied if it did not have this dependence?

I did warn you to stop with the aggression Vinnie. So ok, I am out of this review now. No more from me until the end when I write my formal review. Good luck with the review Vinnie. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/

On Mon, Jul 3, 2017 at 6:19 AM, Paul A. Bristow via Boost < boost@lists.boost.org> wrote:

...

I think that makes sense in a high level HTTP library. For a low level HTTP library I think it a design mistake, much simpler is much better. Less is more. I've used the above serialiser design on a number of occasions now, it's very efficient, composable, and flexible. It *does* push understanding of HTTP onto the end user, but then if the end user doesn't understand HTTP, they wouldn't be able to use a low level library anyway.

If BEAST is accepted, is there any reason why such an even--lower-level library could not (or should not) be written (and also accepted)?

I don't see why not. For example I think it was a mistake to reject Synapse on the basis of "we have a signal library in Boost already", because there is literally nothing in common between it and Boost Signal, except that the latter can be used in a subset of the cases where Synapse can be used. If we reject a library it should be on its own merits, not on the merits of another library, except if the differences are trivial.

Just one perhaps uninformed remark. I have glanced over this thread as I like to keep in touch with what happens in boost, but have no need for a websocket interface. To me, Vinnies library seems to be a well-designed and capable library. But it seems to me that the library tries to accomplish two things that coud be separated: parsing of http-headers and transmission of http-headers. My guess is that - as Niall pointed out - there is a large group of developers that could benefit from the parsing part alone, and probably also a group that would perform their own parsing and just use the transfer part. If this is so, Beast should be split into two libraries. Perhaps a fast way to accomplish that would be to have a small helper library that could convert to/from asio-buffers to e.g. std::vector<(w)char> and std::(w)string? Even if this caused suboptimal performance, my guess is that this approach would have appeal and not frighten to many users. We have had libraries in boost with suboptimal performance before (lexical_cast is one such example) where later revisions reduced or eliminated the performance loss. /Peter On Mon, Jul 3, 2017 at 3:21 PM, Vinnie Falco via Boost <boost@lists.boost.org> wrote:

On Mon, Jul 3, 2017 at 5:33 AM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

...

...ASIO...

You've said a lot and I'd like some clarity, is your issue with beast::http::basic_parser and beast::serializer that it depends on <boost/asio/buffer.hpp>? Would your needs be satisfied if it did not have this dependence?

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

On 03/07/2017 14:21, Vinnie Falco via Boost wrote:

On Mon, Jul 3, 2017 at 5:33 AM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

...

...ASIO...

You've said a lot and I'd like some clarity, is your issue with beast::http::basic_parser and beast::serializer that it depends on <boost/asio/buffer.hpp>? Would your needs be satisfied if it did not have this dependence?

If you also stopped implementing i/o, and instead facilitated i/o, I believe I could live with your other design choices. You've made a good library Vinnie. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/

On Mon, Jul 3, 2017 at 8:24 AM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

If you also stopped implementing i/o, and instead facilitated i/o, I believe I could live with your other design choices.

Are you suggesting that Beast should not support reading and writing HTTP messages on Asio streams? -------------------------------------------------- On Sun, Jul 2, 2017 at 8:55 AM, Niall Douglas wrote:

...

..you say Beast should not provide functions to send and receive HTTP messages using Asio streams?

Absolutely correct.

On Sat, Jul 1, 2017 at 1:38 PM, Niall Douglas via Boost < boost@lists.boost.org> wrote:

The reason why according to you is for the buffer infrastructure. But as I've already told you, that's a relic from a decade ago. New code neither ought to nor needs to use that. We have far better available today.

"A relic from a decade ago" is not an argument.

On Sun, Jul 2, 2017 at 8:07 AM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

...

Very little in your library has, or ought to have, anything to do with i/o.

Is my understanding correct that you say Beast should not provide functions to send and receive HTTP messages using Asio streams? Thanks

2017-07-01 22:38 GMT+02:00 Niall Douglas via Boost <boost@lists.boost.org>:

If that's too experimental for you (though the Ranges TS is a TS just like the Networking TS), then do as Boost.Spirit does and work with iterator pairs. So for example, if I want to know what the Content-Length header is, upon query I get back an iterator pair pointing to the front of the storage and one after the end of the storage. Similarly, if I set the Content-Length header, I supply an iterator pair to the new contents.

That would be pretty bad as HTTP also involves connection management (e.g. "connection: close"). It'd work for an HTTP parser. But Beast seems pretty low-level already. -- Vinícius dos Santos Oliveira https://vinipsmaker.github.io/

On Sat, Jul 1, 2017 at 7:46 AM, Niall Douglas via Boost < boost@lists.boost.org> wrote:

There are lots of end users who would like to speak HTTP without enabling C++ exceptions for cultural or safety validation reasons: embedded devices, IoT, games, medical devices etc.

You forgot to mention "unpredictability" of exceptions. :) Also, there is no error handling in games.

Beast is not useful to any of those use cases if it insists on ASIO and throws exceptions, neither of which are necessary nor make for a better HTTP utilities library.

They're certainly not necessary for users who don't need robust error handling.

On Sat, Jul 1, 2017 at 12:52 PM, Niall Douglas via Boost <boost@lists.boost.org> wrote:

Anyone not error handling in HTTP needs their head examined.

I'll kindly ask that you do not make inflammatory statements in threads which mention Beast in the subject line. Some folks have relatives or friends who may be mentally ill, and your ad-hominem implies that those individuals are somehow less worthy of respect. Such language is non-inclusive and diminishes the stature of the list (in my opinion).

I only found the time to join the discussion now 😢 I'd appreciate if 4 more days were given to review it, but I'll do my best to review it until July 10. This is the first message I'll answer from the whole discussion. Time to have fun discussing HTTP 😀 2017-07-01 16:46 GMT+02:00 Niall Douglas via Boost <boost@lists.boost.org>:

- I think basic_parser should be designed to use forward iterators instead of requiring random access, or accept partial input in chunks whilst keeping state. This would allow it to work seamlessly with discontiguous underlying storage.

After the review on my library, I noted there was a big interest in low-level control for handling HTTP messages. I developed a parser library which has the good properties of the NodeJS parser (no syscalls, no allocation/internal-buffers, interruptable at any time...) and avoids its bad design decisions[1]. In less than 170 lines you can handle the receiving part of your library: https://github.com/vinipsmaker/tufao/blob/43cd33f3388fd7061dedc75d683810a258... And I don't assume you're going to read RFC7230. Decoded values are decoded (different than NodeJS "easy-to-use-wrong" parser). Anyway, it's fairly easy to adapt this parser model to C++ iterator model (I had this concern in mind when designing the API — as a matter of fact, one of the test files adapts the parser model so I can define a lot of tests just by comparing two vectors[2]). And I might answer this same message again after finishing the review. Lots of hours ahead of me to spend with HTTP. [1] https://vinipsmaker.wordpress.com/2016/08/05/boost-http-has-a-new-parser/ [2] https://github.com/BoostGSoC14/boost.http/blob/b00380a1f8c2588baa4cd57324606... -- Vinícius dos Santos Oliveira https://vinipsmaker.github.io/

2017-07-08 16:51 GMT+02:00 Vinnie Falco via Boost <boost@lists.boost.org>:

On Sat, Jul 8, 2017 at 7:19 AM, Vinícius dos Santos Oliveira via Boost <boost@lists.boost.org> wrote:

...

I only found the time to join the discussion now 😢

Glad to see you here Vinícius! I see you've been busy working on your repositories (I check on them from time to time) - that's great!

Yep. I've been more busy with my work than my open source contributions, but I donate time to my open repos from time to time.

Anyway, it's fairly easy to adapt this parser model to C++ iterator model

Using templated iterators (I think that's the direction you're implying) offers flexibility. But there are advantages when using a raw pointer to a contiguous piece of memory holding the entire HTTP header or other structured element (chunk-header, final-chunk). Such as using SSE4.2 intrinsics to accelerate the parsing:

<https://github.com/vinniefalco/Beast/blob/6f88f0182d461e9cabe55032f966c9 ca4f77bf46/include/beast/http/detail/basic_parser.hpp#L223>

Nope. I'd point to the documentation, but I'm still writing that. You can just look at the link I provided, it's the parser I wrote being used on a Qt application. No problematic dependency on Asio's executor/io_service. I agree with the “raw pointer to a contiguous piece of memory” and that's why I use Asio's buffer abstractions (sorry Niall, but there is no need to reinvent the wheel).

After the review on my library, I noted there was a big interest in low-level control for handling HTTP messages.

Can you please list the low-level control features or maybe link to the corresponding boost-dev posts which describe the desired low-level control? I thought I addressed them but I'd like to be sure

Thanks

The discussion was so big and happened years ago. Can't do that. I just wrote the summary as a list of issues to fix in my all design (so everything which was already right is lost in the discussions of this mailing list). Just provide a parser better than NodeJS and you're good for the most uncommon users. No allocations etc. -- Vinícius dos Santos Oliveira https://vinipsmaker.github.io/

On Sat, Jul 1, 2017 at 4:42 PM, Zach Laine via Boost <boost@lists.boost.org> wrote: Thanks for checking out the library!

* Is there a reason why buffer_cat_view and buffer_prefix_view are distinct types?

Yes, buffer_cat_view and buffer_prefix_view are two completely different things. A buffer_cat_view represents a buffer sequence of buffer sequences, while a buffer_prefix_view is just a range adapter over a single buffer sequence.

I think you just need to make a single type that owns a sequence of buffers, and maintains a pair of iterators or indices into the owned sequence.

You've more or less described buffer_prefix_view. A rough description of buffer_cat_view would be "a type that owns a heterogenous list of buffer sequences, whose iterators reflect the current position within a variant consisting of each iterator types from the list of buffer sequences." Quite different :)

* Similarly, as a user I'd prefer not to have string_body as a distinct type (unless it's a convenience typedef for dynamic_body<std::string>). If std::string is not a model of DynamicBuffer, I think DynamicBuffer could stand a slight reformulation.

std::string does not model DynamicBuffer, which itself is a Net-TS concept and thus immutable from Beast's perspective. Therefore beast::http::basic_dynamic_body cannot work with std::string and beast::http::string_body becomes necessary . I will likely add beast::http::basic_string_body to allow the allocator to be customized.

* I get why message<> has a Body template parameter, but why is Fields configurable? There is no message<> instantiation under examples/ or include/ that uses anything other than basic_fields<> that I could find. Do people ever use something else? Again, a brief rationale would be useful. I saw the "HTTP Message Container" page, btw, but while it suggested someone might want a different Fields template param, it didn't indicate why.

I have not done extensive work in this area of the library yet but I assure you it is a useful feature. A custom Fields implementation could support only a subset of all possible fields (for example it might only be capable of representing Content-Length, Transfer-Encodoing, Server, and User-Agent), storing the field name in a small integer enum instead of keeping the entire text. It could store Content-Length as an integer instead of a string. Custom Fields could use its own strategy for storing the field/value pairs. Most importantly, a custom Fields could be used to adapt another library's message model into something that Beast can consume. Or to allow a Beast message to be consumable by another library with a proper wrapper. All of these contemplated use-cases require the user to also provide a subclass of basic_parser, as the beast::http::parser is only usable with beast::http::basic_fields. These use-cases seem exotic but I feel that the niches they satisfy will come up and have merit. Think embedded devices or Internet of Things. Creating examples of custom Fields is on my to-do: <https://github.com/vinniefalco/Beast/issues/504>

* Allocators are gross. I know this is a controversial position,

I agree, that is controversial! And I'm not entirely happy with the interface of AllocatorAwareContainer but it is what it is. Which is to say it is standardized.

I think they're far more trouble than they're worth. Since you already have buffers that seem to me to cover all the major allocation strategies already, can't you get away with adding a buffer adapter that takes any random access container as its underlying storage, and be done with it?

That already exists, you can use beast::buffers_adapter on any linear buffer (including one provided by a container) and then use that adapter with beast::http::basic_dynamic_body. There are only a few AllocatorAwareContainer in Beast, those include basic_flat_buffer, basic_multi_buffer, but most importantly http::basic_fields. I don't think removing allocator support in http::basic_fields is such a good idea, especially when one of the examples already demonstrates how performance may be improved through its use: <https://github.com/vinniefalco/Beast/blob/78a065ba39836d91d7e70d93de7f9140f518083b/example/http-server-fast/fields_alloc.hpp#L86> Folks who want to get the most out of Beast will be taking advantage of Beast's allocator support so I must respectfully disagree with your position.

* The Quick Start contains its subsections in full, and then there are TOC links to each subsection separately. Probably one or the other would do.

This is intended, I want every example to have a ToC entry so that users can go to it quickly. Maybe if they forget the section it was in then the Toc entry will jog their memory.

* Each of the example links takes me to a GitHub page for the file linked. That should change to inline source like the Quick Start code.

Hmm I don't know about that, the server-framework is enormous. Including the example source code in bulk would make it hard to navigate. And I plan on more examples, I don't think that scales. Of course, if the consensus is that this change reflects an improvement then I will make it gladly. I have opened this issue for discussion: <https://github.com/vinniefalco/Beast/issues/565>

The same thing is true of all the reference documentation.

I'm not sure what you mean here.

* HTTP Crawl seems to be doing synchronous reads. Is there an example elsewhere that does a bunch of reads like this asynchronously?

Not yet but I can add something like that: <https://github.com/vinniefalco/Beast/issues/566>

* At least http_server_small.cpp (and maybe others? I didn't go back and check) has only Christopher M. Kohlhoff as an author. Vinnie, you probably need a copyright audit of all your files.

The copyright is correct, Chris is the author of http_server_small.cpp and http_server_fast.cpp

* I found the mix of function and type entries in "Table 6. Buffer Algorithms" to be initially quite confusing. Maybe separate them?

How about changing the title to "Buffer Algorithms and Types"?

* The buffer_prefix_view documentation in "Table 6. Buffer Algorithms" says "This is the type of buffer returned by buffer_prefix.", but 2/3 overloads with that name return something else.

Yeah that's a little confusing. I'll sort it: <https://github.com/vinniefalco/Beast/issues/567>

* HTTP Message Container says this: ... If it needs to access the other fields, it can do so using tag dispatch ... Why would I have a third overload and use tag dispatching? Most users are going to be really confused by what I quoted above, I think.

You're right. I think I had functions like this in mind: <https://github.com/vinniefalco/Beast/blob/78a065ba39836d91d7e70d93de7f9140f518083b/include/beast/http/impl/basic_parser.ipp#L347> The doc is a bit confusing, I will fix it.

* Please add an explicit Rationale section for smaller design choices. The FAQ and design justification sections that exist are fine, but they mostly cover the biggest-picture items. Each of those small design choices are going to go out the window if you can't remember a strong argument for one of them 5 years from now, when LEWG are asking you why a certain interface is the way it is.

Good advice but either vague or broad in scope (every design choice?), if there are specific design choices that should be explained then this becomes more actionable: <https://github.com/vinniefalco/Beast/issues/569> Thanks

On Sat, Jul 1, 2017 at 6:20 PM, Zach Laine via Boost <boost@lists.boost.org> wrote:

They don't seem that different to me. The data look the same from the outside, but the underlying structure is different. The underlying structure doesn't seem like it *needs* to be different, though. One could be flattened into the other. These are all views, right? Cheap-to-copy views? So, in terms of the existing types, if buffer_cat() returned a buffer_prefix_view with n=0, would that break things? I mean a semantic breakage, not a compilation breakage.

Perhaps it is possible to combine the functionality of buffer_prefix_view and buffer_cat_view, but it would come at the expense of a larger object. Buffer sequences are supposed to be copyable and normally they are not particularly large. For example beast::multi_buffer::const_buffers_type costs just one pointer and therefore pretty darn cheap. But some parts of Beast declare truly gnarly buffer sequences, like this one: <https://github.com/vinniefalco/Beast/blob/78a065ba39836d91d7e70d93de7f9140f518083b/include/beast/http/serializer.hpp#L190> using ch2_t = consuming_buffers<buffer_cat_view< detail::chunk_header, boost::asio::const_buffers_1, boost::asio::const_buffers_1, typename reader::const_buffers_type, boost::asio::const_buffers_1, boost::asio::const_buffers_1, boost::asio::const_buffers_1, boost::asio::const_buffers_1>>; reader::const_buffers_type can itself be a buffer_cat_view or some other sort of adapted buffer sequence, it depends on the traits of the Body being used. Chris was very helpful in working with me to optimize the http-server-fast example, he pointed out that the size of this buffer was exactly 256 bytes which unfortunately is one byte larger than it needs to be in order to take advantage of an Boost.Asio implementation optimization. I got lucky and found an unused member in consuming_buffers which I was able to remove. The moral of the story is that sizeof(ConstBufferSequence) counts, so these buffer range adapters need to be separate in order to be as small as possible. Thanks

On Sun, Jul 2, 2017 at 10:33 AM, Mike Gresens via Boost <boost@lists.boost.org> wrote:

As a user I'd prefer not to have string_body as a distinct type too. Same for string_view_body. Both are tightly bounded to distinct types.

To be clear, Peter was asking why not `response<std::string>` which is a different but legitimate question. There are design reasons why `std::string` cannot be a choice for Body.

So with a concept of "character sequence" we could introduce a "sequence_body" working with models like std::string, std::string_view, boost::string_ref, std::vector<char>, etc.

What do you propose replacing this declaration with? response<string_body> req; Thanks

On Sun, Jul 2, 2017 at 11:52 AM, Mike Gresens via Boost <boost@lists.boost.org> wrote:

Am 02.07.2017 um 20:34 schrieb Vinnie Falco via Boost: response<string_body> req;"

nothing to change here since

using string_body = beast::http::sequence_body<std::string>; // convenience decl in beast

Phew, okay, so we are on the same page. I don't want users to have to write `beast::http::const_sequence_body<std::string>` What you are asking for is a feature request, which requires no change in interface to any algorithm. That's a much easier ask. You already proposed two containers which were merged to the master branch in the example/common directory (thanks for your contribution!) <https://github.com/vinniefalco/Beast/blob/78a065ba39836d91d7e70d93de7f9140f518083b/example/common/const_body.hpp> <https://github.com/vinniefalco/Beast/blob/78a065ba39836d91d7e70d93de7f9140f518083b/example/common/mutable_body.hpp> These are not public interfaces yet, but I could be convinced to make them public interfaces if: 1. There is established practice that is either standardized or close to it [1] 2. The names and implementation of the concept checks either use existing standard library facilities if they exist, or provide an implementation that tracks the standard (or proposal) 3. The documentation (javadoc comments and exposition in the documentation) meets the quality requirements of the library 4. There is general support (which there very likely already is) I don't mind doing this work myself but I am not in a hurry to do so since existing use of Beast will be unaffected and there are other priorities. On the other hand if someone else wants to submit a pull request, I wouldn't turn it away! [1] This might be related http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3884.pdf

What follows is my review of the Beast library: A. Should the library be accepted into Boost? Yes. I vote ACCEPT, with no conditions for acceptance. B. What is your evaluation of the design? Good. As someone who is only barely familiar with using ASIO, it took some time to understand and appreciate the library. Beast is designed with the same goals that make the much of C++ standard library and other Boost libraries useful: It tries to provide as much control to the user of the library, and aims to make as few assumptions as possible that would alienate potential use cases. If there is an avenue for customization, whether it is the policy for dynamic allocation or otherwise, the library tries to expose it to the user. C. What is your evaluation of the implementation? The implementation is of a high quality, implemented using modern idiomatic C++, careful chooses dependencies on other well maintained Boost libraries when possible to avoid duplicating already well-implemented functionality. It has generally correct support for the C++11 allocator model. Many of the interfaces which are templates are additionally constrained with compile time checks and assertions, to make sure that any user supplied types satisfy those concepts correctly. The library code looks to be well covered by tests, and the library's test suite appears extensive, inluding benchmarks, code coverage, as well as tests for implementation detail components (like various traits and helper functions). D. What is your evaluation of the documentation? Excellent. Even though I believe the interfaces are well designed, the library is sufficiently complex that without its current level of documentation, it would have taken me several additional hours (especially as non-ASIO user) to understand how to use it. E. What is your evaluation of the potential usefulness of the library? It looks very useful, albeit _to me_ (given my unfamiliarity with HTTP, and with ASIO), not very simple to use without the aid of the examples. Intuitively, I can see developers proficient with the domain being able to use it to write powerful high level libraries or applications. F. Did you try to use the library? With which compiler(s)? Did you have any problems? Yes. I tried the tests, examples, benchmarks with gcc 6.3 (with libstdc++), with clang 3.9.1 (with libstdc++ and libc++). Additionally I wrote a HTTP client program by copying an example and modifying it. G. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? Not more than a fwe hours over the course of this review, which includes: - Getting a little familiar with ASIO. - Reading through the Beast implementation and examples code. - Building and running the tests and examples (with b2, against current Boost master branch). - Using Beast to write a simple HTTP client program. H. Are you knowledgeable about the problem domain? Not about HTTP, Websockets, or even the ASIO library. (Only about TCP, sockets, and general network I/O). Some minor suggestions, after reading through the latest code: 1. Test Jamfile should be changed so that b2 simply works, without first having to run b2 headers. 2. Include C library facility headers after C++ standard library headers. For example: #include <memory> #include <cstdint> 3. Various default-initialization new[] could be boost::make_unique_noinit: p_ = boost::make_unique_noinit<std::uint8_t[]>(capacity_); rd_.buf = boost::make_unique_noinit<std::uint8_t[]>(rd_.buf_size); 4. Alternatively: Perhaps those cases of std::unique_ptr could instead support allocators. 5. There are clamp() utilities defined in two places (ranges.hpp and clamp.hpp). Maybe all of these can exist in one place. 6. Trivial utilities (like clamp, ascii_tolower, etc.) could be constexpr. Since you support C++11 they can be written as single return expression. template<class T> constexpr inline T clamp(T x, T y) { return x < y ? x : y; } 7. Rely on Boost.Config when possible for detecting MSVC, in case compilers emulate it. Use BOOST_MSVC_FULL_VER instead of _MSC_FULL_VER. 8. Change the style of feature detection or defect macros, to not be defined to 1 or 0. Instead they should just be defined or not defined, consistent with Boost.Config. #define BEAST_NO_BOOST_STRING_VIEW #if !defined(BEAST_NO_BOOST_STRING_VIEW) 9. string_view 'value' parameters might be more idiomatic (and even optimal), instead of const-reference parameters. bool iequals(string_view lhs, string_view rhs) 10. empty_base_optimization should check is_final on more than just clang. Other C++ implementations also support this trait. 11. Maybe basic_multi_buffer(const Allocator&) be marked explicit. Note: None of the above may be required, actionable, or even worth the author considering. They are just thoughts that I noted while I was reading the code. Thank you, Vinnie, for proposing your efforts for inclusion in Boost. Glen

On 7/4/17 6:30 PM, Glen Fernandes via Boost wrote:

Some minor suggestions, after reading through the latest code:

1. Test Jamfile should be changed so that b2 simply works, without first having to run b2 headers.

Hmmm - how can one do this? I presume tests and library include things like #include <boost/config.hpp> and #include <boost/beast/...hpp> How is this going to function without having run b2 headers first? Robert Ramey

On Tue, Jul 4, 2017 at 9:21 PM, Glen Fernandes via Boost <boost@lists.boost.org> wrote:

$ cd libs/smart_ptr/test

Beast is not designed to build in-tree since it is not yet part of Boost

On 7/5/2017 12:25 AM, Vinnie Falco via Boost wrote:

On Tue, Jul 4, 2017 at 9:21 PM, Glen Fernandes via Boost <boost@lists.boost.org> wrote:

...

$ cd libs/smart_ptr/test

Beast is not designed to build in-tree since it is not yet part of Boost

Still the ideal of reviewing a library usually means that the library can be cloned beneath the Boost libs sub-directory and just "work", whether the library is part of Boost or not.

On Wed, Jul 5, 2017 at 9:26 AM, Robert Ramey via Boost <boost@lists.boost.org> wrote:

When one puts his library "out there" the hope is that people will use it and he will get feed back from users, bug fixes etc. But to make the library easy to use within boost is to make it hard to use outside of boost. A library should "just work" whether or not the user has boost installed or not. or whether he is running as a subdirectory of boost or not. Or whether he uses b2 or not.

Yes that was my thinking as well when I set it up. However, Peter's suggestion to add: project boost/libs : requirements <implicit-dependency>/boost//headers Seems to work. Or rather, it didn't break anything. So the problem you allude to might be easily fixed by just having a better "best practices" document. Maybe there could be a "blank" repository under boostorg/ that implements a "Hello world!" function in a header file and is organized exactly like a boost library with tests, b2/cmake, Travis, Appveyor, CircleCI integration, documentation, and works in or out of tree. Someone could just clone this blank project and then they are good to go. Thanks

On Wed, Jul 5, 2017 at 9:59 AM, Peter Dimov via Boost <boost@lists.boost.org> wrote:

...when the library is put under $(BOOST_ROOT)/libs, one just needs to delete the empty Jamroot for it to pick up the Boost Jamroot.

That's a hassle because if you want to work with Git now what you are making commits which remove or re-add the Jamroot?

On Wed, Jul 5, 2017 at 10:12 AM, Robert Ramey via Boost <boost@lists.boost.org> wrote:

...

I'd like a repository like boostorg/ci which I can add to my project using either git-subtree or git-submodule which comes with handy scripts and files such as travis.yml, appveyor.yml, circleci.yml includes that I can use. Unfortunately I don't think its possible to have .yml include files so I will settle for a command line utility which reads a config file which I create and then auto-generates CI files for Travis, Appveyor, CircleCI, any others. I would like it to support code coverage, valgrind, undefined behavior sanitizer, address sanitizer, I want any of the tests that it runs to run under a debugger on Travis and have the stack trace utilities so if it crashes the Travis output shows a stack.

On Wed, Jul 5, 2017 at 3:01 PM, Edward Diener via Boost <boost@lists.boost.org> wrote:

You are saying that a library that is supposed to be reviewed for inclusion in Boost is supposed to somehow work outside of Boost ? I do not think so and can't even begin to understand that sort of logic.

Speaking for my own library Beast, I have developed a following of users over the lifetime of the library and these users aren't cloning the Beast repository into boost/libs/beast. They are getting it from vcpkg, git-subtree or git-submodule into their own repository, or cloning it out-of-tree. It would be a poor experience if Beast did not build out-of-tree or if it required users to put files into their system provided Boost installation or to create their own Boost installation. It was stated somewhere that Boost reviewers like to see instances of libraries that are already being used in practice. A library can work out-of-tree or it can be unpopular - pick one.

On 7/5/17 3:01 PM, Edward Diener via Boost wrote:

You are saying that a library that is supposed to be reviewed for inclusion in Boost is supposed to somehow work outside of Boost ?

yep - that's exactly what I'm saying.

I do not think so and can't even begin to understand that sort of logic.

It's pretty simple. A library submitted for review is not yet a boost library. It's submitted with the idea that should it be accepted into boost it will be "boostified". To require this extra work on a library which might not be accepted has always been considered an overly onerous requirement for review. Off topic - I believe that boost libraries should also work outside of boost. Robert Ramey

May be there could be more hints for client/server developers. E.g. where is the best place / how is the best practice to add cookie-handling, Authentication challenges, etc.

Funnily, for someone who actually solved numerous of the issues this question can easily be rephrased as:

Now I finally learned how drive a bicycle, lets ask tips for driving semi-trailer.

And this is **the major** problem of the library. In the CppCMS frameworks the HTTP part is less than 2.5% of the core code. The rest is the "minor" stuff like cookies, session, url parsing and much-much more. And note - I'm talking about server part only Now once you choose to use Beast you'll hit the wall very quickly as you'll have to do all tricks your own and they will consume most of your work for example URL decoding, parsing query string, parsing or generating trivial forms or handling trivial cookies. So instead of concentrating on your application issues you are going to deal with 1001 small issues that require both experience and knowledge to do them right. The biggest problem is actually that vast majority security issues do not come from HTTP parsing at all, but rather all "minor stuff" [1] that left out of scope of the Beast. This question you had written had popped for me the big red flag - without proper well organized tools that handle the "minor stuff" Beast users are virtually doomed to writing insecure code. Unlike most HTTP servers/clients are build with security by design - do safe stuff by default, Beast lives this ALL to end user. This part **extremely** concerns me - as somebody who actually developed both web services and tools to make them secure and aware of unforgiving nature of WWW. Artyom Beilis [1]: http://cppcms.com/wikipp/en/page/secure_programming

On Sun, Jul 9, 2017 at 9:38 AM, Daniela Engert via Boost <boost@lists.boost.org> wrote:

That said, I agree on the opinion which other reviewers already stated on that it lacks some convenience helper functions when it comes to draining or construction of requests. I'd love to see those added to the library.

Yeah this message is coming through loud and clear.. LOL... I will make adjustments to the scope of Beast.

* Beast's zlib emits a few harmless but stupid warnings C4127 "conditional expression is constant" (Microsoft, get rid of them!) and quite some narrowing warnings C4244. These need some cleansing before we can use Beast in our team because of our /W4 /WX policy.

It should be "real" zlib not Beast zlib which is emitting those warnings. There are two versions of zlib in Beast: 1. The one in <beast/zlib/*> is a header-only port of zlib to modern-ish C++11. There is a warning "‘*((void*)& old_flush +4)’ may be used uninitialized in this function [-Wmaybe-uninitialized]". 2. The original zlib distribution in test/zlib/zlib-1.2.8. This is used for the tests, to compare Beast's output with zlib's output, and to ensure that output produced by zlib can be consumed by Beast and vice-versa. I am reluctant to change the source code for zlib because I'd like it to perfectly track the upstream repository (it was brought in using git-subtree). For #1 The variable is not used uninitialized, I believe it is a false positive cause by the implementation of boost::optional. I have spent a lot of time trying to fix this, to no avail - if someone smarter than me can offer a patch I would be grateful. For #2 the warning should not affect users, since it is limited to the tests.

* Beast's test-suite uses the deprecated Boost.Coroutine library which leads to deprecation warnings

Beast's tests use Boost.Asio coroutines (to make sure they work with Beast). It is Asio that uses the deprecated Boost.Coroutine. Asio needs to be updated to work with Coroutine2. I can't do much about this other than not use coroutines in the tests, which would yield less coverage.

* there is an error in the Jamfile of the test-suite which creates a truncated compiler options that msvc complains about (this is already rectified in a later commit, though)

This is a weird issue in bjam would should be fixed in Boost 1.66.0.

I didn't implement the actual download so far because Beast's file_body is not part of the review branch.

`file_body` is now a public interface as of version 74 (no more "out of scope" hehe) Thanks

2017-07-09 18:38 GMT+02:00 Daniela Engert via Boost <boost@lists.boost.org>:

* Beast's test-suite uses the deprecated Boost.Coroutine library which leads to deprecation warnings

warnings can be disabled or boost.coroutien2 (C++11) could be used instead

2017-07-09 20:22 GMT+02:00 Vinnie Falco via Boost <boost@lists.boost.org>:

On Sun, Jul 9, 2017 at 11:02 AM, Oliver Kowalke via Boost <boost@lists.boost.org> wrote:

...

warnings can be disabled

I do disable them but it doesn't help: <https://github.com/vinniefalco/Beast/blob/1bc30cb6e4ce0fd2e288b2cd74d18f 857801cbc7/Jamroot#L82>

...

or boost.coroutien2 (C++11) could be used instead

The problem is that Beast doesn't use Boost.Coroutine. It is Asio that uses Boost.Coroutine. Beast just uses boost::asio::spawn which launches a coroutine using Boost.Coroutine.

in order to use Boost.Coroutine2 (and I think this is a good idea), Boost.Asio has to be patched to use it. The problem is this will break everyone using Asio coroutines, as Coroutine2 is 1. not compatible with the Coroutine interface, and 2. not available in C++11.

I've had already provided a pull-request: https://github.com/boostorg/asio/pull/52

-----Original Message----- From: Boost [mailto:boost-bounces@lists.boost.org] On Behalf Of Vinnie Falco via Boost Sent: 13 July 2017 04:40 To: Boost Cc: Vinnie Falco Subject: Re: [boost] [review][beast] Review of Beast starts today : July 1 - July 10

On Sun, Jul 9, 2017 at 9:38 AM, Daniela Engert via Boost <boost@lists.boost.org> wrote:

...

* Beast's zlib emits a few harmless but stupid warnings C4127 "conditional expression is constant" (Microsoft, get rid of them!) and quite some narrowing warnings C4244. These need some cleansing before we can use Beast in our team because of our /W4 /WX policy.

The last of the warnings has been fixed,

:-)

this one was incredibly annoying!

They all are!

Nik helped, he came up with calling is_initialized() (the guidance on the boost page was insufficient):

<https://github.com/vinniefalco/Beast/pull/639>

I couldn't see the changes needed from this. If either of you have a moment to explain briefly what is missing from the guidance note https://svn.boost.org/trac10/wiki/Guidelines/WarningsGuidelines then I'll update it. This may save others from being incredibly annoyed ;-) Paul --- Paul A. Bristow Prizet Farmhouse Kendal UK LA8 8AB +44 (0) 1539 561830