Re: [boost] [review][beast] Review of Beast starts today : July 1 - July 10

Subject: Re: [boost] [review][beast] Review of Beast starts today : July 1 - July 10
From: Vinnie Falco (vinnie.falco_at_[hidden])
Date: 2017-07-01 16:02:59

On Sat, Jul 1, 2017 at 7:46 AM, Niall Douglas via Boost
<boost_at_[hidden]> 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_serializing.html#beast.using_http.buffer_oriented_serializing.write_to_std_ostream
http://vinniefalco.github.io/beast/beast/using_http/buffer_oriented_parsing.html#beast.using_http.buffer_oriented_parsing.read_from_std_istream

> 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/78a065ba39836d91d7e70d93de7f9140f518083b/example/http-server-fast/fields_alloc.hpp#L86

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.html
http://vinniefalco.github.io/beast/beast/ref/beast__http__string_view_body.html
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