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-02 00:22:40

On Sat, Jul 1, 2017 at 4:42 PM, Zach Laine via Boost
<boost_at_[hidden]> 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